Option to dump foreign data in pg_dump

Hi

pá 28. 6. 2019 v 16:50 odesílatel Luis Carril <luis.carril@swarm64.com>
napsal:

Hello,
pg_dump ignores the dumping of data in foreign tables
on purpose, this patch makes it optional as the user maybe
wants to manage the data in the foreign servers directly from
Postgres. Opinions?

It has sense for me

Pavel

On 28 Jun 2019, at 16:49, Luis Carril <luis.carril@swarm64.com> wrote:

pg_dump ignores the dumping of data in foreign tables
on purpose, this patch makes it optional as the user maybe
wants to manage the data in the foreign servers directly from
Postgres. Opinions?

Wouldn’t that have the potential to make restores awkward for FDWs that aren’t
writeable? Basically, how can the risk of foot-gunning be minimized to avoid
users ending up with dumps that are hard to restore?

cheers ./daniel

pá 28. 6. 2019 v 17:17 odesílatel Daniel Gustafsson <daniel@yesql.se>
napsal:

On 28 Jun 2019, at 16:49, Luis Carril <luis.carril@swarm64.com> wrote:

pg_dump ignores the dumping of data in foreign tables
on purpose, this patch makes it optional as the user maybe
wants to manage the data in the foreign servers directly from
Postgres. Opinions?

Wouldn’t that have the potential to make restores awkward for FDWs that
aren’t
writeable? Basically, how can the risk of foot-gunning be minimized to
avoid
users ending up with dumps that are hard to restore?

It can be used for migrations, porting, testing (where FDW sources are not
accessible).

pg_dump has not any safeguards against bad usage. But this feature has
sense only if foreign tables are dumped as classic tables - so some special
option is necessary

Pavel

Daniel Gustafsson <daniel@yesql.se> writes:

On 28 Jun 2019, at 16:49, Luis Carril <luis.carril@swarm64.com> wrote:
pg_dump ignores the dumping of data in foreign tables
on purpose, this patch makes it optional as the user maybe
wants to manage the data in the foreign servers directly from
Postgres. Opinions?

Wouldn’t that have the potential to make restores awkward for FDWs that aren’t
writeable?

Yeah, I think the feature as-proposed is a shotgun that's much more likely
to cause problems than solve them. Almost certainly, what people would
really need is the ability to dump individual foreign tables' data not
everything. (I also note that the main reason for "dump everything",
namely to get a guaranteed-consistent snapshot, isn't really valid for
foreign tables anyhow.)

I'm tempted to suggest that the way to approach this is to say that if you
explicitly select some foreign table(s) with "-t", then we'll dump their
data, unless you suppress that with "-s". No new switch needed.

Another way of looking at it, which responds more directly to Daniel's
point about non-writable FDWs, could be to have a switch that says "dump
foreign tables' data if their FDW is one of these".

regards, tom lane

pá 28. 6. 2019 v 17:30 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Daniel Gustafsson <daniel@yesql.se> writes:

On 28 Jun 2019, at 16:49, Luis Carril <luis.carril@swarm64.com> wrote:
pg_dump ignores the dumping of data in foreign tables
on purpose, this patch makes it optional as the user maybe
wants to manage the data in the foreign servers directly from
Postgres. Opinions?

Wouldn’t that have the potential to make restores awkward for FDWs that

aren’t

writeable?

Yeah, I think the feature as-proposed is a shotgun that's much more likely
to cause problems than solve them. Almost certainly, what people would
really need is the ability to dump individual foreign tables' data not
everything. (I also note that the main reason for "dump everything",
namely to get a guaranteed-consistent snapshot, isn't really valid for
foreign tables anyhow.)

I agree so major usage is dumping data. But can be interesting some
transformation from foreign table to classic table (when schema was created
by IMPORT FOREIGN SCHEMA).

I'm tempted to suggest that the way to approach this is to say that if you
explicitly select some foreign table(s) with "-t", then we'll dump their
data, unless you suppress that with "-s". No new switch needed.

Another way of looking at it, which responds more directly to Daniel's
point about non-writable FDWs, could be to have a switch that says "dump
foreign tables' data if their FDW is one of these".

Restoring content of FDW table via pg_restore or psql can be dangerous -
there I see a risk, and can be nice to allow it only with some form of
safeguard.

I think so important questions is motivation for dumping FDW - a) clonning
(has sense for me and it is safe), b) real backup (requires writeable FDW)
- has sense too, but I see a possibility of unwanted problems.

Regards

Pavel

Restoring content of FDW table via pg_restore or psql can be dangerous - there I see a risk, and can be nice to allow it only >with some form of safeguard.

I think so important questions is motivation for dumping FDW - a) clonning (has sense for me and it is safe), b) real backup >(requires writeable FDW) - has sense too, but I see a possibility of unwanted problems.

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. Also I think it is responsability of the user to know if the FDW that are being used are read-only or not.

Cheers
Luis M Carril

On 28 Jun 2019, at 17:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah, I think the feature as-proposed is a shotgun that's much more likely
to cause problems than solve them. Almost certainly, what people would
really need is the ability to dump individual foreign tables' data not
everything. (I also note that the main reason for "dump everything",
namely to get a guaranteed-consistent snapshot, isn't really valid for
foreign tables anyhow.)

I think this is sort of key here, the consistency guarantees are wildly
different. A note about this should perhaps be added to the docs for the
option discussed here?

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.

cheers ./daniel

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.
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.
It would be nice to emit an error if the fdw is read-only, but that information is not available in the catalog.

Cheers
Luis M Carril

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.

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

On 15.07.19 12:06, Daniel Gustafsson wrote:

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

Hi Luis,
Here is a few comment for me

*I suggest the option to be just –foreign-data because if we make it
–include-foreign-data its expected to have –exclude-foreign-data option
too.

*please add test case

* + if (tdinfo->filtercond || tbinfo->relkind == RELKIND_FOREIGN_TABLE)

filter condition is not implemented completely yet so the logic only work
on foreign table so I think its better to handle it separately

* I don’t understand the need for changing SELECT query .we can use the
same SELECT query syntax for both regular table and foreign table

regards

Surafel

On Mon, Jul 15, 2019 at 6:09 PM Luis Carril <luis.carril@swarm64.com> wrote:

On 15.07.19 12:06, Daniel Gustafsson wrote:

Few comments:

As you have specified required_argument in above:
+ {"include-foreign-data", required_argument, NULL, 11},

The below check may not be required:
+ if (strcmp(optarg, "") == 0)
+ {
+ pg_log_error("empty string is not a valid pattern in --include-foreign-data");
+ exit_nicely(1);
+ }

+ if (foreign_servers_include_patterns.head != NULL)
+ {
+ expand_foreign_server_name_patterns(fout, &foreign_servers_include_patterns,
+ &foreign_servers_include_oids);
+ if (foreign_servers_include_oids.head == NULL)
+ fatal("no matching foreign servers were found");
+ }
+

The above check if (foreign_servers_include_oids.head == NULL) may not
be required, as there is a check present inside
expand_foreign_server_name_patterns to handle this error:
+
+ res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+ if (PQntuples(res) == 0)
+ fatal("no matching foreign servers were found for pattern \"%s\"", cell->val);
+

+static void
+expand_foreign_server_name_patterns(Archive *fout,
+ SimpleStringList *patterns,
+ SimpleOidList *oids)
+{
+ PQExpBuffer query;
+ PGresult   *res;
+ SimpleStringListCell *cell;
+ int i;
+
+ if (patterns->head == NULL)
+ return; /* nothing to do */
+

The above check for patterns->head may not be required as similar
check exists before this function is called:
+ if (foreign_servers_include_patterns.head != NULL)
+ {
+ expand_foreign_server_name_patterns(fout, &foreign_servers_include_patterns,
+ &foreign_servers_include_oids);
+ if (foreign_servers_include_oids.head == NULL)
+ fatal("no matching foreign servers were found");
+ }
+

+ /* Skip FOREIGN TABLEs (no data to dump) if not requested explicitly */
+ if (tbinfo->relkind == RELKIND_FOREIGN_TABLE &&
+ (foreign_servers_include_oids.head == NULL ||
+ !simple_oid_list_member(&foreign_servers_include_oids,
tbinfo->foreign_server_oid)))
simple_oid_list_member can be split into two lines

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Thu, Sep 19, 2019 at 3:08 PM vignesh C <vignesh21@gmail.com> wrote:

On Mon, Jul 15, 2019 at 6:09 PM Luis Carril <luis.carril@swarm64.com> wrote:

On 15.07.19 12:06, Daniel Gustafsson wrote:

Few comments:

As you have specified required_argument in above:
+ {"include-foreign-data", required_argument, NULL, 11},

The below check may not be required:
+ if (strcmp(optarg, "") == 0)
+ {
+ pg_log_error("empty string is not a valid pattern in --include-foreign-data");
+ exit_nicely(1);
+ }

+ if (foreign_servers_include_patterns.head != NULL)
+ {
+ expand_foreign_server_name_patterns(fout, &foreign_servers_include_patterns,
+ &foreign_servers_include_oids);
+ if (foreign_servers_include_oids.head == NULL)
+ fatal("no matching foreign servers were found");
+ }
+

The above check if (foreign_servers_include_oids.head == NULL) may not
be required, as there is a check present inside
expand_foreign_server_name_patterns to handle this error:
+
+ res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+ if (PQntuples(res) == 0)
+ fatal("no matching foreign servers were found for pattern \"%s\"", cell->val);
+

+static void
+expand_foreign_server_name_patterns(Archive *fout,
+ SimpleStringList *patterns,
+ SimpleOidList *oids)
+{
+ PQExpBuffer query;
+ PGresult   *res;
+ SimpleStringListCell *cell;
+ int i;
+
+ if (patterns->head == NULL)
+ return; /* nothing to do */
+

The above check for patterns->head may not be required as similar
check exists before this function is called:
+ if (foreign_servers_include_patterns.head != NULL)
+ {
+ expand_foreign_server_name_patterns(fout, &foreign_servers_include_patterns,
+ &foreign_servers_include_oids);
+ if (foreign_servers_include_oids.head == NULL)
+ fatal("no matching foreign servers were found");
+ }
+

+ /* Skip FOREIGN TABLEs (no data to dump) if not requested explicitly */
+ if (tbinfo->relkind == RELKIND_FOREIGN_TABLE &&
+ (foreign_servers_include_oids.head == NULL ||
+ !simple_oid_list_member(&foreign_servers_include_oids,
tbinfo->foreign_server_oid)))
simple_oid_list_member can be split into two lines

Also can we include few tests for this feature.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

Hello,
thanks for the comments!

*I suggest the option to be just –foreign-data because if we make it –include-foreign-data its expected to have –exclude-foreign-data option too.

Several pg_dump options have no counterpart, e.g --enable-row-security does not have a disable (which is the default). Also calling it --foreign-data would sound similar to the --table, by default all tables are dumped, but with --table only the selected tables are dumped. While without --include-foreign-data all data is excluded, and only with the option some foreign data would be included.

*please add test case

I added tests cases for the invalid inputs. I'll try to make a test case for the actual dump of foreign data, but that requires more setup, because a functional fdw is needed there.

* + if (tdinfo->filtercond || tbinfo->relkind == RELKIND_FOREIGN_TABLE)

filter condition is not implemented completely yet so the logic only work on foreign table so I think its better to handle it separately

Note that there is another if condition that actually applies the the filtercondition if provided, also for a foreign table we need to do a COPY SELECT instead of a COPY TO

* I don’t understand the need for changing SELECT query .we can use the same SELECT query syntax for both regular table and foreign table

To which query do you refer? In the patch there are three queries: 1 retrieves foreign servers, another is the SELECT in the COPY that now it applies in case of a filter condition of a foreign table, and a third that retrieves the oid of a given foreign server.

As you have specified required_argument in above:
+ {"include-foreign-data", required_argument, NULL, 11},

The below check may not be required:
+ if (strcmp(optarg, "") == 0)
+ {
+ pg_log_error("empty string is not a valid pattern in --include-foreign-data");
+ exit_nicely(1);
+ }

We need to conserve this check to avoid that the use of '--include-foreign-data=', which would match all foreign servers. And in previous messages it was established that that behavior is too coarse.

+ if (foreign_servers_include_patterns.head != NULL)
+ {
+ expand_foreign_server_name_patterns(fout, &foreign_servers_include_patterns,
+ &foreign_servers_include_oids);
+ if (foreign_servers_include_oids.head == NULL)
+ fatal("no matching foreign servers were found");
+ }
+

The above check if (foreign_servers_include_oids.head == NULL) may not
be required, as there is a check present inside
expand_foreign_server_name_patterns to handle this error:
+
+ res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+ if (PQntuples(res) == 0)
+ fatal("no matching foreign servers were found for pattern \"%s\"", cell->val);
+

Removed

+static void
+expand_foreign_server_name_patterns(Archive *fout,
+ SimpleStringList *patterns,
+ SimpleOidList *oids)
+{
+ PQExpBuffer query;
+ PGresult   *res;
+ SimpleStringListCell *cell;
+ int i;
+
+ if (patterns->head == NULL)
+ return; /* nothing to do */
+

The above check for patterns->head may not be required as similar
check exists before this function is called:
+ if (foreign_servers_include_patterns.head != NULL)
+ {
+ expand_foreign_server_name_patterns(fout, &foreign_servers_include_patterns,
+ &foreign_servers_include_oids);
+ if (foreign_servers_include_oids.head == NULL)
+ fatal("no matching foreign servers were found");
+ }
+

I think that it is better that the function expand_foreign_server_name do not rely on a non-NULL head, so it checks it by itself, and is closer to the other expand_* functions.
Instead I've removed the check before the function is called.

+ /* Skip FOREIGN TABLEs (no data to dump) if not requested explicitly */
+ if (tbinfo->relkind == RELKIND_FOREIGN_TABLE &&
+ (foreign_servers_include_oids.head == NULL ||
+ !simple_oid_list_member(&foreign_servers_include_oids,
tbinfo->foreign_server_oid)))
simple_oid_list_member can be split into two lines

Done

Cheers
Luis M Carril

On Fri, Sep 20, 2019 at 6:20 PM Luis Carril <luis.carril@swarm64.com> wrote:

Hello,
thanks for the comments!

* + if (tdinfo->filtercond || tbinfo->relkind == RELKIND_FOREIGN_TABLE)

filter condition is not implemented completely yet so the logic only work
on foreign table so I think its better to handle it separately

Note that there is another if condition that actually applies the the
filtercondition if provided, also for a we need to do a COPY SELECT instead
of a COPY TO

but we can't supplied where clause in pg_dump yet so filtercondtion is
always NULL and the logic became true only on foreign table.

* I don’t understand the need for changing SELECT query .we can use the
same SELECT query syntax for both regular table and foreign table

To which query do you refer? In the patch there are three queries: 1
retrieves foreign servers, another is the SELECT in the COPY that now it
applies in case of a filter condition of a foreign table, and a third that
retrieves the oid of a given foreign server.

SELECT on COPY

regards
Surafel

On Fri, Sep 20, 2019 at 6:20 PM Luis Carril <luis.carril@swarm64.com<mailto:luis.carril@swarm64.com>> wrote:
Hello,
thanks for the comments!

* + if (tdinfo->filtercond || tbinfo->relkind == RELKIND_FOREIGN_TABLE)

filter condition is not implemented completely yet so the logic only work on foreign table so I think its better to handle it separately

Note that there is another if condition that actually applies the the filtercondition if provided, also for a we need to do a COPY SELECT instead of a COPY TO

but we can't supplied where clause in pg_dump yet so filtercondtion is always NULL and the logic became true only on foreign table.

* I don’t understand the need for changing SELECT query .we can use the same SELECT query syntax for both regular table and foreign table

To which query do you refer? In the patch there are three queries: 1 retrieves foreign servers, another is the SELECT in the COPY that now it applies in case of a filter condition of a foreign table, and a third that retrieves the oid of a given foreign server.

SELECT on COPY

regards
Surafel
If we have a non-foreign table and filtercond is NULL, then we can do a `COPY table columns TO stdout`.
But if the table is foreign, the `COPY foreign-table columns TO stdout` is not supported by Postgres, so we have to do a `COPY (SELECT columns FROM foreign-table) TO sdout`

Now if in any case the filtercond is non-NULL, ie we have a WHERE clause, then for non-foreign and foreign tables we have to do a:
`COPY (SELECT columns FROM table) TO sdout`

So the COPY of a foreign table has to be done using the sub-SELECT just as a non-foreign table with filtercond, not like a non-foreign table without filtercond.

Cheers

Luis M Carril

On Sat, 2019-11-09 at 21:38 +0100, Daniel Gustafsson wrote:

I took a look at this patch again today for a review of the latest version.
While I still think it's a potential footgun due to read-only FDW's, I can see
usecases for having it so I'm mildly +1 on adding it.

I don't feel good about this feature.
pg_dump should not dump any data that are not part of the database
being dumped.

If you restore such a dump, the data will be inserted into the foreign table,
right? Unless someone emptied the remote table first, this will add
duplicated data to that table.
I think that is an unpleasant surprise. I'd expect that if I drop a database
and restore it from a dump, it should be as it was before. This change would
break that assumption.

What are the use cases of a dump with foreign table data?

Unless I misunderstood something there, -1.

Yours,
Laurenz Albe

Hello

a new version of the patch with the tests from Daniel (thanks!) and the nitpicks.

I don't feel good about this feature.
pg_dump should not dump any data that are not part of the database
being dumped.

If you restore such a dump, the data will be inserted into the foreign table,
right? Unless someone emptied the remote table first, this will add
duplicated data to that table.
I think that is an unpleasant surprise. I'd expect that if I drop a database
and restore it from a dump, it should be as it was before. This change would
break that assumption.

What are the use cases of a dump with foreign table data?

Unless I misunderstood something there, -1.

This feature is opt-in so if the user makes dumps of a remote server explicitly by other means, then the user would not need to use these option.

But, not all foreign tables are necessarily in a remote server like the ones referenced by the postgres_fdw.
In FDWs like swarm64da, cstore, citus or timescaledb, the foreign tables are part of your database, and one could expect that a dump of the database includes data from these FDWs.

Cheers

Luis M Carril