Date: Wed, 28 Jun 2023 16:24:02 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: pgsql-hackers@postgresql.org
Subject: vacuumdb/clusterdb/reindexdb: allow specifying objects to process in
 all databases
Message-ID: <20230628232402.GA1954626@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="5mCyUwZo2JvN/JJP"
Content-Disposition: inline
Archived-At: 
 <https://www.postgresql.org/message-id/20230628232402.GA1954626%40nathanxps13>
Precedence: bulk


--5mCyUwZo2JvN/JJP
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

While working on some other patches, I found myself wanting to use the
following command to vacuum the catalogs in all databases in a cluster:

	vacuumdb --all --schema pg_catalog

However, this presently fails with the following error:

	cannot vacuum specific schema(s) in all databases

AFAICT there no technical reason to block this, and the resulting behavior
feels intuitive to me, so I wrote 0001 to allow it.  0002 allows specifying
tables to process in all databases in clusterdb, and 0003 allows specifying
tables, indexes, schemas, or the system catalogs to process in all
databases in reindexdb.

I debated also allowing users to specify different types of objects in the
same command (e.g., "vacuumdb --schema myschema --table mytable"), but it
looked like this would require a more substantial rewrite, and I didn't
feel that the behavior was intuitive.  For the example I just gave, does
the user expect us to process both the "myschema" schema and the "mytable"
table, or does the user want us to process the "mytable" table in the
"myschema" schema?  In vacuumdb, this is already blocked, but reindexdb
accepts combinations of tables, schemas, and indexes (yet disallows
specifying --system along with other types of objects).  Since this is
inconsistent with vacuumdb and IMO ambiguous, I've restricted such
combinations in 0003.

Thoughts?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--5mCyUwZo2JvN/JJP
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0001-vacuumdb-allow-specifying-tables-or-schemas-to-pr.patch"

From 272375cee9214da54f423241b5bee7b8a1f8faa3 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 28 Jun 2023 15:12:18 -0700
Subject: [PATCH v1 1/3] vacuumdb: allow specifying tables or schemas to
 process in all databases

---
 src/bin/scripts/t/100_vacuumdb.pl | 26 +++++++++++++-------------
 src/bin/scripts/vacuumdb.c        | 19 +++++--------------
 2 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index eccfcc54a1..43fba676f1 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -161,7 +161,7 @@ $node->issues_sql_like(
 	'vacuumdb --schema');
 $node->issues_sql_like(
 	[ 'vacuumdb', '--exclude-schema', '"Foo"', 'postgres' ],
-	qr/(?:(?!VACUUM "Foo".bar).)*/,
+	qr/(?:(?!VACUUM \(SKIP_DATABASE_STATS\) "Foo".bar).)*/,
 	'vacuumdb --exclude-schema');
 $node->command_fails_like(
 	[ 'vacuumdb', '-N', 'pg_catalog', '-t', 'pg_class', 'postgres', ],
@@ -175,18 +175,18 @@ $node->command_fails_like(
 	[ 'vacuumdb', '-n', 'pg_catalog', '-N', '"Foo"', 'postgres' ],
 	qr/cannot vacuum all tables in schema\(s\) and exclude schema\(s\) at the same time/,
 	'cannot use options -n and -N at the same time');
-$node->command_fails_like(
-	[ 'vacuumdb', '-a', '-N', '"Foo"' ],
-	qr/cannot exclude specific schema\(s\) in all databases/,
-	'cannot use options -a and -N at the same time');
-$node->command_fails_like(
-	[ 'vacuumdb', '-a', '-n', '"Foo"' ],
-	qr/cannot vacuum specific schema\(s\) in all databases/,
-	'cannot use options -a and -n at the same time');
-$node->command_fails_like(
-	[ 'vacuumdb', '-a', '-t', '"Foo".bar' ],
-	qr/cannot vacuum specific table\(s\) in all databases/,
-	'cannot use options -a and -t at the same time');
+$node->issues_sql_like(
+	[ 'vacuumdb', '-a', '-N', 'pg_catalog' ],
+	qr/(?:(?!VACUUM \(SKIP_DATABASE_STATS\) pg_catalog.pg_class).)*/,
+	'vacuumdb -a -N');
+$node->issues_sql_like(
+	[ 'vacuumdb', '-a', '-n', 'pg_catalog' ],
+	qr/VACUUM \(SKIP_DATABASE_STATS\) pg_catalog.pg_class/,
+	'vacuumdb -a -n');
+$node->issues_sql_like(
+	[ 'vacuumdb', '-a', '-t', 'pg_class' ],
+	qr/VACUUM \(SKIP_DATABASE_STATS\) pg_catalog.pg_class/,
+	'vacuumdb -a -t');
 $node->command_fails_like(
 	[ 'vacuumdb', '-a', '-d', 'postgres' ],
 	qr/cannot vacuum all databases and a specific one at the same time/,
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 4b17a07089..d7f4871198 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -72,6 +72,7 @@ static void vacuum_one_database(ConnParams *cparams,
 static void vacuum_all_databases(ConnParams *cparams,
 								 vacuumingOptions *vacopts,
 								 bool analyze_in_stages,
+								 SimpleStringList *objects,
 								 int concurrentCons,
 								 const char *progname, bool echo, bool quiet);
 
@@ -376,6 +377,7 @@ main(int argc, char *argv[])
 
 		vacuum_all_databases(&cparams, &vacopts,
 							 analyze_in_stages,
+							 &objects,
 							 concurrentCons,
 							 progname, echo, quiet);
 	}
@@ -427,18 +429,6 @@ check_objfilter(void)
 		(objfilter & OBJFILTER_DATABASE))
 		pg_fatal("cannot vacuum all databases and a specific one at the same time");
 
-	if ((objfilter & OBJFILTER_ALL_DBS) &&
-		(objfilter & OBJFILTER_TABLE))
-		pg_fatal("cannot vacuum specific table(s) in all databases");
-
-	if ((objfilter & OBJFILTER_ALL_DBS) &&
-		(objfilter & OBJFILTER_SCHEMA))
-		pg_fatal("cannot vacuum specific schema(s) in all databases");
-
-	if ((objfilter & OBJFILTER_ALL_DBS) &&
-		(objfilter & OBJFILTER_SCHEMA_EXCLUDE))
-		pg_fatal("cannot exclude specific schema(s) in all databases");
-
 	if ((objfilter & OBJFILTER_TABLE) &&
 		(objfilter & OBJFILTER_SCHEMA))
 		pg_fatal("cannot vacuum all tables in schema(s) and specific table(s) at the same time");
@@ -873,6 +863,7 @@ static void
 vacuum_all_databases(ConnParams *cparams,
 					 vacuumingOptions *vacopts,
 					 bool analyze_in_stages,
+					 SimpleStringList *objects,
 					 int concurrentCons,
 					 const char *progname, bool echo, bool quiet)
 {
@@ -905,7 +896,7 @@ vacuum_all_databases(ConnParams *cparams,
 
 				vacuum_one_database(cparams, vacopts,
 									stage,
-									NULL,
+									objects,
 									concurrentCons,
 									progname, echo, quiet);
 			}
@@ -919,7 +910,7 @@ vacuum_all_databases(ConnParams *cparams,
 
 			vacuum_one_database(cparams, vacopts,
 								ANALYZE_NO_STAGE,
-								NULL,
+								objects,
 								concurrentCons,
 								progname, echo, quiet);
 		}
-- 
2.25.1


--5mCyUwZo2JvN/JJP
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0002-clusterdb-allow-specifying-tables-to-process-in-a.patch"

From 69333f395b1a5fafc29150c077763a711b204f97 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 28 Jun 2023 15:12:58 -0700
Subject: [PATCH v1 2/3] clusterdb: allow specifying tables to process in all
 databases

---
 src/bin/scripts/clusterdb.c            | 28 +++++++++++++++++---------
 src/bin/scripts/t/011_clusterdb_all.pl | 11 ++++++++++
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c
index e3585b3272..8af8aa0de8 100644
--- a/src/bin/scripts/clusterdb.c
+++ b/src/bin/scripts/clusterdb.c
@@ -21,8 +21,9 @@
 
 static void cluster_one_database(const ConnParams *cparams, const char *table,
 								 const char *progname, bool verbose, bool echo);
-static void cluster_all_databases(ConnParams *cparams, const char *progname,
-								  bool verbose, bool echo, bool quiet);
+static void cluster_all_databases(ConnParams *cparams, SimpleStringList *tables,
+								  const char *progname, bool verbose, bool echo,
+								  bool quiet);
 static void help(const char *progname);
 
 
@@ -147,12 +148,10 @@ main(int argc, char *argv[])
 		if (dbname)
 			pg_fatal("cannot cluster all databases and a specific one at the same time");
 
-		if (tables.head != NULL)
-			pg_fatal("cannot cluster specific table(s) in all databases");
-
 		cparams.dbname = maintenance_db;
 
-		cluster_all_databases(&cparams, progname, verbose, echo, quiet);
+		cluster_all_databases(&cparams, &tables,
+							  progname, verbose, echo, quiet);
 	}
 	else
 	{
@@ -226,8 +225,9 @@ cluster_one_database(const ConnParams *cparams, const char *table,
 
 
 static void
-cluster_all_databases(ConnParams *cparams, const char *progname,
-					  bool verbose, bool echo, bool quiet)
+cluster_all_databases(ConnParams *cparams, SimpleStringList *tables,
+					  const char *progname, bool verbose, bool echo,
+					  bool quiet)
 {
 	PGconn	   *conn;
 	PGresult   *result;
@@ -249,7 +249,17 @@ cluster_all_databases(ConnParams *cparams, const char *progname,
 
 		cparams->override_dbname = dbname;
 
-		cluster_one_database(cparams, NULL, progname, verbose, echo);
+		if (tables->head != NULL)
+		{
+			SimpleStringListCell *cell;
+
+			for (cell = tables->head; cell; cell = cell->next)
+				cluster_one_database(cparams, cell->val,
+									 progname, verbose, echo);
+		}
+		else
+			cluster_one_database(cparams, NULL,
+								 progname, verbose, echo);
 	}
 
 	PQclear(result);
diff --git a/src/bin/scripts/t/011_clusterdb_all.pl b/src/bin/scripts/t/011_clusterdb_all.pl
index eb904c08c7..4c2e5515f7 100644
--- a/src/bin/scripts/t/011_clusterdb_all.pl
+++ b/src/bin/scripts/t/011_clusterdb_all.pl
@@ -21,4 +21,15 @@ $node->issues_sql_like(
 	qr/statement: CLUSTER.*statement: CLUSTER/s,
 	'cluster all databases');
 
+$node->safe_psql('postgres',
+    'CREATE TABLE test1 (a int); CREATE INDEX test1x ON test1 (a); CLUSTER test1 USING test1x'
+);
+$node->safe_psql('template1',
+    'CREATE TABLE test1 (a int); CREATE INDEX test1x ON test1 (a); CLUSTER test1 USING test1x'
+);
+$node->issues_sql_like(
+	[ 'clusterdb', '-a', '-t', 'test1' ],
+	qr/statement: CLUSTER public\.test1/s,
+	'cluster specific table in all databases');
+
 done_testing();
-- 
2.25.1


--5mCyUwZo2JvN/JJP
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0003-reindexdb-allow-specifying-objects-to-process-in-.patch"

From 1593e2204f13ca7b947bb6e133b1dc23ab95c4c6 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 28 Jun 2023 15:36:28 -0700
Subject: [PATCH v1 3/3] reindexdb: allow specifying objects to process in all
 databases

---
 src/bin/scripts/reindexdb.c            | 118 +++++++++++++++----------
 src/bin/scripts/t/090_reindexdb.pl     |  14 +++
 src/bin/scripts/t/091_reindexdb_all.pl |  20 +++++
 3 files changed, 105 insertions(+), 47 deletions(-)

diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index 3e8f6aca40..282c2cafbd 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -46,7 +46,10 @@ static void reindex_one_database(ConnParams *cparams, ReindexType type,
 static void reindex_all_databases(ConnParams *cparams,
 								  const char *progname, bool echo,
 								  bool quiet, bool verbose, bool concurrently,
-								  int concurrentCons, const char *tablespace);
+								  int concurrentCons, const char *tablespace,
+								  bool syscatalog, SimpleStringList *schemas,
+								  SimpleStringList *tables,
+								  SimpleStringList *indexes);
 static void run_reindex_command(PGconn *conn, ReindexType type,
 								const char *name, bool echo, bool verbose,
 								bool concurrently, bool async,
@@ -203,25 +206,7 @@ main(int argc, char *argv[])
 
 	setup_cancel_handler(NULL);
 
-	if (alldb)
-	{
-		if (dbname)
-			pg_fatal("cannot reindex all databases and a specific one at the same time");
-		if (syscatalog)
-			pg_fatal("cannot reindex all databases and system catalogs at the same time");
-		if (schemas.head != NULL)
-			pg_fatal("cannot reindex specific schema(s) in all databases");
-		if (tables.head != NULL)
-			pg_fatal("cannot reindex specific table(s) in all databases");
-		if (indexes.head != NULL)
-			pg_fatal("cannot reindex specific index(es) in all databases");
-
-		cparams.dbname = maintenance_db;
-
-		reindex_all_databases(&cparams, progname, echo, quiet, verbose,
-							  concurrently, concurrentCons, tablespace);
-	}
-	else if (syscatalog)
+	if (syscatalog)
 	{
 		if (schemas.head != NULL)
 			pg_fatal("cannot reindex specific schema(s) and system catalogs at the same time");
@@ -229,36 +214,41 @@ main(int argc, char *argv[])
 			pg_fatal("cannot reindex specific table(s) and system catalogs at the same time");
 		if (indexes.head != NULL)
 			pg_fatal("cannot reindex specific index(es) and system catalogs at the same time");
+	}
 
-		if (concurrentCons > 1)
-			pg_fatal("cannot use multiple jobs to reindex system catalogs");
-
-		if (dbname == NULL)
-		{
-			if (getenv("PGDATABASE"))
-				dbname = getenv("PGDATABASE");
-			else if (getenv("PGUSER"))
-				dbname = getenv("PGUSER");
-			else
-				dbname = get_user_name_or_exit(progname);
-		}
-
-		cparams.dbname = dbname;
+	if (schemas.head != NULL && tables.head != NULL)
+		pg_fatal("cannot reindex specific schema(s) and specific table(s) at the same time");
+	if (schemas.head != NULL && indexes.head != NULL)
+		pg_fatal("cannot reindex specific schema(s) and specific index(es) at the same time");
+	if (tables.head != NULL && indexes.head != NULL)
+		pg_fatal("cannot reindex specific table(s) and specific index(es) at the same time");
 
-		reindex_one_database(&cparams, REINDEX_SYSTEM, NULL,
-							 progname, echo, verbose,
-							 concurrently, 1, tablespace);
-	}
-	else
+	if (concurrentCons > 1)
 	{
 		/*
 		 * Index-level REINDEX is not supported with multiple jobs as we
 		 * cannot control the concurrent processing of multiple indexes
 		 * depending on the same relation.
 		 */
-		if (concurrentCons > 1 && indexes.head != NULL)
+		if (indexes.head != NULL)
 			pg_fatal("cannot use multiple jobs to reindex indexes");
+		if (syscatalog)
+			pg_fatal("cannot use multiple jobs to reindex system catalogs");
+	}
 
+	if (alldb)
+	{
+		if (dbname)
+			pg_fatal("cannot reindex all databases and a specific one at the same time");
+
+		cparams.dbname = maintenance_db;
+
+		reindex_all_databases(&cparams, progname, echo, quiet, verbose,
+							  concurrently, concurrentCons, tablespace,
+							  syscatalog, &schemas, &tables, &indexes);
+	}
+	else
+	{
 		if (dbname == NULL)
 		{
 			if (getenv("PGDATABASE"))
@@ -271,6 +261,11 @@ main(int argc, char *argv[])
 
 		cparams.dbname = dbname;
 
+		if (syscatalog)
+			reindex_one_database(&cparams, REINDEX_SYSTEM, NULL,
+								 progname, echo, verbose,
+								 concurrently, 1, tablespace);
+
 		if (schemas.head != NULL)
 			reindex_one_database(&cparams, REINDEX_SCHEMA, &schemas,
 								 progname, echo, verbose,
@@ -287,10 +282,11 @@ main(int argc, char *argv[])
 								 concurrently, concurrentCons, tablespace);
 
 		/*
-		 * reindex database only if neither index nor table nor schema is
-		 * specified
+		 * reindex database only if neither index nor table nor schema nor
+		 * system catalogs is specified
 		 */
-		if (indexes.head == NULL && tables.head == NULL && schemas.head == NULL)
+		if (!syscatalog && indexes.head == NULL &&
+			tables.head == NULL && schemas.head == NULL)
 			reindex_one_database(&cparams, REINDEX_DATABASE, NULL,
 								 progname, echo, verbose,
 								 concurrently, concurrentCons, tablespace);
@@ -711,7 +707,9 @@ static void
 reindex_all_databases(ConnParams *cparams,
 					  const char *progname, bool echo, bool quiet, bool verbose,
 					  bool concurrently, int concurrentCons,
-					  const char *tablespace)
+					  const char *tablespace, bool syscatalog,
+					  SimpleStringList *schemas, SimpleStringList *tables,
+					  SimpleStringList *indexes)
 {
 	PGconn	   *conn;
 	PGresult   *result;
@@ -733,9 +731,35 @@ reindex_all_databases(ConnParams *cparams,
 
 		cparams->override_dbname = dbname;
 
-		reindex_one_database(cparams, REINDEX_DATABASE, NULL,
-							 progname, echo, verbose, concurrently,
-							 concurrentCons, tablespace);
+		if (syscatalog)
+			reindex_one_database(cparams, REINDEX_SYSTEM, NULL,
+								 progname, echo, verbose,
+								 concurrently, 1, tablespace);
+
+		if (schemas->head != NULL)
+			reindex_one_database(cparams, REINDEX_SCHEMA, schemas,
+								 progname, echo, verbose,
+								 concurrently, concurrentCons, tablespace);
+
+		if (indexes->head != NULL)
+			reindex_one_database(cparams, REINDEX_INDEX, indexes,
+								 progname, echo, verbose,
+								 concurrently, 1, tablespace);
+
+		if (tables->head != NULL)
+			reindex_one_database(cparams, REINDEX_TABLE, tables,
+								 progname, echo, verbose,
+								 concurrently, concurrentCons, tablespace);
+
+		/*
+		 * reindex database only if neither index nor table nor schema nor
+		 * system catalogs is specified
+		 */
+		if (!syscatalog && indexes->head == NULL &&
+			tables->head == NULL && schemas->head == NULL)
+			reindex_one_database(cparams, REINDEX_DATABASE, NULL,
+								 progname, echo, verbose,
+								 concurrently, concurrentCons, tablespace);
 	}
 
 	PQclear(result);
diff --git a/src/bin/scripts/t/090_reindexdb.pl b/src/bin/scripts/t/090_reindexdb.pl
index b663d0e741..8ccae5ae4a 100644
--- a/src/bin/scripts/t/090_reindexdb.pl
+++ b/src/bin/scripts/t/090_reindexdb.pl
@@ -262,4 +262,18 @@ $node->command_ok(
 	[ 'reindexdb', '-j', '2', '--concurrently', '-d', 'postgres' ],
 	'parallel reindexdb on database, concurrently');
 
+# invalid combinations of objects
+$node->command_fails([ 'reindexdb', '-s', '-t', 'test1' ],
+	'specify both --system and --table');
+$node->command_fails([ 'reindexdb', '-s', '-i', 'test1x' ],
+	'specify both --system and --index');
+$node->command_fails([ 'reindexdb', '-s', '-S', 'pg_catalog' ],
+	'specify both --system and --schema');
+$node->command_fails([ 'reindexdb', '-t', 'test1', '-i', 'test1x' ],
+	'specify both --table and --index');
+$node->command_fails([ 'reindexdb', '-t', 'test1', '-S', 'pg_catalog' ],
+	'specify both --table and --schema');
+$node->command_fails([ 'reindexdb', '-i', 'test1x', '-S', 'pg_catalog' ],
+	'specify both --index and --schema');
+
 done_testing();
diff --git a/src/bin/scripts/t/091_reindexdb_all.pl b/src/bin/scripts/t/091_reindexdb_all.pl
index ac62b9b558..9bf67878ab 100644
--- a/src/bin/scripts/t/091_reindexdb_all.pl
+++ b/src/bin/scripts/t/091_reindexdb_all.pl
@@ -13,9 +13,29 @@ $node->start;
 
 $ENV{PGOPTIONS} = '--client-min-messages=WARNING';
 
+$node->safe_psql('postgres',
+    'CREATE TABLE test1 (a int); CREATE INDEX test1x ON test1 (a);');
+$node->safe_psql('template1',
+    'CREATE TABLE test1 (a int); CREATE INDEX test1x ON test1 (a);');
 $node->issues_sql_like(
 	[ 'reindexdb', '-a' ],
 	qr/statement: REINDEX.*statement: REINDEX/s,
 	'reindex all databases');
+$node->issues_sql_like(
+	[ 'reindexdb', '-a', '-s' ],
+	qr/statement: REINDEX SYSTEM postgres/s,
+	'reindex system catalogs in all databases');
+$node->issues_sql_like(
+	[ 'reindexdb', '-a', '-S', 'public' ],
+	qr/statement: REINDEX SCHEMA public/s,
+	'reindex schema in all databases');
+$node->issues_sql_like(
+	[ 'reindexdb', '-a', '-i', 'test1x' ],
+	qr/statement: REINDEX INDEX public\.test1x/s,
+	'reindex index in all databases');
+$node->issues_sql_like(
+	[ 'reindexdb', '-a', '-t', 'test1' ],
+	qr/statement: REINDEX TABLE public\.test1/s,
+	'reindex table in all databases');
 
 done_testing();
-- 
2.25.1


--5mCyUwZo2JvN/JJP--