Re: heap_lock_updated_tuple_rec can leak a buffer refcount

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Subject: Re: heap_lock_updated_tuple_rec can leak a buffer refcount
Date: 2018-03-02 21:56:32
Message-ID: 31560.1520027792@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru> writes:
> Looks like a leak indeed, the fix seems right.

Yup, it's a leak. It's hard to hit because you need to be starting
with an update of a tuple in an all-visible page; otherwise we never
pin the vm page so there's nothing to leak. But if you lobotomize
the test a few lines above so that it always pins the vm page, then
the regression tests (specifically combocid) reveal the leak, and
show that the proposed patch indeed fixes it.

However ... with said lobotomization, the isolation tests trigger an
Assert(BufferIsPinned(buffer)) inside visibilitymap_pin, showing that
there's another bug here too. That seems to be because at the bottom
of the outer loop, we do

if (vmbuffer != InvalidBuffer)
ReleaseBuffer(vmbuffer);

and then loop back around with vmbuffer still not equal to InvalidBuffer.
This causes the next loop iteration's visibilitymap_pin call to think it
needs to release that vmbuffer pin a second time; kaboom.

And eyeing this, I see still a third problem: if either of the "goto l4"
jumps occur, we'll loop back to l4 with vmbuffer possibly pinned, and then
if the new page isn't all-visible, we'll just set vmbuffer = InvalidBuffer
and leak the pin that way. (If it is all-visible, we unpin the old page
correctly in the visibilitymap_pin call, but that can charitably be
described as accidental.)

In short, this is pretty darn broken. We need to treat the vmbuffer
variable honestly as state that may persist across either the outer loop
or the "goto l4" sub-loop. Furthermore, it's not really cool to use
"vmbuffer == InvalidBuffer" as the indicator of whether we acquired the
vmbuffer pin pre-lock. To do that, we'd be forced to drop the old pin in
the not-all-visible path, even though we might need it right back again.
Also, remembering that one vm page serves many heap pages, even if we have
a vm pin for the "wrong" page it's conceivable that it'd be the right one
for the next time we actually need it. So we should use the
visibilitymap_pin API the way it's meant to be used, and hold any vm pin
we've acquired until the very end of the function.

Hence, I propose the attached patch. The test lobotomization
(the "if (1) //" change) isn't meant for commit but shows how I tested
the take-the-pin paths. This passes make check-world with or without
the lobotomization change.

regards, tom lane

Attachment Content-Type Size
fix-vmbuffer-pin-maintenance-v2.patch text/x-diff 3.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Gould 2018-03-02 22:00:37 Re: [patch] BUG #15005: ANALYZE can make pg_class.reltuples inaccurate.
Previous Message Pavel Stehule 2018-03-02 21:34:42 Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan