Thread: [HACKERS] dump a comment of a TSDictionary

Greeting,s

* Giuseppe Broccolo (giuseppe.broccolo@2ndquadrant.it) wrote:
> I've seen that pg_dump execute the dump of an eventual comment of a
> TSDictionary without
> specifying the namespace where it is defined:

Great catch!

> This is actually a problem if a new TSDictionary is created, in a different
> schema specified by
> the dumped search_path setting. I'd propose to change the current call in
> src/bin/pg_dump/pg_dump.c:

Right.  I've got a few other patches queued up for pg_dump, so I'll
take care of this also.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Giuseppe Broccolo (giuseppe.broccolo@2ndquadrant.it) wrote:
>> I've seen that pg_dump execute the dump of an eventual comment of a
>> TSDictionary without specifying the namespace where it is defined:

> Great catch!

One of my smarter CS professors taught me that whenever you find a bug,
you should look around to see where else you made the same mistake.
Checking for other errors of the same ilk is depressingly fruitful :-(
It looks to me like operator classes, operator families, and all four
types of text-search objects have this same error, and have for a long
time.  I'm also wondering if it wouldn't be wise for the dumpComment()
call for procedural languages to use "lanschema", so that the comment
TocEntry matches the parent object in both cases for PL objects.

I'm also kind of wondering why the ArchiveEntry() calls for casts and
transforms specify "pg_catalog" as the schema but we don't do that in
their dumpComment() calls.  Maybe we don't need that hack and should
specify NULL instead.

> Right.  I've got a few other patches queued up for pg_dump, so I'll
> take care of this also.

Do you want to deal with this whole mess, or shall I have a go at it?
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Giuseppe Broccolo (giuseppe.broccolo@2ndquadrant.it) wrote:
> >> I've seen that pg_dump execute the dump of an eventual comment of a
> >> TSDictionary without specifying the namespace where it is defined:
>
> > Great catch!
>
> One of my smarter CS professors taught me that whenever you find a bug,
> you should look around to see where else you made the same mistake.

Indeed, was planning to do so.

> Checking for other errors of the same ilk is depressingly fruitful :-(

Ah, ouch.

> It looks to me like operator classes, operator families, and all four
> types of text-search objects have this same error, and have for a long
> time.  I'm also wondering if it wouldn't be wise for the dumpComment()
> call for procedural languages to use "lanschema", so that the comment
> TocEntry matches the parent object in both cases for PL objects.

That does sound like a good idea..

> I'm also kind of wondering why the ArchiveEntry() calls for casts and
> transforms specify "pg_catalog" as the schema but we don't do that in
> their dumpComment() calls.  Maybe we don't need that hack and should
> specify NULL instead.

Hmmm, probably, though I've not looked specifically.

> > Right.  I've got a few other patches queued up for pg_dump, so I'll
> > take care of this also.
>
> Do you want to deal with this whole mess, or shall I have a go at it?

I'm just about to push the pg_upgrade fixes for large object comments
and security labels, followed not too far behind by the fix for the
public ACL in pg_dump --clean mode.  I have no objection to you handling
these and I can go look at some of the other items on my plate.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Do you want to deal with this whole mess, or shall I have a go at it?

> I'm just about to push the pg_upgrade fixes for large object comments
> and security labels, followed not too far behind by the fix for the
> public ACL in pg_dump --clean mode.  I have no objection to you handling
> these and I can go look at some of the other items on my plate.

OK, I'll wait till you've pushed those, just to avoid any risk of merge
problems.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> Do you want to deal with this whole mess, or shall I have a go at it?
>
> > I'm just about to push the pg_upgrade fixes for large object comments
> > and security labels, followed not too far behind by the fix for the
> > public ACL in pg_dump --clean mode.  I have no objection to you handling
> > these and I can go look at some of the other items on my plate.
>
> OK, I'll wait till you've pushed those, just to avoid any risk of merge
> problems.

Alright, I've pushed the large object fix for pg_upgrade, hopefully the
buildfarm will stay clean.  I'm going to do a bit more testing on the
public ACL/pg_dump --clean patch, but it's also quite a localized patch,
so I think you're good to go now.

Thanks!

Stephen

Giuseppe Broccolo <giuseppe.broccolo@2ndquadrant.it> writes:
> I've seen that pg_dump execute the dump of an eventual comment of a
> TSDictionary without specifying the namespace where it is defined:
> https://github.com/postgres/postgres/blob/master/src/bin/pg_dump/pg_dump.c#L13542

Yup, this is clearly an error --- thanks for spotting it!  I've pushed
a fix for this and related mistakes.

> This is actually a problem if a new TSDictionary is created, in a different
> schema specified by the dumped search_path setting.

Just out of curiosity, do you have a concrete test case where it failed
that way?  AFAICS the emitted SQL would be like

SET search_path = schema1, pg_catalog;

CREATE TEXT SEARCH DICTIONARY somedict (...);

COMMENT ON TEXT SEARCH DICTIONARY somedict IS '...';

SET search_path = schema2, pg_catalog;

CREATE TEXT SEARCH DICTIONARY somedict (...);

COMMENT ON TEXT SEARCH DICTIONARY somedict IS '...';

so it should accidentally work anyway.  It's possible that a parallel
restore would get it wrong, or that a schema-selective restore would
omit comments it should include, but I couldn't reproduce a failure
in simple cases.
        regards, tom lane