Re: Import Statistics in postgres_fdw before resorting to sampling.

>
> * 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!