Re: bug fix and documentation improvement about vacuumdb

[Kuwamura Masaki]

>
> I've applied this down to v16 now, thanks for the submission!
>

Thanks for pushing!

Masaki Kuwamura

Re: bug fix and documentation improvement about vacuumdb

[Daniel Gustafsson]

> On 24 Sep 2023, at 10:22, Kuwamura Masaki  
> wrote:
> 
> LGTM too!

I've applied this down to v16 now, thanks for the submission!

--
Daniel Gustafsson

Re: bug fix and documentation improvement about vacuumdb

[Kuwamura Masaki]

LGTM too!

>> a bit to make the diff smaller,
I couldn't think from that perspective. Thanks for your update, Daniel-san.

Masaki Kuwamura

Re: bug fix and documentation improvement about vacuumdb

[Nathan Bossart]

On Fri, Sep 22, 2023 at 02:58:20PM +0200, Daniel Gustafsson wrote:
> I had a look at this and tweaked the testcase a bit to make the diff smaller,
> as well as removed the (in some cases) superfluous space in the generated SQL
> query mentioned upthread.  The attached two patches is what I propose we 
> commit
> to fix this, with a backpatch to v16 where it was introduced.

LGTM

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

Re: bug fix and documentation improvement about vacuumdb

[Daniel Gustafsson]

> On 22 Sep 2023, at 11:08, Kuwamura Masaki  
> wrote:
> 
> No worries at all.  If you look at the page now you will see all green
> checkmarks indicating that the patch was tested in CI.  So now we know that
> your tests fail without the fix and work with the fix applied, so all is well.
> 
> Thank you for your kind words! 
> 
> And it seems to me that all concerns are resolved.
> I'll change the patch status to Ready for Committer.
> If you have or find any flaw, let me know that.

I had a look at this and tweaked the testcase a bit to make the diff smaller,
as well as removed the (in some cases) superfluous space in the generated SQL
query mentioned upthread.  The attached two patches is what I propose we commit
to fix this, with a backpatch to v16 where it was introduced.

--
Daniel Gustafsson



v3-0002-vacuumdb-Reword-help-message-for-clarity.patch
Description: Binary data


v3-0001-vacuumdb-Fix-excluding-multiple-schemas-with-N.patch
Description: Binary data

Re: bug fix and documentation improvement about vacuumdb

[Kuwamura Masaki]

>
> No worries at all.  If you look at the page now you will see all green
> checkmarks indicating that the patch was tested in CI.  So now we know that
> your tests fail without the fix and work with the fix applied, so all is
> well.
>

Thank you for your kind words!

And it seems to me that all concerns are resolved.
I'll change the patch status to Ready for Committer.
If you have or find any flaw, let me know that.

Best Regards,

Masaki Kuwamura

Re: bug fix and documentation improvement about vacuumdb

[Daniel Gustafsson]

> On 21 Sep 2023, at 03:53, Kuwamura Masaki  
> wrote:

> When sending an update, please include the previous patch as well with your 
> new
> tests as a 0002 patch in a patchset.  The CFBot can only apply and build/test
> patches when the entire patchset is attached to the email.  The below
> testresults indicate that the patch failed the new tests (which in a way is
> good since without the fix the tests *should* fail), since the fix patch was
> not applied:
> 
> http://cfbot.cputube.org/masaki-kuwamura.html 
> 
> I'm sorry, I didn't know that. I attached both the test and fix patch to this 
> mail.

No worries at all.  If you look at the page now you will see all green
checkmarks indicating that the patch was tested in CI.  So now we know that
your tests fail without the fix and work with the fix applied, so all is well.

--
Daniel Gustafsson

Re: bug fix and documentation improvement about vacuumdb

[Kuwamura Masaki]

>
> I agree.  Supporting pattern matching should, if anyone is interested in
> trying, be done separately in its own thread, no need to move the goalposts
> here. Sorry if I made it sound like so upthread.
>
 I got it.


> When sending an update, please include the previous patch as well with
> your new
> tests as a 0002 patch in a patchset.  The CFBot can only apply and
> build/test
> patches when the entire patchset is attached to the email.  The below
> testresults indicate that the patch failed the new tests (which in a way is
> good since without the fix the tests *should* fail), since the fix patch
> was
> not applied:
>
> http://cfbot.cputube.org/masaki-kuwamura.html
>
I'm sorry, I didn't know that. I attached both the test and fix patch to
this mail.
(The fix patch is clearly Nathan-san's though)
If I'm still in a wrong way, please let me know.

Masaki Kuwamura


v1_vacuumdb_add_tests.patch
Description: Binary data


vacuumdb_fix.patch
Description: Binary data

Re: bug fix and documentation improvement about vacuumdb

[Daniel Gustafsson]

> On 20 Sep 2023, at 11:46, Kuwamura Masaki  
> wrote:

> I think that supporting pattern matching is quite nice.
> But it will be not only tough but also a breaking change, I wonder.
> So I guess this change should be commited either way.

I agree.  Supporting pattern matching should, if anyone is interested in
trying, be done separately in its own thread, no need to move the goalposts
here. Sorry if I made it sound like so upthread.

> The attached patch includes new tests for this bug.
> Also, I fixed the current test for -N option seems to be incorrect.

When sending an update, please include the previous patch as well with your new
tests as a 0002 patch in a patchset.  The CFBot can only apply and build/test
patches when the entire patchset is attached to the email.  The below
testresults indicate that the patch failed the new tests (which in a way is
good since without the fix the tests *should* fail), since the fix patch was
not applied:

http://cfbot.cputube.org/masaki-kuwamura.html

--
Daniel Gustafsson

Re: bug fix and documentation improvement about vacuumdb

[Kuwamura Masaki]

Thank you for all your reviews!

>>> 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.

I think that supporting pattern matching is quite nice.
But it will be not only tough but also a breaking change, I wonder.
So I guess this change should be commited either way.

>>> Yeah, I think we can fix the JOIN as you suggest.  I quickly put a patch
>>> together to demonstrate.
>
> Looks good from a quick skim.

I do agree with this updates. Thank you!

>> We should probably add some tests...
>
> Agreed.

The attached patch includes new tests for this bug.
Also, I fixed the current test for -N option seems to be incorrect.

>>> It seems to work fine. However, if we're aiming for consistent
>>> spacing, the "IS NULL" (two spaces in between) might be an concern.
>>
>> I don't think that's a problem.  I would rather have readable C code and
two
>> spaces in the generated SQL than contorting the C code to produce less
>> whitespace in a query few will read in its generated form.
>
> I think we could pretty easily avoid the extra space and keep the C code
> relatively readable.  These sorts of things bug me, too (see 2af3336).

Though I don't think it affects readability, I'm neutral about this.

>> >> 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.
>
> +1.

That make sense. I retract my suggestion.


v1_vacuumdb_add_tests.patch
Description: Binary data

Re: bug fix and documentation improvement about vacuumdb

[Nathan Bossart]

On Fri, Sep 15, 2023 at 10:13:10AM +0200, Daniel Gustafsson wrote:
>> On 15 Sep 2023, at 04:39, Kyotaro Horiguchi  wrote:
>> It seems to work fine. However, if we're aiming for consistent
>> spacing, the "IS NULL" (two spaces in between) might be an concern.
> 
> I don't think that's a problem.  I would rather have readable C code and two
> spaces in the generated SQL than contorting the C code to produce less
> whitespace in a query few will read in its generated form.

I think we could pretty easily avoid the extra space and keep the C code
relatively readable.  These sorts of things bug me, too (see 2af3336).

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

Re: bug fix and documentation improvement about vacuumdb

[Daniel Gustafsson]

> On 15 Sep 2023, at 04:39, Kyotaro Horiguchi  wrote:
> At Thu, 14 Sep 2023 07:57:57 -0700, Nathan Bossart  
> wrote in 

>> Yeah, I think we can fix the JOIN as you suggest.  I quickly put a patch
>> together to demonstrate.  

Looks good from a quick skim.

>> We should probably add some tests...

Agreed.

> It seems to work fine. However, if we're aiming for consistent
> spacing, the "IS NULL" (two spaces in between) might be an concern.

I don't think that's a problem.  I would rather have readable C code and two
spaces in the generated SQL than contorting the C code to produce less
whitespace in a query few will read in its generated form.

--
Daniel Gustafsson

Re: bug fix and documentation improvement about vacuumdb

[Kyotaro Horiguchi]

At Thu, 14 Sep 2023 07:57:57 -0700, Nathan Bossart  
wrote in 
> On Thu, Sep 14, 2023 at 02:06:51PM +0200, Daniel Gustafsson wrote:
> > 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(_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...

It seems to work fine. However, if we're aiming for consistent
spacing, the "IS NULL" (two spaces in between) might be an concern.

> >> 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.

+1.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: bug fix and documentation improvement about vacuumdb

[Nathan Bossart]

On Thu, Sep 14, 2023 at 02:06:51PM +0200, Daniel Gustafsson wrote:
>> On 14 Sep 2023, at 13:21, Kuwamura Masaki  
>> 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(_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(_query, " JOIN listed_objects"
-			 " ON listed_objects.object_oid ");
-
-		if (objfilter & OBJFILTER_SCHEMA_EXCLUDE)
-			appendPQExpBufferStr(_query, "OPERATOR(pg_catalog.!=) ");
-		else
-			appendPQExpBufferStr(_query, "OPERATOR(pg_catalog.=) ");
+		appendPQExpBufferStr(_query, " LEFT JOIN listed_objects"
+			 " ON listed_objects.object_oid"
+			 " OPERATOR(pg_catalog.=) ");
 
 		if (objfilter & OBJFILTER_TABLE)
 			appendPQExpBufferStr(_query, "c.oid\n");
 		else
 			appendPQExpBufferStr(_query, "ns.oid\n");
+
+		appendPQExpBuffer(_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(_query, " WHERE c.relkind OPERATOR(pg_catalog.=) ANY (array["
-			 CppAsString2(RELKIND_RELATION) ", "
-			 CppAsString2(RELKIND_MATVIEW) "])\n");
+		appendPQExpBuffer(_query,
+		  " %s c.relkind OPERATOR(pg_catalog.=) ANY (array["
+		  CppAsString2(RELKIND_RELATION) ", "
+		  CppAsString2(RELKIND_MATVIEW) "])\n",
+		  has_where ? "AND" : "WHERE");
 		has_where = true;
 	}
 

Re: bug fix and documentation improvement about vacuumdb

[Daniel Gustafsson]

> On 14 Sep 2023, at 13:21, Kuwamura Masaki  
> 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?

> 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(_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.

> 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?

--
Daniel Gustafsson

bug fix and documentation improvement about vacuumdb

[Kuwamura Masaki]

Hi there,

I have 1 trivial fix, 1 bug fix, and 1 suggestion about vacuumdb.

First, I noticed that the help message of `vacuumdb` is a bit incorrect.

`vacuumdb -?` displays the following message
```
...
  -n, --schema=PATTERNvacuum tables in the specified schema(s)
only
  -N, --exclude-schema=PATTERNdo not vacuum tables in the specified
schema(s)

...
```
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.

Second, when we use multiple -N options, vacuumdb runs incorrectly as shown
below.
```
$ psql
=# CREATE SCHEMA s1;
=# CREATE SCHEMA s2;
=# CREATE SCHEMA s3;
=# CREATE TABLE s1.t(i int);
=# CREATE TABLE s2.t(i int);
=# CREATE TABLE s3.t(i int);
=# ALTER SYSTEM SET log_statement TO 'all';
=# SELECT pg_reload_conf();
=# \q
$ vacuumdb -N s1 -N s2
```
We expect that tables in schemas s1 and s2 should not be vacuumed, while
the
others should be. However, logfile says like this.
```
LOG:  statement: VACUUM (SKIP_DATABASE_STATS) pg_catalog.pg_proc;
LOG:  statement: VACUUM (SKIP_DATABASE_STATS) pg_catalog.pg_proc;

...

LOG:  statement: VACUUM (SKIP_DATABASE_STATS) s2.t;
LOG:  statement: VACUUM (SKIP_DATABASE_STATS) s1.t;
LOG:  statement: VACUUM (ONLY_DATABASE_STATS);
```
Even specified by -N, s1.t and s2.t are vacuumed, and also the others are
vacuumed
twice. The attached patch 0002 fixes this.

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.

Thoughts?

Masaki Kuwamura


v1-0001-vacuumdb-Fix-help-message.patch
Description: Binary data


v1-0002-vacuumdb-Fix-bug-multiple-N-switches.patch
Description: Binary data