Re: OOM in spgist insert

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: OOM in spgist insert
Date: 2021-05-13 22:20:27
Message-ID: 1724899.1620944427@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Here's a patch that attempts to prevent the infinite loop in spgdoinsert
by checking whether the proposed leaf tuple is getting smaller at each
iteration.

We can't be totally rigid about that, because for example if the opclass
shortens a 7-byte string to 5 bytes, that will make no difference in the
tuple's size after alignment. I first tried to handle that by checking
datumGetSize() of the key datum itself, but observed that spgtextproc.c
has some cases where it'll return an empty leaf-datum string at two
levels before succeeding. Maybe it'd be okay to fail that on the
grounds that it can't become any smaller later. But on the whole, and
considering the existing comment's concerns about opclasses that don't
shorten the datum every time, it seems like a good idea to allow some
fuzz here. So what this patch does is to allow up to 10 cycles with no
reduction in the actual leaf tuple size before failing. That way we can
handle slop due to alignment roundoff and slop due to opclass corner
cases with a single, very cheap mechanism. It does mean that we might
build a few more useless inner tuples before failing --- but that seems
like a good tradeoff for *not* failing in cases where the opclass is
able to shorten the leaf datum sufficiently.

I have not bothered to tease apart the query-cancel and infinite-loop
parts of the patch, but probably should do that before committing.

What do people think about back-patching this? In existing branches,
we've defined it to be an opclass bug if it fails to shorten the leaf
datum enough. But not having any defenses against that seems like
not a great idea. OTOH, the 10-cycles-to-show-progress rule could be
argued to be an API break.

regards, tom lane

Attachment Content-Type Size
fix-spgist-infinite-loop-for-big-tuples.patch text/x-diff 12.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2021-05-13 22:26:56 Re: OOM in spgist insert
Previous Message Jan Wieck 2021-05-13 21:42:52 Re: Always bump PG_CONTROL_VERSION?