On Thu, Sep 14, 2023 at 02:06:51PM +0200, Daniel Gustafsson wrote:
>> On 14 Sep 2023, at 13:21, Kuwamura Masaki <kuwam...@db.is.i.nagoya-u.ac.jp>
>> wrote:
>
>> PATTERN should be changed to SCHEMA because -n and -N options don't support
>> pattern matching for schema names. The attached patch 0001 fixes this.
>
> True, there is no pattern matching performed. I wonder if it's worth lifting
> the pattern matching from pg_dump into common code such that tools like this
> can use it?
I agree that this should be changed to SCHEMA. It might be tough to add
pattern matching with the current catalog query, and I don't know whether
there is demand for such a feature, but I wouldn't discourage someone from
trying.
>> Second, when we use multiple -N options, vacuumdb runs incorrectly as shown
>> below.
>> ...
>
>> Even specified by -N, s1.t and s2.t are vacuumed, and also the others are
>> vacuumed
>> twice. The attached patch 0002 fixes this.
>
> I can reproduce that, a single -N works but adding multiple -N's makes none of
> them excluded. The current coding does this:
>
> if (objfilter & OBJFILTER_SCHEMA_EXCLUDE)
> appendPQExpBufferStr(&catalog_query, "OPERATOR(pg_catalog.!=) ");
>
> If the join is instead made to exclude the oids in listed_objects with a left
> join and a clause on object_oid being null I can make the current query work
> without adding a second clause. I don't have strong feelings wrt if we should
> add a NOT IN () or fix this JOIN, but we shouldn't have a faulty join together
> with the fix. With your patch the existing join is left in place, let's fix
> that.
Yeah, I think we can fix the JOIN as you suggest. I quickly put a patch
together to demonstrate. We should probably add some tests...
>> Third, for the description of the -N option, I wonder if "vacuum all tables
>> except
>> in the specified schema(s)" might be clearer. The current one says nothing
>> about
>> tables not in the specified schema.
>
> Maybe, but the point of vacuumdb is to analyze a database so I'm not sure who
> would expect anything else than vacuuming everything but the excluded schema
> when specifying -N. What else could "vacuumdb -N foo" be interpreted to do
> that can be confusing?
I agree with Daniel on this one.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index f03d5b1c6c..5e05f27462 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -662,18 +662,19 @@ vacuum_one_database(ConnParams *cparams,
/* Used to match the tables or schemas listed by the user */
if (objects_listed)
{
- appendPQExpBufferStr(&catalog_query, " JOIN listed_objects"
- " ON listed_objects.object_oid ");
-
- if (objfilter & OBJFILTER_SCHEMA_EXCLUDE)
- appendPQExpBufferStr(&catalog_query, "OPERATOR(pg_catalog.!=) ");
- else
- appendPQExpBufferStr(&catalog_query, "OPERATOR(pg_catalog.=) ");
+ appendPQExpBufferStr(&catalog_query, " LEFT JOIN listed_objects"
+ " ON listed_objects.object_oid"
+ " OPERATOR(pg_catalog.=) ");
if (objfilter & OBJFILTER_TABLE)
appendPQExpBufferStr(&catalog_query, "c.oid\n");
else
appendPQExpBufferStr(&catalog_query, "ns.oid\n");
+
+ appendPQExpBuffer(&catalog_query,
+ " WHERE listed_objects.object_oid IS %s NULL\n",
+ (objfilter & OBJFILTER_SCHEMA_EXCLUDE) ? "" : "NOT");
+ has_where = true;
}
/*
@@ -684,9 +685,11 @@ vacuum_one_database(ConnParams *cparams,
*/
if ((objfilter & OBJFILTER_TABLE) == 0)
{
- appendPQExpBufferStr(&catalog_query, " WHERE c.relkind OPERATOR(pg_catalog.=) ANY (array["
- CppAsString2(RELKIND_RELATION) ", "
- CppAsString2(RELKIND_MATVIEW) "])\n");
+ appendPQExpBuffer(&catalog_query,
+ " %s c.relkind OPERATOR(pg_catalog.=) ANY (array["
+ CppAsString2(RELKIND_RELATION) ", "
+ CppAsString2(RELKIND_MATVIEW) "])\n",
+ has_where ? "AND" : "WHERE");
has_where = true;
}
