Horton

Thread

Commits

  1. Prevent infinite insertion loops in spgdoinsert().

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

  3. Refactor CHECK_FOR_INTERRUPTS() to add flexibility.

  1. OOM in spgist insert

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-12T07:08:53Z

    While testing something on spgist I found that at certain point while
inserting in spgist it is going for doPickSplit, but even after split
is is not able to find a place to insert a tuple and it keeping going
in that loop infinitely it seems and finally error out with OOM
because in this loop we are continuously allocating memory for the
tuple in temp context but since we are never coming out of the loop it
is erroring out with OOM.

To reproduce load the data from the attached script 'data_load.sql'
and run below commands

------Load data before running this using 'data_load.sql'
-------Test case start---
create extension spgist_name_ops;

select opcname, amvalidate(opc.oid)
from pg_opclass opc join pg_am am on am.oid = opcmethod
where amname = 'spgist' and opcname = 'name_ops';

-- warning expected here
select opcname, amvalidate(opc.oid)
from pg_opclass opc join pg_am am on am.oid = opcmethod
where amname = 'spgist' and opcname = 'name_ops_old';

create table t(f1 name, f2 integer, f3 text);
create index on t using spgist(f1) include(f2, f3);
\d+ t_f1_f2_f3_idx

insert into t select proname, case when length(proname) % 2 = 0 then
pronargs else null end, prosrc from pg_proc_test;

---- Test case end

--Memory allocation stack----
#1 0x0000000000bf96c5 in palloc0 (size=9696) at mcxt.c:1133
#2 0x000000000056b24b in spgFormLeafTuple (state=0x7ffedea15b80,
heapPtr=0x27df306, datums=0x7ffedea15660, isnulls=0x7ffedea15640) at
spgutils.c:892
#3 0x000000000055e15c in doPickSplit (index=0x7fa4b1ddd5c8,
state=0x7ffedea15b80, current=0x7ffedea159c0, parent=0x7ffedea159a0,
newLeafTuple=0x27df300, level=9,
isNulls=false, isNew=true) at spgdoinsert.c:848
#4 0x0000000000561e53 in spgdoinsert (index=0x7fa4b1ddd5c8,
state=0x7ffedea15b80, heapPtr=0x27718d8, datums=0x7ffedea15cc0,
isnulls=0x7ffedea15ca0)
at spgdoinsert.c:2115


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  2. Re: OOM in spgist insert

    Pavel Borisov <pashkin.elfe@gmail.com> — 2021-05-12T08:13:25Z

    ср, 12 мая 2021 г. в 11:09, Dilip Kumar <dilipbalaut@gmail.com>:

> While testing something on spgist I found that at certain point while
> inserting in spgist it is going for doPickSplit, but even after split
> is is not able to find a place to insert a tuple and it keeping going
> in that loop infinitely it seems and finally error out with OOM
> because in this loop we are continuously allocating memory for the
> tuple in temp context but since we are never coming out of the loop it
> is erroring out with OOM.
>
> My first idea is that this is the case when index tuple doesn't fit into
one index page. As INCLUDED columns are added as is the tuple can not be
made shorter by prefix-stripping. Seems we should check every index tuple
length to fit the page before its insertion. Will see the code little bit
later.

Thanks for the reporting!

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>

  3. Re: OOM in spgist insert

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-12T08:36:47Z

    On Wed, 12 May 2021 at 1:43 PM, Pavel Borisov <pashkin.elfe@gmail.com>
wrote:

> ср, 12 мая 2021 г. в 11:09, Dilip Kumar <dilipbalaut@gmail.com>:
>
>> While testing something on spgist I found that at certain point while
>> inserting in spgist it is going for doPickSplit, but even after split
>> is is not able to find a place to insert a tuple and it keeping going
>> in that loop infinitely it seems and finally error out with OOM
>> because in this loop we are continuously allocating memory for the
>> tuple in temp context but since we are never coming out of the loop it
>> is erroring out with OOM.
>>
>> My first idea is that this is the case when index tuple doesn't fit into
> one index page. As INCLUDED columns are added as is the tuple can not be
> made shorter by prefix-stripping. Seems we should check every index tuple
> length to fit the page before its insertion.
>

Thanks for looking into this.

Will see the code little bit later.
>

Ok

> --
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  4. Re: OOM in spgist insert

    Pavel Borisov <pashkin.elfe@gmail.com> — 2021-05-12T08:39:04Z

    ср, 12 мая 2021 г. в 12:36, Dilip Kumar <dilipbalaut@gmail.com>:

> On Wed, 12 May 2021 at 1:43 PM, Pavel Borisov <pashkin.elfe@gmail.com>
> wrote:
>
>> ср, 12 мая 2021 г. в 11:09, Dilip Kumar <dilipbalaut@gmail.com>:
>>
>>> While testing something on spgist I found that at certain point while
>>> inserting in spgist it is going for doPickSplit, but even after split
>>> is is not able to find a place to insert a tuple and it keeping going
>>> in that loop infinitely it seems and finally error out with OOM
>>> because in this loop we are continuously allocating memory for the
>>> tuple in temp context but since we are never coming out of the loop it
>>> is erroring out with OOM.
>>>
>>> My first idea is that this is the case when index tuple doesn't fit into
>> one index page. As INCLUDED columns are added as is the tuple can not be
>> made shorter by prefix-stripping. Seems we should check every index tuple
>> length to fit the page before its insertion.
>>
>
> Thanks for looking into this.
>
> Will see the code little bit later.
>>
>
> Ok
>
PFA v1 patch. Does this help?


-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>

  5. Re: OOM in spgist insert

    Pavel Borisov <pashkin.elfe@gmail.com> — 2021-05-12T08:51:04Z

    ср, 12 мая 2021 г. в 12:39, Pavel Borisov <pashkin.elfe@gmail.com>:

> ср, 12 мая 2021 г. в 12:36, Dilip Kumar <dilipbalaut@gmail.com>:
>
>> On Wed, 12 May 2021 at 1:43 PM, Pavel Borisov <pashkin.elfe@gmail.com>
>> wrote:
>>
>>> ср, 12 мая 2021 г. в 11:09, Dilip Kumar <dilipbalaut@gmail.com>:
>>>
>>>> While testing something on spgist I found that at certain point while
>>>> inserting in spgist it is going for doPickSplit, but even after split
>>>> is is not able to find a place to insert a tuple and it keeping going
>>>> in that loop infinitely it seems and finally error out with OOM
>>>> because in this loop we are continuously allocating memory for the
>>>> tuple in temp context but since we are never coming out of the loop it
>>>> is erroring out with OOM.
>>>>
>>>> My first idea is that this is the case when index tuple doesn't fit
>>> into one index page. As INCLUDED columns are added as is the tuple can not
>>> be made shorter by prefix-stripping. Seems we should check every index
>>> tuple length to fit the page before its insertion.
>>>
>>
>> Thanks for looking into this.
>>
>> Will see the code little bit later.
>>>
>>
>> Ok
>>
> PFA v1 patch. Does this help?
>
I've made a mistake in attributes count in v1. PFA v2


-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>

  6. Re: OOM in spgist insert

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-12T09:06:24Z

    On Wed, May 12, 2021 at 2:21 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:

>>
>> PFA v1 patch. Does this help?
>
> I've made a mistake in attributes count in v1. PFA v2

V2 works.  Thanks for fixing this quickly,  I think you can add a
comment for the new error condition you added.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  7. Re: OOM in spgist insert

    Pavel Borisov <pashkin.elfe@gmail.com> — 2021-05-12T11:41:21Z

    >
> V2 works.  Thanks for fixing this quickly,  I think you can add a
> comment for the new error condition you added.
>
Added comments. PFA v3


-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>

  8. Re: OOM in spgist insert

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-12T11:56:33Z

    On Wed, May 12, 2021 at 5:11 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>>
>> V2 works.  Thanks for fixing this quickly,  I think you can add a
>> comment for the new error condition you added.
>
> Added comments. PFA v3

Thanks.

+ *
+ * For indexes with INCLUDEd columns we do not know whether we can reduce
+ * index tuple size by suffixing its key part or we will go into the
+ * endless loop on pick-split (in case included columns data is big enough

 INCLUDEd -> why you have used a mixed case here?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  9. Re: OOM in spgist insert

    Pavel Borisov <pashkin.elfe@gmail.com> — 2021-05-12T12:05:33Z

    >
>  INCLUDEd -> why you have used a mixed case here?
>
It is current practice to call INCLUDE columns in capital, you can find
many places in the current code. But case mixture can be avoided indeed ))
PFA v4

  10. Re: OOM in spgist insert

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-12T14:45:06Z

    On Wed, May 12, 2021 at 5:35 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>>
>>  INCLUDEd -> why you have used a mixed case here?
>
> It is current practice to call INCLUDE columns in capital, you can find many places in the current code. But case mixture can be avoided indeed ))
> PFA v4

Okay, that makes sense.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  11. Re: OOM in spgist insert

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-13T15:25:23Z

    Pavel Borisov <pashkin.elfe@gmail.com> writes:
> [ v4-0001-When-there-are-INCLUDEd-columns-in-SpGist-check-t.patch ]

I don't like this patch one bit --- it's basically disabling a fairly
important SPGiST feature as soon as there are included columns.
What's more, it's not really giving us any better defense against
the infinite-picksplit-loop problem than we had before.

I wonder if we should give up on the theory posited circa
spgdoinsert.c:2213:

                     * Note: if the opclass sets longValuesOK, we rely on the
                     * choose function to eventually shorten the leafDatum
                     * enough to fit on a page.  We could add a test here to
                     * complain if the datum doesn't get visibly shorter each
                     * time, but that could get in the way of opclasses that
                     * "simplify" datums in a way that doesn't necessarily
                     * lead to physical shortening on every cycle.

The argument that there might be a longValuesOK opclass that *doesn't*
shorten the datum each time seems fairly hypothetical to me.

An alternative way of looking at things is that this is the opclass's
fault.  It should have realized that it's not making progress, and
thrown an error.  However, I'm not sure if the opclass picksplit
function has enough info to throw a meaningful error.  It looks to
me like the trouble case from spg_text_picksplit's point of view
is that it's given a single tuple containing a zero-length string,
so that there is no prefix it can strip.  However, it seems possible
that that could be a legitimate case.  Even if it's not, the opclass
function doesn't have (for instance) the name of the index to cite
in an error message.  Nor did we give it a way to return a failure
indication, which is seeming like a mistake right now.

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.

			regards, tom lane

  12. Re: OOM in spgist insert

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-13T15:53:51Z

    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.

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.

The btree code modified was found to be an actual problem in production
when a btree is corrupted in such a way that vacuum would get an
infinite loop.  I don't remember the exact details but I think we saw
vacuum running for a couple of weeks, and had to restart the server in
order to terminate it (since it wouldn't respond to signals).

-- 
Álvaro Herrera       Valdivia, Chile
"I am amazed at [the pgsql-sql] mailing list for the wonderful support, and
lack of hesitasion in answering a lost soul's question, I just wished the rest
of the mailing list could be like this."                               (Fotis)
               (http://archives.postgresql.org/pgsql-sql/2006-06/msg00265.php)

  13. Re: OOM in spgist insert

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-13T16:29:55Z

    On 2021-May-13, Alvaro Herrera wrote:

> The btree code modified was found to be an actual problem in production
> when a btree is corrupted in such a way that vacuum would get an
> infinite loop.  I don't remember the exact details but I think we saw
> vacuum running for a couple of weeks, and had to restart the server in
> order to terminate it (since it wouldn't respond to signals).

(Looking again, the nbtpage.c hunk might have been made obsolete by
c34787f91058 and other commits).

-- 
Álvaro Herrera       Valdivia, Chile

  14. Re: OOM in spgist insert

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-13T16:31:52Z

    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

  15. Re: OOM in spgist insert

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-13T17:29:43Z

    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.

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.

			regards, tom lane

  16. Re: OOM in spgist insert

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-13T22:20:27Z

    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

  17. Re: OOM in spgist insert

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-13T22:26:56Z

    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)

  18. Re: OOM in spgist insert

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-13T22:49:33Z

    On 2021-May-13, Tom Lane wrote:

> 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.

I think if the alternative is to throw an error, we can afford to retry
quite a few more times than 10 in order not have that called an API
break.  Say, retry (MAXIMUM_ALIGNOF << 3) times or so (if you want to
parameterize on maxalign).  It's not like this is going to be a
performance drag where not needed .. but I think leaving back-branches
unfixed is not great.

I did run Dilip's test case as well as your new regression test, and
both work as intended with your new code (and both OOM-crash the
original code).

-- 
Álvaro Herrera       Valdivia, Chile

  19. Re: OOM in spgist insert

    Pavel Borisov <pashkin.elfe@gmail.com> — 2021-05-13T22:49:49Z

    I think it's good to backpatch the check as it doesn't change acceptable
behavior, just replace infinite loop with the acceptable thing.

  20. Re: OOM in spgist insert

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-13T23:04:19Z

    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

  21. Re: OOM in spgist insert

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-14T01:01:18Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-May-13, Tom Lane wrote:
>> 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.

> I think if the alternative is to throw an error, we can afford to retry
> quite a few more times than 10 in order not have that called an API
> break.  Say, retry (MAXIMUM_ALIGNOF << 3) times or so (if you want to
> parameterize on maxalign).  It's not like this is going to be a
> performance drag where not needed .. but I think leaving back-branches
> unfixed is not great.

Hm.  Index bloat is not something to totally ignore, though, so I'm
not sure what the best cutoff is.

Anyway, here is a patch set teased apart into committable bites,
and with your other points addressed.

			regards, tom lane

  22. Re: OOM in spgist insert

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-14T01:44:07Z

    I wrote:
> Anyway, here is a patch set teased apart into committable bites,
> and with your other points addressed.

Oh, maybe some docs would be a good thing too ...

			regards, tom lane

  23. Re: OOM in spgist insert

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-14T05:27:34Z

    On Fri, May 14, 2021 at 6:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > On 2021-May-13, Tom Lane wrote:
> >> 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.
>
> > I think if the alternative is to throw an error, we can afford to retry
> > quite a few more times than 10 in order not have that called an API
> > break.  Say, retry (MAXIMUM_ALIGNOF << 3) times or so (if you want to
> > parameterize on maxalign).  It's not like this is going to be a
> > performance drag where not needed .. but I think leaving back-branches
> > unfixed is not great.
>
> Hm.  Index bloat is not something to totally ignore, though, so I'm
> not sure what the best cutoff is.
>
> Anyway, here is a patch set teased apart into committable bites,
> and with your other points addressed.

I have tested with my original issue and this patch is fixing the
issue.  Thanks!


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  24. Re: OOM in spgist insert

    Pavel Borisov <pashkin.elfe@gmail.com> — 2021-05-14T08:21:28Z

    Now when checking for shortening of  leaf tuple is added LongValuesOK
become slightly redundant. I'd propose to replace it with more legible name
as LongValuesOK doesn't mean they are warranted to be ok just that we can
try to shorten them? What about tryShortening, trySuffixing or
can(Try)ShortenTuple?


-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>

  25. Re: OOM in spgist insert

    Pavel Borisov <pashkin.elfe@gmail.com> — 2021-05-14T08:27:14Z

    >
>
> Now when checking for shortening of  leaf tuple is added LongValuesOK
> become slightly redundant. I'd propose to replace it with more legible name
> as LongValuesOK doesn't mean they are warranted to be ok just that we can
> try to shorten them? What about tryShortening, trySuffixing or
> can(Try)ShortenTuple?
>
Or maybe it's even more logical now to invert it and make
dontTrySuffixing for use in the opclasses for kd-tree, quadtree etc which
are warranted to have the same key data length at any tree level ?

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>

  26. Re: OOM in spgist insert

    Robert Haas <robertmhaas@gmail.com> — 2021-05-14T12:12:11Z

    On Thu, May 13, 2021 at 6:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> OTOH, the 10-cycles-to-show-progress rule could be
> argued to be an API break.

Not being familiar with this code, I don't really understand why 10
cycles to show progress wouldn't, like 640kB, be enough for anyone.
But as far as back-patching the code goals, I think the question is
not so much whether this restriction could hypothetically break
anything but whether it will actually break anything, which leads to
the question of how many spgist opclasses we think exist outside of
core. I did a Google search and found some evidence that PostGIS might
have such things, and also this:

https://github.com/fake-name/pg-spgist_hamming

There might be other things, but I did not find them.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  27. Re: OOM in spgist insert

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-14T13:40:53Z

    Pavel Borisov <pashkin.elfe@gmail.com> writes:
> Now when checking for shortening of  leaf tuple is added LongValuesOK
> become slightly redundant. I'd propose to replace it with more legible name
> as LongValuesOK doesn't mean they are warranted to be ok just that we can
> try to shorten them? What about tryShortening, trySuffixing or
> can(Try)ShortenTuple?

That field name is part of the opclass API.  I fear it's several years
too late to rename it for no compelling reason.

			regards, tom lane

  28. Re: OOM in spgist insert

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-14T13:54:33Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, May 13, 2021 at 6:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> OTOH, the 10-cycles-to-show-progress rule could be
>> argued to be an API break.

> Not being familiar with this code, I don't really understand why 10
> cycles to show progress wouldn't, like 640kB, be enough for anyone.

Yeah, after further thought I'm thinking that that ought to be plenty.
In released branches, that code will never execute at all unless the
datum-to-be-indexed is larger than ~8kB.  If you are starting with,
say, a 30kB string, you'd better be removing a heck of a lot more than
one byte per tree level, or your search performance is going to be
abysmal.  Maybe algorithmic oddities would sometimes result in
seemingly making no progress for one cycle, but I doubt there's need
for more slop than that.  In this light, a 10-cycle grace period seems
if anything excessive.

> But as far as back-patching the code goals, I think the question is
> not so much whether this restriction could hypothetically break
> anything but whether it will actually break anything, which leads to
> the question of how many spgist opclasses we think exist outside of
> core.

That is also an interesting viewpoint.  codesearch.debian.net knows
of no external SPGiST opclasses other than PostGIS'.  They don't
seem to have indexed the two you found on github, though.  None of
those four set longValuesOK to true, which means that the whole
discussion is moot for them.  So it's entirely plausible that
spgtextproc.c is the only affected code anywhere.

Of course, that conclusion could lead to the position that there's
no point in back-patching anyway, since there's no reason to think
that spgtextproc.c is buggy.

			regards, tom lane

  29. Re: OOM in spgist insert

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-14T20:23:37Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> 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.

I've now pushed that macro change ...

> The btree code modified was found to be an actual problem in production
> when a btree is corrupted in such a way that vacuum would get an
> infinite loop.  I don't remember the exact details but I think we saw
> vacuum running for a couple of weeks, and had to restart the server in
> order to terminate it (since it wouldn't respond to signals).

... but I think this bit still needs work, if we still want it at all.
The problem is that it seems to believe that ProcessInterrupts is
guaranteed not to return, which is far from the truth.  Maybe it was
true once, but we've grown a lot of accretions on it that will just
clear InterruptPending and return.  I see that the "return false"
leads directly to an "Assert(false)", which seems unhelpful.

			regards, tom lane