Обсуждение: bug fix and documentation improvement about vacuumdb

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

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

> 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

On Thu, Sep 14, 2023 at 02:06:51PM +0200, Daniel Gustafsson wrote:
>> On 14 Sep 2023, at 13:21, Kuwamura Masaki <kuwamura@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

At Thu, 14 Sep 2023 07:57:57 -0700, Nathan Bossart <nathandbossart@gmail.com> 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(&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...

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

> On 15 Sep 2023, at 04:39, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> At Thu, 14 Sep 2023 07:57:57 -0700, Nathan Bossart <nathandbossart@gmail.com> 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

On Fri, Sep 15, 2023 at 10:13:10AM +0200, Daniel Gustafsson wrote:
>> On 15 Sep 2023, at 04:39, Kyotaro Horiguchi <horikyota.ntt@gmail.com> 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

> On 20 Sep 2023, at 11:46, Kuwamura Masaki <kuwamura@db.is.i.nagoya-u.ac.jp> 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

> On 21 Sep 2023, at 03:53, Kuwamura Masaki <kuwamura@db.is.i.nagoya-u.ac.jp> 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 <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

> On 22 Sep 2023, at 11:08, Kuwamura Masaki <kuwamura@db.is.i.nagoya-u.ac.jp> 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

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

> On 24 Sep 2023, at 10:22, Kuwamura Masaki <kuwamura@db.is.i.nagoya-u.ac.jp> wrote:
>
> LGTM too!

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

--
Daniel Gustafsson