From 0a5f06b84de76939cf9805e4266d47c2e8bf66df Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 26 Jul 2022 10:16:26 +0900 Subject: [PATCH] Fix a few issues with REINDEX grammar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- doc/src/sgml/ref/reindex.sgml | 3 +- src/backend/parser/gram.y | 36 ++++++++++----------- src/bin/scripts/t/090_reindexdb.pl | 37 +++++++++++++++------- src/test/regress/expected/create_index.out | 7 ++-- src/test/regress/sql/create_index.sql | 2 ++ 5 files changed, 49 insertions(+), 36 deletions(-) diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 6750eb6e47..fcbda88149 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -22,8 +22,7 @@ PostgreSQL documentation REINDEX [ ( option [, ...] ) ] { INDEX | TABLE | SCHEMA } [ CONCURRENTLY ] name -REINDEX [ ( option [, ...] ) ] DATABASE [ CONCURRENTLY ] [ name ] -REINDEX [ ( option [, ...] ) ] SYSTEM [ name ] +REINDEX [ ( option [, ...] ) ] { DATABASE | SYSTEM } [ CONCURRENTLY ] [ name ] where option can be one of: diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index ace4fb5c77..f9037761f9 100644 --- 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 generic_option_elem alter_generic_option_elem %type generic_option_list alter_generic_option_list -%type reindex_target_type +%type reindex_target_relation reindex_target_all %type opt_reindex_option_list %type 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] - * REINDEX [ (options) ] DATABASE [CONCURRENTLY] [] - * REINDEX [ (options) ] SYSTEM [] + * REINDEX [ (options) ] {INDEX | TABLE | SCHEMA} [CONCURRENTLY] + * REINDEX [ (options) ] {DATABASE | SYSTEM} [CONCURRENTLY] [] *****************************************************************************/ 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 + | REINDEX opt_reindex_option_list reindex_target_all 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 - { - 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 b5fff5a9cf..e706d686e3 100644 --- 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\\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__index|relfilenode is unchanged -test1|pg_toast.pg_toast__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__index|OID is unchanged|relfilenode is unchanged +test1|pg_toast.pg_toast__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__index|relfilenode has changed -test1|pg_toast.pg_toast__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__index|OID is unchanged|relfilenode has changed +test1|pg_toast.pg_toast__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__index|OID is unchanged|relfilenode is unchanged +test1|pg_toast.pg_toast__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 a913a1846f..6cd57e3eaa 100644 --- 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 4b75790e47..a3738833b2 100644 --- 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