Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Mark PQfn() unsafe and fix overrun in frontend LO interface.
- bd48114937c8 19 (unreleased) cited
Update libpq to make new features of FE/BE protocol available to
- efc3a25bb02a 7.4.1 cited

future of PQfn()

Nathan Bossart <nathandbossart@gmail.com> — 2026-05-26T16:05:47Z

PQfn() was marked "somewhat obsolete" in commit efc3a25bb0 (2003), and was
later marked "unsafe" in commit bd48114937 (2026).  I looked around for
third-party code that uses this interface but found none, and the only
internal usage is for the frontend LO interface.  As part of the latter
commit, a special PQnfn() function was added for use by the fronend LO
functions, but this was not exported by libpq.

Given the above, I'd like to propose retiring PQfn() in v20.  Since it's an
exported symbol, we can't just delete the code, but we could have it
unconditionally error.  Assuming folks are okay with that, I'm wondering
what we should do with the relevant documentation.  Should we leave a stub
with a note about its removal, or should we just wipe all mentions?  I'm
currently leaning towards leaving a note, but I could see the argument
that's not even worth doing given the lack of uptake.

The other question is what to do with the frontend LO code.  The simplest
thing we can do is to leave PQnfn() around as an internal function that is
only used by this interface.  Alternatively, we could take our own advice
and used a prepared statement with binary transmission of params/results,
but that has two key problems: 1) potential name collisions with
user-created prepared statements and 2) breakage after DISCARD/DEALLOCATE,
which I haven't come up with a good way to deal with.  Another approach we
could take is to just send the query via PQexecParams(), but a simple test
(creating and unlinking 10K LOs) showed a ~41% slowdown compared to HEAD.
So, I guess we'll need to keep PQnfn() around for now...

Thoughts?

-- 
nathan

Re: future of PQfn()

Christoph Berg <myon@debian.org> — 2026-05-26T16:53:31Z

Re: Nathan Bossart
> PQfn() was marked "somewhat obsolete" in commit efc3a25bb0 (2003), and was
> later marked "unsafe" in commit bd48114937 (2026).  I looked around for
> third-party code that uses this interface but found none

I found some references via Debian's codesearch:

https://sources.debian.org/src/fpc/3.2.2+dfsg-50/fpcsrc/packages/postgres/src/postgres3dyn.pp?hl=141#L141

  PQfn : function (conn:PPGconn; fnid:longint; result_buf:Plongint; result_len:Plongint; result_is_int:longint;args:PPQArgBlock; nargs:longint):PPGresult;cdecl;
{ Accessor functions for PGresult objects  }

https://sources.debian.org/src/clisp/1:2.49.20250504.gitf662209-2/modules/postgresql/postgresql.lisp?hl=375#L375

;; "Fast path" interface --- not really recommended for application use
(def-call-out PQfn (:return-type PGresult)
  (:arguments (conn PGconn) (fnid int) (result_buf (c-ptr int) :out)
              (result_len (c-ptr int) :out) (result_is_int int)
              (args (c-array-ptr PQArgBlock)) ; at least nargs
              (nargs int)))

https://sources.debian.org/src/rust-pq-sys/0.7.5-1/src/bindings_linux_32.rs?hl=745#L745

unsafe extern "C" {
    pub fn PQfn(
        conn: *mut PGconn,
        fnid: ::std::os::raw::c_int,
        result_buf: *mut ::std::os::raw::c_int,
        result_len: *mut ::std::os::raw::c_int,
        result_is_int: ::std::os::raw::c_int,
        args: *const PQArgBlock,
        nargs: ::std::os::raw::c_int,
    ) -> *mut PGresult;
}

https://sources.debian.org/src/vala/0.56.19-1/vapi/libpq.vapi?hl=349#L349

	[CCode (cname = "PQfn")]
		public Result fn (int fnid, [CCode (array_length = false)] int[] result_buf, out int result_len, int result_is_int, ArgBlock[] args);

https://sources.debian.org/src/libdbi-drivers/0.9.0-13/drivers/pgsql/dbd_pgsql.h?hl=650#L650

#define PGSQL_CUSTOM_FUNCTIONS { \
...
        "PQfn", \

All seem to be libpq wrappers.

Then there is a handful of packages with a copy of libpq-fe.h that
includes the function declaration.

... plus two other mentioning PQfn as "unimplemented".

> Given the above, I'd like to propose retiring PQfn() in v20.  Since it's an
> exported symbol, we can't just delete the code, but we could have it
> unconditionally error.

Keeping the symbol but making it return an error seems ok from the
above I think.

Christoph

Re: future of PQfn()

Christoph Berg <myon@debian.org> — 2026-05-26T16:55:15Z

Re: To Nathan Bossart
> I found some references via Debian's codesearch:

I meant to include the URL for that:

https://codesearch.debian.net/search?q=PQfn%5Cb&literal=0

PQnfn has no hits outside of postgresql-18.

Christoph

Re: future of PQfn()

Jacob Champion <jacob.champion@enterprisedb.com> — 2026-05-26T17:42:47Z

On Tue, May 26, 2026 at 9:05 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Given the above, I'd like to propose retiring PQfn() in v20.  Since it's an
> exported symbol, we can't just delete the code, but we could have it
> unconditionally error.

That seems reasonable to me. (We may want to exercise the
PqMsg_FunctionCall message more explicitly in our test suite at the
same time we do that...)

> Assuming folks are okay with that, I'm wondering
> what we should do with the relevant documentation.  Should we leave a stub
> with a note about its removal, or should we just wipe all mentions?  I'm
> currently leaning towards leaving a note, but I could see the argument
> that's not even worth doing given the lack of uptake.

I think we can probably wipe it out, personally.

> The other question is what to do with the frontend LO code.  The simplest
> thing we can do is to leave PQnfn() around as an internal function that is
> only used by this interface.  Alternatively, we could take our own advice
> and used a prepared statement with binary transmission of params/results,
> but that has two key problems: 1) potential name collisions with
> user-created prepared statements and 2) breakage after DISCARD/DEALLOCATE,
> which I haven't come up with a good way to deal with.  Another approach we
> could take is to just send the query via PQexecParams(), but a simple test
> (creating and unlinking 10K LOs) showed a ~41% slowdown compared to HEAD.
> So, I guess we'll need to keep PQnfn() around for now...

Short-term, keeping it around seems fine.

Long-term, it doesn't feel great that the alternatives we tell other
people to use are... worse. Surely other clients of libpq run into the
layering violation problem with prepared statements, as well?

--Jacob

Re: future of PQfn()

Nathan Bossart <nathandbossart@gmail.com> — 2026-05-26T19:55:09Z

On Tue, May 26, 2026 at 10:42:47AM -0700, Jacob Champion wrote:
> Short-term, keeping it around seems fine.
> 
> Long-term, it doesn't feel great that the alternatives we tell other
> people to use are... worse. Surely other clients of libpq run into the
> layering violation problem with prepared statements, as well?

Yup.  Here's a related note in JDBC:

	https://github.com/pgjdbc/pgjdbc/blob/cf2d89ec/docs/content/documentation/server-prepare.md?plain=1#L305-L315

I wonder how difficult it would be to teach the protocol to advise clients
when prepared statements are deallocated...

FWIW I'm less concerned about the name collision problem.  I was thinking
we could just document that libpq manages statements with a prefix like
"libpq_internal_".  Any problems in that area seem likely to be intentional
breakage that we needn't worry about.

-- 
nathan

Re: future of PQfn()

Jelte Fennema-Nio <postgres@jeltef.nl> — 2026-05-26T21:36:49Z

On Tue, 26 May 2026 at 18:05, Nathan Bossart <nathandbossart@gmail.com> 
wrote:
> Another approach we
> could take is to just send the query via PQexecParams(), but a simple test
> (creating and unlinking 10K LOs) showed a ~41% slowdown compared to HEAD.
> So, I guess we'll need to keep PQnfn() around for now...
>
> Thoughts?

I had a small WIP patch (fully AI generated and not yet vetted by me)
lying around for unrelated reasons to make the extended protocol perform
closer to the simple protocol with pgbench --select-only by using
CreateOneShotCachedPlan to create the plan for unnamed prepared
statements (see attached).

Could you share the simple LO test you were running here and/or rerun it
with this patch applied? I'd love to know if the patch reduces the
slowdown significantly, or if something else is the bottleneck.

Re: future of PQfn()

Nathan Bossart <nathandbossart@gmail.com> — 2026-05-27T18:23:04Z

On Tue, May 26, 2026 at 11:36:49PM +0200, Jelte Fennema-Nio wrote:
> Could you share the simple LO test you were running here and/or rerun it
> with this patch applied? I'd love to know if the patch reduces the
> slowdown significantly, or if something else is the bottleneck.

The test is just:

    int main()
    {
        PCconn *conn = PQsetdb(NULL, NULL, NULL, NULL, "postgres");

        for (int i = 0; i < 1000000; i++)
            lo_create(conn, i);

        for (int i = 0; i < 1000000; i++)
            lo_unlink(conn, i);
    }

Before applying your patch -> 0.457 seconds (best of ~10)
After applying your patch  -> 0.444 seconds (best of ~10)

For reference, HEAD runs this test in 0.319 seconds.

I've attached my work-in-progress patches here, in case you're interested.
0001 switches the frontend LO interface to use prepared statements, and
0002 removes PQfn() entirely.  0003 applies on top of those two and
switches the frontend LO interface to use PQexecParams() instead.

-- 
nathan

Re: future of PQfn()

Jacob Champion <jacob.champion@enterprisedb.com> — 2026-05-27T21:39:53Z

On Tue, May 26, 2026 at 12:55 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
> I wonder how difficult it would be to teach the protocol to advise clients
> when prepared statements are deallocated...

Probably not too difficult. But it seems like most, if not all, of the
stuff in the DISCARD ALL umbrella is a target for a feature like
that... This feels a lot like the perennial request for proxies to be
able to separate their own context from the per-application/per-user
contexts running on top of them.

--Jacob

Re: future of PQfn()

Nathan Bossart <nathandbossart@gmail.com> — 2026-05-28T15:17:41Z

On Wed, May 27, 2026 at 02:39:53PM -0700, Jacob Champion wrote:
> On Tue, May 26, 2026 at 12:55 PM Nathan Bossart
> <nathandbossart@gmail.com> wrote:
>> I wonder how difficult it would be to teach the protocol to advise clients
>> when prepared statements are deallocated...
> 
> Probably not too difficult. But it seems like most, if not all, of the
> stuff in the DISCARD ALL umbrella is a target for a feature like
> that... This feels a lot like the perennial request for proxies to be
> able to separate their own context from the per-application/per-user
> contexts running on top of them.

I've been thinking through a bunch of options here, and a new protocol
message seems like the best choice, if for no other reason than it solves a
general problem for clients.  I'm working on a patch that I'll post in a
new thread.

-- 
nathan

Re: future of PQfn()

Nathan Bossart <nathandbossart@gmail.com> — 2026-05-29T15:14:29Z

On Wed, May 27, 2026 at 02:39:53PM -0700, Jacob Champion wrote:
> On Tue, May 26, 2026 at 12:55 PM Nathan Bossart
> <nathandbossart@gmail.com> wrote:
>> I wonder how difficult it would be to teach the protocol to advise clients
>> when prepared statements are deallocated...
> 
> Probably not too difficult. But it seems like most, if not all, of the
> stuff in the DISCARD ALL umbrella is a target for a feature like
> that... This feels a lot like the perennial request for proxies to be
> able to separate their own context from the per-application/per-user
> contexts running on top of them.

Here is a work-in-progress patch set that goes this direction.  This
introduces a callback mechanism in libpq that is used to handle statement
deallocation notifications.  Older servers/clients fall back to
PQexecParams(), which is slower, but the alternative is to leave PQnfn()
and related code around indefinitely.

I'm wondering whether this new message type is general enough.  For
example, perhaps we could make an extensible message type for tracking
various things.  And I want to ensure this is useful for other clients,
too.

-- 
nathan

Re: future of PQfn()

Jacob Champion <jacob.champion@enterprisedb.com> — 2026-05-29T15:43:07Z

On Fri, May 29, 2026 at 8:14 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Here is a work-in-progress patch set that goes this direction.

At a high level, I think advertising support for a single new message
needs to be done in a protocol extension rather than a minor version
bump.

> This
> introduces a callback mechanism in libpq that is used to handle statement
> deallocation notifications.  Older servers/clients fall back to
> PQexecParams(), which is slower, but the alternative is to leave PQnfn()
> and related code around indefinitely.

IMO there's no hurry in getting rid of that path. If we decide to go
this direction, a fallback to PQnfn() seems like it'd fine for a few
releases; we could eventually swap to a PQexecParams() fallback and
get rid of the extra code once the older servers have aged out.

> I'm wondering whether this new message type is general enough.  For
> example, perhaps we could make an extensible message type for tracking
> various things.  And I want to ensure this is useful for other clients,
> too.

If it's just a general notification message, what does negotiating
"support" mean? Is best-effort notification okay, if the client has no
idea what a future message type means, or if the server doesn't send
the specific type of message the client is hoping for?

(In general, I'm kind of down on the "notify the client that X
happened" method of working around architectural issues. Maybe that's
what we need to move this specific part forward, but it doesn't feel
like a long-term solution and I don't know that we need to genericize
it without a solid set of use cases.)

--Jacob

Re: future of PQfn()

Nathan Bossart <nathandbossart@gmail.com> — 2026-05-29T16:10:58Z

On Fri, May 29, 2026 at 08:43:07AM -0700, Jacob Champion wrote:
> On Fri, May 29, 2026 at 8:14 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Here is a work-in-progress patch set that goes this direction.
> 
> At a high level, I think advertising support for a single new message
> needs to be done in a protocol extension rather than a minor version
> bump.

WFM

>> This
>> introduces a callback mechanism in libpq that is used to handle statement
>> deallocation notifications.  Older servers/clients fall back to
>> PQexecParams(), which is slower, but the alternative is to leave PQnfn()
>> and related code around indefinitely.
> 
> IMO there's no hurry in getting rid of that path. If we decide to go
> this direction, a fallback to PQnfn() seems like it'd fine for a few
> releases; we could eventually swap to a PQexecParams() fallback and
> get rid of the extra code once the older servers have aged out.

That's fine with me, too.

>> I'm wondering whether this new message type is general enough.  For
>> example, perhaps we could make an extensible message type for tracking
>> various things.  And I want to ensure this is useful for other clients,
>> too.
> 
> If it's just a general notification message, what does negotiating
> "support" mean? Is best-effort notification okay, if the client has no
> idea what a future message type means, or if the server doesn't send
> the specific type of message the client is hoping for?

That's what I had in mind.  But if we don't have anything specific in mind
that this mechanism could be extended to support, maybe we shouldn't
bother.  Especially if we can just add protocol extensions as necessary.

> (In general, I'm kind of down on the "notify the client that X
> happened" method of working around architectural issues. Maybe that's
> what we need to move this specific part forward, but it doesn't feel
> like a long-term solution and I don't know that we need to genericize
> it without a solid set of use cases.)

I'm certainly open to other ideas, but I'm afraid this is the best I've
come up with in my admittedly limited time thinking about the problem.

-- 
nathan

Re: future of PQfn()

Jacob Champion <jacob.champion@enterprisedb.com> — 2026-05-29T16:33:03Z

On Fri, May 29, 2026 at 9:11 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I'm certainly open to other ideas, but I'm afraid this is the best I've
> come up with in my admittedly limited time thinking about the problem.

No worries -- I hadn't meant to block progress here on protocol
design. I think keeping PQnfn() for the immediate future is a good
plan. I just wanted to plant a seed for getting away from this problem
eventually.

(As for pie-in-the-sky alternative ideas, the ability for middleware
to separate contexts or streams of packets has come up before. libpq
could theoretically mark its own "context" of server-side allocations
that are not touched by an application-context DISCARD.)

--Jacob

Re: future of PQfn()

Nathan Bossart <nathandbossart@gmail.com> — 2026-05-29T16:42:27Z

For future reference in the archives, I'm moving the discussion about 0001
(the prepared statement deallocation notification mechanism) to a new
thread:

	https://postgr.es/m/ahm_4eOKkkKJ3Gds%40nathan

On Fri, May 29, 2026 at 09:33:03AM -0700, Jacob Champion wrote:
> On Fri, May 29, 2026 at 9:11 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> I'm certainly open to other ideas, but I'm afraid this is the best I've
>> come up with in my admittedly limited time thinking about the problem.
> 
> No worries -- I hadn't meant to block progress here on protocol
> design. I think keeping PQnfn() for the immediate future is a good
> plan. I just wanted to plant a seed for getting away from this problem
> eventually.
> 
> (As for pie-in-the-sky alternative ideas, the ability for middleware
> to separate contexts or streams of packets has come up before. libpq
> could theoretically mark its own "context" of server-side allocations
> that are not touched by an application-context DISCARD.)

Along these lines, I did consider "pinning" statements or even having
"built-in" ones for libpq.  I didn't like the "pinning" idea because that
seemed problematic for connection poolers.  And the "built-in" idea seemed
too libpq-centric for what I'd argue is a general problem.  The other ideas
involved guessing at what's happening based on the queries or somehow
trying to handle failures due to missing/wrong prepared statements, none of
which felt viable.

-- 
nathan

Re: future of PQfn()

Jacob Champion <jacob.champion@enterprisedb.com> — 2026-05-29T17:04:16Z

On Fri, May 29, 2026 at 9:42 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Along these lines, I did consider "pinning" statements or even having
> "built-in" ones for libpq.  I didn't like the "pinning" idea because that
> seemed problematic for connection poolers.

Right -- whether a general context, or multiplexed streams, or
explicit pins, proxies would have to be intimately aware of them. They
can then layer their own on top, or make sure different client
requests don't conflict, or release them on client disconnection...

> And the "built-in" idea seemed
> too libpq-centric for what I'd argue is a general problem.  The other ideas
> involved guessing at what's happening based on the queries or somehow
> trying to handle failures due to missing/wrong prepared statements, none of
> which felt viable.

Agreed. (Although, for that last point, I wondered whether we could
make this idempotent somehow. I think the answer is "not worth it".)

--Jacob