launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Diff comments:

> diff --git a/database/schema/patch-2210-31-0.sql b/database/schema/patch-2210-31-0.sql
> new file mode 100644
> index 0000000..9bcceb5
> --- /dev/null
> +++ b/database/schema/patch-2210-31-0.sql
> @@ -0,0 +1,21 @@
> +-- Copyright 2021 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +ALTER TABLE Message
> +    ADD COLUMN date_deleted timestamp without time zone,
> +    ADD COLUMN date_last_edit timestamp without time zone;
> +
> +CREATE TABLE MessageRevision (
> +    id serial PRIMARY KEY,
> +    message integer NOT NULL REFERENCES Message,
> +    content text,

Indeed, this is not covering well multi-part messages and losing information in such cases, specially the `blob` of those chunks.

But I think I don't fully agree on putting all historical revisions directly into `Message` / `MessageChunk`. By doing so, we would increase complexity on reading queries in several places, and those datasets would start growing faster than they are doing today, which could potentially make querying them even more expensive in a near-ish future.

I would suggest another approach: keeping `Message` and `MessageChunk` tables as "the current state of the messages and their chunks", as they are today. But instead of having just `MessageRevision` with the historical text, mimic the `MessageChunk` structure in a `MessageRevisionChunk` table. 

This way, when a user edits the text part of a message, we would:
- create a new `MessageRevision`;
- move all existing message chunks with `content` to the new `MessageRevisionChunk` table, linked to the newly created `MessageRevision`;
- remove all current `MessageChunk` with `content` (keeping the ones with `blobs`);
- create a single `MessageChunk` with the new text `content`.

Do you think this covers all our cases without losing anything?

> +    date_created timestamp without time zone,
> +    date_deleted timestamp without time zone
> +);
> +
> +CREATE UNIQUE INDEX messagerevision__message__date_created__key
> +    ON MessageRevision(message, date_created);
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 31, 0);


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/401976
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:comment-editing-db-patch.