Re: Allowing REINDEX to have an optional name

On Wed, 29 Jun 2022 at 05:35, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Tue, Jun 28, 2022 at 11:02:25AM +0100, Simon Riggs wrote:
> > Attached patch is tested, documented and imho ready to be committed,
> > so I will mark it so in CFapp.

Thanks for the review Michael.

> The behavior introduced by this patch should be reflected in
> reindexdb. See in particular reindex_one_database(), where a
> REINDEX_SYSTEM is enforced first on the catalogs for the
> non-concurrent mode when running the reindex on a database.

Originally, I was trying to avoid changing prior behavior, but now
that we have agreed to do so, this makes sense.

That section of code has been removed, tests updated. No changes to
docs seem to be required.

> +-- unqualified reindex database
> +-- if you want to test REINDEX DATABASE, uncomment the following line,
> +-- but note that this adds about 0.5s to the regression tests and the
> +-- results are volatile when run in parallel to other tasks. Note also
> +-- that REINDEX SYSTEM is specifically not tested because it can deadlock.
> +-- REINDEX (VERBOSE) DATABASE;
>
> No need to add that IMHO.

That was more a comment to reviewer, but I think something should be
said for later developers.

> REINDEX [ ( <replaceable class="parameter">option</replaceable> [,
> ...] ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [
> CONCURRENTLY ] <replaceable class="parameter">name</replaceable>
> +REINDEX [ ( <replaceable class="parameter">option</replaceable> [,
> ...] ) ] { DATABASE | SYSTEM } [ CONCURRENTLY ] [ <replaceable
> class="parameter">name</replaceable> ]
>
> Shouldn't you remove DATABASE and SYSTEM from the first line, keeping
> only INDEX. TABLE and SCHEMA? The second line, with its optional
> "name" would cover the DATABASE and SYSTEM cases at 100%.

I agree that your proposal is clearer. Done.

> - if (strcmp(objectName, get_database_name(objectOid)) != 0)
> + if (objectName && strcmp(objectName, get_database_name(objectOid)) != 0)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("can only reindex the currently open database")));
> if (!pg_database_ownercheck(objectOid, GetUserId()))
> aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_DATABASE,
> - objectName);
> + get_database_name(objectOid));
>
> This could call get_database_name() just once.

It could, but I couldn't see any benefit in changing that for the code
under discussion.

If calling get_database_name() multiple times is an issue, I've added
a cache for that - another patch attached, if you think its worth it.

> + * You might think it would be good to include catalogs,
> + * but doing that can deadlock, so isn't much use in real world,
> + * nor can we safely test that it even works.
>
> Not sure what you mean here exactly.

REINDEX SYSTEM can deadlock, which is why we are avoiding it.

This was a comment to later developers as to why things are done that
way. Feel free to update the wording or location, but something should
be mentioned to avoid later discussion.

Thanks for the review, new version attached.

--
Simon Riggs http://www.EnterpriseDB.com/