Re: Import Statistics in postgres_fdw before resorting to sampling.

On Sat, Apr 4, 2026 at 1:01 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
>>
>> * 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.

Given that we can't guarantee the freshness, I think that that's the
only choice.

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

> I'm curious, is the efficiency of PQsendQuery over PQsendQueryParams that significant?

No, it isn't, but note that PQsendQuery uses simple query protocol
while PQsendQueryParams uses extended query protocol, and that the
overhead incurred by the former protocol is smaller than that by the
latter protocol, so it's better to use PQsendQuery here.

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

Sorry, but I believe that that makes the code easy to read.

>> One thing I'm wondering is: we should rename ImportStatistics to
>> ImportForeignStatistics, for consistency with ImportForeignSchema?

> ImportForeignStatistics makes sense to me.

Will rename.

>> 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.

That sounds better, so will rename.

Thanks for reviewing!

Best regards,
Etsuro Fujita