Re: Allowing REINDEX to have an optional name

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.

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.

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

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

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

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