Re: pg_dump 9.6 doesn't honour pg_extension_config_dump for sequences

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

> Now that we've got the ability to indicate which pieces of a object
> should be dumped, however, it seems like we could just check in
> checkExtensionMembership() if it's a config table and set DATA then if
> it is, possibly removing the need for processExtensionTables() entirely.

LIkely so.

> That's really future-work, but I wanted to mention my thoughts here in
> case someone wants to work on it, or I might once I've cleared off the
> other things on my plate. For now, I'll go ahead and commit your
> suggested fix, and thanks for including a new regression test to cover
> this.

Thanks. There are still no tests for dumpable tables though.
--
Michael