Re: [Proposal] vacuumdb --schema only

Thanks for the new patch!  I think this is on the right track.

On Wed, Apr 20, 2022 at 05:15:02PM +0200, Gilles Darold wrote:

Le 18/04/2022 à 23:56, Nathan Bossart a écrit :

-	if (!tables_listed)
+	if (!objects_listed || objfilter == OBJFILTER_SCHEMA)

Do we need to check for objects_listed here?  IIUC we can just check for
objfilter != OBJFILTER_TABLE.

Yes we need it otherwise test 'vacuumdb with view' fail because we are not
trying to vacuum the view so the PG doesn't report:

    WARNING:  cannot vacuum non-tables or special system tables

My point is that the only time we don't want to filter for relevant
relation types is when the user provides a list of tables.  So my
suggestion would be to simplify this to the following:
	if (objfilter != OBJFILTER_TABLE)	{		appendPQExpBufferStr(...);		has_where = true;	}

Right, I must have gotten mixed up in the test results. Fixed.

Unless I'm missing something, schema_is_exclude appears to only be used for
error checking and doesn't impact the generated catalog query.  It looks
like the relevant logic disappeared after v4 of the patch.

Right, removed.

I don't think -N works at the moment.  I tested it out, and vacuumdb was
still processing tables in schemas I excluded.  Can we add a test case for
this, too?

Fixed and regression tests added as well as some others to test the filter options compatibility.

+/*
+ * Verify that the filters used at command line are compatible
+ */
+void
+check_objfilter(VacObjectFilter curr_objfilter, VacObjectFilter curr_option)
+{
+	switch (curr_option)
+	{
+		case OBJFILTER_NONE:
+			break;
+		case OBJFILTER_DATABASE:
+			/* When filtering on database name, vacuum on all database is not allowed. */
+			if (curr_objfilter == OBJFILTER_ALL)
+				pg_fatal("cannot vacuum all databases and a specific one at the same time");
+			break;
[...]
+	}
+}

I don't think this handles all combinations.  For example, the following
command does not fail:
	vacuumdb -a -N test postgres

Right, I have fix them all in this new patch.

Furthermore, do you think it'd be possible to dynamically generate the
message?  If it doesn't add too much complexity, this might be a nice way
to simplify this function.

I have tried to avoid reusing the same error message several time by using a new enum and function filter_error(). I also use the same messages with --schema and --exclude-schema related errors.

Patch v10 attached.

-- 
Gilles Darold