Re: Option to dump foreign data in pg_dump

> On 12 Jul 2019, at 16:08, Luis Carril <luis(dot)carril(at)swarm64(dot)com> wrote:
>
> > > On 28 Jun 2019, at 19:55, Luis Carril <luis(dot)carril(at)swarm64(dot)com> wrote:
> > > What about providing a list of FDW servers instead of an all or nothing option? In that way the user really has to do a conscious decision to dump the content of the foreign tables for > > a specific server, this would allow distinction if multiple FDW are being used in the same DB.
>
> > I think this is a good option, the normal exclusion rules can then still apply
> > in case not everything from a specific server is of interest.
>
> Hi, here is a new patch to dump the data of foreign tables using pg_dump.

Cool! Please register this patch in the next commitfest to make sure it
doesn’t get lost on the way. Feel free to mark me as reviewer when adding it.

> This time the user specifies for which foreign servers the data will be dumped, which helps in case of having a mix of writeable and non-writeable fdw in the database.

Looks good, and works as expected.

A few comments on the patch:

Documentation is missing, but you've waited with docs until the functionality
of the patch was fleshed out?

This allows for adding a blanket wildcard with "--include-foreign-data=“ which
includes every foreign server. This seems to go against the gist of the patch,
to require an explicit opt-in per server. Testing for an empty string should
do the trick.

+ case 11: /* include foreign data */
+ simple_string_list_append(&foreign_servers_include_patterns, optarg);
+ break;
+

I don’t think expand_foreign_server_name_patterns should use strict_names, but
rather always consider failures to map as errors.

+ expand_foreign_server_name_patterns(fout, &foreign_servers_include_patterns,
+ &foreign_servers_include_oids,
+ strict_names);

This seems like a bit too ambiguous name, it would be good to indicate in the
name that it refers to a foreign server.

+ Oid serveroid; /* foreign server oid */

As coded there is no warning when asking for foreign data on a schema-only
dump, maybe something like could make usage clearer as this option is similar
in concept to data-only:

+ if (dopt.schemaOnly && foreign_servers_include_patterns.head != NULL)
+ {
+ pg_log_error("options -s/--schema-only and --include-foreign-data cannot be used together");
+ exit_nicely(1);
+ }
+

cheers ./daniel