> > * 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. +1!
