[BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()

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.

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

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

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

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.

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

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.

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

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

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.

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

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

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.

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

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

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&amp;cid=4647152)

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

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.