Subject: Re: use of SPI by postgresImportForeignStatistics

From: Etsuro Fujita <[email-masked]>
Date: Fri, 10 Jul 2026 14:03:04 +0900

On Wed, Jul 8, 2026 at 9:04 PM Etsuro Fujita <[email-masked]> wrote:
> Attached is a new version
> of the patch.  I'm planning to push and back-patch it, if no
> objections.

Committed and back-patched.

Best regards,
Etsuro Fujita

---

From: Fujii Masao <[email-masked]>
Date: Fri, 10 Jul 2026 14:36:40 +0900

> Committed and back-patched.

In the committed patch, I found that the set_*_arg() helper functions
in postgres_fdw.c are declared static in their prototypes, but their
definitions omit the static keyword. This looks like an oversight.

Attached patch adds static to the definitions for consistency and
to make their intended file-local scope explicit. It also fixes a few
nearby comment typos.

Regards,

-- 
Fujii Masao

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 10 Jul 2026 16:40:51 +0900

Fujii-san,

> to make their intended file-local scope explicit. It also fixes a few

Good catch!  Thanks for the patch!  LGTM.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 10 Jul 2026 20:36:57 +0900

> Good catch!  Thanks for the patch!  LGTM.

My compiler (Apple clang version 17.0.0 (clang-1700.4.4.1)) doesn't
output any error/warning about that declaration.  Actually, it's
allowed?  Anyway, +1 for adding it for consistency.

Best regards,
Etsuro Fujita

---

From: Fujii Masao <[email-masked]>
Date: Fri, 10 Jul 2026 21:09:46 +0900

> allowed?  Anyway, +1 for adding it for consistency.

Maybe that's allowed. I wonder adding static to both the declaration
and the definition seems mainly a matter of consistency, readability,
and coding style.

Thanks for the review! I've pushed the patch.

Regards,

-- 
Fujii Masao

---

From: Tom Lane <[email-masked]>
Date: Fri, 10 Jul 2026 10:08:05 -0400

> allowed?  Anyway, +1 for adding it for consistency.

The back story here is that that coding pattern is allowed by standard
C but traditionally (pre-ANSI C or so) wasn't.  We used to have some
buildfarm animals that would warn about it, but none do today.

I think it's a good idea to include "static" in the definitions for
clarity, and so that you don't have to go looking for the declaration
to know if a function is file-local or not.  But the standard doesn't
require that.

Rummaging in the gcc manual, it looks like you can turn on a warning
for this with '-Wtraditional', but that also enables a boatload of
warnings we don't want, so I can't see using it on a regular basis.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Fri, 10 Jul 2026 10:41:38 -0400

> warnings we don't want, so I can't see using it on a regular basis.

I experimented with that for fun.  On current HEAD, it produces
347111 warnings, mostly "traditional C rejects ISO C style function
definitions".  But amongst the dross there's

dbcommands.c:395:1: warning: non-static declaration of 'ScanSourceDatabasePgClassTuple' follows static declaration [-Wtraditional]
event_trigger.c:392:1: warning: non-static declaration of 'SetDatabaseHasLoginEventTriggers' follows static declaration [-Wtraditional]
tablecmds.c:20846:1: warning: non-static declaration of 'ConstraintImpliedByRelConstraint' follows static declaration [-Wtraditional]
nodeModifyTable.c:4154:1: warning: non-static declaration of 'ExecInitMerge' follows static declaration [-Wtraditional]
postmaster.c:4056:1: warning: non-static declaration of 'StartSysLogger' follows static declaration [-Wtraditional]
buf_table.c:48:1: warning: non-static declaration of 'BufTableShmemRequest' follows static declaration [-Wtraditional]
oracle_compat.c:555:1: warning: non-static declaration of 'dobyteatrim' follows static declaration [-Wtraditional]
pg_locale_icu.c:746:1: warning: non-static declaration of 'strncoll_icu_utf8' follows static declaration [-Wtraditional]
pg_locale_icu.c:767:1: warning: non-static declaration of 'strcoll_icu_utf8' follows static declaration [-Wtraditional]
pg_walsummary.c:230:1: warning: non-static declaration of 'walsummary_error_callback' follows static declaration [-Wtraditional]
pg_walsummary.c:245:1: warning: non-static declaration of 'walsummary_read_callback' follows static declaration [-Wtraditional]
vacuumdb.c:327:1: warning: non-static declaration of 'check_objfilter' follows static declaration [-Wtraditional]

if anyone cares to follow that up.

			regards, tom lane