launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'database/schema/comments.sql'
> --- database/schema/comments.sql	2016-01-26 15:47:37 +0000
> +++ database/schema/comments.sql	2016-07-07 14:31:17 +0000
> @@ -1952,6 +1952,7 @@
>  COMMENT ON COLUMN ArchiveAuthToken.date_created IS 'The date and time this token was created.';
>  COMMENT ON COLUMN ArchiveAuthToken.date_deactivated IS 'The date and time this token was deactivated.';
>  COMMENT ON COLUMN ArchiveAuthToken.token IS 'The token text for this authorisation.';
> +COMMENT ON COLUMN ArchiveAuthToken.name IS 'The name for this named token.';

Nothing will actually apply this.  We nowadays usually do this with a COMMENT statement in the patch instead.

>  
>  -- ArchiveDependency
>  COMMENT ON TABLE ArchiveDependency IS 'This table maps a given archive to all other archives it should depend on.';
> 
> === added file 'database/schema/patch-2209-79-0.sql'
> --- database/schema/patch-2209-79-0.sql	1970-01-01 00:00:00 +0000
> +++ database/schema/patch-2209-79-0.sql	2016-07-07 14:31:17 +0000
> @@ -0,0 +1,12 @@
> +-- Copyright 2016 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 ArchiveAuthToken
> +    ALTER COLUMN person DROP NOT NULL,
> +    ADD COLUMN name text;
> +
> +CREATE INDEX archiveauthtoken__name__idx ON ArchiveAuthToken USING btree (name);

The relevant query ends up as something like "SELECT * FROM ArchiveAuthToken WHERE archive = <id> AND date_deactivated IS NULL AND name = <name>;", so I don't see how this index would ever be used.  It should probably be more like:

  CREATE INDEX archiveauthtoken__archive__name__date_deactivated__idx ON ArchiveAuthToken (archive, name) WHERE date_deactivated IS NULL;

... but find some relevant queries by running relevant bits of the test suite under LP_DEBUG_SQL=1 and then run those queries under "EXPLAIN (ANALYZE, BUFFERS)" to make sure that your index is used.

> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2209, 79, 0);


-- 
https://code.launchpad.net/~maxiberta/launchpad/db-named-auth-tokens/+merge/299433
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~maxiberta/launchpad/db-named-auth-tokens into lp:launchpad/db-devel.