From 2cbc3c17a5c11d13c0ac92fe7557c56408f8f3d0 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 19 Jul 2022 11:45:06 +0900 Subject: [PATCH] Rework logic and simplify syntax of REINDEX DATABASE/SYSTEM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per discussion, this commit includes a couple of changes to these two flavors of REINDEX: * The grammar is changed to make the name of the object optional, hence one can rebuild all the indexes of the wanted area by specifying only "REINDEX DATABASE;" or "REINDEX SYSTEM;". Previously, the object name was mandatory and had to match the name of the database on which the command is issued. * REINDEX DATABASE is changed to ignore catalogs, making this task only possible with REINDEX SYSTEM. This is a historical change, but there was no way to work only on the indexes of a database without touching the catalogs. We have discussed more approaches here, like the addition of an option to skip the catalogs without changing the original behavior, but concluded that what we have here is for the best. This builds on top of the TAP tests introduced in 5fb5b6c, showing the change in behavior for REINDEX SYSTEM. reindexdb is updated so as we do not issue an extra REINDEX SYSTEM when working on a database in the non-concurrent case, something that was confusing when --concurrently got introduced, so this simplifies the code. Author: Simon Riggs Reviewed-by: Ashutosh Bapat, Bernd Helmle, Álvaro Herrera, Cary Huang, Michael Paquier Discussion: https://postgr.es/m/CANbhV-H=NH6Om4-X6cRjDWfH_Mu1usqwkuYVp-hwdB_PSHWRfg@mail.gmail.com --- doc/src/sgml/ref/reindex.sgml | 12 +++++++----- src/backend/commands/indexcmds.c | 21 ++++++++++++++++----- src/backend/parser/gram.y | 25 ++++++++++++++++++++++++- src/bin/scripts/reindexdb.c | 12 ------------ src/bin/scripts/t/090_reindexdb.pl | 18 ++++-------------- 5 files changed, 51 insertions(+), 37 deletions(-) diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 336ca24b31..fcbda88149 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -21,7 +21,8 @@ PostgreSQL documentation -REINDEX [ ( option [, ...] ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [ CONCURRENTLY ] name +REINDEX [ ( option [, ...] ) ] { INDEX | TABLE | SCHEMA } [ CONCURRENTLY ] name +REINDEX [ ( option [, ...] ) ] { DATABASE | SYSTEM } [ CONCURRENTLY ] [ name ] where option can be one of: @@ -126,8 +127,9 @@ REINDEX [ ( option [, ...] ) ] { IN DATABASE - Recreate all indexes within the current database. - Indexes on shared system catalogs are also processed. + Recreate all indexes within the current database, except system + catalogs. + Indexes on shared system catalogs are not processed. This form of REINDEX cannot be executed inside a transaction block. @@ -154,8 +156,8 @@ REINDEX [ ( option [, ...] ) ] { IN The name of the specific index, table, or database to be reindexed. Index and table names can be schema-qualified. Presently, REINDEX DATABASE and REINDEX SYSTEM - can only reindex the current database, so their parameter must match - the current database's name. + can only reindex the current database. Their parameter is optional, + and it must match the current database's name. diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index ff847579f3..15a57ea9c3 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2877,11 +2877,16 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, bool concurrent_warning = false; bool tablespace_warning = false; - AssertArg(objectName); Assert(objectKind == REINDEX_OBJECT_SCHEMA || objectKind == REINDEX_OBJECT_SYSTEM || objectKind == REINDEX_OBJECT_DATABASE); + /* + * This matches the options enforced by the grammar, where the object name + * is optional for DATABASE and SYSTEM. + */ + AssertArg(objectName || objectKind != REINDEX_OBJECT_SCHEMA); + if (objectKind == REINDEX_OBJECT_SYSTEM && (params->options & REINDEXOPT_CONCURRENTLY) != 0) ereport(ERROR, @@ -2906,13 +2911,13 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, { objectOid = MyDatabaseId; - 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)); } /* @@ -2970,9 +2975,15 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, !isTempNamespace(classtuple->relnamespace)) continue; - /* Check user/system classification, and optionally skip */ + /* + * Check user/system classification. SYSTEM processes all the + * catalogs, and DATABASE processes everything that's not a catalog. + */ if (objectKind == REINDEX_OBJECT_SYSTEM && - !IsSystemClass(relid, classtuple)) + !IsCatalogRelationOid(relid)) + continue; + else if (objectKind == REINDEX_OBJECT_DATABASE && + IsCatalogRelationOid(relid)) continue; /* diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index c018140afe..df5ceea910 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -560,7 +560,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 reindex_target_multitable +%type reindex_target_type reindex_target_multitable reindex_name_optional %type copy_generic_opt_arg copy_generic_opt_arg_list_item %type copy_generic_opt_elem @@ -9115,6 +9115,24 @@ ReindexStmt: makeDefElem("concurrently", NULL, @3)); $$ = (Node *) n; } + | REINDEX reindex_name_optional + { + ReindexStmt *n = makeNode(ReindexStmt); + n->kind = $2; + n->name = NULL; + n->relation = NULL; + n->params = NIL; + $$ = (Node *)n; + } + | REINDEX '(' utility_option_list ')' reindex_name_optional + { + ReindexStmt *n = makeNode(ReindexStmt); + n->kind = $5; + n->name = NULL; + n->relation = NULL; + n->params = $3; + $$ = (Node *)n; + } | REINDEX '(' utility_option_list ')' reindex_target_type opt_concurrently qualified_name { ReindexStmt *n = makeNode(ReindexStmt); @@ -9151,6 +9169,11 @@ reindex_target_multitable: | SYSTEM_P { $$ = REINDEX_OBJECT_SYSTEM; } | DATABASE { $$ = REINDEX_OBJECT_DATABASE; } ; +/* For these options the name is optional */ +reindex_name_optional: + SYSTEM_P { $$ = REINDEX_OBJECT_SYSTEM; } + | DATABASE { $$ = REINDEX_OBJECT_DATABASE; } + ; /***************************************************************************** * diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c index f3b03ec325..0e93a4eeff 100644 --- a/src/bin/scripts/reindexdb.c +++ b/src/bin/scripts/reindexdb.c @@ -360,18 +360,6 @@ reindex_one_database(ConnParams *cparams, ReindexType type, { case REINDEX_DATABASE: - /* - * Database-wide parallel reindex requires special processing. - * If multiple jobs were asked, we have to reindex system - * catalogs first as they cannot be processed in parallel. - */ - if (concurrently) - pg_log_warning("cannot reindex system catalogs concurrently, skipping all"); - else - run_reindex_command(conn, REINDEX_SYSTEM, PQdb(conn), echo, - verbose, concurrently, false, - tablespace); - /* Build a list of relations from the database */ process_list = get_parallel_object_list(conn, process_type, user_list, echo); diff --git a/src/bin/scripts/t/090_reindexdb.pl b/src/bin/scripts/t/090_reindexdb.pl index 41af8554cd..864707ff92 100644 --- a/src/bin/scripts/t/090_reindexdb.pl +++ b/src/bin/scripts/t/090_reindexdb.pl @@ -83,8 +83,8 @@ $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 has changed -pg_constraint|pg_toast.pg_toast__index|relfilenode has changed + 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), 'relfilenode change after REINDEX DATABASE'); @@ -235,11 +235,6 @@ $node->command_fails( $node->command_fails( [ 'reindexdb', '-j', '2', '-i', 'i1', 'postgres' ], 'parallel reindexdb cannot process indexes'); -$node->issues_sql_like( - [ 'reindexdb', '-j', '2', 'postgres' ], - qr/statement:\ REINDEX SYSTEM postgres; -.*statement:\ REINDEX TABLE public\.test1/s, - 'parallel reindexdb for database issues REINDEX SYSTEM first'); # Note that the ordering of the commands is not stable, so the second # command for s2.t2 is not checked after. $node->issues_sql_like( @@ -249,13 +244,8 @@ $node->issues_sql_like( $node->command_ok( [ 'reindexdb', '-j', '2', '-S', 's3' ], 'parallel reindexdb with empty schema'); -$node->command_checks_all( +$node->command_ok( [ 'reindexdb', '-j', '2', '--concurrently', '-d', 'postgres' ], - 0, - [qr/^$/], - [ - qr/^reindexdb: warning: cannot reindex system catalogs concurrently, skipping all/s - ], - 'parallel reindexdb for system with --concurrently skips catalogs'); + 'parallel reindexdb on database, concurrently'); done_testing();