From: | Stephen Frost <sfrost(at)snowman(dot)net> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Daniele Varrazzo <daniele(dot)varrazzo(at)gmail(dot)com>, pgsql-bugs <pgsql-bugs(at)postgresql(dot)org> |
Subject: | Re: pg_dump 9.6 doesn't honour pg_extension_config_dump for sequences |
Date: | 2017-01-20 01:33:04 |
Message-ID: | 20170120013304.GZ18360@tamriel.snowman.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs |
* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Thu, Jan 19, 2017 at 10:46 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Yeah, that looks like the right answer and matches how dumpTableData
> > handles this, which is also why that's working correctly.
> >
> > I don't particularly like this arrangement though. This works because:
> >
> > getTables() will create the entry for the sequence and call
> > selectDumpableTable(), which calls checkExtensionMembership(), which
> > will set the dump flags to ACL | SECLABEL | POLICY, and then
> > getTableData() will skip adding a data entry for it because DATA isn't
> > set, allowing processExtensionTables() to add the DATA entry later for
> > config tables/sequences.
>
> Yes, I certainly agree with that. When looking at this bug the first
> thing I did was to set DUMP_COMPONENT_DATA in
> checkExtensionMembership() this way actually to see that nothing
> happened.
Nothing happened..? getTableData() should have then created an entry
for it and then dump it out.. If that didn't happen then I'm kind of
curious as to why not.
> It would be also better to have a sanity check like
> (tbinfo->obj.dataObj != NULL && (tbinfo->dump & DUMP_COMPONENT_DATA)
> != 0) in both the sequence and table data dump paths, and use tbinfo
> instead of tdinfo to do this work. This way we check the dependency
> between the DATA flag set and the created dataObj by
> makeTableInfoData().
Well, yes, if we go the route that I suggested and make the tbinfo
actually have the DATA component set for these objects by
checkExtensionMembership(). We can't set that kind of a sanity check
now, of course.
> Thanks. There are still no tests for dumpable tables though.
Agreed. I have a whole slew of additional regression tests slated for
the src/bin/pg_dump/t/ set of tests, I wouldn't complain if someone
wanted to work through adding more regression tests to
src/test/modules/test_pg_dump/t/ for extensions...
Otherwise, I'll get there once I've gotten the code coverage of pg_dump
*without* worrying about extensions up to something more reasonable.
Currently there are far too many cases that aren't covered (as evidenced
by the current rate of bug-fix back-patching that I'm doing..).
Thanks!
Stephen
From | Date | Subject | |
---|---|---|---|
Next Message | Kyotaro HORIGUCHI | 2017-01-20 02:07:29 | Re: Bug in Physical Replication Slots (at least 9.5)? |
Previous Message | Michael Paquier | 2017-01-20 01:25:50 | Re: pg_dump 9.6 doesn't honour pg_extension_config_dump for sequences |