v6-0001-Rework-and-add-more-stuff-for-REINDEX-SYSTEM-DATA.patch

From 6c9e6e88dd663f7341b38a787fee5c7a646ff022 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Sun, 17 Jul 2022 15:00:17 +0900
Subject: [PATCH v6] Rework and add more stuff for REINDEX SYSTEM/DATABASE

---
 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 | 88 +++++++++++++++++++++++-------
 doc/src/sgml/ref/reindex.sgml      | 12 ++--
 5 files changed, 114 insertions(+), 44 deletions(-)

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 <defelt>	generic_option_elem alter_generic_option_elem
 %type <list>	generic_option_list alter_generic_option_list
 
-%type <ival>	reindex_target_type reindex_target_multitable
+%type <ival>	reindex_target_type reindex_target_multitable reindex_name_optional
 
 %type <node>	copy_generic_opt_arg copy_generic_opt_arg_list_item
 %type <defelt>	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 398fc4e6bb..f1e3c0fa21 100644
--- a/src/bin/scripts/t/090_reindexdb.pl
+++ b/src/bin/scripts/t/090_reindexdb.pl
@@ -25,11 +25,6 @@ my $tbspace_name = 'reindex_tbspace';
 $node->safe_psql('postgres',
 	"CREATE TABLESPACE $tbspace_name LOCATION '$tbspace_path';");
 
-$node->issues_sql_like(
-	[ 'reindexdb', 'postgres' ],
-	qr/statement: REINDEX DATABASE postgres;/,
-	'SQL REINDEX run');
-
 # Use text as data type to get a toast table.
 $node->safe_psql('postgres',
 	'CREATE TABLE test1 (a text); CREATE INDEX test1x ON test1 (a);');
@@ -40,6 +35,71 @@ my $toast_table = $node->safe_psql('postgres',
 my $toast_index = $node->safe_psql('postgres',
 	"SELECT indexrelid::regclass FROM pg_index WHERE indrelid = '$toast_table'::regclass;"
 );
+my $catalog_toast_index = $node->safe_psql('postgres',
+	"SELECT indexrelid::regclass FROM pg_index WHERE indrelid = '$toast_table'::regclass;"
+);
+
+# Set of SQL queries to cross-check the state of relfilenodes across
+# REINDEX operations.  A set of relfilenodes is saved from the catalogs
+# and then compared with pg_class.
+$node->safe_psql('postgres',
+	'CREATE TABLE toast_relfilenodes (relname regclass, relfilenode oid);');
+# Save the relfilenode of a set of toast indexes, one from the catalog
+# pg_constraint and one from the test table.  This data is used for checks
+# after some of the REINDEX operations done below, checking if they are
+# changed.
+my $fetch_toast_relfilenodes = qq{SELECT c.oid::regclass, c.relfilenode
+  FROM pg_class a
+    JOIN pg_class b ON (a.oid = b.reltoastrelid)
+    JOIN pg_index i on (a.oid = i.indrelid)
+    JOIN pg_class c on (i.indexrelid = c.oid)
+  WHERE b.oid IN ('pg_constraint'::regclass, 'test1'::regclass)};
+# Same for relfilenodes of normal indexes.  This saves the relfilenode
+# from a catalog of pg_constraint, and the one from the test table.
+my $fetch_index_relfilenodes = qq{SELECT oid, relfilenode
+  FROM pg_class
+  WHERE relname IN ('pg_constraint_oid_index', 'test1x')};
+my $save_relfilenodes =
+	"INSERT INTO toast_relfilenodes $fetch_toast_relfilenodes;"
+  . "INSERT INTO toast_relfilenodes $fetch_index_relfilenodes;";
+
+# Query to compare a set of relfilenodes saved with the contents of pg_class.
+# Note that this does not join using OIDs, as CONCURRENTLY would change them
+# when reindexing.  A filter is applied on the toast index names, even if this
+# does not make a difference between the catalog and normal ones, the ordering
+# based on the name is enough to ensure a fixed output.
+my $compare_relfilenodes =
+  qq(SELECT regexp_replace(b.relname::text, '(pg_toast.pg_toast_)\\d{4,5}(_index)', '\\1<oid>\\2'),
+  CASE WHEN a.relfilenode = b.relfilenode THEN 'relfilenode is unchanged'
+    ELSE 'relfilenode has changed' END
+  FROM toast_relfilenodes b
+    JOIN pg_class a ON b.relname::text = a.oid::regclass::text
+  ORDER BY b.relname::text);
+
+# Save the set of relfilenodes and compare them.
+$node->safe_psql('postgres', $save_relfilenodes);
+$node->issues_sql_like(
+	[ 'reindexdb', 'postgres' ],
+	qr/statement: REINDEX DATABASE postgres;/,
+	'SQL REINDEX run');
+my $relnode_info = $node->safe_psql('postgres', $compare_relfilenodes);
+is( $relnode_info, qq(pg_constraint_oid_index|relfilenode is unchanged
+pg_toast.pg_toast_<oid>_index|relfilenode has changed
+pg_toast.pg_toast_<oid>_index|relfilenode is unchanged
+test1x|relfilenode has changed), 'relfilenode change after REINDEX DATABASE');
+
+# Re-save and run the second one.
+$node->safe_psql('postgres',
+	"TRUNCATE toast_relfilenodes; $save_relfilenodes");
+$node->issues_sql_like(
+	[ 'reindexdb', '-s', 'postgres' ],
+	qr/statement: REINDEX SYSTEM postgres;/,
+	'reindex system tables');
+$relnode_info = $node->safe_psql('postgres', $compare_relfilenodes);
+is( $relnode_info, qq(pg_constraint_oid_index|relfilenode has changed
+pg_toast.pg_toast_<oid>_index|relfilenode is unchanged
+pg_toast.pg_toast_<oid>_index|relfilenode has changed
+test1x|relfilenode is unchanged), 'relfilenode change after REINDEX SYSTEM');
 
 $node->issues_sql_like(
 	[ 'reindexdb', '-t', 'test1', 'postgres' ],
@@ -57,10 +117,6 @@ $node->issues_sql_like(
 	[ 'reindexdb', '-S', 'pg_catalog', 'postgres' ],
 	qr/statement: REINDEX SCHEMA pg_catalog;/,
 	'reindex specific schema');
-$node->issues_sql_like(
-	[ 'reindexdb', '-s', 'postgres' ],
-	qr/statement: REINDEX SYSTEM postgres;/,
-	'reindex system tables');
 $node->issues_sql_like(
 	[ 'reindexdb', '-v', '-t', 'test1', 'postgres' ],
 	qr/statement: REINDEX \(VERBOSE\) TABLE public\.test1;/,
@@ -176,11 +232,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(
@@ -190,13 +241,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 with empty schema, concurrently');
 
 done_testing();
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
 
  <refsynopsisdiv>
 <synopsis>
-REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [ CONCURRENTLY ] <replaceable class="parameter">name</replaceable>
+REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { INDEX | TABLE | SCHEMA } [ CONCURRENTLY ] <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>
 
@@ -126,8 +127,9 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
     <term><literal>DATABASE</literal></term>
     <listitem>
      <para>
-      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 <command>REINDEX</command> cannot be executed inside a
       transaction block.
      </para>
@@ -154,8 +156,8 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
       The name of the specific index, table, or database to be
       reindexed.  Index and table names can be schema-qualified.
       Presently, <command>REINDEX DATABASE</command> and <command>REINDEX SYSTEM</command>
-      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.
      </para>
     </listitem>
    </varlistentry>
-- 
2.36.1