Horton

Re: OOM in spgist insert

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2021-05-13T16:31:52Z
Lists: pgsql-hackers

Attachments

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-May-13, Tom Lane wrote:
>> BTW, another nasty thing I discovered while testing this is that
>> the CHECK_FOR_INTERRUPTS() at line 2146 is useless, because
>> we're holding a buffer lock there so InterruptHoldoffCount > 0.
>> So once you get into this loop you can't even cancel the query.
>> Seems like that needs a fix, too.

Attached is a WIP patch for that part.  Basically, if it looks
like there's an interrupt pending, make spgdoinsert() fall out of
its loop, so it'll release the buffer locks, and then recheck
CHECK_FOR_INTERRUPTS() at a point where it can really throw the
error.  Now, the hole in this approach is what if ProcessInterrupts
still declines to throw the error?  I've made the patch make use of
the existing provision to retry spgdoinsert() in case of deadlock,
so that it just goes around and tries again.  But that doesn't seem
terribly satisfactory, because if InterruptPending remains set then
we'll just have an infinite loop at that outer level.  IOW, this
patch can only cope with the situation where there's not any outer
function holding a buffer lock (or other reason to prevent the
query cancel from taking effect).  Maybe that's good enough, but
I'm not terribly pleased with it.

> This comment made me remember a patch I've had for a while, which splits
> the CHECK_FOR_INTERRUPTS() definition in two -- one of them is
> INTERRUPTS_PENDING_CONDITION() which let us test the condition
> separately; that allows the lock we hold to be released prior to
> actually processing the interrupts.

Hm.  Yeah, I was feeling that this patch is unduly friendly with
the innards of CHECK_FOR_INTERRUPTS().  So maybe some refactoring
is called for, but I'm not quite sure what it should look like.
Testing the condition separately is fine, but what about the case
of ProcessInterrupts refusing to pull the trigger?

			regards, tom lane

Commits

  1. Prevent infinite insertion loops in spgdoinsert().

  2. Fix query-cancel handling in spgdoinsert().

  3. Refactor CHECK_FOR_INTERRUPTS() to add flexibility.