MIME-Version: 1.0
References: 
 <CA+TgmoYqMtWb4zLUkT98oFnEkJ=Wz0Pw-ggDJrp9wnSXPzUaeQ@mail.gmail.com>
 <CAPmGK161TJ4G01eptv3sBqgxa4SNHDE9VRqoC6jy5Y6G3D8yDw@mail.gmail.com>
 <CA+TgmoZpwfTGqqp=0efV99e_BMYWdLFq2XKRWVSDcukifkZMeA@mail.gmail.com>
 <CADkLM=dxe8NbQu4RNi2OFXpaRKdpS0-CHGm6Mqtf_rW155BM4g@mail.gmail.com>
 <CADkLM=cV=uezAKyZdyyxnZOBRhGLUSbeYfDj5_jbDCLP9LP24Q@mail.gmail.com>
 <CA+TgmoaVcgZJpwxyi7HEfNMJhmm7BXkTQ468jvxkW2QzQZRkvg@mail.gmail.com>
 <CADkLM=dy18dDJ+j3-_Ofn11vt5J7zF33oTWnE6N=ePc83uS91A@mail.gmail.com>
 <CAPmGK15MfVT2H+PUvA_nAJvySGr_pAVkVx3KcnVXWGO36acWLA@mail.gmail.com>
 <CADkLM=dr5bfB8YN-bV1U0NDvyVO7a6uuz0BsAi89a1Md4A==8g@mail.gmail.com>
 <CAPmGK14P2GRe78hp6fZ3vExU_9RRDjMsYVOx7AALvUQ-hKTUiQ@mail.gmail.com>
 <CADkLM=cL9ZGvO-72QWyXA5Wqv-4P3T1fp+VFJy5e6k3Hf=POMw@mail.gmail.com>
 <CA+TgmoZdTVeXRQo__cEr_1y+mUO9nubOX=6PEfw7Cr0UfZQn6w@mail.gmail.com>
 <CADkLM=fe=ycPdwT2wE_iYwK8e8viMbWvwWhXR9vDe+55KKF-KQ@mail.gmail.com>
 <CADkLM=dWJaw04u9FU=WkykOy7bu7B5eKmoDkfVz6JXh7mdemkw@mail.gmail.com>
 <CA+TgmoY3a_S0munZ0wg4hQpMKVMyM_jpi6aGhFR0gAm1jLukag@mail.gmail.com>
 <CADkLM=cdKjgphHpFW8kKukHF+DVYKogHcqLXO=BA8MGfiRFqQA@mail.gmail.com>
 <CADkLM=c9BEuA9vPjE9Wco6+Lz-HSnnmBZrFhi=-yNi0K8tmZug@mail.gmail.com>
 <CAPmGK17W1sUB3JnkRoBpgO0_iSH5HRcc0_nxk8Us4KYsQhuQxg@mail.gmail.com>
 <CA+TgmoZd5MDhcx6uav6MWxhM42yWcfaRAHYKNtNkuog=yrKAgw@mail.gmail.com>
 <CADkLM=f_c7Xoq-bSzw28LgvxPa+OQ3rVow=xwkTyjz1J0=pMdw@mail.gmail.com>
 <CAPmGK14dY-Cex2rc08TinQjHR4Q1L6nqPBfdEBRZusP7n_TZSA@mail.gmail.com>
 <CADkLM=enKc-3-uTjUSXXhoRhSKDHVdb_ate+S2dtkEcscW5bvA@mail.gmail.com>
 <CAPmGK17rNA=dknxHCXbUoQ_xe2PQcPq0d-bhTr47wrUfXrDLFA@mail.gmail.com>
 <CADkLM=dYZ2XJ=nUMSYgA6D3VEFQ4rvpyDLKD-RrnfESUGxktiA@mail.gmail.com>
In-Reply-To: 
 <CADkLM=dYZ2XJ=nUMSYgA6D3VEFQ4rvpyDLKD-RrnfESUGxktiA@mail.gmail.com>
From: Etsuro Fujita <etsuro.fujita@gmail.com>
Date: Fri, 3 Jul 2026 18:47:54 +0900
Message-ID: 
 <CAPmGK15eTnEtn_tPm+o0QA-jEnJUW+B2FPAF=vGNkFAGMApWNQ@mail.gmail.com>
Subject: Re: use of SPI by postgresImportForeignStatistics
To: Corey Huinker <corey.huinker@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAPmGK15eTnEtn_tPm%2Bo0QA-jEnJUW%2BB2FPAF%3DvGNkFAGMApWNQ%40mail.gmail.com>
Precedence: bulk

On Thu, Jul 2, 2026 at 5:35=E2=80=AFAM Corey Huinker <corey.huinker@gmail.c=
om> wrote:
> On Wed, Jul 1, 2026 at 8:10=E2=80=AFAM Etsuro Fujita <etsuro.fujita@gmail=
.com> wrote:
>> On Wed, Jul 1, 2026 at 12:55=E2=80=AFAM Corey Huinker <corey.huinker@gma=
il.com> wrote:

>> >> In relation to this change, I moved helper functions set_XXX_arg()
>> >> that you added to relation_stats.c and attribute_stats.c to
>> >> postgres_fdw.c.  The helper functions had soft error handling, but in
>> >> the postgres_fdw use, there is no need for that, so I removed that
>> >> handling as well.
>> >
>> > My inclination would be to move the set_*_arg functions could be moved=
 to some common utility file in v20, as other FDWs will find themselves wit=
h the same need.
>>
>> Seems like a good idea.  I moved the set_*_arg functions to
>> stat_utils.c, and renamed them to stats_set_*_arg.  Attached is a new
>> version of the patch.
>
> I'm concerned that by making these non-error-safe function calls availabl=
e, we create a barrier to making those same functions error-safe in the fut=
ure.
>
>> > As for removing the error-handling, it might still be handy because it=
 would allow us to give a more context-aware error message, rather than the=
 very narrow "this_string is an invalid this_type", for string that the use=
r most certainly never saw. Having said that, it's something we could easil=
y change back to error-safe if we wanted to.
>>
>> Ok, I think we could do so later if needed.
>
> I agree we could do it later if they were private functions, no problem. =
But if they're available outside of postgres_fdw then changing them to be e=
rror-safe potentially disrupts other callers. Someone please let me know if=
 I'm being unnecessarily cautious here.

Ah, I misunderstood your comments.  I agree with you here, so I'll
move back the functions to postgres_fdw.c.

> Other Thoughts:
>
> 1. The internal functions should accept a server_version_number parameter=
, and we should document that setting that parameter to 0 mean that we can =
assume the current version. I know that we presently have no translation is=
sues going forward with statistics, but someday that won't be the case, and=
 if we don't have this parameter in place then it'll be too late to fix it.

Good idea!  Will do.

> 2. Did you put import_relation_statistics and import_attribute_statistics=
 in statistics.h because you see them as long-term publicly visible functio=
ns, and what's in stats_utils.h to be more subject to change? If so, I coul=
d get behind that. If not, then I fall back to my position that they seem l=
ike they belong in new relation_stats.h and attribute_stats.h, or stats_uti=
ls.h.

The answer is the former.  I'll remove the changes made to
stats_utils.h, though.

> 3. The import_stats_functions in their current "bridging" form should do =
null checks on all of the NullableDatum pointers, or at least Assert()s on =
them.

Will do.

Thanks for the comments!

Best regards,
Etsuro Fujita