Option to dump foreign data in pg_dump

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
48 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Option to dump foreign data in pg_dump

Luis Carril
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?

Cheers
Luis M Carril

pg_dump_foreign_data.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Pavel Stehule
Hi

pá 28. 6. 2019 v 16:50 odesílatel Luis Carril <[hidden email]> 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

Cheers
Luis M Carril
Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Daniel Gustafsson
In reply to this post by Luis Carril
> On 28 Jun 2019, at 16:49, Luis Carril <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Pavel Stehule


pá 28. 6. 2019 v 17:17 odesílatel Daniel Gustafsson <[hidden email]> napsal:
> On 28 Jun 2019, at 16:49, Luis Carril <[hidden email]> 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

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Tom Lane-2
In reply to this post by Daniel Gustafsson
Daniel Gustafsson <[hidden email]> writes:
>> On 28 Jun 2019, at 16:49, Luis Carril <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Pavel Stehule


pá 28. 6. 2019 v 17:30 odesílatel Tom Lane <[hidden email]> napsal:
Daniel Gustafsson <[hidden email]> writes:
>> On 28 Jun 2019, at 16:49, Luis Carril <[hidden email]> 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

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Luis Carril
 >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


Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Daniel Gustafsson
> On 28 Jun 2019, at 17:30, Tom Lane <[hidden email]> 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 <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Luis Carril

> > On 28 Jun 2019, at 19:55, Luis Carril <[hidden email]> 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

0001-Support-foreign-data-in-pg_dump.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Daniel Gustafsson
> On 12 Jul 2019, at 16:08, Luis Carril <[hidden email]> wrote:
>
> > > On 28 Jun 2019, at 19:55, Luis Carril <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Luis Carril
On 15.07.19 12:06, Daniel Gustafsson wrote:
On 12 Jul 2019, at 16:08, Luis Carril <[hidden email]> wrote:

On 28 Jun 2019, at 19:55, Luis Carril <[hidden email]> 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


0001-Support-foreign-data-in-pg_dump-v2.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Surafel Temesgen
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

Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

vignesh C
In reply to this post by Luis Carril
On Mon, Jul 15, 2019 at 6:09 PM Luis Carril <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

vignesh C
On Thu, Sep 19, 2019 at 3:08 PM vignesh C <[hidden email]> wrote:

>
> On Mon, Jul 15, 2019 at 6:09 PM Luis Carril <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Luis Carril
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

0001-Support-foreign-data-in-pg_dump-v3.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Surafel Temesgen


On Fri, Sep 20, 2019 at 6:20 PM Luis Carril <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Luis Carril

On Fri, Sep 20, 2019 at 6:20 PM Luis Carril <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Daniel Gustafsson
In reply to this post by Luis Carril
On 20 Sep 2019, at 17:20, Luis Carril <[hidden email]> 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.

The patch applies to master with a little bit of fuzz and builds without warnings.

Regarding the functionality, it's a bit unfortunate that it works differently
for --inserts dumps and COPY dumps.  As an example, suppose we have a file_fdw
table in CSV format with 10 rows where row 10 is malformed.  The COPY dump will
include 9 rows and exit on an error, the --inserts dump won't include any rows
before the error.  Since the dump fails with an error, one can argue that it
doesn't matter too much, but it's still not good to have such different
behaviors based on program internals.  (the example is of course not terribly
realistic but can be extrapolated from.) Maybe I'm the only one concerned though.

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

I agree that --include-foreign-data conveys the meaning of the option better,
+1 for keeping this.

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

This is where it becomes a bit messy IMO.  Given that there has been a lot of
effort spent on adding test coverage for pg_dump, I think it would be a shame
to add such niche functionality without testing more than the program options.
You are however right that in order to test, it requires a fully functional
FDW.

I took the liberty to add a testcase to the pg_dump TAP tests which includes a
dummy FDW that always return 10 predetermined rows (in order to keep tests
stable).  There is so far just a happy-path test, since I don't want to spend
time until it's deemed of interest (it is adding a lot of code for a small
test), but it at least illustrates how this patch could be tested.  The
attached patch builds on top of yours.

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.

I still believe thats the desired functionality.

Also, a few small nitpicks on the patch:

This should probably be PATTERN instead of SERVER, to match the rest of the
help output:

+   printf(_("  --include-foreign-data=SERVER\n"
+            "                               include data of foreign tables with the named\n"
+            "                               foreign servers in dump\n"));


It would be good to add a comment explaining the rationale for adding
RELKIND_FOREIGN_TABLE to this block, to assist readers:

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

cheers ./daniel






pr-foreign_data_dump_test.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Laurenz Albe
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



Reply | Threaded
Open this post in threaded view
|

Re: Option to dump foreign data in pg_dump

Luis Carril

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




0001-Support-foreign-data-in-pg_dump-v4.patch (18K) Download Attachment
123