Re: Option to dump foreign data in pg_dump

[Alvaro Herrera]

On 2020-Mar-24, Alvaro Herrera wrote:

> Hmm, but travis is failing on the cfbot, and I can't see why ...

My only guess, without further output, is that getopt_long is not liking
the [ "--include-foreign-data", "xxx" ] style of arguments in the Perl
array of the command to run (which we don't use with anywhere else in
the files I looked), so I changed it to [ "--include-foreign-data=xxx" ].
If this was not the problem, we'll need more info, which the buildfarm
will give us.

And pushed.  Thanks, Luis, and thanks to all reviewers.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Option to dump foreign data in pg_dump

[Alvaro Herrera]

On 2020-Mar-24, Alvaro Herrera wrote:

> On 2020-Mar-23, Alvaro Herrera wrote:
> 
> > COPY public.ft1 (c1, c2, c3) FROM stdin;
> > pg_dump: error: query failed: ERROR:  foreign-data wrapper "dummy" has no 
> > handler
> > pg_dump: error: query was: COPY (SELECT c1, c2, c3 FROM public.ft1 ) TO 
> > stdout;
> > 
> > Maybe what we should do just verify that you do get that error (and no
> > other errors).
> 
> Done that way.  Will be pushing this shortly.

Hmm, but travis is failing on the cfbot, and I can't see why ...

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Option to dump foreign data in pg_dump

[Alvaro Herrera]

On 2020-Mar-23, Alvaro Herrera wrote:

> COPY public.ft1 (c1, c2, c3) FROM stdin;
> pg_dump: error: query failed: ERROR:  foreign-data wrapper "dummy" has no 
> handler
> pg_dump: error: query was: COPY (SELECT c1, c2, c3 FROM public.ft1 ) TO 
> stdout;
> 
> Maybe what we should do just verify that you do get that error (and no
> other errors).

Done that way.  Will be pushing this shortly.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 4c9456992640431c45ed47aec488ac20cae9a4b0 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Fri, 20 Mar 2020 18:42:03 -0300
Subject: [PATCH v9 1/2] pg_dump: Allow dumping data of specific foreign
 servers

Author: Luis Carril
Discussion: https://postgr.es/m/lejpr01mb0185483c0079d2f651b16231e7...@lejpr01mb0185.deuprd01.prod.outlook.de
---
 doc/src/sgml/ref/pg_dump.sgml |  30 ++
 src/bin/pg_dump/pg_dump.c | 110 --
 src/bin/pg_dump/pg_dump.h |   1 +
 3 files changed, 136 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 13bd320b31..a9bc397165 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -767,6 +767,36 @@ PostgreSQL documentation
   
  
 
+ 
+  --include-foreign-data=foreignserver
+  
+   
+Dump the data for any foreign table with a foreign server
+matching foreignserver
+pattern. Multiple foreign servers can be selected by writing multiple
+--include-foreign-data switches.
+Also, the foreignserver parameter is
+interpreted as a pattern according to the same rules used by
+psql's \d commands (see ),
+so multiple foreign servers can also be selected by writing wildcard characters
+in the pattern.  When using wildcards, be careful to quote the pattern
+if needed to prevent the shell from expanding the wildcards; see
+.
+The only exception is that an empty pattern is disallowed.
+   
+
+   
+
+ When --include-foreign-data is specified,
+ pg_dump does not check that the foreign
+ table is writeable.  Therefore, there is no guarantee that the
+ results of a foreign table dump can be successfully restored.
+
+   
+  
+ 
+
  
   --inserts
   
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 959b36a95c..1849dfe3d7 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -119,6 +119,8 @@ static SimpleStringList table_exclude_patterns = {NULL, NULL};
 static SimpleOidList table_exclude_oids = {NULL, NULL};
 static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
 static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
+static SimpleStringList foreign_servers_include_patterns = {NULL, NULL};
+static SimpleOidList foreign_servers_include_oids = {NULL, NULL};
 
 
 /* placeholders for the delimiters for comments */
@@ -153,6 +155,9 @@ static void expand_schema_name_patterns(Archive *fout,
 		SimpleStringList *patterns,
 		SimpleOidList *oids,
 		bool strict_names);
+static void expand_foreign_server_name_patterns(Archive *fout,
+SimpleStringList *patterns,
+SimpleOidList *oids);
 static void expand_table_name_patterns(Archive *fout,
 	   SimpleStringList *patterns,
 	   SimpleOidList *oids,
@@ -385,6 +390,7 @@ main(int argc, char **argv)
 		{"no-sync", no_argument, NULL, 7},
 		{"on-conflict-do-nothing", no_argument, _nothing, 1},
 		{"rows-per-insert", required_argument, NULL, 10},
+		{"include-foreign-data", required_argument, NULL, 11},
 
 		{NULL, 0, NULL, 0}
 	};
@@ -600,6 +606,11 @@ main(int argc, char **argv)
 dopt.dump_inserts = (int) rowsPerInsert;
 break;
 
+			case 11:			/* include foreign data */
+simple_string_list_append(_servers_include_patterns,
+		  optarg);
+break;
+
 			default:
 fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 exit_nicely(1);
@@ -641,6 +652,12 @@ main(int argc, char **argv)
 		exit_nicely(1);
 	}
 
+	if (dopt.schemaOnly && foreign_servers_include_patterns.head != NULL)
+		fatal("options -s/--schema-only and --include-foreign-data cannot be used together");
+
+	if (numWorkers > 1 && foreign_servers_include_patterns.head != NULL)
+		fatal("option --include-foreign-data is not supported with parallel backup");
+
 	if (dopt.dataOnly && dopt.outputClean)
 	{
 		pg_log_error("options -c/--clean and -a/--data-only cannot be used together");
@@ -808,6 +825,9 @@ main(int argc, char **argv)
 			   _exclude_oids,
 			   false);
 
+	expand_foreign_server_name_patterns(fout, _servers_include_patterns,
+		_servers_include_oids);
+
 	/* non-matching exclusion patterns aren't an error */
 
 	/*
@@ -1011,6 +1031,9 @@ help(const char *progname)
 	

Re: Option to dump foreign data in pg_dump

[Daniel Gustafsson]

> On 23 Mar 2020, at 21:40, Alvaro Herrera  wrote:

> I don't understand why this code specifically disallows the empty string
> as an option to --dump-foreign-data.  The other pattern-matching options
> don't do that.  This seems to have been added in response to Daniel's
> review[1], but I don't quite understand the rationale.  No other option
> behaves that way.  I'm inclined to remove that, and I have done so in
> this version.

It was a response to the discussion upthread about not allowing a blanket dump-
everything statement for foreign data, but rather require some form of opt-in.
The empty string made the code wildcard to all foreign data, which was thought
of as being a footgun for creating problematic dumps.

cheers ./daniel

Re: Option to dump foreign data in pg_dump

[Alvaro Herrera]

On 2020-Jan-29, Peter Eisentraut wrote:

> On 2020-01-21 10:36, Luis Carril wrote:
> > > Yes we can support --include-foreign-data without parallel option and
> > > later add support for parallel option as a different patch.
> > 
> >      I've attached a new version of the patch in which an error is
> > emitted if the parallel backup is used with the --include-foreign-data
> > option.
> 
> This seems like an overreaction.  The whole point of lockTableForWorker() is
> to avoid deadlocks, but foreign tables don't have locks, so it's not a
> problem.  I think you can just skip foreign tables in lockTableForWorker()
> using the same logic that getTables() uses.
> 
> I think parallel data dump would be an especially interesting option when
> using foreign tables, so it's worth figuring this out.

I agree it would be nice to implement this, so I tried to implement it.

I found it's not currently workable, because parallel.c only has a tocEntry
to work with, not a DumpableObject, so it doesn't know that the table is
foreign; to find that out, parallel.c could use findObjectByDumpId, but
parallel.c is used by both pg_dump and pg_restore, and findObjectByDumpId is
in common.c which cannot be linked in pg_restore because of numerous
incompatibilities.

One way to make this work would be to put lockTableForWorker somewhere other
than parallel.c.  Foe example maybe have CreateArchive() set up a new "lock
table" ArchiveHandle function ptr that parallel.c can call;
lockTableForWorker() becomes the pg_dump implementation of that, while
pg_restore uses NULL.

Anyway, I think Luis has it right that this should not be a blocker for
this feature.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Option to dump foreign data in pg_dump

[Alvaro Herrera]

v8 attached.

I modified Luis' v7 a little bit by putting the ftserver acquisition in
the main pg_class query instead of adding one separate query for each
foreign table.  That seems better overall.

I don't understand why this code specifically disallows the empty string
as an option to --dump-foreign-data.  The other pattern-matching options
don't do that.  This seems to have been added in response to Daniel's
review[1], but I don't quite understand the rationale.  No other option
behaves that way.  I'm inclined to remove that, and I have done so in
this version.

I removed DumpOptions new bool flag.  Seems pointless; we can just check
that the list is not null, as we do for other such lists.

I split out the proposed test in a different commit; there's no
consensus that this test is acceptable as-is.  Tom proposed a different
strategy[2]; if you try to dump a table with a dummy handler, you'll get
this:

COPY public.ft1 (c1, c2, c3) FROM stdin;
pg_dump: error: query failed: ERROR:  foreign-data wrapper "dummy" has no 
handler
pg_dump: error: query was: COPY (SELECT c1, c2, c3 FROM public.ft1 ) TO stdout;

Maybe what we should do just verify that you do get that error (and no
other errors).

[1] https://postgr.es/m/e9c5b25c-52e4-49ec-9958-69cd5bd14...@yesql.se
[2] https://postgr.es/m/8001.1573759...@sss.pgh.pa.us

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 7e484cb8b4fa9421d2a64fe98fd1c5058c5a5fd0 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Fri, 20 Mar 2020 18:42:03 -0300
Subject: [PATCH v8 1/2] pg_dump: Allow dumping data of specific foreign
 servers

Author: Luis Carril
Discussion: https://postgr.es/m/lejpr01mb0185483c0079d2f651b16231e7...@lejpr01mb0185.deuprd01.prod.outlook.de
---
 doc/src/sgml/ref/pg_dump.sgml |  30 ++
 src/bin/pg_dump/pg_dump.c | 110 --
 src/bin/pg_dump/pg_dump.h |   1 +
 3 files changed, 136 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 13bd320b31..a9bc397165 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -767,6 +767,36 @@ PostgreSQL documentation
   
  
 
+ 
+  --include-foreign-data=foreignserver
+  
+   
+Dump the data for any foreign table with a foreign server
+matching foreignserver
+pattern. Multiple foreign servers can be selected by writing multiple
+--include-foreign-data switches.
+Also, the foreignserver parameter is
+interpreted as a pattern according to the same rules used by
+psql's \d commands (see ),
+so multiple foreign servers can also be selected by writing wildcard characters
+in the pattern.  When using wildcards, be careful to quote the pattern
+if needed to prevent the shell from expanding the wildcards; see
+.
+The only exception is that an empty pattern is disallowed.
+   
+
+   
+
+ When --include-foreign-data is specified,
+ pg_dump does not check that the foreign
+ table is writeable.  Therefore, there is no guarantee that the
+ results of a foreign table dump can be successfully restored.
+
+   
+  
+ 
+
  
   --inserts
   
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 959b36a95c..1849dfe3d7 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -119,6 +119,8 @@ static SimpleStringList table_exclude_patterns = {NULL, NULL};
 static SimpleOidList table_exclude_oids = {NULL, NULL};
 static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
 static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
+static SimpleStringList foreign_servers_include_patterns = {NULL, NULL};
+static SimpleOidList foreign_servers_include_oids = {NULL, NULL};
 
 
 /* placeholders for the delimiters for comments */
@@ -153,6 +155,9 @@ static void expand_schema_name_patterns(Archive *fout,
 		SimpleStringList *patterns,
 		SimpleOidList *oids,
 		bool strict_names);
+static void expand_foreign_server_name_patterns(Archive *fout,
+SimpleStringList *patterns,
+SimpleOidList *oids);
 static void expand_table_name_patterns(Archive *fout,
 	   SimpleStringList *patterns,
 	   SimpleOidList *oids,
@@ -385,6 +390,7 @@ main(int argc, char **argv)
 		{"no-sync", no_argument, NULL, 7},
 		{"on-conflict-do-nothing", no_argument, _nothing, 1},
 		{"rows-per-insert", required_argument, NULL, 10},
+		{"include-foreign-data", required_argument, NULL, 11},
 
 		{NULL, 0, NULL, 0}
 	};
@@ -600,6 +606,11 @@ main(int argc, char **argv)
 dopt.dump_inserts = (int) rowsPerInsert;
 break;
 
+			case 11:			/* include foreign data */
+simple_string_list_append(_servers_include_patterns,
+		  optarg);
+break;
+
 			

Re: Option to dump foreign data in pg_dump

[Alvaro Herrera]

On 2020-Mar-23, Alvaro Herrera wrote:

> > This seems like an overreaction.  The whole point of lockTableForWorker() is
> > to avoid deadlocks, but foreign tables don't have locks, so it's not a
> > problem.  I think you can just skip foreign tables in lockTableForWorker()
> > using the same logic that getTables() uses.
> > 
> > I think parallel data dump would be an especially interesting option when
> > using foreign tables, so it's worth figuring this out.
> 
> I agree it would be nice to implement this, so I tried to implement it.

(Here's patch for this, which of course doesn't compile)

diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index c25e3f7a88..b3000da409 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -1316,17 +1316,33 @@ IsEveryWorkerIdle(ParallelState *pstate)
  * then we know that somebody else has requested an ACCESS EXCLUSIVE lock and
  * so we have a deadlock.  We must fail the backup in that case.
  */
+#include "pg_dump.h"
+#include "catalog/pg_class_d.h"
 static void
 lockTableForWorker(ArchiveHandle *AH, TocEntry *te)
 {
const char *qualId;
PQExpBuffer query;
PGresult   *res;
+   DumpableObject *obj;
 
/* Nothing to do for BLOBS */
if (strcmp(te->desc, "BLOBS") == 0)
return;
 
+   /*
+* Nothing to do for foreign tables either, since they don't support 
LOCK
+* TABLE.
+*/
+   obj = findObjectByDumpId(te->dumpId);
+   if (obj->objType == DO_TABLE_DATA)
+   {
+   TableInfo *tabinfo = (TableInfo *) obj;
+
+   if (tabinfo->relkind == RELKIND_FOREIGN_TABLE)
+   return;
+   }
+
query = createPQExpBuffer();
 
qualId = fmtQualifiedId(te->namespace, te->tag);

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Option to dump foreign data in pg_dump

[David Steele]

Hi Luis,

Please don't top post. Also be careful to quote prior text when 
replying.  Your message was pretty hard to work through -- i.e. figuring 
out what you said vs. what you were replying to.


On 3/5/20 8:51 AM, Luis Carril wrote:


At this point I would like to leave the patch as is, and discuss further 
improvement in a future patch.


I have marked this as Need Review since the author wants the patch 
considered as-is.


I think Ashutosh, at least, has concerns about the patch as it stands, 
but does anyone else want to chime in?


Regards,
--
-David
da...@pgmasters.net

Re: Option to dump foreign data in pg_dump

[Luis Carril]

Hi everyone,

I am just responding on the latest mail on this thread. But the question is 
about functionality. The proposal is to add a single flag 
--include-foreign-data which controls whether or not data is dumped for all the 
foreign tables in a database. That may not serve the purpose. A foreign table 
may point to a view, materialized view or inheritance tree, and so on. A 
database can have foreign tables pointing to all of those kinds. Restoring data 
to a view won't be possible and restoring it into an inheritance tree would 
insert it into the parent only and not the children. Further, a user may not 
want the data to be dumped for all the foreign tables since their usages are 
different esp. considering restore. I think a better option is to extract data 
in a foreign table using --table if that's the only usage. Otherwise, we need a 
foreign table level flag indicating whether pg_dump should dump the data for 
that foreign table or not.

The option enables the user to dump data of tables backed by a specific 
foreign_server. It is up to the user to guarantee that the foreign server is 
also writable, that is the reason to make the option opt-in. The option can be 
combined with --table to dump specific tables if needed. If the user has 
different foreign servers in the database has to make the conscious decision of 
dumping each one of them. Without this option the user is totally unable to do 
it.


> On 2020-01-21 10:36, Luis Carril wrote:
>>> Yes we can support --include-foreign-data without parallel option and
>>> later add support for parallel option as a different patch.
>>
>> Hi,
>>
>>  I've attached a new version of the patch in which an error is
>> emitted if the parallel backup is used with the --include-foreign-data
>> option.
>
> This seems like an overreaction.  The whole point of
> lockTableForWorker() is to avoid deadlocks, but foreign tables don't
> have locks, so it's not a problem.  I think you can just skip foreign
> tables in lockTableForWorker() using the same logic that getTables() uses.
>
> I think parallel data dump would be an especially interesting option
> when using foreign tables, so it's worth figuring this out.

What do you think of Peter's comment?
I took a look at it, we could skip foreign tables by checking the catalog in 
lockTableForWorker but this would imply an extra query per call to the function 
(as in getTables), which would be irrelevant for most of the cases. Or we could 
pass in the TocEntry that it is a foreign table (although that seems highly 
specific).
Also, would it not be possible to offer support of LOCK TABLE on foreign tables?

At this point I would like to leave the patch as is, and discuss further 
improvement in a future patch.

Luis M.


From: Ashutosh Bapat 
Sent: Wednesday, March 4, 2020 5:39 PM
To: David Steele 
Cc: Luis Carril ; vignesh C ; 
Peter Eisentraut ; Alvaro Herrera 
; Daniel Gustafsson ; Laurenz Albe 
; PostgreSQL Hackers 

Subject: Re: Option to dump foreign data in pg_dump

I am just responding on the latest mail on this thread. But the question is 
about functionality. The proposal is to add a single flag 
--include-foreign-data which controls whether or not data is dumped for all the 
foreign tables in a database. That may not serve the purpose. A foreign table 
may point to a view, materialized view or inheritance tree, and so on. A 
database can have foreign tables pointing to all of those kinds. Restoring data 
to a view won't be possible and restoring it into an inheritance tree would 
insert it into the parent only and not the children. Further, a user may not 
want the data to be dumped for all the foreign tables since their usages are 
different esp. considering restore. I think a better option is to extract data 
in a foreign table using --table if that's the only usage. Otherwise, we need a 
foreign table level flag indicating whether pg_dump should dump the data for 
that foreign table or not.

On Wed, Mar 4, 2020 at 12:41 AM David Steele 
mailto:da...@pgmasters.net>> wrote:
Hi Luis,

On 1/29/20 11:05 AM, Peter Eisentraut wrote:
> On 2020-01-21 10:36, Luis Carril wrote:
>>> Yes we can support --include-foreign-data without parallel option and
>>> later add support for parallel option as a different patch.
>>
>> Hi,
>>
>>  I've attached a new version of the patch in which an error is
>> emitted if the parallel backup is used with the --include-foreign-data
>> option.
>
> This seems like an overreaction.  The whole point of
> lockTableForWorker() is to avoid deadlocks, but foreign tables don't
> have locks, so it's not a problem.  I think you can just skip foreign
> tables in lockTableForWorker() using the same logic that getTables() uses.
>
> I think parallel data dump would be an especially inte

Re: Option to dump foreign data in pg_dump

[Ashutosh Bapat]

I am just responding on the latest mail on this thread. But the question is
about functionality. The proposal is to add a single flag
--include-foreign-data which controls whether or not data is dumped for all
the foreign tables in a database. That may not serve the purpose. A foreign
table may point to a view, materialized view or inheritance tree, and so
on. A database can have foreign tables pointing to all of those kinds.
Restoring data to a view won't be possible and restoring it into an
inheritance tree would insert it into the parent only and not the children.
Further, a user may not want the data to be dumped for all the foreign
tables since their usages are different esp. considering restore. I think a
better option is to extract data in a foreign table using --table if that's
the only usage. Otherwise, we need a foreign table level flag indicating
whether pg_dump should dump the data for that foreign table or not.

On Wed, Mar 4, 2020 at 12:41 AM David Steele  wrote:

> Hi Luis,
>
> On 1/29/20 11:05 AM, Peter Eisentraut wrote:
> > On 2020-01-21 10:36, Luis Carril wrote:
> >>> Yes we can support --include-foreign-data without parallel option and
> >>> later add support for parallel option as a different patch.
> >>
> >> Hi,
> >>
> >>  I've attached a new version of the patch in which an error is
> >> emitted if the parallel backup is used with the --include-foreign-data
> >> option.
> >
> > This seems like an overreaction.  The whole point of
> > lockTableForWorker() is to avoid deadlocks, but foreign tables don't
> > have locks, so it's not a problem.  I think you can just skip foreign
> > tables in lockTableForWorker() using the same logic that getTables()
> uses.
> >
> > I think parallel data dump would be an especially interesting option
> > when using foreign tables, so it's worth figuring this out.
>
> What do you think of Peter's comment?
>
> Regards,
> --
> -David
> da...@pgmasters.net
>
>
>

-- 
Best Wishes,
Ashutosh Bapat

Re: Option to dump foreign data in pg_dump

[David Steele]

Hi Luis,

On 1/29/20 11:05 AM, Peter Eisentraut wrote:

On 2020-01-21 10:36, Luis Carril wrote:

Yes we can support --include-foreign-data without parallel option and
later add support for parallel option as a different patch.


Hi,

 I've attached a new version of the patch in which an error is 
emitted if the parallel backup is used with the --include-foreign-data 
option.


This seems like an overreaction.  The whole point of 
lockTableForWorker() is to avoid deadlocks, but foreign tables don't 
have locks, so it's not a problem.  I think you can just skip foreign 
tables in lockTableForWorker() using the same logic that getTables() uses.


I think parallel data dump would be an especially interesting option 
when using foreign tables, so it's worth figuring this out.


What do you think of Peter's comment?

Regards,
--
-David
da...@pgmasters.net

Re: Option to dump foreign data in pg_dump

[vignesh C]

On Wed, Jan 29, 2020 at 2:00 PM Luis Carril  wrote:
>
> Thanks for working on the comments. I noticed one behavior is
> different when --table option is specified. When --table is specified
> the following are not getting dumped:
> CREATE SERVER foreign_server
>
> I felt the above also should be included as part of the dump when
> include-foreign-data option is specified.
>
> Yes, it also happens on master. A dump of a foreign table using --table, 
> which only dumps the table definition, does not include the extension nor the 
> server.
> I guess that the idea behind --table is that the table prerequisites should 
> already exist on the database.
>
> A similar behavior can be reproduced for a non foreign table. If a table is 
> created in a specific schema, dumping only the table with --table does not 
> dump the schema definition.
>
> So I think we do not need to dump the server with the table.
>

Thanks for the clarification, the behavior sounds reasonable to me
unless others have a different opinion on this.

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

Re: Option to dump foreign data in pg_dump

[Peter Eisentraut]

On 2020-01-21 10:36, Luis Carril wrote:

Yes we can support --include-foreign-data without parallel option and
later add support for parallel option as a different patch.


Hi,

     I've attached a new version of the patch in which an error is 
emitted if the parallel backup is used with the --include-foreign-data 
option.


This seems like an overreaction.  The whole point of 
lockTableForWorker() is to avoid deadlocks, but foreign tables don't 
have locks, so it's not a problem.  I think you can just skip foreign 
tables in lockTableForWorker() using the same logic that getTables() uses.


I think parallel data dump would be an especially interesting option 
when using foreign tables, so it's worth figuring this out.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Option to dump foreign data in pg_dump

[Luis Carril]

Thanks for working on the comments. I noticed one behavior is
different when --table option is specified. When --table is specified
the following are not getting dumped:
CREATE SERVER foreign_server

I felt the above also should be included as part of the dump when
include-foreign-data option is specified.

Yes, it also happens on master. A dump of a foreign table using --table, which 
only dumps the table definition, does not include the extension nor the server.
I guess that the idea behind --table is that the table prerequisites should 
already exist on the database.

A similar behavior can be reproduced for a non foreign table. If a table is 
created in a specific schema, dumping only the table with --table does not dump 
the schema definition.

So I think we do not need to dump the server with the table.

Cheers

Luis M Carril

Re: Option to dump foreign data in pg_dump

[vignesh C]

On Tue, Jan 21, 2020 at 3:06 PM Luis Carril  wrote:
>
> Yes we can support --include-foreign-data without parallel option and
> later add support for parallel option as a different patch.
>
> Hi,
>
> I've attached a new version of the patch in which an error is emitted if 
> the parallel backup is used with the --include-foreign-data option.
>

Thanks for working on the comments. I noticed one behavior is
different when --table option is specified. When --table is specified
the following are not getting dumped:
CREATE SERVER foreign_server

I felt the above also should be included as part of the dump when
include-foreign-data option is specified.

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

Re: Option to dump foreign data in pg_dump

[Luis Carril]

Yes we can support --include-foreign-data without parallel option and
later add support for parallel option as a different patch.

Hi,

I've attached a new version of the patch in which an error is emitted if 
the parallel backup is used with the --include-foreign-data option.


Cheers


Luis M. Carril


diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 4bcd4bdaef..319851b936 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -767,6 +767,34 @@ PostgreSQL documentation
   
  
 
+ 
+  --include-foreign-data=foreignserver
+  
+   
+Dump the data for any foreign table with a foreign server
+matching foreignserver
+pattern. Multiple foreign servers can be selected by writing multiple --include-foreign-data.
+Also, the foreignserver parameter is
+interpreted as a pattern according to the same rules used by
+psql's \d commands (see ),
+so multiple foreign servers can also be selected by writing wildcard characters
+in the pattern.  When using wildcards, be careful to quote the pattern
+if needed to prevent the shell from expanding the wildcards;  see
+.
+The only exception is that an empty pattern is disallowed.
+   
+
+   
+
+ When --include-foreign-data is specified, pg_dump
+ does not check if the foreign table is also writeable. Therefore, there is no guarantee that 
+ the results of a foreign table dump can be successfully restored by themselves into a clean database.
+
+   
+  
+ 
+
  
   --inserts
   
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 8c0cedcd98..d255162c7a 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -141,6 +141,7 @@ typedef struct _dumpOptions
 	bool		aclsSkip;
 	const char *lockWaitTimeout;
 	int			dump_inserts;	/* 0 = COPY, otherwise rows per INSERT */
+	bool		include_foreign_data;
 
 	/* flags for various command-line long options */
 	int			disable_dollar_quoting;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 799b6988b7..13156357dd 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -120,6 +120,8 @@ static SimpleStringList table_exclude_patterns = {NULL, NULL};
 static SimpleOidList table_exclude_oids = {NULL, NULL};
 static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
 static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
+static SimpleStringList foreign_servers_include_patterns = {NULL, NULL};
+static SimpleOidList foreign_servers_include_oids = {NULL, NULL};
 
 
 char		g_opaque_type[10];	/* name for the opaque type */
@@ -156,6 +158,9 @@ static void expand_schema_name_patterns(Archive *fout,
 		SimpleStringList *patterns,
 		SimpleOidList *oids,
 		bool strict_names);
+static void expand_foreign_server_name_patterns(Archive *fout,
+		SimpleStringList *patterns,
+		SimpleOidList *oids);
 static void expand_table_name_patterns(Archive *fout,
 	   SimpleStringList *patterns,
 	   SimpleOidList *oids,
@@ -388,6 +393,7 @@ main(int argc, char **argv)
 		{"no-sync", no_argument, NULL, 7},
 		{"on-conflict-do-nothing", no_argument, _nothing, 1},
 		{"rows-per-insert", required_argument, NULL, 10},
+		{"include-foreign-data", required_argument, NULL, 11},
 
 		{NULL, 0, NULL, 0}
 	};
@@ -604,6 +610,13 @@ main(int argc, char **argv)
 dopt.dump_inserts = (int) rowsPerInsert;
 break;
 
+			case 11:/* include foreign data */
+if (strcmp(optarg, "") == 0)
+	fatal("empty string is not a valid pattern in --include-foreign-data");
+simple_string_list_append(_servers_include_patterns, optarg);
+dopt.include_foreign_data = true;
+break;
+
 			default:
 fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 exit_nicely(1);
@@ -645,6 +658,12 @@ main(int argc, char **argv)
 		exit_nicely(1);
 	}
 
+	if (dopt.schemaOnly && foreign_servers_include_patterns.head != NULL)
+		fatal("options -s/--schema-only and --include-foreign-data cannot be used together");
+
+	if (numWorkers > 1 && foreign_servers_include_patterns.head != NULL)
+		fatal("option --include-foreign-data is not supported with parallel backup");
+
 	if (dopt.dataOnly && dopt.outputClean)
 	{
 		pg_log_error("options -c/--clean and -a/--data-only cannot be used together");
@@ -812,6 +831,9 @@ main(int argc, char **argv)
 			   _exclude_oids,
 			   false);
 
+	expand_foreign_server_name_patterns(fout, _servers_include_patterns,
+		_servers_include_oids);
+
 	/* non-matching exclusion patterns aren't an error */
 
 	/*
@@ -1035,6 +1057,9 @@ help(const char *progname)
 	printf(_("  --use-set-session-authorization\n"
 			 "   use SET SESSION AUTHORIZATION commands instead of\n"
 			 "   

Re: Option to dump foreign data in pg_dump

[vignesh C]

On Mon, Jan 20, 2020 at 8:34 PM Luis Carril  wrote:
>
>
> Hi Vignesh,
>
>yes you are right I could reproduce it also with 'file_fdw'. The issue is 
> that LOCK is not supported on foreign tables, so I guess that the safest 
> solution is to make the --include-foreign-data incompatible with --jobs, 
> because skipping the locking for foreign tables maybe can lead to a deadlock 
> anyway. Suggestions?
>

Yes we can support --include-foreign-data without parallel option and
later add support for parallel option as a different patch.

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

Re: Option to dump foreign data in pg_dump

[Luis Carril]

On Tue, Jan 14, 2020 at 5:22 PM Luis Carril 
mailto:luis.car...@swarm64.com>> wrote:
Can you have a look at dump with parallel option. Parallel option will
take a lock on table while invoking lockTableForWorker. May be this is
not required for foreign tables.
Thoughts?
I tried with -j and found no issue. I guess that the foreign table needs 
locking anyway to prevent anyone to modify it while is being dumped.


I'm able to get the problem with the following steps:
Bring up a postgres setup with servers running in 5432 & 5433 port.

Execute the following commands in Server1 configured on 5432 port:

  *   CREATE EXTENSION postgres_fdw;

  *   CREATE SERVER foreign_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS 
(host '127.0.0.1', port '5433', dbname 'postgres');

  *   create user user1 password '123';

  *   alter user user1 with superuser;

  *   CREATE USER MAPPING FOR user1 SERVER foreign_server OPTIONS (user 
'user1', password '123');

Execute the following commands in Server2 configured on 5433 port:

  *   create user user1 password '123';

  *   alter user user1 with superuser;

Execute the following commands in Server2 configured on 5433 port as user1 user:

  *   create schema test;

  *   create table test.test1(id int);

  *   insert into test.test1 values(10);

Execute the following commands in Server1 configured on 5432 port as user1 user:

  *   CREATE FOREIGN TABLE foreign_table1 (id integer NOT NULL) SERVER 
foreign_server OPTIONS (schema_name 'test', table_name 'test1');

Without parallel option, the operation is successful:

  *   ./pg_dump -d postgres -f dumpdir -U user1 -F d  --include-foreign-data 
foreign_server

With parallel option it fails:

  *   ./pg_dump -d postgres -f dumpdir1 -U user1 -F d -j 5 
--include-foreign-data foreign_server

pg_dump: error: could not obtain lock on relation "public.foreign_table1"
This usually means that someone requested an ACCESS EXCLUSIVE lock on the table 
after the pg_dump parent process had gotten the initial ACCESS SHARE lock on 
the table.
pg_dump: error: a worker process died unexpectedly

There may be simpler steps than this to reproduce the issue, i have not try to 
optimize it.

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


Hi Vignesh,

   yes you are right I could reproduce it also with 'file_fdw'. The issue is 
that LOCK is not supported on foreign tables, so I guess that the safest 
solution is to make the --include-foreign-data incompatible with --jobs, 
because skipping the locking for foreign tables maybe can lead to a deadlock 
anyway. Suggestions?

Cheers
Luis M Carril


From: vignesh C 
Sent: Thursday, January 16, 2020 10:01 AM
To: Luis Carril 
Cc: Alvaro Herrera ; Daniel Gustafsson 
; Laurenz Albe ; PostgreSQL Hackers 

Subject: Re: Option to dump foreign data in pg_dump

On Tue, Jan 14, 2020 at 5:22 PM Luis Carril 
mailto:luis.car...@swarm64.com>> wrote:
Can you have a look at dump with parallel option. Parallel option will
take a lock on table while invoking lockTableForWorker. May be this is
not required for foreign tables.
Thoughts?
I tried with -j and found no issue. I guess that the foreign table needs 
locking anyway to prevent anyone to modify it while is being dumped.


I'm able to get the problem with the following steps:
Bring up a postgres setup with servers running in 5432 & 5433 port.

Execute the following commands in Server1 configured on 5432 port:

  *   CREATE EXTENSION postgres_fdw;

  *   CREATE SERVER foreign_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS 
(host '127.0.0.1', port '5433', dbname 'postgres');

  *   create user user1 password '123';

  *   alter user user1 with superuser;

  *   CREATE USER MAPPING FOR user1 SERVER foreign_server OPTIONS (user 
'user1', password '123');

Execute the following commands in Server2 configured on 5433 port:

  *   create user user1 password '123';

  *   alter user user1 with superuser;

Execute the following commands in Server2 configured on 5433 port as user1 user:

  *   create schema test;

  *   create table test.test1(id int);

  *   insert into test.test1 values(10);

Execute the following commands in Server1 configured on 5432 port as user1 user:

  *   CREATE FOREIGN TABLE foreign_table1 (id integer NOT NULL) SERVER 
foreign_server OPTIONS (schema_name 'test', table_name 'test1');

Without parallel option, the operation is successful:

  *   ./pg_dump -d postgres -f dumpdir -U user1 -F d  --include-foreign-data 
foreign_server

With parallel option it fails:

  *   ./pg_dump -d postgres -f dumpdir1 -U user1 -F d -j 5 
--include-foreign-data foreign_server

pg_dump: error: could not obtain lock on relation "public.foreign_table1"
This usually means that someone requested an ACCESS EXCLUSIVE lock on the table 
after the pg_dump parent process had gotten the initial ACCESS SHARE lock on 
the table.
pg_dump: error: a worker process died unexpectedly

There may

Re: Option to dump foreign data in pg_dump

[vignesh C]

On Tue, Jan 14, 2020 at 5:22 PM Luis Carril  wrote:

> Can you have a look at dump with parallel option. Parallel option will
> take a lock on table while invoking lockTableForWorker. May be this is
> not required for foreign tables.
> Thoughts?
>
> I tried with -j and found no issue. I guess that the foreign table needs
> locking anyway to prevent anyone to modify it while is being dumped.
>
>
I'm able to get the problem with the following steps:
Bring up a postgres setup with servers running in 5432 & 5433 port.

Execute the following commands in Server1 configured on 5432 port:

   - CREATE EXTENSION postgres_fdw;


   - CREATE SERVER foreign_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS
   (host '127.0.0.1', port '5433', dbname 'postgres');


   - create user user1 password '123';


   - alter user user1 with superuser;


   - CREATE USER MAPPING FOR user1 SERVER foreign_server OPTIONS (user
   'user1', password '123');


Execute the following commands in Server2 configured on 5433 port:

   - create user user1 password '123';


   - alter user user1 with superuser;

Execute the following commands in Server2 configured on 5433 port as user1
user:

   - create schema test;


   - create table test.test1(id int);


   - insert into test.test1 values(10);


Execute the following commands in Server1 configured on 5432 port as user1
user:

   - CREATE FOREIGN TABLE foreign_table1 (id integer NOT NULL) SERVER
   foreign_server OPTIONS (schema_name 'test', table_name 'test1');


Without parallel option, the operation is successful:

   - ./pg_dump -d postgres -f dumpdir -U user1 -F d  --include-foreign-data
   foreign_server


With parallel option it fails:

   - ./pg_dump -d postgres -f dumpdir1 -U user1 -F d -j 5
   --include-foreign-data foreign_server

pg_dump: error: could not obtain lock on relation "public.foreign_table1"
This usually means that someone requested an ACCESS EXCLUSIVE lock on the
table after the pg_dump parent process had gotten the initial ACCESS SHARE
lock on the table.
pg_dump: error: a worker process died unexpectedly

There may be simpler steps than this to reproduce the issue, i have not try
to optimize it.

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

Re: Option to dump foreign data in pg_dump

[Luis Carril]

Can you have a look at dump with parallel option. Parallel option will
take a lock on table while invoking lockTableForWorker. May be this is
not required for foreign tables.
Thoughts?
I tried with -j and found no issue. I guess that the foreign table needs 
locking anyway to prevent anyone to modify it while is being dumped.

Cheers,

Luis M Carril

From: vignesh C 
Sent: Tuesday, January 14, 2020 1:48 AM
To: Luis Carril 
Cc: Alvaro Herrera ; Daniel Gustafsson 
; Laurenz Albe ; PostgreSQL Hackers 

Subject: Re: Option to dump foreign data in pg_dump

On Fri, Nov 29, 2019 at 2:10 PM Luis Carril  wrote:
>
> Luis,
>
> It seems you've got enough support for this concept, so let's move
> forward with this patch.  There are some comments from Tom about the
> patch; would you like to send an updated version perhaps?
>
> Thanks
>
> Hi,
>
>  I've attached a new version (v6) removing the superfluous JOIN that Tom 
> identified, and not collecting the oids (avoiding the query) if the option is 
> not used at all.
>
> About the testing issues that Tom mentioned:
> I do not see how can we have a pure SQL dummy FDW that tests the 
> functionality. Because the only way to identify if the data of a foreign 
> table for the chosen server is dumped is if the COPY statement appears in the 
> output, but if the C callbacks of the FDW are not implemented, then the 
> SELECT that dumps the data to generate the COPY cannot be executed.
> Also, to test that the include option chooses only the data of the  specified 
> foreign servers we would need some negative testing, i.e. that the COPY 
> statement for the non-desired table does not appear. But I do not find these 
> kind of tests in the test suite, even for other selective options like 
> --table or --exclude-schema.
>

Can you have a look at dump with parallel option. Parallel option will
take a lock on table while invoking lockTableForWorker. May be this is
not required for foreign tables.
Thoughts?

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

Re: Option to dump foreign data in pg_dump

[vignesh C]

On Fri, Nov 29, 2019 at 2:10 PM Luis Carril  wrote:
>
> Luis,
>
> It seems you've got enough support for this concept, so let's move
> forward with this patch.  There are some comments from Tom about the
> patch; would you like to send an updated version perhaps?
>
> Thanks
>
> Hi,
>
>  I've attached a new version (v6) removing the superfluous JOIN that Tom 
> identified, and not collecting the oids (avoiding the query) if the option is 
> not used at all.
>
> About the testing issues that Tom mentioned:
> I do not see how can we have a pure SQL dummy FDW that tests the 
> functionality. Because the only way to identify if the data of a foreign 
> table for the chosen server is dumped is if the COPY statement appears in the 
> output, but if the C callbacks of the FDW are not implemented, then the 
> SELECT that dumps the data to generate the COPY cannot be executed.
> Also, to test that the include option chooses only the data of the  specified 
> foreign servers we would need some negative testing, i.e. that the COPY 
> statement for the non-desired table does not appear. But I do not find these 
> kind of tests in the test suite, even for other selective options like 
> --table or --exclude-schema.
>

Can you have a look at dump with parallel option. Parallel option will
take a lock on table while invoking lockTableForWorker. May be this is
not required for foreign tables.
Thoughts?

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

Re: Option to dump foreign data in pg_dump

[Luis Carril]

Luis,

It seems you've got enough support for this concept, so let's move
forward with this patch.  There are some comments from Tom about the
patch; would you like to send an updated version perhaps?

Thanks
Hi,

 I've attached a new version (v6) removing the superfluous JOIN that Tom 
identified, and not collecting the oids (avoiding the query) if the option is 
not used at all.

About the testing issues that Tom mentioned:
I do not see how can we have a pure SQL dummy FDW that tests the functionality. 
Because the only way to identify if the data of a foreign table for the chosen 
server is dumped is if the COPY statement appears in the output, but if the C 
callbacks of the FDW are not implemented, then the SELECT that dumps the data 
to generate the COPY cannot be executed.
Also, to test that the include option chooses only the data of the  specified 
foreign servers we would need some negative testing, i.e. that the COPY 
statement for the non-desired table does not appear. But I do not find these 
kind of tests in the test suite, even for other selective options like --table 
or --exclude-schema.


Cheers
Luis M Carril


From: Alvaro Herrera 
Sent: Thursday, November 28, 2019 3:31 PM
To: Luis Carril 
Cc: Daniel Gustafsson ; Laurenz Albe 
; vignesh C ; PostgreSQL Hackers 

Subject: Re: Option to dump foreign data in pg_dump

On 2019-Nov-12, Luis Carril wrote:

> The nitpicks have been addressed.  However, it seems that the new file
> containing the test FDW seems missing from the new version of the patch.  Did
> you forget to git add the file?
>
> Yes, I forgot, thanks for noticing. New patch attached again.

Luis,

It seems you've got enough support for this concept, so let's move
forward with this patch.  There are some comments from Tom about the
patch; would you like to send an updated version perhaps?

Thanks

--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 4bcd4bdaef..319851b936 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -767,6 +767,34 @@ PostgreSQL documentation
   
  
 
+ 
+  --include-foreign-data=foreignserver
+  
+   
+Dump the data for any foreign table with a foreign server
+matching foreignserver
+pattern. Multiple foreign servers can be selected by writing multiple --include-foreign-data.
+Also, the foreignserver parameter is
+interpreted as a pattern according to the same rules used by
+psql's \d commands (see ),
+so multiple foreign servers can also be selected by writing wildcard characters
+in the pattern.  When using wildcards, be careful to quote the pattern
+if needed to prevent the shell from expanding the wildcards;  see
+.
+The only exception is that an empty pattern is disallowed.
+   
+
+   
+
+ When --include-foreign-data is specified, pg_dump
+ does not check if the foreign table is also writeable. Therefore, there is no guarantee that 
+ the results of a foreign table dump can be successfully restored by themselves into a clean database.
+
+   
+  
+ 
+
  
   --inserts
   
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 8c0cedcd98..d255162c7a 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -141,6 +141,7 @@ typedef struct _dumpOptions
 	bool		aclsSkip;
 	const char *lockWaitTimeout;
 	int			dump_inserts;	/* 0 = COPY, otherwise rows per INSERT */
+	bool		include_foreign_data;
 
 	/* flags for various command-line long options */
 	int			disable_dollar_quoting;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index bf69adc2f4..c0258c5c99 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -120,6 +120,8 @@ static SimpleStringList table_exclude_patterns = {NULL, NULL};
 static SimpleOidList table_exclude_oids = {NULL, NULL};
 static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
 static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
+static SimpleStringList foreign_servers_include_patterns = {NULL, NULL};
+static SimpleOidList foreign_servers_include_oids = {NULL, NULL};
 
 
 char		g_opaque_type[10];	/* name for the opaque type */
@@ -156,6 +158,9 @@ static void expand_schema_name_patterns(Archive *fout,
 		SimpleStringList *patterns,
 		SimpleOidList *oids,
 		bool strict_names);
+static void expand_foreign_server_name_patterns(Archive *fout,
+		SimpleStringList *patterns,
+		SimpleOidList *oids);
 static void expand_table_name_patterns(Archive *fout,
 	   SimpleStringList *patterns,
 	   SimpleOidList *oids,
@@ -388,6 +393,7 @@ main(int argc, char **argv)
 		{"no-syn

Re: Option to dump foreign data in pg_dump

[Alvaro Herrera]

On 2019-Nov-12, Luis Carril wrote:

> The nitpicks have been addressed.  However, it seems that the new file
> containing the test FDW seems missing from the new version of the patch.  Did
> you forget to git add the file?
> 
> Yes, I forgot, thanks for noticing. New patch attached again.

Luis,

It seems you've got enough support for this concept, so let's move
forward with this patch.  There are some comments from Tom about the
patch; would you like to send an updated version perhaps?

Thanks

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Option to dump foreign data in pg_dump

[Tom Lane]

Alvaro Herrera  writes:
> On 2019-Nov-12, Luis Carril wrote:
>> 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.

> BTW these are not FDWs in the "foreign" sense at all; they're just
> abusing the FDW system in order to be able to store data in some
> different way.  The right thing to do IMO is to port these systems to be
> users of the new storage abstraction (table AM).  If we do that, what
> value is there to the feature being proposed here?

That is a pretty valid point.  I'm not sure however that there would
be *no* use-cases for the proposed option if all of those FDWs were
converted to table AMs.  Also, even if the authors of those systems
are all hard at work on such a conversion, it'd probably be years
before the FDW implementations disappear from the wild.

Having said that, I'm ending up -0.5 or so on the patch as it stands,
mainly because it seems like it is bringing way more maintenance
burden than it's realistically worth.  I'm particularly unhappy about
the proposed regression test additions --- the cycles added to
check-world, and the maintenance effort that's inevitably going to be
needed for all that code, seem unwarranted for something that's at
best a very niche use-case.  And, despite the bulk of the test
additions, they're in no sense offering an end-to-end test, because
that would require successfully reloading the data as well.

That objection could be addressed, perhaps, by scaling down the tests
to just have a goal of exercising the new pg_dump option-handling
code, and not to attempt to do meaningful data extraction from a
foreign table.  You could do that with an entirely dummy foreign data
wrapper and server (cf. sql/foreign_data.sql).  I'm imagining perhaps
create two dummy servers, of which only one has a table, and we ask to
dump data from the other one.  This would cover parsing and validation
of the --include-foreign-data option, and make sure that we don't dump
from servers we're not supposed to.  It doesn't actually dump any
data, but that part is a completely trivial aspect of the patch,
really, and almost all of the code relevant to that does get tested
already.

In the department of minor nitpicks ... why bother with joining to
pg_foreign_server in the query that retrieves a foreign table's
server OID?  ft.ftserver is already the answer you seek.  Also,
I think it'd be wise from a performance standpoint to skip doing
that query altogether in the normal case where --include-foreign-data
hasn't been requested.

regards, tom lane

Re: Option to dump foreign data in pg_dump

[Daniel Gustafsson]

> On 12 Nov 2019, at 15:21, Luis Carril  wrote:
> 
>> The nitpicks have been addressed.  However, it seems that the new file
>> containing the test FDW seems missing from the new version of the patch.  Did
>> you forget to git add the file?
> Yes, I forgot, thanks for noticing. New patch attached again.

The patch applies, compiles and tests clean.  The debate whether we want to
allow dumping of foreign data at all will continue but I am marking the patch
as ready for committer as I believe it is ready for input on that level.

cheers ./daniel

Re: Option to dump foreign data in pg_dump

[Alvaro Herrera]

On 2019-Nov-12, Luis Carril wrote:

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

BTW these are not FDWs in the "foreign" sense at all; they're just
abusing the FDW system in order to be able to store data in some
different way.  The right thing to do IMO is to port these systems to be
users of the new storage abstraction (table AM).  If we do that, what
value is there to the feature being proposed here?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Option to dump foreign data in pg_dump

[Luis Carril]

The nitpicks have been addressed.  However, it seems that the new file
containing the test FDW seems missing from the new version of the patch.  Did
you forget to git add the file?

Yes, I forgot, thanks for noticing. New patch attached again.


Cheers


Luis M Carril

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 4bcd4bdaef..319851b936 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -767,6 +767,34 @@ PostgreSQL documentation
   
  
 
+ 
+  --include-foreign-data=foreignserver
+  
+   
+Dump the data for any foreign table with a foreign server
+matching foreignserver
+pattern. Multiple foreign servers can be selected by writing multiple --include-foreign-data.
+Also, the foreignserver parameter is
+interpreted as a pattern according to the same rules used by
+psql's \d commands (see ),
+so multiple foreign servers can also be selected by writing wildcard characters
+in the pattern.  When using wildcards, be careful to quote the pattern
+if needed to prevent the shell from expanding the wildcards;  see
+.
+The only exception is that an empty pattern is disallowed.
+   
+
+   
+
+ When --include-foreign-data is specified, pg_dump
+ does not check if the foreign table is also writeable. Therefore, there is no guarantee that 
+ the results of a foreign table dump can be successfully restored by themselves into a clean database.
+
+   
+  
+ 
+
  
   --inserts
   
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index bf69adc2f4..31465dec79 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -120,6 +120,8 @@ static SimpleStringList table_exclude_patterns = {NULL, NULL};
 static SimpleOidList table_exclude_oids = {NULL, NULL};
 static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
 static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
+static SimpleStringList foreign_servers_include_patterns = {NULL, NULL};
+static SimpleOidList foreign_servers_include_oids = {NULL, NULL};
 
 
 char		g_opaque_type[10];	/* name for the opaque type */
@@ -156,6 +158,9 @@ static void expand_schema_name_patterns(Archive *fout,
 		SimpleStringList *patterns,
 		SimpleOidList *oids,
 		bool strict_names);
+static void expand_foreign_server_name_patterns(Archive *fout,
+		SimpleStringList *patterns,
+		SimpleOidList *oids);
 static void expand_table_name_patterns(Archive *fout,
 	   SimpleStringList *patterns,
 	   SimpleOidList *oids,
@@ -388,6 +393,7 @@ main(int argc, char **argv)
 		{"no-sync", no_argument, NULL, 7},
 		{"on-conflict-do-nothing", no_argument, _nothing, 1},
 		{"rows-per-insert", required_argument, NULL, 10},
+		{"include-foreign-data", required_argument, NULL, 11},
 
 		{NULL, 0, NULL, 0}
 	};
@@ -604,6 +610,15 @@ main(int argc, char **argv)
 dopt.dump_inserts = (int) rowsPerInsert;
 break;
 
+			case 11:/* include foreign data */
+if (strcmp(optarg, "") == 0)
+{
+	pg_log_error("empty string is not a valid pattern in --include-foreign-data");
+	exit_nicely(1);
+}
+simple_string_list_append(_servers_include_patterns, optarg);
+break;
+
 			default:
 fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 exit_nicely(1);
@@ -645,6 +660,12 @@ main(int argc, char **argv)
 		exit_nicely(1);
 	}
 
+	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);
+	}
+
 	if (dopt.dataOnly && dopt.outputClean)
 	{
 		pg_log_error("options -c/--clean and -a/--data-only cannot be used together");
@@ -812,6 +833,9 @@ main(int argc, char **argv)
 			   _exclude_oids,
 			   false);
 
+	expand_foreign_server_name_patterns(fout, _servers_include_patterns,
+		_servers_include_oids);
+
 	/* non-matching exclusion patterns aren't an error */
 
 	/*
@@ -1035,6 +1059,9 @@ help(const char *progname)
 	printf(_("  --use-set-session-authorization\n"
 			 "   use SET SESSION AUTHORIZATION commands instead of\n"
 			 "   ALTER OWNER commands to set ownership\n"));
+	printf(_("  --include-foreign-data=PATTERN\n"
+			 "   include data of foreign tables with the named\n"
+			 "   foreign servers in dump\n"));
 
 	printf(_("\nConnection options:\n"));
 	printf(_("  -d, --dbname=DBNAME  database to dump\n"));
@@ -1333,6 +1360,53 @@ expand_schema_name_patterns(Archive *fout,
 	destroyPQExpBuffer(query);
 }
 
+/*
+ * Find the OIDs of all foreign servers matching the given list of patterns,
+ * and append them to the given OID list.
+ */
+static void

Re: Option to dump foreign data in pg_dump

[Daniel Gustafsson]

> On 12 Nov 2019, at 12:12, Luis Carril  wrote:

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

The nitpicks have been addressed.  However, it seems that the new file
containing the test FDW seems missing from the new version of the patch.  Did
you forget to git add the file?

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

Right, given the deliberate opt-in which is required I don't see much risk of
unpleasant user surprises.  There are no doubt foot-guns available with this
feature, as has been discussed upthread, but the current proposal is IMHO
minimizing them.

cheers ./daniel

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


diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 4bcd4bdaef..319851b936 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -767,6 +767,34 @@ PostgreSQL documentation
   
  
 
+ 
+  --include-foreign-data=foreignserver
+  
+   
+Dump the data for any foreign table with a foreign server
+matching foreignserver
+pattern. Multiple foreign servers can be selected by writing multiple --include-foreign-data.
+Also, the foreignserver parameter is
+interpreted as a pattern according to the same rules used by
+psql's \d commands (see ),
+so multiple foreign servers can also be selected by writing wildcard characters
+in the pattern.  When using wildcards, be careful to quote the pattern
+if needed to prevent the shell from expanding the wildcards;  see
+.
+The only exception is that an empty pattern is disallowed.
+   
+
+   
+
+ When --include-foreign-data is specified, pg_dump
+ does not check if the foreign table is also writeable. Therefore, there is no guarantee that 
+ the results of a foreign table dump can be successfully restored by themselves into a clean database.
+
+   
+  
+ 
+
  
   --inserts
   
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index bf69adc2f4..31465dec79 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -120,6 +120,8 @@ static SimpleStringList table_exclude_patterns = {NULL, NULL};
 static SimpleOidList table_exclude_oids = {NULL, NULL};
 static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
 static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
+static SimpleStringList foreign_servers_include_patterns = {NULL, NULL};
+static SimpleOidList foreign_servers_include_oids = {NULL, NULL};
 
 
 char		g_opaque_type[10];	/* name for the opaque type */
@@ -156,6 +158,9 @@ static void expand_schema_name_patterns(Archive *fout,
 		SimpleStringList *patterns,
 		SimpleOidList *oids,
 		bool strict_names);
+static void expand_foreign_server_name_patterns(Archive *fout,
+		SimpleStringList *patterns,
+		SimpleOidList *oids);
 static void expand_table_name_patterns(Archive *fout,
 	   SimpleStringList *patterns,
 	   SimpleOidList *oids,
@@ -388,6 +393,7 @@ main(int argc, char **argv)
 		{"no-sync", no_argument, NULL, 7},
 		{"on-conflict-do-nothing", no_argument, _nothing, 1},
 		{"rows-per-insert", required_argument, NULL, 10},
+		{"include-foreign-data", required_argument, NULL, 11},
 
 		{NULL, 0, NULL, 0}
 	};
@@ -604,6 +610,15 @@ main(int argc, char **argv)
 dopt.dump_inserts = (int) rowsPerInsert;
 break;
 
+			case 11:/* include foreign data */
+if (strcmp(optarg, "") == 0)
+{
+	pg_log_error("empty string is not a valid pattern in --include-foreign-data");
+	exit_nicely(1);
+}
+simple_string_list_append(_servers_include_patterns, optarg);
+break;
+
 			default:
 fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 exit_nicely(1);
@@ -645,6 +660,12 @@ main(int argc, char **argv)
 		exit_nicely(1);
 	}
 
+	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);
+	}
+
 	if (dopt.dataOnly && dopt.outputClean)
 	{
 		pg_log_error("options -c/--clean and -a/--data-only cannot be used together");
@@ -812,6 +833,9 @@ main(int argc, char **argv)
 			   _exclude_oids,
 			   false);
 
+	expand_foreign_server_name_patterns(fout, _servers_include_patterns,
+		_servers_include_oids);
+
 	/* non-matching 

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

Re: Option to dump foreign data in pg_dump

[Daniel Gustafsson]

On 20 Sep 2019, at 17:20, Luis Carril  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 seeusecases 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 differentlyfor --inserts dumps and COPY dumps.  As an example, suppose we have a file_fdwtable in CSV format with 10 rows where row 10 is malformed.  The COPY dump willinclude 9 rows and exit on an error, the --inserts dump won't include any rowsbefore the error.  Since the dump fails with an error, one can argue that itdoesn't matter too much, but it's still not good to have such differentbehaviors based on program internals.  (the example is of course not terriblyrealistic 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 ofeffort spent on adding test coverage for pg_dump, I think it would be a shameto add such niche functionality without testing more than the program options.You are however right that in order to test, it requires a fully functionalFDW.I took the liberty to add a testcase to the pg_dump TAP tests which includes adummy FDW that always return 10 predetermined rows (in order to keep testsstable).  There is so far just a happy-path test, since I don't want to spendtime until it's deemed of interest (it is adding a lot of code for a smalltest), but it at least illustrates how this patch could be tested.  Theattached 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 thehelp output:+   printf(_("  --include-foreign-data="">+    "   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 addingRELKIND_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
Description: Binary data

Re: Option to dump foreign data in pg_dump

[Luis Carril]

On Fri, Sep 20, 2019 at 6:20 PM Luis Carril 
mailto:luis.car...@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

Re: Option to dump foreign data in pg_dump

[vignesh C]

On Thu, Sep 19, 2019 at 3:08 PM vignesh C  wrote:
>
> On Mon, Jul 15, 2019 at 6:09 PM Luis Carril  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, _servers_include_patterns,
> + _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, _servers_include_patterns,
> + _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(_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

Re: Option to dump foreign data in pg_dump

[vignesh C]

On Mon, Jul 15, 2019 at 6:09 PM Luis Carril  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, _servers_include_patterns,
+ _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, _servers_include_patterns,
+ _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(_servers_include_oids,
tbinfo->foreign_server_oid)))
simple_oid_list_member can be split into two lines

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

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  wrote:



On 28 Jun 2019, at 19:55, Luis Carril  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(_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, 
_servers_include_patterns,
+   _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
From 7bab80b983cf3ce9e8ecdf7d1a9113d08347e047 Mon Sep 17 00:00:00 2001
From: Luis Carril 
Date: Fri, 28 Jun 2019 16:05:43 +0200
Subject: [PATCH] Support foreign data in pg_dump

---
 doc/src/sgml/ref/pg_dump.sgml |  28 +
 src/bin/pg_dump/pg_dump.c | 115 --
 src/bin/pg_dump/pg_dump.h |   1 +
 3 files changed, 138 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 8fa2314347..db52abe39b 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -767,6 +767,34 @@ PostgreSQL documentation
   
  
 
+ 
+  --include-foreign-data=foreignserver
+  
+   
+Dump the data for any foreign table with a foreign server
+matching foreignserver
+pattern. Multiple foreign servers can be selected by writing multiple --include-foreign-data.
+Also, the foreignserver parameter is
+interpreted as a pattern according to the same rules used by
+psql's \d commands (see ),
+so multiple foreign servers can also be selected by writing wildcard characters
+in the pattern.  When using wildcards, be careful to quote the pattern
+if needed to prevent the shell from expanding the wildcards;  see
+.
+The only exception is that an empty pattern is disallowed.
+   
+
+   
+
+ When --include-foreign-data is specified, pg_dump
+ does not check if the foreign table is also writeable. Therefore, there is no guarantee that 
+ the results of a foreign table dump can be successfully restored by themselves into a clean database.
+
+   
+  
+ 
+
  
   --inserts
   
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 806fc78f04..b1c21eede5 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -123,6 +123,8 @@ static SimpleStringList table_exclude_patterns = {NULL, NULL};
 static SimpleOidList table_exclude_oids = {NULL, NULL};
 static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
 static SimpleOidList tabledata_exclude_oids = 

Re: Option to dump foreign data in pg_dump

[Daniel Gustafsson]

> On 12 Jul 2019, at 16:08, Luis Carril  wrote:
> 
> > > On 28 Jun 2019, at 19:55, Luis Carril  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(_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, 
_servers_include_patterns,
+   _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

Re: Option to dump foreign data in pg_dump

[Luis Carril]

> > On 28 Jun 2019, at 19:55, Luis Carril  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
From d24cbf4ad0852b079d0e16103486873ab6bb8b69 Mon Sep 17 00:00:00 2001
From: Luis Carril 
Date: Fri, 28 Jun 2019 16:05:43 +0200
Subject: [PATCH] Support foreign data in pg_dump

---
 src/bin/pg_dump/pg_dump.c | 107 +++---
 src/bin/pg_dump/pg_dump.h |   1 +
 2 files changed, 102 insertions(+), 6 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 806fc78f04..ceff6a1744 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -123,6 +123,8 @@ static SimpleStringList table_exclude_patterns = {NULL, NULL};
 static SimpleOidList table_exclude_oids = {NULL, NULL};
 static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
 static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
+static SimpleStringList foreign_servers_include_patterns = {NULL, NULL};
+static SimpleOidList foreign_servers_include_oids = {NULL, NULL};
 
 
 char		g_opaque_type[10];	/* name for the opaque type */
@@ -159,6 +161,10 @@ static void expand_schema_name_patterns(Archive *fout,
 		SimpleStringList *patterns,
 		SimpleOidList *oids,
 		bool strict_names);
+static void expand_foreign_server_name_patterns(Archive *fout,
+		SimpleStringList *patterns,
+		SimpleOidList *oids,
+		bool strict_names);
 static void expand_table_name_patterns(Archive *fout,
 	   SimpleStringList *patterns,
 	   SimpleOidList *oids,
@@ -391,6 +397,7 @@ main(int argc, char **argv)
 		{"no-sync", no_argument, NULL, 7},
 		{"on-conflict-do-nothing", no_argument, _nothing, 1},
 		{"rows-per-insert", required_argument, NULL, 10},
+		{"include-foreign-data", required_argument, NULL, 11},
 
 		{NULL, 0, NULL, 0}
 	};
@@ -607,6 +614,10 @@ main(int argc, char **argv)
 dopt.dump_inserts = (int) rowsPerInsert;
 break;
 
+			case 11:/* include foreign data */
+simple_string_list_append(_servers_include_patterns, optarg);
+break;
+
 			default:
 fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 exit_nicely(1);
@@ -815,6 +826,15 @@ main(int argc, char **argv)
 			   _exclude_oids,
 			   false);
 
+	if (foreign_servers_include_patterns.head != NULL)
+	{
+		expand_foreign_server_name_patterns(fout, _servers_include_patterns,
+			_servers_include_oids,
+			strict_names);
+		if (foreign_servers_include_oids.head == NULL)
+			fatal("no matching foreign servers were found");
+	}
+
 	/* non-matching exclusion patterns aren't an error */
 
 	/*
@@ -1038,6 +1058,9 @@ help(const char *progname)
 	printf(_("  --use-set-session-authorization\n"
 			 "   use SET SESSION AUTHORIZATION commands instead of\n"
 			 "   ALTER OWNER commands to set ownership\n"));
+	printf(_("  --include-foreign-data=SERVER\n"
+			 "   include data of foreign tables with the named\n"
+			 "   foreign servers in dump\n"));
 
 	printf(_("\nConnection options:\n"));
 	printf(_("  -d, --dbname=DBNAME  database to dump\n"));
@@ -1336,6 +1359,54 @@ expand_schema_name_patterns(Archive *fout,
 	destroyPQExpBuffer(query);
 }
 
+/*
+ * Find the OIDs of all foreign servers matching the given list of patterns,
+ * and append them to the given OID list.
+ */
+static void
+expand_foreign_server_name_patterns(Archive *fout,
+			SimpleStringList *patterns,
+			SimpleOidList *oids,
+			bool strict_names)
+{
+	PQExpBuffer query;
+	PGresult   *res;
+	SimpleStringListCell *cell;
+	int			i;
+
+	if (patterns->head == NULL)
+		return;	/* nothing to do */
+
+	query = createPQExpBuffer();
+
+	/*
+	 * The loop below runs multiple SELECTs might sometimes result in
+	 * duplicate entries in the OID list, but we don't care.
+	 */
+
+	for (cell = patterns->head; cell; cell = cell->next)
+	{
+		appendPQExpBuffer(query,
+		  "SELECT oid FROM pg_catalog.pg_foreign_server s\n");
+		processSQLNamePattern(GetConnection(fout), query, cell->val, false,
+			  false, NULL, "s.srvname", NULL, NULL);
+
+		res = 

Re: Option to dump foreign data in pg_dump

[Daniel Gustafsson]

> On 28 Jun 2019, at 17:30, Tom Lane  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  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

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

Re: Option to dump foreign data in pg_dump

[Pavel Stehule]

pá 28. 6. 2019 v 17:30 odesílatel Tom Lane  napsal:

> Daniel Gustafsson  writes:
> >> On 28 Jun 2019, at 16:49, Luis Carril  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
>
>
>

Re: Option to dump foreign data in pg_dump

[Tom Lane]

Daniel Gustafsson  writes:
>> On 28 Jun 2019, at 16:49, Luis Carril  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

Re: Option to dump foreign data in pg_dump

[Pavel Stehule]

pá 28. 6. 2019 v 17:17 odesílatel Daniel Gustafsson 
napsal:

> > On 28 Jun 2019, at 16:49, Luis Carril  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
>
>

Re: Option to dump foreign data in pg_dump

[Pavel Stehule]

Hi

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

Re: Option to dump foreign data in pg_dump

[Daniel Gustafsson]

> On 28 Jun 2019, at 16:49, Luis Carril  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

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
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index db30b54a92..a21027a61d 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -159,6 +159,7 @@ typedef struct _dumpOptions
 	int			use_setsessauth;
 	int			enable_row_security;
 	int			load_via_partition_root;
+	int			include_foreign_data;
 
 	/* default, if no "inclusion" switches appear, is to dump everything */
 	bool		include_everything;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 8909a45d61..8854f70305 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -391,6 +391,7 @@ main(int argc, char **argv)
 		{"no-sync", no_argument, NULL, 7},
 		{"on-conflict-do-nothing", no_argument, _nothing, 1},
 		{"rows-per-insert", required_argument, NULL, 10},
+		{"include-foreign-data", no_argument, _foreign_data, 1},
 
 		{NULL, 0, NULL, 0}
 	};
@@ -1038,6 +1039,7 @@ help(const char *progname)
 	printf(_("  --use-set-session-authorization\n"
 			 "   use SET SESSION AUTHORIZATION commands instead of\n"
 			 "   ALTER OWNER commands to set ownership\n"));
+	printf(_("  --include-foreign-data   include data of foreign tables in dump\n"));
 
 	printf(_("\nConnection options:\n"));
 	printf(_("  -d, --dbname=DBNAME  database to dump\n"));
@@ -1812,7 +1814,7 @@ dumpTableData_copy(Archive *fout, void *dcontext)
 	 */
 	column_list = fmtCopyColumnList(tbinfo, clistBuf);
 
-	if (tdinfo->filtercond)
+	if (tdinfo->filtercond || tbinfo->relkind == RELKIND_FOREIGN_TABLE)
 	{
 		/* Note: this syntax is only supported in 8.2 and up */
 		appendPQExpBufferStr(q, "COPY (SELECT ");
@@ -1824,9 +1826,11 @@ dumpTableData_copy(Archive *fout, void *dcontext)
 		}
 		else
 			appendPQExpBufferStr(q, "* ");
-		appendPQExpBuffer(q, "FROM %s %s) TO stdout;",
-		  fmtQualifiedDumpable(tbinfo),
-		  tdinfo->filtercond);
+
+		appendPQExpBuffer(q, "FROM %s", fmtQualifiedDumpable(tbinfo));
+		if (tdinfo->filtercond)
+			appendPQExpBuffer(q, " %s", tdinfo->filtercond);
+		appendPQExpBuffer(q, ") TO stdout;");
 	}
 	else
 	{
@@ -2343,7 +2347,7 @@ makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo)
 	if (tbinfo->relkind == RELKIND_VIEW)
 		return;
 	/* Skip FOREIGN TABLEs (no data to dump) */
-	if (tbinfo->relkind == RELKIND_FOREIGN_TABLE)
+	if (tbinfo->relkind == RELKIND_FOREIGN_TABLE && !dopt->include_foreign_data)
 		return;
 	/* Skip partitioned tables (data in partitions) */
 	if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 24719cefe2..14d62e9781 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -82,6 +82,7 @@ static int	no_role_passwords = 0;
 static int	server_version;
 static int	load_via_partition_root = 0;
 static int	on_conflict_do_nothing = 0;
+static int	include_foreign_data = 0;
 
 static char role_catalog[10];
 #define PG_AUTHID "pg_authid"
@@ -147,6 +148,7 @@ main(int argc, char *argv[])
 		{"no-unlogged-table-data", no_argument, _unlogged_table_data, 1},
 		{"on-conflict-do-nothing", no_argument, _conflict_do_nothing, 1},
 		{"rows-per-insert", required_argument, NULL, 7},
+		{"include-foreign-data", no_argument, _foreign_data, 1},
 
 		{NULL, 0, NULL, 0}
 	};
@@ -432,6 +434,8 @@ main(int argc, char *argv[])
 		appendPQExpBufferStr(pgdumpopts, " --no-unlogged-table-data");
 	if (on_conflict_do_nothing)
 		appendPQExpBufferStr(pgdumpopts, " --on-conflict-do-nothing");
+	if (include_foreign_data)
+		appendPQExpBufferStr(pgdumpopts, " --include-foreign-data");
 
 	/*
 	 * If there was a database specified on the command line, use that,
@@ -658,6 +662,7 @@ help(void)
 	printf(_("  --use-set-session-authorization\n"
 			 "   use SET SESSION AUTHORIZATION commands instead of\n"
 			 "   ALTER OWNER commands to set ownership\n"));
+	printf(_("  --include-foreign-data   include data of foreign tables in dump\n"));
 
 	printf(_("\nConnection options:\n"));
 	printf(_("  -d, --dbname=CONNSTR connect using connection string\n"));