Hello,
thanks for the comments!
*I suggest the option to be just –foreign-data because if we make it
–include-foreign-data its expected to have –exclude-foreign-data option too.
Several pg_dump options have no counterpart, e.g --enable-row-security does not
have a disable (which is the default). Also calling it --foreign-data would
sound similar to the --table, by default all tables are dumped, but with
--table only the selected tables are dumped. While without
--include-foreign-data all data is excluded, and only with the option some
foreign data would be included.
*please add test case
I added tests cases for the invalid inputs. I'll try to make a test case for
the actual dump of foreign data, but that requires more setup, because a
functional fdw is needed there.
* + if (tdinfo->filtercond || tbinfo->relkind == RELKIND_FOREIGN_TABLE)
filter condition is not implemented completely yet so the logic only work on
foreign table so I think its better to handle it separately
Note that there is another if condition that actually applies the the
filtercondition if provided, also for a foreign table we need to do a COPY
SELECT instead of a COPY TO
* I don’t understand the need for changing SELECT query .we can use the same
SELECT query syntax for both regular table and foreign table
To which query do you refer? In the patch there are three queries: 1 retrieves
foreign servers, another is the SELECT in the COPY that now it applies in case
of a filter condition of a foreign table, and a third that retrieves the oid of
a given foreign server.
> As you have specified required_argument in above:
> + {"include-foreign-data", required_argument, NULL, 11},
>
> The below check may not be required:
> + if (strcmp(optarg, "") == 0)
> + {
> + pg_log_error("empty string is not a valid pattern in
> --include-foreign-data");
> + exit_nicely(1);
> + }
We need to conserve this check to avoid that the use of
'--include-foreign-data=', which would match all foreign servers. And in
previous messages it was established that that behavior is too coarse.
>
> + if (foreign_servers_include_patterns.head != NULL)
> + {
> + expand_foreign_server_name_patterns(fout, &foreign_servers_include_patterns,
> + &foreign_servers_include_oids);
> + if (foreign_servers_include_oids.head == NULL)
> + fatal("no matching foreign servers were found");
> + }
> +
>
> The above check if (foreign_servers_include_oids.head == NULL) may not
> be required, as there is a check present inside
> expand_foreign_server_name_patterns to handle this error:
> +
> + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
> + if (PQntuples(res) == 0)
> + fatal("no matching foreign servers were found for pattern \"%s\"",
> cell->val);
> +
Removed
>
> +static void
> +expand_foreign_server_name_patterns(Archive *fout,
> + SimpleStringList *patterns,
> + SimpleOidList *oids)
> +{
> + PQExpBuffer query;
> + PGresult *res;
> + SimpleStringListCell *cell;
> + int i;
> +
> + if (patterns->head == NULL)
> + return; /* nothing to do */
> +
>
> The above check for patterns->head may not be required as similar
> check exists before this function is called:
> + if (foreign_servers_include_patterns.head != NULL)
> + {
> + expand_foreign_server_name_patterns(fout, &foreign_servers_include_patterns,
> + &foreign_servers_include_oids);
> + if (foreign_servers_include_oids.head == NULL)
> + fatal("no matching foreign servers were found");
> + }
> +
I think that it is better that the function expand_foreign_server_name do not
rely on a non-NULL head, so it checks it by itself, and is closer to the other
expand_* functions.
Instead I've removed the check before the function is called.
>
> + /* Skip FOREIGN TABLEs (no data to dump) if not requested explicitly */
> + if (tbinfo->relkind == RELKIND_FOREIGN_TABLE &&
> + (foreign_servers_include_oids.head == NULL ||
> + !simple_oid_list_member(&foreign_servers_include_oids,
> tbinfo->foreign_server_oid)))
> simple_oid_list_member can be split into two lines
Done
Cheers
Luis M Carril
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
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--include-foreign-data=<replaceable class="parameter">foreignserver</replaceable></option></term>
+ <listitem>
+ <para>
+ Dump the data for any foreign table with a foreign server
+ matching <replaceable class="parameter">foreignserver</replaceable>
+ pattern. Multiple foreign servers can be selected by writing multiple <option>--include-foreign-data</option>.
+ Also, the <replaceable class="parameter">foreignserver</replaceable> parameter is
+ interpreted as a pattern according to the same rules used by
+ <application>psql</application>'s <literal>\d</literal> commands (see <xref
+ linkend="app-psql-patterns" endterm="app-psql-patterns-title"/>),
+ 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
+ <xref linkend="pg-dump-examples" endterm="pg-dump-examples-title"/>.
+ The only exception is that an empty pattern is disallowed.
+ </para>
+
+ <note>
+ <para>
+ When <option>--include-foreign-data</option> is specified, <application>pg_dump</application>
+ 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.
+ </para>
+ </note>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>--inserts</option></term>
<listitem>
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index f01fea5b91..c32af1348f 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,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,
@@ -391,6 +396,7 @@ main(int argc, char **argv)
{"no-sync", no_argument, NULL, 7},
{"on-conflict-do-nothing", no_argument, &dopt.do_nothing, 1},
{"rows-per-insert", required_argument, NULL, 10},
+ {"include-foreign-data", required_argument, NULL, 11},
{NULL, 0, NULL, 0}
};
@@ -607,6 +613,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(&foreign_servers_include_patterns, optarg);
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit_nicely(1);
@@ -648,6 +663,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");
@@ -815,6 +836,9 @@ main(int argc, char **argv)
&tabledata_exclude_oids,
false);
+ expand_foreign_server_name_patterns(fout, &foreign_servers_include_patterns,
+ &foreign_servers_include_oids);
+
/* non-matching exclusion patterns aren't an error */
/*
@@ -1038,6 +1062,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 +1363,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
+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 */
+
+ 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 = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+ if (PQntuples(res) == 0)
+ fatal("no matching foreign servers were found for pattern \"%s\"", cell->val);
+
+ for (i = 0; i < PQntuples(res); i++)
+ {
+ simple_oid_list_append(oids, atooid(PQgetvalue(res, i, 0)));
+ }
+
+ PQclear(res);
+ resetPQExpBuffer(query);
+ }
+
+ destroyPQExpBuffer(query);
+}
+
/*
* Find the OIDs of all tables matching the given list of patterns,
* and append them to the given OID list. See also expand_dbname_patterns()
@@ -1812,7 +1886,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 +1898,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
{
@@ -2342,8 +2418,11 @@ makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo)
/* Skip VIEWs (no data to dump) */
if (tbinfo->relkind == RELKIND_VIEW)
return;
- /* Skip FOREIGN TABLEs (no data to dump) */
- if (tbinfo->relkind == RELKIND_FOREIGN_TABLE)
+ /* Skip FOREIGN TABLEs (no data to dump) if not requested explicitly */
+ if (tbinfo->relkind == RELKIND_FOREIGN_TABLE &&
+ (foreign_servers_include_oids.head == NULL ||
+ !simple_oid_list_member(&foreign_servers_include_oids,
+ tbinfo->foreign_server_oid)))
return;
/* Skip partitioned tables (data in partitions) */
if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
@@ -6651,6 +6730,26 @@ getTables(Archive *fout, int *numTables)
tblinfo[i].ispartition = (strcmp(PQgetvalue(res, i, i_ispartition), "t") == 0);
tblinfo[i].partbound = pg_strdup(PQgetvalue(res, i, i_partbound));
+ if (tblinfo[i].relkind == RELKIND_FOREIGN_TABLE)
+ {
+ PQExpBuffer query_server = createPQExpBuffer();
+ PGresult *res_server;
+
+ /* retrieve the oid of the foreign server*/
+ appendPQExpBuffer(query_server,
+ "SELECT fs.oid "
+ "FROM pg_catalog.pg_foreign_table ft "
+ "JOIN pg_catalog.pg_foreign_server fs "
+ "ON (fs.oid = ft.ftserver) "
+ "WHERE ft.ftrelid = '%u'",
+ tblinfo[i].dobj.catId.oid);
+
+ res_server = ExecuteSqlQueryForSingleRow(fout, query_server->data);
+ tblinfo[i].foreign_server_oid = atooid(PQgetvalue(res_server, 0, 0));
+ PQclear(res_server);
+ destroyPQExpBuffer(query_server);
+ }
+
/*
* Read-lock target tables to make sure they aren't DROPPED or altered
* in schema before we get around to dumping them.
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index ec5a924b8f..427d69e657 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -322,6 +322,7 @@ typedef struct _tableInfo
char *partbound; /* partition bound definition */
bool needs_override; /* has GENERATED ALWAYS AS IDENTITY */
char *amname; /* relation access method */
+ Oid foreign_server_oid; /* foreign server oid */
/*
* Stuff computed only for dumpable tables.
diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl
index 9ca8a8e608..559b4b3e01 100644
--- a/src/bin/pg_dump/t/001_basic.pl
+++ b/src/bin/pg_dump/t/001_basic.pl
@@ -4,7 +4,7 @@ use warnings;
use Config;
use PostgresNode;
use TestLib;
-use Test::More tests => 74;
+use Test::More tests => 78;
my $tempdir = TestLib::tempdir;
my $tempdir_short = TestLib::tempdir_short;
@@ -49,6 +49,18 @@ command_fails_like(
'pg_dump: options -s/--schema-only and -a/--data-only cannot be used together'
);
+command_fails_like(
+ [ 'pg_dump', '-s', '--include-foreign-data', 'xxx' ],
+ qr/\Qpg_dump: error: options -s\/--schema-only and --include-foreign-data cannot be used together\E/,
+ 'pg_dump: options -s/--schema-only and --include-foreign-data cannot be used together'
+);
+
+command_fails_like(
+ [ 'pg_dump', '--include-foreign-data', '' ],
+ qr/\Qpg_dump: error: empty string is not a valid pattern in --include-foreign-data\E/,
+ 'pg_dump: empty string is not a valid pattern in --include-foreign-data'
+);
+
command_fails_like(
['pg_restore'],
qr{\Qpg_restore: error: one of -d/--dbname and -f/--file must be specified\E},
