Fix a few issues with REINDEX grammar

This addresses a couple of bugs in the REINDEX grammar, introduced by
83011ce:
- A name was never specified for DATABASE/SYSTEM, even if the query
included one.  This caused such REINDEX queries to always work with any
object name, but we should complain if the object name specified does
not match the name of the database we are connected to.  A test is added
for this case in the main regression test suite, provided by Álvaro.
- REINDEX SYSTEM CONCURRENTLY [name] was getting rejected in the
parser.  Concurrent rebuilds are not supported for catalogs but the
error provided at execution time is more helpful for the user, and
allowing this flavor results in a simplification of the parsing logic.
- REINDEX DATABASE CONCURRENTLY was rebuilding the index in a
non-concurrent way, as the option was not being appended correctly in
the list of DefElems in ReindexStmt (REINDEX (CONCURRENTLY) DATABASE was
working fine.  A test is added in the TAP tests of reindexdb for this
case, where we already have a REINDEX DATABASE CONCURRENTLY query
running on a small-ish instance.  This relies on the work done in
2cbc3c1 for SYSTEM, but here we check if the OIDs of the index relations
match or not after the concurrent rebuild.  Note that in order to get
this part to work, I had to tweak the tests so as the index OID and
names are saved separately.  This change not affect the reliability or
of the coverage of the existing tests.

While on it, I have implemented a tweak in the grammar to reduce the
parsing by one branch, simplifying things even more.

Author: Michael Paquier, Álvaro Herrera
Discussion: https://postgr.es/m/YttqI6O64wDxGn0K@paquier.xyz

diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 6750eb6e47f83be25f3ee56816119bdbef4e80a1..fcbda881494a9003d8e1efbba8cc5c06f5bda46c 100644 (file)
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -22,8 +22,7 @@ PostgreSQL documentation
  <refsynopsisdiv>
 <synopsis>
 REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { INDEX | TABLE | SCHEMA } [ CONCURRENTLY ] <replaceable class="parameter">name</replaceable>
-REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] DATABASE [ CONCURRENTLY ] [ <replaceable class="parameter">name</replaceable> ]
-REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] SYSTEM [ <replaceable class="parameter">name</replaceable> ]
+REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DATABASE | SYSTEM } [ CONCURRENTLY ] [ <replaceable class="parameter">name</replaceable> ]
 
 <phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase>
 

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index ace4fb5c77881eac228a92d8150d754f3b0e931e..f9037761f964b3555cce40c262eca38e20442dd6 100644 (file)
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -564,7 +564,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type <defelt> generic_option_elem alter_generic_option_elem
 %type <list>   generic_option_list alter_generic_option_list
 
-%type <ival>   reindex_target_type
+%type <ival>   reindex_target_relation reindex_target_all
 %type <list>   opt_reindex_option_list
 
 %type <node>   copy_generic_opt_arg copy_generic_opt_arg_list_item
@@ -9092,13 +9092,12 @@ DropTransformStmt: DROP TRANSFORM opt_if_exists FOR Typename LANGUAGE name opt_d
  *
  *     QUERY:
  *
- *     REINDEX [ (options) ] {TABLE | INDEX | SCHEMA} [CONCURRENTLY] <name>
- *     REINDEX [ (options) ] DATABASE [CONCURRENTLY] [<name>]
- *     REINDEX [ (options) ] SYSTEM [<name>]
+ *     REINDEX [ (options) ] {INDEX | TABLE | SCHEMA} [CONCURRENTLY] <name>
+ *     REINDEX [ (options) ] {DATABASE | SYSTEM} [CONCURRENTLY] [<name>]
  *****************************************************************************/
 
 ReindexStmt:
-           REINDEX opt_reindex_option_list reindex_target_type opt_concurrently qualified_name
+           REINDEX opt_reindex_option_list reindex_target_relation opt_concurrently qualified_name
                {
                    ReindexStmt *n = makeNode(ReindexStmt);
 
@@ -9116,37 +9115,36 @@ ReindexStmt:
                    ReindexStmt *n = makeNode(ReindexStmt);
 
                    n->kind = REINDEX_OBJECT_SCHEMA;
-                   n->name = $5;
                    n->relation = NULL;
+                   n->name = $5;
                    n->params = $2;
                    if ($4)
                        n->params = lappend(n->params,
                                            makeDefElem("concurrently", NULL, @4));
                    $$ = (Node *) n;
                }
-           | REINDEX opt_reindex_option_list DATABASE opt_concurrently opt_single_name
-               {
-                   ReindexStmt *n = makeNode(ReindexStmt);
-                   n->kind = REINDEX_OBJECT_DATABASE;
-                   n->name = NULL;
-                   n->relation = NULL;
-                   n->params = $2;
-                   $$ = (Node *) n;
-               }
-           | REINDEX opt_reindex_option_list SYSTEM_P opt_single_name
+           | REINDEX opt_reindex_option_list reindex_target_all opt_concurrently opt_single_name
                {
                    ReindexStmt *n = makeNode(ReindexStmt);
-                   n->kind = REINDEX_OBJECT_SYSTEM;
-                   n->name = NULL;
+
+                   n->kind = $3;
                    n->relation = NULL;
+                   n->name = $5;
                    n->params = $2;
+                   if ($4)
+                       n->params = lappend(n->params,
+                                           makeDefElem("concurrently", NULL, @4));
                    $$ = (Node *) n;
                }
        ;
-reindex_target_type:
+reindex_target_relation:
            INDEX                   { $$ = REINDEX_OBJECT_INDEX; }
            | TABLE                 { $$ = REINDEX_OBJECT_TABLE; }
        ;
+reindex_target_all:
+           SYSTEM_P                { $$ = REINDEX_OBJECT_SYSTEM; }
+           | DATABASE              { $$ = REINDEX_OBJECT_DATABASE; }
+       ;
 opt_reindex_option_list:
            '(' utility_option_list ')'             { $$ = $2; }
            | /* EMPTY */                           { $$ = NULL; }

diff --git a/src/bin/scripts/t/090_reindexdb.pl b/src/bin/scripts/t/090_reindexdb.pl
index b5fff5a9cf1dd10ca96b801beffed66e79ad5d92..e706d686e39384bb84c7cfd3949426857d7eaae7 100644 (file)
--- a/src/bin/scripts/t/090_reindexdb.pl
+++ b/src/bin/scripts/t/090_reindexdb.pl
@@ -40,12 +40,12 @@ my $toast_index = $node->safe_psql('postgres',
 # REINDEX operations.  A set of relfilenodes is saved from the catalogs
 # and then compared with pg_class.
 $node->safe_psql('postgres',
-   'CREATE TABLE index_relfilenodes (parent regclass, indname regclass, relfilenode oid);'
+   'CREATE TABLE index_relfilenodes (parent regclass, indname text, indoid oid, relfilenode oid);'
 );
 # Save the relfilenode of a set of toast indexes, one from the catalog
 # pg_constraint and one from the test table.
 my $fetch_toast_relfilenodes =
-  qq{SELECT b.oid::regclass, c.oid::regclass, c.relfilenode
+  qq{SELECT b.oid::regclass, c.oid::regclass::text, c.oid, c.relfilenode
   FROM pg_class a
     JOIN pg_class b ON (a.oid = b.reltoastrelid)
     JOIN pg_index i on (a.oid = i.indrelid)
@@ -53,7 +53,7 @@ my $fetch_toast_relfilenodes =
   WHERE b.oid IN ('pg_constraint'::regclass, 'test1'::regclass)};
 # Same for relfilenodes of normal indexes.  This saves the relfilenode
 # from an index of pg_constraint, and from the index of the test table.
-my $fetch_index_relfilenodes = qq{SELECT i.indrelid, a.oid, a.relfilenode
+my $fetch_index_relfilenodes = qq{SELECT i.indrelid, a.oid::regclass::text, a.oid, a.relfilenode
   FROM pg_class a
     JOIN pg_index i ON (i.indexrelid = a.oid)
   WHERE a.relname IN ('pg_constraint_oid_index', 'test1x')};
@@ -69,6 +69,8 @@ my $save_relfilenodes =
 # parent table is included to provide more context.
 my $compare_relfilenodes = qq(SELECT b.parent::regclass,
   regexp_replace(b.indname::text, '(pg_toast.pg_toast_)\\d+(_index)', '\\1<oid>\\2'),
+  CASE WHEN a.oid = b.indoid THEN 'OID is unchanged'
+    ELSE 'OID has changed' END,
   CASE WHEN a.relfilenode = b.relfilenode THEN 'relfilenode is unchanged'
     ELSE 'relfilenode has changed' END
   FROM index_relfilenodes b
@@ -83,10 +85,10 @@ $node->issues_sql_like(
    'SQL REINDEX run');
 my $relnode_info = $node->safe_psql('postgres', $compare_relfilenodes);
 is( $relnode_info,
-   qq(pg_constraint|pg_constraint_oid_index|relfilenode is unchanged
-pg_constraint|pg_toast.pg_toast_<oid>_index|relfilenode is unchanged
-test1|pg_toast.pg_toast_<oid>_index|relfilenode has changed
-test1|test1x|relfilenode has changed),
+   qq(pg_constraint|pg_constraint_oid_index|OID is unchanged|relfilenode is unchanged
+pg_constraint|pg_toast.pg_toast_<oid>_index|OID is unchanged|relfilenode is unchanged
+test1|pg_toast.pg_toast_<oid>_index|OID is unchanged|relfilenode has changed
+test1|test1x|OID is unchanged|relfilenode has changed),
    'relfilenode change after REINDEX DATABASE');
 
 # Re-save and run the second one.
@@ -98,10 +100,10 @@ $node->issues_sql_like(
    'reindex system tables');
 $relnode_info = $node->safe_psql('postgres', $compare_relfilenodes);
 is( $relnode_info,
-   qq(pg_constraint|pg_constraint_oid_index|relfilenode has changed
-pg_constraint|pg_toast.pg_toast_<oid>_index|relfilenode has changed
-test1|pg_toast.pg_toast_<oid>_index|relfilenode is unchanged
-test1|test1x|relfilenode is unchanged),
+   qq(pg_constraint|pg_constraint_oid_index|OID is unchanged|relfilenode has changed
+pg_constraint|pg_toast.pg_toast_<oid>_index|OID is unchanged|relfilenode has changed
+test1|pg_toast.pg_toast_<oid>_index|OID is unchanged|relfilenode is unchanged
+test1|test1x|OID is unchanged|relfilenode is unchanged),
    'relfilenode change after REINDEX SYSTEM');
 
 $node->issues_sql_like(
@@ -132,11 +134,22 @@ $node->issues_sql_like(
    qr/statement: REINDEX \(VERBOSE, TABLESPACE $tbspace_name\) TABLE public\.test1;/,
    'reindex with verbose output and tablespace');
 
-# the same with --concurrently
+# Same with --concurrently.
+# Save the state of the relations and compare them after the DATABASE
+# rebuild.
+$node->safe_psql('postgres',
+   "TRUNCATE index_relfilenodes; $save_relfilenodes");
 $node->issues_sql_like(
    [ 'reindexdb', '--concurrently', 'postgres' ],
    qr/statement: REINDEX DATABASE CONCURRENTLY postgres;/,
    'SQL REINDEX CONCURRENTLY run');
+$relnode_info = $node->safe_psql('postgres', $compare_relfilenodes);
+is( $relnode_info,
+   qq(pg_constraint|pg_constraint_oid_index|OID is unchanged|relfilenode is unchanged
+pg_constraint|pg_toast.pg_toast_<oid>_index|OID is unchanged|relfilenode is unchanged
+test1|pg_toast.pg_toast_<oid>_index|OID has changed|relfilenode has changed
+test1|test1x|OID has changed|relfilenode has changed),
+   'OID change after REINDEX DATABASE CONCURRENTLY');
 
 $node->issues_sql_like(
    [ 'reindexdb', '--concurrently', '-t', 'test1', 'postgres' ],

diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index a913a1846f98a5bcf5ad8afe7456662bed946971..6cd57e3eaa700bc2e849d02dc06e57a80650ff4d 100644 (file)
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2521,9 +2521,7 @@ ERROR:  cannot reindex system catalogs concurrently
 REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
 ERROR:  cannot reindex system catalogs concurrently
 REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
-ERROR:  syntax error at or near "CONCURRENTLY"
-LINE 1: REINDEX SYSTEM CONCURRENTLY postgres;
-                       ^
+ERROR:  cannot reindex system catalogs concurrently
 REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
 ERROR:  cannot reindex system catalogs concurrently
 REINDEX (CONCURRENTLY) SYSTEM;  -- ditto
@@ -2531,6 +2529,9 @@ ERROR:  cannot reindex system catalogs concurrently
 -- Warns about catalog relations
 REINDEX SCHEMA CONCURRENTLY pg_catalog;
 WARNING:  cannot reindex system catalogs concurrently, skipping all
+-- Not the current database
+REINDEX DATABASE not_current_database;
+ERROR:  can only reindex the currently open database
 -- Check the relation status, there should not be invalid indexes
 \d concur_reindex_tab
          Table "public.concur_reindex_tab"

diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 4b75790e472e7df1d70eb1c9a2054f49ec2f8566..a3738833b28a81b9d57b38ee47f17f6db3e3bd0b 100644 (file)
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1076,6 +1076,8 @@ REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
 REINDEX (CONCURRENTLY) SYSTEM;  -- ditto
 -- Warns about catalog relations
 REINDEX SCHEMA CONCURRENTLY pg_catalog;
+-- Not the current database
+REINDEX DATABASE not_current_database;
 
 -- Check the relation status, there should not be invalid indexes
 \d concur_reindex_tab