On 20 Sep 2019, at 17:20, Luis Carril <luis.car...@swarm64.com> wrote: I took a look at this patch again today for a review of the latest version. While I still think it's a potential footgun due to read-only FDW's, I can see usecases for having it so I'm mildly +1 on adding it. The patch applies to master with a little bit of fuzz and builds without warnings. Regarding the functionality, it's a bit unfortunate that it works differently for --inserts dumps and COPY dumps. As an example, suppose we have a file_fdw table in CSV format with 10 rows where row 10 is malformed. The COPY dump will include 9 rows and exit on an error, the --inserts dump won't include any rows before the error. Since the dump fails with an error, one can argue that it doesn't matter too much, but it's still not good to have such different behaviors based on program internals. (the example is of course not terribly realistic but can be extrapolated from.) Maybe I'm the only one concerned though. *I suggest the option to be just –foreign-data because if we make it –include-foreign-data its expected to have –exclude-foreign-data option too.Several pg_dump options have no counterpart, e.g --enable-row-security does not have a disable (which is the default). Also calling it --foreign-data would sound similar to the --table, by default all tables are dumped, but with --table only the selected tables are dumped. While without --include-foreign-data all data is excluded, and only with the option some foreign data would be included. I agree that --include-foreign-data conveys the meaning of the option better, +1 for keeping this. *please add test caseI added tests cases for the invalid inputs. I'll try to make a test case for the actual dump of foreign data, but that requires more setup, because a functional fdw is needed there. This is where it becomes a bit messy IMO. Given that there has been a lot of effort spent on adding test coverage for pg_dump, I think it would be a shame to add such niche functionality without testing more than the program options. You are however right that in order to test, it requires a fully functional FDW. I took the liberty to add a testcase to the pg_dump TAP tests which includes a dummy FDW that always return 10 predetermined rows (in order to keep tests stable). There is so far just a happy-path test, since I don't want to spend time until it's deemed of interest (it is adding a lot of code for a small test), but it at least illustrates how this patch could be tested. The attached patch builds on top of yours. 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.The below check may not be required: I still believe thats the desired functionality. Also, a few small nitpicks on the patch: This should probably be PATTERN instead of SERVER, to match the rest of the help output: + printf(_(" --include-foreign-data="">+ " include data of foreign tables with the named\n" + " foreign servers in dump\n")); It would be good to add a comment explaining the rationale for adding RELKIND_FOREIGN_TABLE to this block, to assist readers: - if (tdinfo->filtercond) + if (tdinfo->filtercond || tbinfo->relkind == RELKIND_FOREIGN_TABLE) cheers ./daniel |
pr-foreign_data_dump_test.patch
Description: Binary data
