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-13T23:04:19Z
Lists: pgsql-hackers 
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Hmm, it looks OK to me, but I wonder why you kept the original
> CHECK_FOR_INTERRUPTS()s since these would be done once we've broken out
> of the loop anyway.  I tested Dilip's original test case and while we
> still die on OOM, we're able to interrupt it before dying.

Hm.  My thought was that in the cases where InterruptPending is set for
some reason other than a query cancel, we could let ProcessInterrupts
service it at less cost than abandoning and retrying the index insertion.
On reflection though, that only works for the first CHECK_FOR_INTERRUPTS
at the top of the loop, and only the first time through, because during
later calls we'll be holding buffer locks.

Maybe the best idea is to have one CHECK_FOR_INTERRUPTS at the top of
the function, in hopes of clearing out any already-pending interrupts,
and then just use the condition test inside the loop.

> Not related to this patch -- I was bothered by the UnlockReleaseBuffer
> calls at the bottom of spgdoinsert that leave the buffer still set in
> the structs.  It's not a problem if you look only at this routine, but I
> notice that callee doPickSplit does the same thing, so maybe there is
> some weird situation in which you could end up with the buffer variable
> set, but we don't hold lock nor pin on the page, so an attempt to clean
> up would break.

Maybe I'm confused, but aren't those just local variables that are about
to go out of scope anyway?  Clearing them seems more likely to draw
compiler warnings about dead stores than accomplish something useful.

			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.