I have been finding a lot of interesting stuff while looking into
the buffer reference count/leakage issue.

It turns out that there were two specific things that were camouflaging
the existence of bugs in this area:

1. The BufferLeakCheck routine that's run at transaction commit was
only looking for nonzero PrivateRefCount to indicate a missing unpin.
It failed to notice nonzero LastRefCount --- which meant that an
error in refcount save/restore usage could leave a buffer pinned,
and BufferLeakCheck wouldn't notice.

2. The BufferIsValid macro, which you'd think just checks whether
it's handed a valid buffer identifier or not, actually did more:
it only returned true if the buffer ID was valid *and* the buffer
had positive PrivateRefCount. That meant that the common pattern
if (BufferIsValid(buf))
ReleaseBuffer(buf);
wouldn't complain if it were handed a valid but already unpinned buffer.
And that behavior masks bugs that result in buffers being unpinned too
early. For example, consider a sequence like

1. LockBuffer (buffer now has refcount 1). Store reference to
a tuple on that buffer page in a tuple table slot.
2. Copy buffer reference to a second tuple-table slot, but forget to
increment buffer's refcount.
3. Release second tuple table slot. Buffer refcount drops to 0,
so it's unpinned.
4. Release original tuple slot. Because of BufferIsValid behavior,
no assert happens here; in fact nothing at all happens.

This is, of course, buggy code: during the interval from 3 to 4 you
still have an apparently valid tuple reference in the original slot,
which someone might try to use; but the buffer it points to is unpinned
and could be replaced at any time by another backend.

In short, we had errors that would mask both missing-pin bugs and
missing-unpin bugs. And naturally there were a few such bugs lurking
behind them...

3. The buffer refcount save/restore stuff, which I had suspected
was useless, is not only useless but also buggy. The reason it's
buggy is that it only works if used in a nested fashion. You could
save state A, pin some buffers, save state B, pin some more
buffers, restore state B (thereby unpinning what you pinned since
the save), and finally restore state A (unpinning the earlier stuff).
What you could not do is save state A, pin, save B, pin more, then
restore state A --- that might unpin some of A's buffers, or some
of B's buffers, or some unforeseen combination thereof. If you
restore A and then restore B, you do not necessarily return to a zero-
pins state, either. And it turns out the actual usage pattern was a
nearly random sequence of saves and restores, compounded by a failure to
do all of the restores reliably (which was masked by the oversight in
BufferLeakCheck).


What I have done so far is to rip out the buffer refcount save/restore
support (including LastRefCount), change BufferIsValid to a simple
validity check (so that you get an assert if you unpin something that
was pinned), change ExecStoreTuple so that it increments the refcount
when it is handed a buffer reference (for symmetry with ExecClearTuple's
decrement of the refcount), and fix about a dozen bugs exposed by these
changes.

I am still getting Buffer Leak notices in the "misc" regression test,
specifically in the queries that invoke more than one SQL function.
What I find there is that SQL functions are not always run to
completion. Apparently, when a function can return multiple tuples,
it won't necessarily be asked to produce them all. And when it isn't,
postquel_end() isn't invoked for the function's current query, so its
tuple table isn't cleared, so we have dangling refcounts if any of the
tuples involved are in disk buffers.

It may be that the save/restore code was a misguided attempt to fix
this problem. I can't tell. But I think what we really need to do is
find some way of ensuring that Postquel function execution contexts
always get shut down by the end of the query, so that they don't leak
resources.

I suppose a straightforward approach would be to keep a list of open
function contexts somewhere (attached to the outer execution context,
perhaps), and clean them up at outer-plan shutdown.

What I am wondering, though, is whether this addition is actually
necessary, or is it a bug that the functions aren't run to completion
in the first place? I don't really understand the semantics of this
"nested dot notation". I suppose it is a Berkeleyism; I can't find
anything about it in the SQL92 document. The test cases shown in the
misc regress test seem peculiar, not to say wrong. For example:

regression=> SELECT p.hobbies.equipment.name, p.hobbies.name, p.name FROM person p;
name |name |name
-------------+-----------+-----
advil |posthacking|mike
peet's coffee|basketball |joe
hightops |basketball |sally
(3 rows)

which doesn't appear to agree with the contents of the underlying
relations:

regression=> SELECT * FROM hobbies_r;
name |person
-----------+------
posthacking|mike
posthacking|jeff
basketball |joe
basketball |sally
skywalking |
(5 rows)

regression=> SELECT * FROM equipment_r;
name |hobby
-------------+-----------
advil |posthacking
peet's coffee|posthacking
hightops |basketball
guts |skywalking
(4 rows)

I'd have expected an output along the lines of

advil |posthacking|mike
peet's coffee|posthacking|mike
hightops |basketball |joe
hightops |basketball |sally

Is the regression test's expected output wrong, or am I misunderstanding
what this query is supposed to do? Is there any documentation anywhere
about how SQL functions returning multiple tuples are supposed to
behave?

regards, tom lane

Search Discussions

  • Tom Lane at Sep 23, 1999 at 1:22 am

    I wrote:
    What I have done so far is to rip out the buffer refcount save/restore
    support (including LastRefCount), change BufferIsValid to a simple
    validity check (so that you get an assert if you unpin something that
    was pinned), ...
    er, make that "unpin something that *wasn't* pinned".

    regards, tom lane
  • Jan Wieck at Sep 23, 1999 at 1:26 am

    Tom Lane wrote:

    [...]

    What I am wondering, though, is whether this addition is actually
    necessary, or is it a bug that the functions aren't run to completion
    in the first place? I don't really understand the semantics of this
    "nested dot notation". I suppose it is a Berkeleyism; I can't find
    anything about it in the SQL92 document. The test cases shown in the
    misc regress test seem peculiar, not to say wrong. For example:

    [...]

    Is the regression test's expected output wrong, or am I misunderstanding
    what this query is supposed to do? Is there any documentation anywhere
    about how SQL functions returning multiple tuples are supposed to
    behave?
    I've said some time (maybe too long) ago, that SQL functions
    returning tuple sets are broken in general. This nested dot
    notation (which I think is an artefact from the postquel
    querylanguage) is implemented via set functions.

    Set functions have total different semantics from all other
    functions. First they don't really return a tuple set as
    someone might think - all that screwed up code instead
    simulates that they return something you could consider a
    scan of the last SQL statement in the function. Then, on
    each subsequent call inside of the same command, they return
    a "tupletable slot" containing the next found tuple (that's
    why their Func node is mangled up after the first call).

    Second they have a targetlist what I think was originally
    intended to extract attributes out of the tuples returned
    when the above scan is asked to get the next tuple. But as I
    read the code it invokes the function again and this might
    cause the resource leakage you see.

    Third, all this seems to never have been implemented
    (thought?) to the end. A targetlist doesn't make sense at
    this place because it could at max contain a single attribute
    - so a single attno would have the same power. And if set
    functions could appear in the rangetable (FROM clause), than
    they would be treated as that and regular Var nodes in the
    query would do it.

    I think you shouldn't really care for that regression test
    and maybe we should disable set functions until we really
    implement stored procedures returning sets in the rangetable.

    Set functions where planned by Stonebraker's team as
    something that today is called stored procedures. But AFAIK
    they never reached the useful state because even in Postgres
    4.2 you haven't been able to get more than one attribute out
    of a set function. It was a feature of the postquel
    querylanguage that you could get one attribute from a set
    function via

    RETRIEVE (attributename(setfuncname()))

    While working on the constraint triggers I've came across
    another regression test (triggers :-) that's errorneous too.
    The funny_dup17 trigger proc executes an INSERT into the same
    relation where it get fired for by a previous INSERT. And it
    stops this recursion only if it reaches a nesting level of
    17, which could only occur if it is fired DURING the
    execution of it's own SPI_exec(). After Vadim quouted some
    SQL92 definitions about when constraint checks and triggers
    are to be executed, I decided to fire regular triggers at the
    end of a query too. Thus, there is absolutely no nesting
    possible for AFTER triggers resulting in an endless loop.


    Jan

    --

    #======================================================================#
    # It's easier to get forgiveness for being wrong than for being right. #
    # Let's break this rule - forgive me. #
    #========================================= wieck@debis.com (Jan Wieck) #
  • Tom Lane at Sep 23, 1999 at 2:18 pm

    Jan Wieck writes:
    Tom Lane wrote:
    What I am wondering, though, is whether this addition is actually
    necessary, or is it a bug that the functions aren't run to completion
    in the first place?
    I've said some time (maybe too long) ago, that SQL functions
    returning tuple sets are broken in general.
    Indeed they are. Try this on for size (using the regression database):

    SELECT p.name, p.hobbies.equipment.name FROM person p;
    SELECT p.hobbies.equipment.name, p.name FROM person p;

    You get different result sets!?

    The problem in this example is that ExecTargetList returns the isDone
    flag from the last targetlist entry, regardless of whether there are
    incomplete iterations in previous entries. More generally, the buffer
    leak problem that I started with only occurs if some Iter nodes are not
    run to completion --- but execQual.c has no mechanism to make sure that
    they have all reached completion simultaneously.

    What we really need to make functions-returning-sets work properly is
    an implementation somewhat like aggregate functions. We need to make
    a list of all the Iter nodes present in a targetlist and cycle through
    the values returned by each in a methodical fashion (run the rightmost
    through its full cycle, then advance the next-to-rightmost one value,
    run the rightmost through its cycle again, etc etc). Also there needs
    to be an understanding of the hierarchy when an Iter appears in the
    arguments of another Iter's function. (You cycle the upper one for
    *each* set of arguments created by cycling its sub-Iters.)

    I am not particularly interested in working on this feature right now,
    since AFAIK it's a Berkeleyism not found in SQL92. What I've done
    is to hack ExecTargetList so that it behaves semi-sanely when there's
    more than one Iter at the top level of the target list --- it still
    doesn't really give the right answer, but at least it will keep
    generating tuples until all the Iters are done at the same time.
    It happens that that's enough to give correct answers for the examples
    shown in the misc regress test. Even when it fails to generate all
    the possible combinations, there will be no buffer leaks.

    So, I'm going to declare victory and go home ;-). We ought to add a
    TODO item along the lines of
    * Functions returning sets don't really work right
    in hopes that someone will feel like tackling this someday.

    regards, tom lane
  • Jan Wieck at Sep 23, 1999 at 3:35 pm

    Tom Lane wrote:
    wieck@debis.com (Jan Wieck) writes:
    Tom Lane wrote:
    What we really need to make functions-returning-sets work properly is
    an implementation somewhat like aggregate functions. We need to make
    a list of all the Iter nodes present in a targetlist and cycle through
    the values returned by each in a methodical fashion (run the rightmost
    through its full cycle, then advance the next-to-rightmost one value,
    run the rightmost through its cycle again, etc etc). Also there needs
    to be an understanding of the hierarchy when an Iter appears in the
    arguments of another Iter's function. (You cycle the upper one for
    *each* set of arguments created by cycling its sub-Iters.)
    Shouldn't a function returning a SET of tuples cause a proper
    join?


    Jan

    --

    #======================================================================#
    # It's easier to get forgiveness for being wrong than for being right. #
    # Let's break this rule - forgive me. #
    #========================================= wieck@debis.com (Jan Wieck) #
  • Tom Lane at Sep 23, 1999 at 9:43 pm

    Jan Wieck writes:
    Shouldn't a function returning a SET of tuples cause a proper
    join?
    Join on what? The semantics suggested by the existing regress tests
    (for lack of any actual documentation :-() certainly appear to be
    straight Cartesian product.

    Anyway, I have no intention of spending more time on this feature now.
    There's lots of higher-priority problems...

    regards, tom lane
  • Bruce Momjian at Sep 23, 1999 at 3:38 pm

    So, I'm going to declare victory and go home ;-). We ought to add a
    TODO item along the lines of
    * Functions returning sets don't really work right
    in hopes that someone will feel like tackling this someday.
    Added to TODO, with your e-mail messages archived:

    * Functions returning sets don't really work right(see TODO.detail/functions)

    --
    Bruce Momjian | http://www.op.net/~candle
    maillist@candle.pha.pa.us | (610) 853-3000
    + If your life is a hard drive, | 830 Blythe Avenue
    + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
  • Andreas Zeugswetter at Sep 23, 1999 at 8:08 am

    Is the regression test's expected output wrong, or am I
    misunderstanding
    what this query is supposed to do? Is there any
    documentation anywhere
    about how SQL functions returning multiple tuples are supposed to
    behave?
    They are supposed to behave somewhat like a view.
    Not all rows are necessarily fetched.
    If used in a context that needs a single row answer,
    and the answer has multiple rows it is supposed to
    runtime elog. Like in:

    select * from tbl where col=funcreturningmultipleresults();
    -- this must elog

    while this is ok:
    select * from tbl where col in (select funcreturningmultipleresults());

    But the caller could only fetch the first row if he wanted.

    The nested notation is supposed to call the function passing it the tuple
    as the first argument. This is what can be used to "fake" a column
    onto a table (computed column).
    That is what I use it for. I have never used it with a
    returns setof function, but reading the comments in the regression test,
    -- mike needs advil and peet's coffee,
    -- joe and sally need hightops, and
    -- everyone else is fine.
    it looks like the results you expected are correct, and currently the
    wrong result is given.

    But I think this query could also elog whithout removing substantial
    functionality.

    SELECT p.name, p.hobbies.name, p.hobbies.equipment.name FROM person p;

    Actually for me it would be intuitive, that this query return one row per
    person, but elog on those that have more than one hobbie or a hobbie that
    needs more than one equipment. Those that don't have a hobbie should
    return name|NULL|NULL. A hobbie that does'nt need equipment name|hobbie|NULL.

    Andreas
  • Tom Lane at Sep 23, 1999 at 2:51 pm

    Andreas Zeugswetter writes:
    That is what I use it for. I have never used it with a
    returns setof function, but reading the comments in the regression test,
    -- mike needs advil and peet's coffee,
    -- joe and sally need hightops, and
    -- everyone else is fine.
    it looks like the results you expected are correct, and currently the
    wrong result is given.
    Yes, I have concluded the same (and partially fixed it, per my previous
    message).
    Those that don't have a hobbie should return name|NULL|NULL. A hobbie
    that does'nt need equipment name|hobbie|NULL.
    That's a good point. Currently (both with and without my uncommitted
    fix) you get *no* rows out from ExecTargetList if there are any Iters
    that return empty result sets. It might be more reasonable to treat an
    empty result set as if it were NULL, which would give the behavior you
    suggest.

    This would be an easy change to my current patch, and I'm prepared to
    make it before committing what I have, if people agree that that's a
    more reasonable definition. Comments?

    regards, tom lane

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedSep 23, '99 at 12:06a
activeSep 23, '99 at 9:43p
posts9
users4
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase