Re: Option to dump foreign data in pg_dump

> On 12 Jul 2019, at 16:08, Luis Carril <luis.carril@swarm64.com> wrote:
>
> > > On 28 Jun 2019, at 19:55, Luis Carril <luis.carril@swarm64.com> wrote:
> > > What about providing a list of FDW servers instead of an all or nothing option? In that way the user really has
todo a conscious decision to dump the content of the foreign tables for > > a specific server, this would allow
distinctionif 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
ofwriteable 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