On 2021-May-13, Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > (Looking again, the nbtpage.c hunk might have been made obsolete by
> > c34787f91058 and other commits).
>
> OK. Here's a revision that adopts your idea, except that I left
> out the nbtpage.c change since you aren't sure of that part.
Thanks.
> I added a macro that allows spgdoinsert to Assert that it's not
> called in a context where the infinite-loop-due-to-InterruptPending
> risk would arise. This is a little bit fragile, because it'd be
> easy for ill-considered changes to ProcessInterrupts to break it,
> but it's better than nothing.
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.
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. I don't know enough about spgist to figure out how to
craft a test case, maybe it's impossible to reach for some reason, but
it seems glass-in-the-beach sort of thing.
--
Álvaro Herrera 39°49'30"S 73°17'W
"Someone said that it is at least an order of magnitude more work to do
production software than a prototype. I think he is wrong by at least
an order of magnitude." (Brian Kernighan)