Re: Option to dump foreign data in pg_dump

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