pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
19+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2021-09-13 df3640e529 f5e0ff4631 58cf794ca6 Fix reorder buffer memory accounting for toast changes.
* [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
@ 2021-08-13 09:45 Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Drouvot, Bertrand @ 2021-08-13 09:45 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; dilipbalaut@gmail.com
Hi hackers,
While working on [1], it appears that (on master) the issue reproduction
(of toast rewrite not resetting the toast_hash) is triggering a failed
assertion:
#2 0x0000000000b29fab in ExceptionalCondition (conditionName=0xce6850
"(rb->size >= sz) && (txn->size >= sz)", errorType=0xce5f84
"FailedAssertion", fileName=0xce5fd0 "reorderbuffer.c", lineNumber=3141)
at assert.c:69
#3 0x00000000008ff1fb in ReorderBufferChangeMemoryUpdate (rb=0x11a7a40,
change=0x11c94b8, addition=false) at reorderbuffer.c:3141
#4 0x00000000008fab27 in ReorderBufferReturnChange (rb=0x11a7a40,
change=0x11c94b8, upd_mem=true) at reorderbuffer.c:477
#5 0x0000000000902ec1 in ReorderBufferToastReset (rb=0x11a7a40,
txn=0x11b1998) at reorderbuffer.c:4799
#6 0x00000000008faaa2 in ReorderBufferReturnTXN (rb=0x11a7a40,
txn=0x11b1998) at reorderbuffer.c:448
#7 0x00000000008fc95b in ReorderBufferCleanupTXN (rb=0x11a7a40,
txn=0x11b1998) at reorderbuffer.c:1540
while on 12.5 for example, we would get (with the exact same repro):
ERROR: could not open relation with OID 0
The failed assertion is happening in the PG_CATCH() section of
ReorderBufferProcessTXN().
We entered PG_CATCH() because elog(ERROR, "could not open relation with
OID %u",...) has been triggered in ReorderBufferToastReplace().
But this elog(ERROR,) is being called after
ReorderBufferChangeMemoryUpdate() being triggered with "addition" set to
false.
As a consequence of elog(ERROR,) then ReorderBufferChangeMemoryUpdate()
with "addition" set to true is not called at the end of
ReorderBufferToastReplace().
That leads to a subsequent call to ReorderBufferChangeMemoryUpdate()
(being triggered by 4daa140a2f adding ReorderBufferToastReset() calls to
ReorderBufferReturnTXN()) triggering the failed assertion.
Please find attached a patch proposal to avoid the failed assertion (by
ensuring that ReorderBufferChangeMemoryUpdate() being triggered with
"addition" set to false in ReorderBufferToastReplace() is done after the
elog(ERROR,)).
Adding Amit and Dilip as they are also aware of [1] and have worked on
4daa140a2f.
Also adding this patch in the commitfest.
Thanks
Bertrand
[1]:
https://www.postgresql.org/message-id/b5146fb1-ad9e-7d6e-f980-98ed68744a7c@amazon.com
Attachments:
v1-0001-ReorderBufferChangeMemoryUpdate-failed-assertion.patch (text/plain, 1.1K) download
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 7378beb684..27332a4cfd 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4610,6 +4610,11 @@ ReorderBufferToastReplace(ReorderBuffer *rb, ReorderBufferTXN *txn,
if (txn->toast_hash == NULL)
return;
+ toast_rel = RelationIdGetRelation(relation->rd_rel->reltoastrelid);
+ if (!RelationIsValid(toast_rel))
+ elog(ERROR, "could not open relation with OID %u",
+ relation->rd_rel->reltoastrelid);
+
/*
* We're going to modify the size of the change, so to make sure the
* accounting is correct we'll make it look like we're removing the change
@@ -4624,11 +4629,6 @@ ReorderBufferToastReplace(ReorderBuffer *rb, ReorderBufferTXN *txn,
desc = RelationGetDescr(relation);
- toast_rel = RelationIdGetRelation(relation->rd_rel->reltoastrelid);
- if (!RelationIsValid(toast_rel))
- elog(ERROR, "could not open relation with OID %u",
- relation->rd_rel->reltoastrelid);
-
toast_desc = RelationGetDescr(toast_rel);
/* should we allocate from stack instead? */
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
@ 2021-09-06 10:34 ` Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-13 16:52 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 2 replies; 19+ messages in thread
From: Amit Kapila @ 2021-09-06 10:34 UTC (permalink / raw)
To: Drouvot, Bertrand <bdrouvot@amazon.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Dilip Kumar <dilipbalaut@gmail.com>
On Fri, Aug 13, 2021 at 3:15 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>
>
> Please find attached a patch proposal to avoid the failed assertion (by ensuring that ReorderBufferChangeMemoryUpdate() being triggered with "addition" set to false in ReorderBufferToastReplace() is done after the elog(ERROR,)).
>
The error can occur at multiple places (like via palloc or various
other places) between the first time we subtract the change_size and
add it back after the change is re-computed. I think the correct fix
would be that in the beginning we just compute the change_size by
ReorderBufferChangeSize and then after re-computing the change, we
just subtract the old change_size and add the new change_size. What do
you think?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
@ 2021-09-06 11:54 ` Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
1 sibling, 1 reply; 19+ messages in thread
From: Dilip Kumar @ 2021-09-06 11:54 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Drouvot, Bertrand <bdrouvot@amazon.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Sep 6, 2021 at 4:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Aug 13, 2021 at 3:15 PM Drouvot, Bertrand <bdrouvot@amazon.com>
> wrote:
> >
> >
> > Please find attached a patch proposal to avoid the failed assertion (by
> ensuring that ReorderBufferChangeMemoryUpdate() being triggered with
> "addition" set to false in ReorderBufferToastReplace() is done after the
> elog(ERROR,)).
> >
>
> The error can occur at multiple places (like via palloc or various
> other places) between the first time we subtract the change_size and
> add it back after the change is re-computed. I think the correct fix
> would be that in the beginning we just compute the change_size by
> ReorderBufferChangeSize and then after re-computing the change, we
> just subtract the old change_size and add the new change_size. What do
> you think?
>
Yeah, that seems more logical to me.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
@ 2021-09-06 15:24 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Drouvot, Bertrand @ 2021-09-06 15:24 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 9/6/21 1:54 PM, Dilip Kumar wrote:
>
> *CAUTION*: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
>
>
> On Mon, Sep 6, 2021 at 4:04 PM Amit Kapila <amit.kapila16@gmail.com
> <mailto:amit.kapila16@gmail.com>> wrote:
>
> On Fri, Aug 13, 2021 at 3:15 PM Drouvot, Bertrand
> <bdrouvot@amazon.com <mailto:bdrouvot@amazon.com>> wrote:
> >
> >
> > Please find attached a patch proposal to avoid the failed
> assertion (by ensuring that ReorderBufferChangeMemoryUpdate()
> being triggered with "addition" set to false in
> ReorderBufferToastReplace() is done after the elog(ERROR,)).
> >
>
> The error can occur at multiple places (like via palloc or various
> other places) between the first time we subtract the change_size and
> add it back after the change is re-computed. I think the correct fix
> would be that in the beginning we just compute the change_size by
> ReorderBufferChangeSize and then after re-computing the change, we
> just subtract the old change_size and add the new change_size. What do
> you think?
>
>
> Yeah, that seems more logical to me.
Thanks for your feedback!
That seems indeed more logical, so I see 3 options to do so:
1) Add a new API say ReorderBufferChangeMemorySubstractSize() (with a
Size as one parameter) and make use of it in ReorderBufferToastReplace()
2) Add a new "Size" parameter to ReorderBufferChangeMemoryUpdate(), so
that if this parameter is > 0 then it would be used instead of "sz =
ReorderBufferChangeSize(change)"
3) Do the substraction directly into ReorderBufferToastReplace()
without any API
I'm inclined to go for option 2), what do you think?
Thanks
Bertrand
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
@ 2021-09-06 15:43 ` Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Dilip Kumar @ 2021-09-06 15:43 UTC (permalink / raw)
To: Drouvot, Bertrand <bdrouvot@amazon.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Sep 6, 2021 at 8:54 PM Drouvot, Bertrand <bdrouvot@amazon.com>
wrote:
> Thanks for your feedback!
>
> That seems indeed more logical, so I see 3 options to do so:
>
> 1) Add a new API say ReorderBufferChangeMemorySubstractSize() (with a
> Size as one parameter) and make use of it in ReorderBufferToastReplace()
>
> 2) Add a new "Size" parameter to ReorderBufferChangeMemoryUpdate(), so
> that if this parameter is > 0 then it would be used instead of "sz =
> ReorderBufferChangeSize(change)"
>
> 3) Do the substraction directly into ReorderBufferToastReplace() without
> any API
>
> I'm inclined to go for option 2), what do you think?
>
Yet another option could be to create a new API say
ReorderBufferReplaceChangeMemoryUpdate(), which takes, 2 parameters,
oldchange, and newchange as inputs, it will compute the difference and
add/subtract that size. Logically, that is what we are actually trying to
do right? i.e. replacing old change with the new change.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
@ 2021-09-07 03:08 ` Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:37 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Amit Kapila @ 2021-09-07 03:08 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; +Cc: Drouvot, Bertrand <bdrouvot@amazon.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Sep 6, 2021 at 9:14 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Sep 6, 2021 at 8:54 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>>
>> Thanks for your feedback!
>>
>> That seems indeed more logical, so I see 3 options to do so:
>>
>> 1) Add a new API say ReorderBufferChangeMemorySubstractSize() (with a Size as one parameter) and make use of it in ReorderBufferToastReplace()
>>
>> 2) Add a new "Size" parameter to ReorderBufferChangeMemoryUpdate(), so that if this parameter is > 0 then it would be used instead of "sz = ReorderBufferChangeSize(change)"
>>
>> 3) Do the substraction directly into ReorderBufferToastReplace() without any API
>>
>> I'm inclined to go for option 2), what do you think?
>
Isn't it better if we use option 2) at all places as then we won't
need any special check inside ReorderBufferChangeMemoryUpdate()?
> Yet another option could be to create a new API say ReorderBufferReplaceChangeMemoryUpdate(), which takes, 2 parameters, oldchange, and newchange as inputs, it will compute the difference and add/subtract that size. Logically, that is what we are actually trying to do right? i.e. replacing old change with the new change.
>
Note that in ReorderBufferToastReplace(), the new tuple is replaced in
change so by the time we want to do this computation oldchange won't
be preserved.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
@ 2021-09-07 05:37 ` Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 05:40 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Dilip Kumar @ 2021-09-07 05:37 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Drouvot, Bertrand <bdrouvot@amazon.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Sep 7, 2021 at 8:38 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Sep 6, 2021 at 9:14 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Mon, Sep 6, 2021 at 8:54 PM Drouvot, Bertrand <bdrouvot@amazon.com>
> wrote:
> >>
> >> Thanks for your feedback!
> >>
> >> That seems indeed more logical, so I see 3 options to do so:
> >>
> >> 1) Add a new API say ReorderBufferChangeMemorySubstractSize() (with a
> Size as one parameter) and make use of it in ReorderBufferToastReplace()
> >>
> >> 2) Add a new "Size" parameter to ReorderBufferChangeMemoryUpdate(), so
> that if this parameter is > 0 then it would be used instead of "sz =
> ReorderBufferChangeSize(change)"
> >>
> >> 3) Do the substraction directly into ReorderBufferToastReplace()
> without any API
> >>
> >> I'm inclined to go for option 2), what do you think?
> >
>
> Isn't it better if we use option 2) at all places as then we won't
> need any special check inside ReorderBufferChangeMemoryUpdate()?
>
If we want to do this then be careful about
REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID change.
Basically, ReorderBufferChangeMemoryUpdate() ignores this type of change
whereas ReorderBufferChangeSize(), consider at
least sizeof(ReorderBufferChange) bytes to this change. So if we compute
the size using ReorderBufferChangeSize() outside of
ReorderBufferChangeMemoryUpdate(), then total size will be different from
what we have now. Logically, we should be ignoring/asserting
REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID in ReorderBufferChangeSize(),
because ReorderBufferChangeMemoryUpdate() is the only caller for this
function.
> > Yet another option could be to create a new API say
> ReorderBufferReplaceChangeMemoryUpdate(), which takes, 2 parameters,
> oldchange, and newchange as inputs, it will compute the difference and
> add/subtract that size. Logically, that is what we are actually trying to
> do right? i.e. replacing old change with the new change.
> >
>
> Note that in ReorderBufferToastReplace(), the new tuple is replaced in
> change so by the time we want to do this computation oldchange won't
> be preserved.
>
Right
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:37 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
@ 2021-09-07 05:40 ` Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:58 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Amit Kapila @ 2021-09-07 05:40 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; +Cc: Drouvot, Bertrand <bdrouvot@amazon.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Sep 7, 2021 at 11:08 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Sep 7, 2021 at 8:38 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Mon, Sep 6, 2021 at 9:14 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>> >
>> > On Mon, Sep 6, 2021 at 8:54 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>> >>
>> >> Thanks for your feedback!
>> >>
>> >> That seems indeed more logical, so I see 3 options to do so:
>> >>
>> >> 1) Add a new API say ReorderBufferChangeMemorySubstractSize() (with a Size as one parameter) and make use of it in ReorderBufferToastReplace()
>> >>
>> >> 2) Add a new "Size" parameter to ReorderBufferChangeMemoryUpdate(), so that if this parameter is > 0 then it would be used instead of "sz = ReorderBufferChangeSize(change)"
>> >>
>> >> 3) Do the substraction directly into ReorderBufferToastReplace() without any API
>> >>
>> >> I'm inclined to go for option 2), what do you think?
>> >
>>
>> Isn't it better if we use option 2) at all places as then we won't
>> need any special check inside ReorderBufferChangeMemoryUpdate()?
>
>
> If we want to do this then be careful about REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID change. Basically, ReorderBufferChangeMemoryUpdate() ignores this type of change whereas ReorderBufferChangeSize(), consider at least sizeof(ReorderBufferChange) bytes to this change. So if we compute the size using ReorderBufferChangeSize() outside of ReorderBufferChangeMemoryUpdate(), then total size will be different from what we have now. Logically, we should be ignoring/asserting REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID in ReorderBufferChangeSize(), because ReorderBufferChangeMemoryUpdate() is the only caller for this function.
>
Why can't we simply ignore it in ReorderBufferChangeMemoryUpdate() as
we are doing now?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:37 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 05:40 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
@ 2021-09-07 05:58 ` Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 06:03 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
0 siblings, 1 reply; 19+ messages in thread
From: Dilip Kumar @ 2021-09-07 05:58 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Drouvot, Bertrand <bdrouvot@amazon.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Sep 7, 2021 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> Isn't it better if we use option 2) at all places as then we won't
> >> need any special check inside ReorderBufferChangeMemoryUpdate()?
> >
> >
> > If we want to do this then be careful about
> REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID change. Basically,
> ReorderBufferChangeMemoryUpdate() ignores this type of change whereas
> ReorderBufferChangeSize(), consider at least sizeof(ReorderBufferChange)
> bytes to this change. So if we compute the size using
> ReorderBufferChangeSize() outside of ReorderBufferChangeMemoryUpdate(),
> then total size will be different from what we have now. Logically, we
> should be ignoring/asserting REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID in
> ReorderBufferChangeSize(), because ReorderBufferChangeMemoryUpdate() is the
> only caller for this function.
> >
>
> Why can't we simply ignore it in ReorderBufferChangeMemoryUpdate() as
> we are doing now?
>
Yeah right, we can actually do that, it doesn't matter even if we are
passing the size from outside.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:37 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 05:40 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:58 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
@ 2021-09-07 06:03 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 06:51 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Drouvot, Bertrand @ 2021-09-07 06:03 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 9/7/21 7:58 AM, Dilip Kumar wrote:
>
> On Tue, Sep 7, 2021 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com
> <mailto:amit.kapila16@gmail.com>> wrote:
>
> >> Isn't it better if we use option 2) at all places as then we won't
> >> need any special check inside ReorderBufferChangeMemoryUpdate()?
> >
> >
> > If we want to do this then be careful about
> REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID change. Basically,
> ReorderBufferChangeMemoryUpdate() ignores this type of change
> whereas ReorderBufferChangeSize(), consider at least
> sizeof(ReorderBufferChange) bytes to this change. So if we
> compute the size using ReorderBufferChangeSize() outside of
> ReorderBufferChangeMemoryUpdate(), then total size will be
> different from what we have now. Logically, we should be
> ignoring/asserting REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID in
> ReorderBufferChangeSize(), because
> ReorderBufferChangeMemoryUpdate() is the only caller for this
> function.
> >
>
> Why can't we simply ignore it in ReorderBufferChangeMemoryUpdate() as
> we are doing now?
>
>
> Yeah right, we can actually do that, it doesn't matter even if we are
> passing the size from outside.
Agree, if no objections, I'll prepare a patch with the modified approach
of option 2) proposed by Amit (means passing the size from the outside
in all the cases).
Thanks
Bertrand
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:37 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 05:40 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:58 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 06:03 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
@ 2021-09-07 06:51 ` Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 07:11 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
0 siblings, 1 reply; 19+ messages in thread
From: Amit Kapila @ 2021-09-07 06:51 UTC (permalink / raw)
To: Drouvot, Bertrand <bdrouvot@amazon.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Sep 7, 2021 at 11:33 AM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>
> Hi,
>
> On 9/7/21 7:58 AM, Dilip Kumar wrote:
>
>
> On Tue, Sep 7, 2021 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
>> >> Isn't it better if we use option 2) at all places as then we won't
>> >> need any special check inside ReorderBufferChangeMemoryUpdate()?
>> >
>> >
>> > If we want to do this then be careful about REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID change. Basically, ReorderBufferChangeMemoryUpdate() ignores this type of change whereas ReorderBufferChangeSize(), consider at least sizeof(ReorderBufferChange) bytes to this change. So if we compute the size using ReorderBufferChangeSize() outside of ReorderBufferChangeMemoryUpdate(), then total size will be different from what we have now. Logically, we should be ignoring/asserting REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID in ReorderBufferChangeSize(), because ReorderBufferChangeMemoryUpdate() is the only caller for this function.
>> >
>>
>> Why can't we simply ignore it in ReorderBufferChangeMemoryUpdate() as
>> we are doing now?
>
>
> Yeah right, we can actually do that, it doesn't matter even if we are passing the size from outside.
>
> Agree, if no objections, I'll prepare a patch with the modified approach of option 2) proposed by Amit (means passing the size from the outside in all the cases).
>
Sounds reasonable. Another point that needs some thought is do we want
to backpatch this change till v13? I am not sure if there is any
user-visible bug here but maybe it is still good to fix this in back
branches. What do you think?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:37 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 05:40 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:58 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 06:03 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 06:51 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
@ 2021-09-07 07:11 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 08:32 ` Re: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
0 siblings, 1 reply; 19+ messages in thread
From: Drouvot, Bertrand @ 2021-09-07 07:11 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 9/7/21 8:51 AM, Amit Kapila wrote:
> On Tue, Sep 7, 2021 at 11:33 AM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>> Hi,
>>
>> On 9/7/21 7:58 AM, Dilip Kumar wrote:
>>
>>
>> On Tue, Sep 7, 2021 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>>>>> Isn't it better if we use option 2) at all places as then we won't
>>>>> need any special check inside ReorderBufferChangeMemoryUpdate()?
>>>>
>>>> If we want to do this then be careful about REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID change. Basically, ReorderBufferChangeMemoryUpdate() ignores this type of change whereas ReorderBufferChangeSize(), consider at least sizeof(ReorderBufferChange) bytes to this change. So if we compute the size using ReorderBufferChangeSize() outside of ReorderBufferChangeMemoryUpdate(), then total size will be different from what we have now. Logically, we should be ignoring/asserting REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID in ReorderBufferChangeSize(), because ReorderBufferChangeMemoryUpdate() is the only caller for this function.
>>>>
>>> Why can't we simply ignore it in ReorderBufferChangeMemoryUpdate() as
>>> we are doing now?
>>
>> Yeah right, we can actually do that, it doesn't matter even if we are passing the size from outside.
>>
>> Agree, if no objections, I'll prepare a patch with the modified approach of option 2) proposed by Amit (means passing the size from the outside in all the cases).
>>
> Sounds reasonable. Another point that needs some thought is do we want
> to backpatch this change till v13? I am not sure if there is any
> user-visible bug here but maybe it is still good to fix this in back
> branches. What do you think?
Yes, +1 to backpatch till v13 "per precaution".
I will first come back with a proposed version for master and once we
agree on a final/polished version then I'll do the backpatch ones (if
needed).
Thanks
Bertrand
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:37 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 05:40 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:58 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 06:03 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 06:51 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 07:11 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
@ 2021-09-07 08:32 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-09 11:16 ` Re: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Drouvot, Bertrand @ 2021-09-07 08:32 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 9/7/21 9:11 AM, Drouvot, Bertrand wrote:
>
> On 9/7/21 8:51 AM, Amit Kapila wrote:
>> On Tue, Sep 7, 2021 at 11:33 AM Drouvot, Bertrand
>> <bdrouvot@amazon.com> wrote:
>>> Hi,
>>>
>>> On 9/7/21 7:58 AM, Dilip Kumar wrote:
>>>
>>>
>>> On Tue, Sep 7, 2021 at 11:10 AM Amit Kapila
>>> <amit.kapila16@gmail.com> wrote:
>>>
>>>>>> Isn't it better if we use option 2) at all places as then we won't
>>>>>> need any special check inside ReorderBufferChangeMemoryUpdate()?
>>>>>
>>>>> If we want to do this then be careful about
>>>>> REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID change. Basically,
>>>>> ReorderBufferChangeMemoryUpdate() ignores this type of change
>>>>> whereas ReorderBufferChangeSize(), consider at least
>>>>> sizeof(ReorderBufferChange) bytes to this change. So if we
>>>>> compute the size using ReorderBufferChangeSize() outside of
>>>>> ReorderBufferChangeMemoryUpdate(), then total size will be
>>>>> different from what we have now. Logically, we should be
>>>>> ignoring/asserting REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID in
>>>>> ReorderBufferChangeSize(), because
>>>>> ReorderBufferChangeMemoryUpdate() is the only caller for this
>>>>> function.
>>>>>
>>>> Why can't we simply ignore it in ReorderBufferChangeMemoryUpdate() as
>>>> we are doing now?
>>>
>>> Yeah right, we can actually do that, it doesn't matter even if we
>>> are passing the size from outside.
>>>
>>> Agree, if no objections, I'll prepare a patch with the modified
>>> approach of option 2) proposed by Amit (means passing the size from
>>> the outside in all the cases).
>>>
>> Sounds reasonable. Another point that needs some thought is do we want
>> to backpatch this change till v13? I am not sure if there is any
>> user-visible bug here but maybe it is still good to fix this in back
>> branches. What do you think?
>
> Yes, +1 to backpatch till v13 "per precaution".
>
> I will first come back with a proposed version for master and once we
> agree on a final/polished version then I'll do the backpatch ones (if
> needed).
Please find enclosed patch v2 (for the master branch) implementing the
modified approach of option 2) proposed by Amit.
Thanks
Bertrand
Attachments:
v2-0001-ReorderBufferChangeMemoryUpdate-failed-assertion.patch (text/plain, 3.6K) download
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 7378beb684..d409011ba7 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -290,7 +290,8 @@ static void ReorderBufferToastAppendChunk(ReorderBuffer *rb, ReorderBufferTXN *t
*/
static Size ReorderBufferChangeSize(ReorderBufferChange *change);
static void ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
- ReorderBufferChange *change, bool addition);
+ ReorderBufferChange *change,
+ bool addition, Size sz);
/*
* Allocate a new ReorderBuffer and clean out any old serialized state from
@@ -474,7 +475,8 @@ ReorderBufferReturnChange(ReorderBuffer *rb, ReorderBufferChange *change,
{
/* update memory accounting info */
if (upd_mem)
- ReorderBufferChangeMemoryUpdate(rb, change, false);
+ ReorderBufferChangeMemoryUpdate(rb, change, false,
+ ReorderBufferChangeSize(change));
/* free contained data */
switch (change->action)
@@ -792,7 +794,8 @@ ReorderBufferQueueChange(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn,
txn->nentries_mem++;
/* update memory accounting information */
- ReorderBufferChangeMemoryUpdate(rb, change, true);
+ ReorderBufferChangeMemoryUpdate(rb, change, true,
+ ReorderBufferChangeSize(change));
/* process partial change */
ReorderBufferProcessPartialChange(rb, txn, change, toast_insert);
@@ -3100,9 +3103,8 @@ ReorderBufferAddNewCommandId(ReorderBuffer *rb, TransactionId xid,
static void
ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
ReorderBufferChange *change,
- bool addition)
+ bool addition, Size sz)
{
- Size sz;
ReorderBufferTXN *txn;
ReorderBufferTXN *toptxn;
@@ -3127,8 +3129,6 @@ ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
else
toptxn = txn;
- sz = ReorderBufferChangeSize(change);
-
if (addition)
{
txn->size += sz;
@@ -4359,7 +4359,8 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
* update the accounting too (subtracting the size from the counters). And
* we don't want to underflow there.
*/
- ReorderBufferChangeMemoryUpdate(rb, change, true);
+ ReorderBufferChangeMemoryUpdate(rb, change, true,
+ ReorderBufferChangeSize(change));
}
/*
@@ -4605,17 +4606,19 @@ ReorderBufferToastReplace(ReorderBuffer *rb, ReorderBufferTXN *txn,
TupleDesc toast_desc;
MemoryContext oldcontext;
ReorderBufferTupleBuf *newtup;
+ Size old_size;
/* no toast tuples changed */
if (txn->toast_hash == NULL)
return;
/*
- * We're going to modify the size of the change, so to make sure the
- * accounting is correct we'll make it look like we're removing the change
- * now (with the old size), and then re-add it at the end.
+ * We're going to modify the size of the change. So, to make sure the
+ * accounting is correct we record the current change size and then after
+ * re-computing the change we'll subtract the recorded size and then
+ * re-add the new change size at the end.
*/
- ReorderBufferChangeMemoryUpdate(rb, change, false);
+ old_size = ReorderBufferChangeSize(change);
oldcontext = MemoryContextSwitchTo(rb->context);
@@ -4766,8 +4769,11 @@ ReorderBufferToastReplace(ReorderBuffer *rb, ReorderBufferTXN *txn,
MemoryContextSwitchTo(oldcontext);
+ /* substract the old change size */
+ ReorderBufferChangeMemoryUpdate(rb, change, false, old_size);
/* now add the change back, with the correct size */
- ReorderBufferChangeMemoryUpdate(rb, change, true);
+ ReorderBufferChangeMemoryUpdate(rb, change, true,
+ ReorderBufferChangeSize(change));
}
/*
* Re: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:37 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 05:40 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:58 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 06:03 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 06:51 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 07:11 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 08:32 ` Re: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
@ 2021-09-09 11:16 ` Amit Kapila <amit.kapila16@gmail.com>
2021-09-09 13:16 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
0 siblings, 1 reply; 19+ messages in thread
From: Amit Kapila @ 2021-09-09 11:16 UTC (permalink / raw)
To: Drouvot, Bertrand <bdrouvot@amazon.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Sep 7, 2021 at 2:02 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>
> On 9/7/21 9:11 AM, Drouvot, Bertrand wrote:
> >
>
> Please find enclosed patch v2 (for the master branch) implementing the
> modified approach of option 2) proposed by Amit.
>
The patch looks good to me. I have made a minor modification in the
comment to make it explicit why we are not subtracting the size
immediately and ran pgindent. Kindly prepare back-branch patches and
test the same.
--
With Regards,
Amit Kapila.
Attachments:
v3-0001-Fix-reorder-buffer-memory-accounting-for-toast-ch.patch (application/octet-stream, 5.1K) download
From cc176fc03bd23af2f44614330038959a568359fb Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Thu, 9 Sep 2021 16:21:44 +0530
Subject: [PATCH v4] Fix reorder buffer memory accounting for toast changes.
While processing toast changes in logical decoding, we rejigger the
tuple change to point to in-memory toast tuples instead to on-disk toast
tuples. And, to make sure the memory accounting is correct, we were
subtracting the old change size and then after re-computing the new tuple,
re-adding its size at the end. Now, if there is any error before we add
the new size, we will release the changes and that will update the
accounting info (subtracting the size from the counters). And we were
underflowing there which leads to an assertion failure in assert enabled
builds and wrong memory accounting in reorder buffer otherwise.
Author: Bertrand Drouvot
Reviewed-by: Amit Kapila
Backpatch-through: 13, where memory accounting was introduced
Discussion: https://postgr.es/m/92b0ee65-b8bd-e42d-c082-4f3f4bf12d34@amazon.com
---
src/backend/replication/logical/reorderbuffer.c | 36 ++++++++++++++++---------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 7378beb..bb0e579 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -290,7 +290,8 @@ static void ReorderBufferToastAppendChunk(ReorderBuffer *rb, ReorderBufferTXN *t
*/
static Size ReorderBufferChangeSize(ReorderBufferChange *change);
static void ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
- ReorderBufferChange *change, bool addition);
+ ReorderBufferChange *change,
+ bool addition, Size sz);
/*
* Allocate a new ReorderBuffer and clean out any old serialized state from
@@ -474,7 +475,8 @@ ReorderBufferReturnChange(ReorderBuffer *rb, ReorderBufferChange *change,
{
/* update memory accounting info */
if (upd_mem)
- ReorderBufferChangeMemoryUpdate(rb, change, false);
+ ReorderBufferChangeMemoryUpdate(rb, change, false,
+ ReorderBufferChangeSize(change));
/* free contained data */
switch (change->action)
@@ -792,7 +794,8 @@ ReorderBufferQueueChange(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn,
txn->nentries_mem++;
/* update memory accounting information */
- ReorderBufferChangeMemoryUpdate(rb, change, true);
+ ReorderBufferChangeMemoryUpdate(rb, change, true,
+ ReorderBufferChangeSize(change));
/* process partial change */
ReorderBufferProcessPartialChange(rb, txn, change, toast_insert);
@@ -3100,9 +3103,8 @@ ReorderBufferAddNewCommandId(ReorderBuffer *rb, TransactionId xid,
static void
ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
ReorderBufferChange *change,
- bool addition)
+ bool addition, Size sz)
{
- Size sz;
ReorderBufferTXN *txn;
ReorderBufferTXN *toptxn;
@@ -3127,8 +3129,6 @@ ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
else
toptxn = txn;
- sz = ReorderBufferChangeSize(change);
-
if (addition)
{
txn->size += sz;
@@ -4359,7 +4359,8 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
* update the accounting too (subtracting the size from the counters). And
* we don't want to underflow there.
*/
- ReorderBufferChangeMemoryUpdate(rb, change, true);
+ ReorderBufferChangeMemoryUpdate(rb, change, true,
+ ReorderBufferChangeSize(change));
}
/*
@@ -4605,17 +4606,23 @@ ReorderBufferToastReplace(ReorderBuffer *rb, ReorderBufferTXN *txn,
TupleDesc toast_desc;
MemoryContext oldcontext;
ReorderBufferTupleBuf *newtup;
+ Size old_size;
/* no toast tuples changed */
if (txn->toast_hash == NULL)
return;
/*
- * We're going to modify the size of the change, so to make sure the
- * accounting is correct we'll make it look like we're removing the change
- * now (with the old size), and then re-add it at the end.
+ * We're going to modify the size of the change. So, to make sure the
+ * accounting is correct we record the current change size and then after
+ * re-computing the change we'll subtract the recorded size and then
+ * re-add the new change size at the end. We don't immediately subtract
+ * the old size because if there is any error before we add the new size,
+ * we will release the changes and that will update the accounting info
+ * (subtracting the size from the counters). And we don't want to
+ * underflow there.
*/
- ReorderBufferChangeMemoryUpdate(rb, change, false);
+ old_size = ReorderBufferChangeSize(change);
oldcontext = MemoryContextSwitchTo(rb->context);
@@ -4766,8 +4773,11 @@ ReorderBufferToastReplace(ReorderBuffer *rb, ReorderBufferTXN *txn,
MemoryContextSwitchTo(oldcontext);
+ /* substract the old change size */
+ ReorderBufferChangeMemoryUpdate(rb, change, false, old_size);
/* now add the change back, with the correct size */
- ReorderBufferChangeMemoryUpdate(rb, change, true);
+ ReorderBufferChangeMemoryUpdate(rb, change, true,
+ ReorderBufferChangeSize(change));
}
/*
--
1.8.3.1
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:37 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 05:40 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:58 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 06:03 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 06:51 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 07:11 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 08:32 ` Re: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-09 11:16 ` Re: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
@ 2021-09-09 13:16 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-13 06:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
0 siblings, 1 reply; 19+ messages in thread
From: Drouvot, Bertrand @ 2021-09-09 13:16 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 9/9/21 1:16 PM, Amit Kapila wrote:
> On Tue, Sep 7, 2021 at 2:02 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>> On 9/7/21 9:11 AM, Drouvot, Bertrand wrote:
>> Please find enclosed patch v2 (for the master branch) implementing the
>> modified approach of option 2) proposed by Amit.
>>
> The patch looks good to me. I have made a minor modification in the
> comment to make it explicit why we are not subtracting the size
> immediately
Thanks!
Indeed, good point to also mention in the comment why it has been done
that way.
> Kindly prepare back-branch patches and
> test the same.
Please find enclosed the modified (and tested) version for the
REL_13_STABLE branch (the master patch version also applied on
REL_14_STABLE).
Thanks
Bertrand
Attachments:
v3-0001-Fix-reorder-buffer-memory-accounting-for-toast-ch-13_STABLE.patch (text/plain, 5.1K) download
From 71f035b9f34e98251b91bcba3030d8b3317904c6 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Thu, 9 Sep 2021 16:21:44 +0530
Subject: [PATCH v3] Fix reorder buffer memory accounting for toast changes.
While processing toast changes in logical decoding, we rejigger the
tuple change to point to in-memory toast tuples instead to on-disk toast
tuples. And, to make sure the memory accounting is correct, we were
subtracting the old change size and then after re-computing the new tuple,
re-adding its size at the end. Now, if there is any error before we add
the new size, we will release the changes and that will update the
accounting info (subtracting the size from the counters). And we were
underflowing there which leads to an assertion failure in assert enabled
builds and wrong memory accounting in reorder buffer otherwise.
Author: Bertrand Drouvot
Reviewed-by: Amit Kapila
Backpatch-through: 13, where memory accounting was introduced
Discussion: https://postgr.es/m/92b0ee65-b8bd-e42d-c082-4f3f4bf12d34@amazon.com
---
src/backend/replication/logical/reorderbuffer.c | 37 +++++++++++++++----------
1 file changed, 23 insertions(+), 14 deletions(-)
100.0% src/backend/replication/logical/
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index b904697f18..9264dc247a 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -262,7 +262,8 @@ static void ReorderBufferToastAppendChunk(ReorderBuffer *rb, ReorderBufferTXN *t
*/
static Size ReorderBufferChangeSize(ReorderBufferChange *change);
static void ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
- ReorderBufferChange *change, bool addition);
+ ReorderBufferChange *change,
+ bool addition, Size sz);
/*
* Allocate a new ReorderBuffer and clean out any old serialized state from
@@ -425,7 +426,8 @@ void
ReorderBufferReturnChange(ReorderBuffer *rb, ReorderBufferChange *change)
{
/* update memory accounting info */
- ReorderBufferChangeMemoryUpdate(rb, change, false);
+ ReorderBufferChangeMemoryUpdate(rb, change, false,
+ ReorderBufferChangeSize(change));
/* free contained data */
switch (change->action)
@@ -647,7 +649,8 @@ ReorderBufferQueueChange(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn,
txn->nentries_mem++;
/* update memory accounting information */
- ReorderBufferChangeMemoryUpdate(rb, change, true);
+ ReorderBufferChangeMemoryUpdate(rb, change, true,
+ ReorderBufferChangeSize(change));
/* check the memory limits and evict something if needed */
ReorderBufferCheckMemoryLimit(rb);
@@ -2160,10 +2163,8 @@ ReorderBufferAddNewCommandId(ReorderBuffer *rb, TransactionId xid,
static void
ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
ReorderBufferChange *change,
- bool addition)
+ bool addition, Size sz)
{
- Size sz;
-
Assert(change->txn);
/*
@@ -2174,8 +2175,6 @@ ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
if (change->action == REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID)
return;
- sz = ReorderBufferChangeSize(change);
-
if (addition)
{
change->txn->size += sz;
@@ -3073,7 +3072,8 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
* update the accounting too (subtracting the size from the counters). And
* we don't want to underflow there.
*/
- ReorderBufferChangeMemoryUpdate(rb, change, true);
+ ReorderBufferChangeMemoryUpdate(rb, change, true,
+ ReorderBufferChangeSize(change));
}
/*
@@ -3321,17 +3321,23 @@ ReorderBufferToastReplace(ReorderBuffer *rb, ReorderBufferTXN *txn,
TupleDesc toast_desc;
MemoryContext oldcontext;
ReorderBufferTupleBuf *newtup;
+ Size old_size;
/* no toast tuples changed */
if (txn->toast_hash == NULL)
return;
/*
- * We're going to modify the size of the change, so to make sure the
- * accounting is correct we'll make it look like we're removing the change
- * now (with the old size), and then re-add it at the end.
+ * We're going to modify the size of the change. So, to make sure the
+ * accounting is correct we record the current change size and then after
+ * re-computing the change we'll subtract the recorded size and then
+ * re-add the new change size at the end. We don't immediately subtract
+ * the old size because if there is any error before we add the new size,
+ * we will release the changes and that will update the accounting info
+ * (subtracting the size from the counters). And we don't want to
+ * underflow there.
*/
- ReorderBufferChangeMemoryUpdate(rb, change, false);
+ old_size = ReorderBufferChangeSize(change);
oldcontext = MemoryContextSwitchTo(rb->context);
@@ -3482,8 +3488,11 @@ ReorderBufferToastReplace(ReorderBuffer *rb, ReorderBufferTXN *txn,
MemoryContextSwitchTo(oldcontext);
+ /* substract the old change size */
+ ReorderBufferChangeMemoryUpdate(rb, change, false, old_size);
/* now add the change back, with the correct size */
- ReorderBufferChangeMemoryUpdate(rb, change, true);
+ ReorderBufferChangeMemoryUpdate(rb, change, true,
+ ReorderBufferChangeSize(change));
}
/*
--
2.16.6
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:37 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 05:40 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:58 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 06:03 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 06:51 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 07:11 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 08:32 ` Re: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-09 11:16 ` Re: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-09 13:16 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
@ 2021-09-13 06:34 ` Drouvot, Bertrand <bdrouvot@amazon.com>
0 siblings, 0 replies; 19+ messages in thread
From: Drouvot, Bertrand @ 2021-09-13 06:34 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
On 9/9/21 3:16 PM, Drouvot, Bertrand wrote:
> Hi,
>
> On 9/9/21 1:16 PM, Amit Kapila wrote:
>> On Tue, Sep 7, 2021 at 2:02 PM Drouvot, Bertrand
>> <bdrouvot@amazon.com> wrote:
>>> On 9/7/21 9:11 AM, Drouvot, Bertrand wrote:
>>> Please find enclosed patch v2 (for the master branch) implementing the
>>> modified approach of option 2) proposed by Amit.
>>>
>> The patch looks good to me. I have made a minor modification in the
>> comment to make it explicit why we are not subtracting the size
>> immediately
>
> Thanks!
>
> Indeed, good point to also mention in the comment why it has been done
> that way.
>
>> Kindly prepare back-branch patches and
>> test the same.
>
> Please find enclosed the modified (and tested) version for the
> REL_13_STABLE branch (the master patch version also applied on
> REL_14_STABLE).
>
I've seen that the patches have been committed, thanks!
I'm marking the corresponding CF entry as committed.
Thanks
Bertrand
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
@ 2021-09-13 16:52 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-09-13 17:12 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 19+ messages in thread
From: Alvaro Herrera @ 2021-09-13 16:52 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Drouvot, Bertrand <bdrouvot@amazon.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Dilip Kumar <dilipbalaut@gmail.com>
On 2021-Sep-06, Amit Kapila wrote:
> The error can occur at multiple places (like via palloc or various
> other places) between the first time we subtract the change_size and
> add it back after the change is re-computed. I think the correct fix
> would be that in the beginning we just compute the change_size by
> ReorderBufferChangeSize and then after re-computing the change, we
> just subtract the old change_size and add the new change_size. What do
> you think?
Am I the only that that thinks this code is doing far too much in a
PG_CATCH block?
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"Those who use electric razors are infidels destined to burn in hell while
we drink from rivers of beer, download free vids and mingle with naked
well shaved babes." (http://slashdot.org/comments.pl?sid=44793&cid=4647152)
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-13 16:52 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2021-09-13 17:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-09-14 09:10 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Tom Lane @ 2021-09-13 17:12 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Drouvot, Bertrand <bdrouvot@amazon.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Dilip Kumar <dilipbalaut@gmail.com>
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Am I the only that that thinks this code is doing far too much in a
> PG_CATCH block?
You mean the one in ReorderBufferProcessTXN? Yeah, that is mighty
ugly. It might be all right given that it almost immediately does
AbortCurrentTransaction, since that should basically clean up
whatever is wrong. But I'm still full of questions after a brief
look at it.
* What is the sense in calling RollbackAndReleaseCurrentSubTransaction
after having done AbortCurrentTransaction?
* Is it really sane that we re-throw the error in some cases and
not others? What is likely to catch that thrown error, and is it
going to be prepared for us having already aborted the transaction?
(It doesn't give me a warm feeling that the code coverage report
shows that path to be un-exercised.)
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Amit Kapila <amit.kapila16@gmail.com>
2021-09-13 16:52 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-09-13 17:12 ` Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-09-14 09:10 ` Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 0 replies; 19+ messages in thread
From: Amit Kapila @ 2021-09-14 09:10 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; Drouvot, Bertrand <bdrouvot@amazon.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Dilip Kumar <dilipbalaut@gmail.com>
On Mon, Sep 13, 2021 at 10:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Am I the only that that thinks this code is doing far too much in a
> > PG_CATCH block?
>
> You mean the one in ReorderBufferProcessTXN? Yeah, that is mighty
> ugly. It might be all right given that it almost immediately does
> AbortCurrentTransaction, since that should basically clean up
> whatever is wrong. But I'm still full of questions after a brief
> look at it.
>
> * What is the sense in calling RollbackAndReleaseCurrentSubTransaction
> after having done AbortCurrentTransaction?
>
It is required for the cases where we are starting internal
sub-transaction (when decoding via SQL SRF). The first
AbortCurrentTransaction will make the current subtransaction state to
TBLOCK_SUBABORT and then RollbackAndReleaseCurrentSubTransaction will
pop and release the current subtransaction. Note that the same
sequence is performed outside catch and if we comment out
RollbackAndReleaseCurrentSubTransaction, there are a lot of failures
in test_decoding. I have manually debugged and checked that if we
don't call RollbackAndReleaseCurrentSubTransaction in the catch block,
it will retain the transaction in a bad state which on the next
operation leads to a FATAL error.
> * Is it really sane that we re-throw the error in some cases and
> not others? What is likely to catch that thrown error, and is it
> going to be prepared for us having already aborted the transaction?
>
There are two different ways which deal with the re-thrown error. For
WALSender processes, we don't start internal subtransaction and on
error, they simply exist. For SQL SRFs, we start internal transactions
which will be released in the catch block, and then the top
transaction will be aborted after we re-throw the error.
The cases where we don't immediately rethrow are specifically for
streaming transactions where we might have already sent some changes
to the subscriber and we want to continue processing and send the
abort to subscribers.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2021-09-14 09:10 UTC | newest]
Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13 09:45 [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 10:34 ` Amit Kapila <amit.kapila16@gmail.com>
2021-09-06 11:54 ` Dilip Kumar <dilipbalaut@gmail.com>
2021-09-06 15:24 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-06 15:43 ` Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 03:08 ` Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:37 ` Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 05:40 ` Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 05:58 ` Dilip Kumar <dilipbalaut@gmail.com>
2021-09-07 06:03 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 06:51 ` Amit Kapila <amit.kapila16@gmail.com>
2021-09-07 07:11 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-07 08:32 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-09 11:16 ` Amit Kapila <amit.kapila16@gmail.com>
2021-09-09 13:16 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-13 06:34 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2021-09-13 16:52 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-09-13 17:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-09-14 09:10 ` Amit Kapila <amit.kapila16@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox