launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



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..593d214
> --- /dev/null
> +++ b/database/schema/patch-2210-31-0.sql
> @@ -0,0 +1,28 @@
> +-- 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_edited timestamp without time zone;
> +
> +CREATE TABLE MessageRevision (
> +    id serial PRIMARY KEY,
> +    message integer NOT NULL REFERENCES Message,
> +    date_created timestamp without time zone,

date_created should be NOT NULL.

> +    date_deleted timestamp without time zone
> +);

I wonder if we'll need a subject column as well at some point (subjects of incoming email are shown in the web UI, so probably ought to be editable)?  Doesn't have to be right now though.

> +
> +CREATE UNIQUE INDEX messagerevision__message__date_created__key
> +    ON MessageRevision(message, date_created);

I'm not wild about making the creation date unique within the message.  I don't have a specific use case that would violate this, but it seems like the sort of thing that could eventually be problematic if multiple message revisions are created within the same transaction due to some kind of automation; I suppose I'm thinking of this due to the bug I recently fixed relating to multiple bug activity entries created within the same transaction (https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/402636).

Could we have an explicit `revision integer NOT NULL` column instead?

> +
> +CREATE TABLE MessageRevisionChunk (
> +    id serial PRIMARY KEY,
> +    messagerevision integer NOT NULL REFERENCES MessageRevisionChunk,

Shouldn't this be `REFERENCES MessageRevision`?

> +    sequence integer NOT NULL,
> +    content text NOT NULL
> +);

Could you add `COMMENT` statements for `MessageRevision` and `MessageRevisionChunk`, as well as their non-obvious columns (you can omit id)?

Both the `Message` and `MessageChunk` tables were created using `WITH (fillfactor='100')` (see https://www.postgresql.org/docs/10/sql-createtable.html#SQL-CREATETABLE-STORAGE-PARAMETERS).  This probably isn't appropriate for `MessageRevision` since its rows will in fact be updated (and is now less appropriate for `Message`, but oh well); but maybe it would be worth setting this on `MessageRevisionChunk`?

> +
> +
> +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.