Horton

Thread

Commits

  1. Don't use a tuplestore if we don't have to for SQL-language functions.

  1. SQL functions: avoid making a tuplestore unnecessarily

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-17T19:59:28Z

    (I'm not proposing this for v18, but I wanted to get the patch
written while functions.c is still fresh in mind.)

The attached patch changes functions.c so that we only make a
tuplestore object if we're actually going to return multiple
rows in it, that is if it's a set-returning function and we
cannot use "lazyEval" mode.  Otherwise, we can just rely on
the result slot we made anyway to hold the one tuple we need.

This saves a small number of cycles by not shoving a tuple into the
tuplestore only to pull it right back out.  But the real reason for
changing it is not speed but resource-management worries.  The
existing code (as it was before 0dca5d68d and is again as of
0313c5dc6) makes the tuplestore in the potentially long-lived fn_mcxt.
This'd be merely a slight annoyance if a tuplestore were purely a
memory object, but it isn't: it might contain an open file.  If it
does, that file reference will be backed by a ResourceOwner,
specifically whichever ResourceOwner was CurrentResourceOwner at the
time of creating the tuplestore.  What I am afraid of is that I don't
think there's any guarantee that that ResourceOwner is as long-lived
as the FmgrInfo.  It should be fine within normal SQL query execution,
where the FmgrInfo will be part of the executor's state tree.  But
there are long-lived FmgrInfos, such as those within the typcache, or
within an index's relcache entry.  What if somebody tries to use a
SQL function to implement functionality that's reached through those
mechanisms?

Given the lack of field complaints over the many years it's been
like this, there doesn't seem to be a live problem.  I think the
explanation for that is
(1) those mechanisms are never used to call set-returning functions,
(2) therefore, the tuplestore will not be called on to hold more
than one result row,
(3) therefore, it won't get large enough that it wants to spill
to disk,
(4) therefore, its potentially dangling resowner pointer is never
used.
However, this is an uncomfortably shaky chain of assumptions.
I want to cut it down by fixing things so that there is no
tuplestore, period, in a non-set-returning SQL function.

Furthermore, this patch changes things so that when we do make a
tuplestore, it lives in the per-query context not the fn_mcxt,
providing additional surety that it won't outlive its ResourceManager.
This is a change I made in 0dca5d68d and then had to undo for
performance reasons, but because of these resource-lifetime
considerations I really want it back that way again.

Since I think there is probably not a reachable bug here today,
I'm going to park this for v19.

			regards, tom lane

  2. Re: SQL functions: avoid making a tuplestore unnecessarily

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-02T18:46:50Z

    I wrote:
> Given the lack of field complaints over the many years it's been
> like this, there doesn't seem to be a live problem.  I think the
> explanation for that is
> (1) those mechanisms are never used to call set-returning functions,
> (2) therefore, the tuplestore will not be called on to hold more
> than one result row,
> (3) therefore, it won't get large enough that it wants to spill
> to disk,
> (4) therefore, its potentially dangling resowner pointer is never
> used.
> However, this is an uncomfortably shaky chain of assumptions.
> I want to cut it down by fixing things so that there is no
> tuplestore, period, in a non-set-returning SQL function.

Following up on this thread: Alexander Lakhin's report at [1] shows
that point (3) above is wrong: the tuplestore code will spill to disk
even when holding just one tuple, if that tuple is bigger than the
tuplestore's allowed work_mem.  (This seems kinda dubious to me, since
no memory savings can ensue.  But I have no desire to rejigger that
right now.)  So there may actually be a live bug associated with
use of a deleted resource owner here.  I've not tried to pursue that
though.

More immediately: Alexander's report also shows that there's an easily
reached bug in HEAD when the tuplestore does spill to disk.  When it
reads that tuple back in, it allocates it in the caller's memory
context, and fmgr_sql is now calling that in the short-lived context
it was called in not in its long-lived fcontext.  The end result of
that is that the long-lived result TupleTableSlot is now holding a
should_free pointer to a short-lived tuple, which ends up in an
attempt to pfree already-wiped storage during the next call of the
SQL function.

The patch I presented here eliminates that problem because with it,
fmgr_sql no longer pulls tuples out of the tuplestore at all.
So I want to apply this patch now instead of holding it for v19.

			regards, tom lane

[1] https://www.postgresql.org/message-id/9f975803-1a1c-4f21-b987-f572e110e860%40gmail.com