Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wf1eJ-005DNe-1k for pgsql-hackers@arkaria.postgresql.org; Wed, 01 Jul 2026 20:35:11 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wf1eH-00Faqr-1F for pgsql-hackers@arkaria.postgresql.org; Wed, 01 Jul 2026 20:35:09 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wf1eG-00Faqj-39 for pgsql-hackers@lists.postgresql.org; Wed, 01 Jul 2026 20:35:09 +0000 Received: from mail-pl1-x62e.google.com ([2607:f8b0:4864:20::62e]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wf1eE-000000018Y5-3LY0 for pgsql-hackers@postgresql.org; Wed, 01 Jul 2026 20:35:07 +0000 Received: by mail-pl1-x62e.google.com with SMTP id d9443c01a7336-2c825c88744so10226195ad.1 for ; Wed, 01 Jul 2026 13:35:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1782938106; cv=none; d=google.com; s=arc-20260327; b=qJ93zOXoL51EH2aJvcqS2mTAhzbzjZEnP2npIzx4UgQ6uTei1xg17d/h7O7vlUJHrA 5qWq1I8GKAYRxk5voUv4XjC0DSi/g/teSeBP2+nzSpMkiAtLLTnnXXOcnbvCqqCZjjsS BYxZHJ1/9KQfMMZfWgeAp/i4XWN6Y51RA0kgtp+zuZ0XyvhuIGV5EFkEWud9kL+aEndU uDQNF/AKHC10w/PQVTft/Clf4VpkN8EYv+AV6M04rxQR7pSSlRwBniWDMPtBOQUTvzAP eoftcUvbCWlIdJu10F7A0wQtNhBSrUhTselFRcClThe2CaZbbYNS120RvDcLi3s4ygUT yuRg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20260327; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=Lm6CdPbdTScH+Q1zJ2Tbdk3sRBOz1n2yOXLT2v9JZzQ=; fh=QJ4jrkOCL8NGw5YEzNM3fXQJZB4ddFEPU1obfTNotbk=; b=rnH0VDtHuONPs2GUhFaTa60mFWQCgvnzwLCxuIccwrNMNl9sg+dcwV4BXQdHlN9VyW P2H7tT1cLRgjQyLR9h+eqK4eWpWTvMTGp2/VokHfwBGZT4X+FWZ+OP03WBLYu4aByjjX hWBOMGm5gKw5kiQDdN1vQNNIySGErd60WJHxlDpaCPY0g/8ifCjLrFY5nWhJLZq70hf5 2kHQfUfIERc0Ae5BGGBsTy+IxQRFILwdoDH4KYq944m/PBXL/0skgpxq/QRTnqTwRXOq jZlShP3AJlIlhHF0hI4SevThlE9C+YRUdtmMXD1JPI2oTOxKasUIAoXL4ryG6WgKqEcU 5/UQ==; darn=postgresql.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1782938106; x=1783542906; darn=postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Lm6CdPbdTScH+Q1zJ2Tbdk3sRBOz1n2yOXLT2v9JZzQ=; b=YyefAV061ZbDaTchNKklrrFoALhKEImPmizbFpyLP8AyFtQpntv0PhmPm/nZxgtQ39 IJxV6RMQNfzcsFobmr3eu3zHE29E0W4k8qqmbjWImPiIgFjwhKB0wzYh4d84sjv2Z4y2 Z0z06UJcenKC9SB0QUc8ERiunG9K2mXiLbfeGkfIUbi12+iSe9utPTFNaLY2tb0o4PzM 8oUmci2Y2MU5idlY5pxbCP/fIsklUf/lur8oui3zKY7+OQM6F/ssXulPdL+TXheQem8y C9ljDxY/HztK4CgWcgL8/eehoBGuN1MEqlniL+R94I24Qzqp95wKfLXT0UY0MmT8Br+J X7Tg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782938106; x=1783542906; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Lm6CdPbdTScH+Q1zJ2Tbdk3sRBOz1n2yOXLT2v9JZzQ=; b=D91mv3cHoSV8ZsCzlyM1GXu4WLt/4TwZQBoMvLkouHeWSkikAMcDM+5eLMAMcf++Oh afhwGQyDzpmfGc9TORRt6nwnTclsN0SRyomVZhfkh17bS8OEzfZH7BZ8uHUYKP1Udhck iypxc52PYhoEeC6uNHQZRwMPQidbWR/LWyjdzqXY1tIbnOp9CNbKv6gfrtFRvA7RomBq 6JVaG7qSitqpwBvFd2+k+ffq5xPRf8HfQH+HxYCSBs5tTwkWScaVnhHMeaHj5CHtiPFB V7WizmySpUYtmdVuxQ/gztH7GhLdafIjvzdGq1a1Ox51SQGLKBSDvwGQHysESNvcVk2d zYPg== X-Forwarded-Encrypted: i=1; AFNElJ/kxitp7eopV93sdamu7SA+PwzGkf0Wm8qs07lL3QxSCdMLDiuvuMfEeTvxarbC+NqdmsYfiU2FVAfEWuU2@postgresql.org X-Gm-Message-State: AOJu0Yx1tGUyM6JjsBOkKmfCsGoRoJID6fwuXfmsc2IUWMyayhAJZzZS 4uSpY7yK8WQST2RfDQynG97IWq1Nb+5EmLnCeaKKGUS4qHs6VtbhqNeiTharZwDg7Pcv80z44V4 4382Dj5aeSbjCJ6m2XUqsHKgIMNhz2IM= X-Gm-Gg: AfdE7cn+aLDKiCl7ctaNWicc0rNteiJOs24dQ3HHkZJ/SrB4QcyvbZK8mIsWVlcWx3W rU6IIufwNP1DZ+OWF4ZHkanTYBBupd+gtWfta11wpPQqSaj42zIFBXn4S98BwK5E4XWTGw8l+ZQ UT7h9YJxBlgIChe5moC3GyT3DTku3JKqRcGTJ42c5UKPGFVXeqpLGxmiSYh4/nEB8x4k6BtYXmE WTOfhQRChmoHC9DpIuXu6uh7U2HXzkyyBpyQGu1cigsLg0EluvhNBGfpq6FU65IXHYWGkgPLU5M 7pgwfYj/M/XhOYwgq35vinSXIIpzaB5n+0YYMXdnjDY7SwQ9B4s6r9svj4hkcfjbYSSVbxPr8I5 fEIVhhS+ukJZRMTP8+SyZZACUkSkl95fqOj5vLjhhjYNsQiBv X-Received: by 2002:a05:6a21:b93:b0:3a2:ccb5:abc7 with SMTP id adf61e73a8af0-3bfed237b5emr3753315637.13.1782938105706; Wed, 01 Jul 2026 13:35:05 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Corey Huinker Date: Wed, 1 Jul 2026 16:34:53 -0400 X-Gm-Features: AVVi8CeDpQYkGWClRrZ5CrnAAgQDzyu5XZMfUZNefJZdUfgys8rYSDTBmpNzlcM Message-ID: Subject: Re: use of SPI by postgresImportForeignStatistics To: Etsuro Fujita Cc: Robert Haas , "pgsql-hackers@postgresql.org" Content-Type: multipart/alternative; boundary="000000000000727b9e065592a073" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000727b9e065592a073 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Jul 1, 2026 at 8:10=E2=80=AFAM Etsuro Fujita wrote: > On Wed, Jul 1, 2026 at 12:55=E2=80=AFAM Corey Huinker > wrote: > > On Tue, Jun 30, 2026 at 7:30=E2=80=AFAM Etsuro Fujita > wrote: > > >> As postgresImportForeignStatistics() is provided for FDWs that want to > >> calculate statistics remotely, so I'm assuming that they have enough > >> knowledge about the stats-data structures. > > CORRECTION: s/postgresImportForeignStatistics()/ImportForeignStatistics()= / > Sorry for that. > > >> 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 wi= th > 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 available, we create a barrier to making those same functions error-safe in the future= . > 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 th= e > very narrow "this_string is an invalid this_type", for string that the us= er > 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 error-safe potentially disrupts other callers. Someone please let me know if I'm being unnecessarily cautious here. I modified the patch further. To support these differences: > > "There are important differences in the parameters needed by the > different types of functions. The pg_restore_*() SQL function calls > need to identify the schema+relname of the relation being modified, > and already have all of the values as typed Datums, whereas the > internal API calls already have an identified and locked open > Relation, but all of the statistical parameters are just cstrings." > > I incorporated your v2-0010 and v2-0012 into the patch: 1) separate > the guts of relation_statistics_update() and > attribute_statistics_update() into new functions > relation_statistics_update_internal() and > attribute_statistics_update_internal(), and 2) modify > import_relation_statistics() and import_attribute_statistics() to call > these new functions instead. Also, I changed the stats-data > argument-type in import_relation_statistics() and > import_attribute_statistics() from NullableDatum * to const > NullableDatum *. > > Best regards, > Etsuro Fujita > 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 issues 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. 2. Did you put import_relation_statistics and import_attribute_statistics in statistics.h because you see them as long-term publicly visible functions, and what's in stats_utils.h to be more subject to change? If so, I could get behind that. If not, then I fall back to my position that they seem like they belong in new relation_stats.h and attribute_stats.h, or stats_utils.h. 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. --000000000000727b9e065592a073 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


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 Co= rey Huinker <corey.huinker@gmail.com> wrote:
> On Tue, Jun 30, 2026 at 7:30=E2=80=AFAM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

>> As postgresImportForeignStatistics() is provided for FDWs that wan= t to
>> calculate statistics remotely, so I'm assuming that they have = enough
>> knowledge about the stats-data structures.

CORRECTION: s/postgresImportForeignStatistics()/ImportForeignStatistics()/<= br> =C2=A0Sorry for that.

>> 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.=C2=A0 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.=C2=A0 I moved the set_*_arg functions to
stat_utils.c, and renamed them to stats_set_*_arg.=C2=A0 Attached is a new<= br> version of the patch.

> 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 th= at the user most certainly never saw. Having said that, it's something = we could easily 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 proble= m. But if they're available outside of postgres_fdw then changing them = to be error-safe potentially disrupts other callers. Someone please let me = know if I'm being unnecessarily cautious here.

I modified the patch further.= =C2=A0 To support these differences:

"There are important differences in the parameters needed by the
different types of functions. The pg_restore_*() SQL function calls
need to identify the schema+relname of the relation being modified,
and already have all of the values as typed Datums, whereas the
internal API calls already have an identified and locked open
Relation, but all of the statistical parameters are just cstrings."
I incorporated your v2-0010 and v2-0012 into the patch: 1) separate
the guts of relation_statistics_update() and
attribute_statistics_update() into new functions
relation_statistics_update_internal() and
attribute_statistics_update_internal(), and 2) modify
import_relation_statistics() and import_attribute_statistics() to call
these new functions instead.=C2=A0 Also, I changed the stats-data
argument-type in import_relation_statistics() and
import_attribute_statistics() from NullableDatum * to const
NullableDatum *.

Best regards,
Etsuro Fujita

Other Thoughts:

1. The internal functions should accept a server_version_number pa= rameter, 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 transla= tion issues going forward with statistics, but someday that won't be th= e case, and if we don't have this parameter in place then it'll be = too late to fix it.

2. Did you put=C2=A0import_rel= ation_statistics and=C2=A0import_attribute_statistics in statistics.h becau= se you see them as long-term publicly visible functions, and what's in = stats_utils.h to be more subject to change? If so, I could get behind that.= If not, then I fall back to my position that they seem like they belong in= new relation_stats.h and attribute_stats.h, or stats_utils.h.=C2=A0
3. The import_stats_functions in their current "bridging&q= uot; form should do null checks on all of the NullableDatum pointers, or at= least Assert()s on them.
--000000000000727b9e065592a073--