Re: use of SPI by postgresImportForeignStatistics

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
Cc: Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: use of SPI by postgresImportForeignStatistics
Date: 2026-08-21 07:15:23
Message-ID: aof7CwOXSNkiMrX-@paquier.xyz
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Aug 20, 2026 at 12:28:30PM -0400, Corey Huinker wrote:
> On Thu, Aug 20, 2026 at 7:43 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
> wrote:
>> On Thu, Aug 20, 2026 at 2:15 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
>> wrote:
>>> 1. Remove special handling of version parameter, add it into existing
>> StatsArgInfo structures. Not the highest priority, but doing it first
>> remove the possibility of having to re-adjust subsequent changes related to
>> the offset I'm proposing.
>>> 2. Remove construction of of FCINFO, going with passing the latter
>> portion of the positional NullableDatum array.

Just looking at v3-0011, expanding functions like
attribute_statistics_update_internal() so as these use a gazzilion of
input arguments is not something I am on board with. That's bug prone
in the long term. If you wish to eliminate the FCINFO part, which is,
well, not completely wrong either and does work, I'd suggest to pass
one pointer to a structure that has access to all these fields. The
NullableDatum seems to be that, but the patch does not use that so I
cannot be entirely sure about the benefit of suggestion 2.

I am unconvinced with the value patches 0012 and the follow-up ones
bring, as well. For example for the recovery check, we just hide more
stuff in thin wrappers, and we don't have that many entrance points
with these SQL stats import functions.

> Oops, forgot to post the commitfest entry:
>
> https://commitfest.postgresql.org/patch/7170/

A replacement of newfcinfo by a different facility in
src/backend/statistics/ has nothing to do with SPI and postgres_fdw.
I'd suggest to:
- Rebase (and refactor) the patch.
- Create a new thread dealing with the specific topic you wish to deal
with.

Spoiler: v3, which is the latest patch set I am aware of in terms of
replacement of the fcinfo, does not apply cleanly on HEAD. Looking at
the patch, you also seem to update multiple times the same area of the
code, that makes the result more confusing to me, because the result
you are looking for is hidden in a forest of cascading changes.
--
Michael

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Ian Lawrence Barwick 2026-08-21 07:15:54 Re: [PATCH] doc: clarify AS requirement when VALUES used in a FROM clause
Previous Message Koshino Taiki 2026-08-21 07:05:32 doc: Reformat SELECT queries using GRAPH_TABLE