Le 20/04/2022 à 19:38, Nathan Bossart a écrit :
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