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

Thanks, I'll do!

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?

I've added the documentation about the option in the pg_dump page

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;
+

Now it errors if any is an empty string.

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);

Removed, ie if nothing match it throws an error.

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 */

Changed to 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);
+     }
+

Added too

cheers ./daniel

Thanks for the comments!

Cheers
Luis M Carril