* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Thu, Jan 19, 2017 at 10:46 PM, Stephen Frost <sfrost@snowman.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