* As above, I think we should disable this feature by default for now, so I modified the patch as such.
+1 so long as it doesn't paint us into a corner.
* You are defining a remote query per version at the top of the file that is used in fetch_attstats, but I think that that reduces readability, making maintenance hard, so I modified that function to build the query dynamically in it, as I proposed before. I also modified that function to use PQsendQuery, not PQsendQueryParams, as in fetch_relstats, for efficiency.
No objections. I find full-formed queries more readable, but I'm clearly in the minority with that opinion.
I'm curious, is the efficiency of PQsendQuery over PQsendQueryParams that significant?
* I moved the code for opening/closing the connection from postgresImportStatistics to fetch_remote_statistics, as the connection is only used in the latter function. I also removed useless cleanup processing in that function.
+1
* I added regression tests.
+1
* I fixed a minor bug I noticed while adding those tests.
* I did some cleanup and editorialization including re-ordering functions in logical order.
This was momentarily confusing as I apply your patch to a separate branch and then compare whole branches, but it checks out.
One thing I'm wondering is: we should rename ImportStatistics to ImportForeignStatistics, for consistency with ImportForeignSchema?
ImportForeignStatistics makes sense to me.
Also, we should rename fetch_stats to import_stats, for consistency with eg, import_default?
restore_stats is probably the better name, given that it is calling the pg_restore_*() functions.
Anyway I think the patch is now in good shape, except comments/docs, which I will work on next.
