Re: pg_dump 9.6 doesn't honour pg_extension_config_dump for sequences

Michael,

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> As far as I can see, the code path to dumpSequenceData() is taken
> correctly for such a sequence. processExtensionTables() creates a
> dataObj using makeTableDataInfo() only for sequences in extensions
> that are dumpable. So I have been wondering what's happening for some
> time, until I noticed that:

> --- a/src/bin/pg_dump/pg_dump.c
> +++ b/src/bin/pg_dump/pg_dump.c
> @@ -15671,7 +15671,7 @@ dumpSequenceData(Archive *fout, TableDataInfo *tdinfo)
> appendPQExpBuffer(query, ", %s, %s);\n",
> last, (called ? "true" : "false"));
>
> - if (tbinfo->dobj.dump & DUMP_COMPONENT_DATA)
> + if (tdinfo->dobj.dump & DUMP_COMPONENT_DATA)
> ArchiveEntry(fout, nilCatalogId, createDumpId(),
> tbinfo->dobj.name,
> tbinfo->dobj.namespace->dobj.name,

> Not sure if I need to laugh or cry. Attached is a patch with the fix
> and new regression tests.

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.

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.

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 again!

Stephen