Re: OOM in spgist insert

Alvaro Herrera 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 > a

Re: OOM in spgist insert

Robert Haas writes: > On Thu, May 13, 2021 at 6:20 PM Tom Lane 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. Y

Re: OOM in spgist insert

Pavel Borisov 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, t

Re: OOM in spgist insert

On Thu, May 13, 2021 at 6:20 PM Tom Lane 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

Re: OOM in spgist insert

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

Re: OOM in spgist insert

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)ShortenT

Re: OOM in spgist insert

On Fri, May 14, 2021 at 6:31 AM Tom Lane wrote: > > Alvaro Herrera 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 a

Re: OOM in spgist insert

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 diff --git a/doc/src/sgml/spgist.sgml b/doc/src/sgml/spgist.sgml index cfb2b3c836..18f1f

Re: OOM in spgist insert

Alvaro Herrera 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. OTO

Re: OOM in spgist insert

Alvaro Herrera 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

Re: OOM in spgist insert

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

Re: OOM in spgist insert

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

Re: OOM in spgist insert

On 2021-May-13, Tom Lane wrote: > Alvaro Herrera 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

Re: OOM in spgist insert

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 t

Re: OOM in spgist insert

Alvaro Herrera 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

Re: OOM in spgist insert

Alvaro Herrera 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

Re: OOM in spgist insert

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

Re: OOM in spgist insert

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

Re: OOM in spgist insert

Pavel Borisov 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 i

Re: OOM in spgist insert

On Wed, May 12, 2021 at 5:35 PM Pavel Borisov 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.

Re: OOM in spgist insert

> > 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 v4-0001-When-there-are-INCLUDEd-columns-in-SpGist-check-t.patch Description: Binary dat

Re: OOM in spgist insert

On Wed, May 12, 2021 at 5:11 PM Pavel Borisov 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 + * ind

Re: OOM in spgist insert

> > 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 v3-0001-When-there-are-INCLUDEd-columns-in-

Re: OOM in spgist insert

On Wed, May 12, 2021 at 2:21 PM Pavel Borisov 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 Enterprise

Re: OOM in spgist insert

ср, 12 мая 2021 г. в 12:39, Pavel Borisov : > ср, 12 мая 2021 г. в 12:36, Dilip Kumar : > >> On Wed, 12 May 2021 at 1:43 PM, Pavel Borisov >> wrote: >> >>> ср, 12 мая 2021 г. в 11:09, Dilip Kumar : >>> While testing something on spgist I found that at certain point while inserting in sp

Re: OOM in spgist insert

ср, 12 мая 2021 г. в 12:36, Dilip Kumar : > On Wed, 12 May 2021 at 1:43 PM, Pavel Borisov > wrote: > >> ср, 12 мая 2021 г. в 11:09, Dilip Kumar : >> >>> While testing something on spgist I found that at certain point while >>> inserting in spgist it is going for doPickSplit, but even after split

Re: OOM in spgist insert

On Wed, 12 May 2021 at 1:43 PM, Pavel Borisov wrote: > ср, 12 мая 2021 г. в 11:09, Dilip Kumar : > >> 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

Re: OOM in spgist insert

ср, 12 мая 2021 г. в 11:09, Dilip Kumar : > 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 f