On Fri, Jul 15, 2022 at 06:21:22PM +0100, Simon Riggs wrote: > That's fixed it on the CFbot. Over to you, Michael. Thanks.
Sure. I have looked over that, and this looks fine overall. I have
made two changes though.
if (objectKind == REINDEX_OBJECT_SYSTEM &&
- !IsSystemClass(relid, classtuple))
+ !IsCatalogRelationOid(relid))
+ continue;
+ else if (objectKind == REINDEX_OBJECT_DATABASE &&
+ IsCatalogRelationOid(relid))
The patch originally relied on IsSystemClass() to decide if a relation
is a catalog table or not. This is not wrong in itself because
ReindexMultipleTables() discards RELKIND_TOAST a couple of lines
above, but I think that we should switch to IsCatalogRelationOid() as
that's the line drawn to check for the catalog-ness of a relation.
The second thing is test coverage. Using a REINDEX DATABASE/SYSTEM
within the main regression test suite is not a good idea, but we
already have those commands running in the reindexdb suite so I could
not resist expanding the test section to track and check relfilenode
changes through four relations for these cases:
- Catalog index.
- Catalog toast index.
- User table index.
- User toast index.
The relfilenodes of those relations are saved in a table and
cross-checked with the contents of pg_class after each REINDEX, on
SYSTEM or DATABASE. There are no new heavy commands, so it does not
make the test longer.
With all that, I finish with the attached. Does that look fine to
you?
--
Michael
From 6c9e6e88dd663f7341b38a787fee5c7a646ff022 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@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
signature.asc
Description: PGP signature
