Thread: Reset snapshot export state on the transaction abort
As reported at [1], if the transaction is aborted during export snapshot then ExportInProgress and SavedResourceOwnerDuringExport are not getting reset and that is throwing an error "clearing exported snapshot in wrong transaction state" while executing the next command. The attached patch clears this state if the transaction is aborted. [1] https://www.postgresql.org/message-id/CAFiTN-tqopqpfS6HHug2nnOGieJJ_nm-Nvy0WBZ=Zpo-LqtSJA@mail.gmail.com -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Mon, Oct 11, 2021 at 08:46:32PM +0530, Dilip Kumar wrote:
> As reported at [1], if the transaction is aborted during export
> snapshot then ExportInProgress and SavedResourceOwnerDuringExport are
> not getting reset and that is throwing an error
> "clearing exported snapshot in wrong transaction state" while
> executing the next command. The attached patch clears this state if
> the transaction is aborted.
Injecting an error is enough to reproduce the failure in a second
command after the first one failed. This could happen on OOM for the
palloc() done at the beginning of SnapBuildInitialSnapshot().
@@ -2698,6 +2698,9 @@ AbortTransaction(void)
/* Reset logical streaming state. */
ResetLogicalStreamingState();
+ /* Reset snapshot export state. */
+ ResetSnapBuildExportSnapshotState();
Shouldn't we care about the case of a sub-transaction abort as well?
See AbortSubTransaction().
--
Michael
Attachment
On Wed, Oct 13, 2021 at 10:17 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Oct 11, 2021 at 08:46:32PM +0530, Dilip Kumar wrote: > > As reported at [1], if the transaction is aborted during export > > snapshot then ExportInProgress and SavedResourceOwnerDuringExport are > > not getting reset and that is throwing an error > > "clearing exported snapshot in wrong transaction state" while > > executing the next command. The attached patch clears this state if > > the transaction is aborted. Correct. > Injecting an error is enough to reproduce the failure in a second > command after the first one failed. This could happen on OOM for the > palloc() done at the beginning of SnapBuildInitialSnapshot(). > > @@ -2698,6 +2698,9 @@ AbortTransaction(void) > /* Reset logical streaming state. */ > ResetLogicalStreamingState(); > > + /* Reset snapshot export state. */ > + ResetSnapBuildExportSnapshotState(); > Shouldn't we care about the case of a sub-transaction abort as well? > See AbortSubTransaction(). Actually, it is not required because 1) Snapshot export can not be allowed within a transaction block, basically, it starts its own transaction block and aborts that while executing any next replication command see SnapBuildClearExportedSnapshot(). So our problem is only if the transaction block internally started for exporting, gets aborted before any next command arrives. So there is no possibility of starting any sub transaction. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Wed, Oct 13, 2021 at 10:53:24AM +0530, Dilip Kumar wrote: > Actually, it is not required because 1) Snapshot export can not be > allowed within a transaction block, basically, it starts its own > transaction block and aborts that while executing any next replication > command see SnapBuildClearExportedSnapshot(). So our problem is only > if the transaction block internally started for exporting, gets > aborted before any next command arrives. So there is no possibility > of starting any sub transaction. Yes, you are right here. I did not remember the semantics this relies on. I have played more with the patch, reviewed the whole, and the fields you are resetting as part of the snapshot builds seem correct to me. So let's fix this. -- Michael
Attachment
On Thu, Oct 14, 2021 at 12:24 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Oct 13, 2021 at 10:53:24AM +0530, Dilip Kumar wrote: > > Actually, it is not required because 1) Snapshot export can not be > > allowed within a transaction block, basically, it starts its own > > transaction block and aborts that while executing any next replication > > command see SnapBuildClearExportedSnapshot(). So our problem is only > > if the transaction block internally started for exporting, gets > > aborted before any next command arrives. So there is no possibility > > of starting any sub transaction. > > Yes, you are right here. I did not remember the semantics this relies > on. I have played more with the patch, reviewed the whole, and the > fields you are resetting as part of the snapshot builds seem correct > to me. So let's fix this. Great, thanks! -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Oct 14, 2021 at 02:58:55PM +0530, Dilip Kumar wrote: > On Thu, Oct 14, 2021 at 12:24 PM Michael Paquier <michael@paquier.xyz> wrote: >> Yes, you are right here. I did not remember the semantics this relies >> on. I have played more with the patch, reviewed the whole, and the >> fields you are resetting as part of the snapshot builds seem correct >> to me. So let's fix this. > > Great, thanks! While double-checking this stuff, I have noticed something that's wrong in the patch when a command that follows a CREATE_REPLICATION_SLOT query resets SnapBuildClearExportedSnapshot(). Once the slot is created, the WAL sender is in a TRANS_INPROGRESS state, meaning that AbortCurrentTransaction() would call AbortTransaction(), hence calling ResetSnapBuildExportSnapshotState() and resetting SavedResourceOwnerDuringExport to NULL before we store a NULL into CurrentResourceOwner :) One solution would be as simple as saving SavedResourceOwnerDuringExport into a temporary variable before calling AbortCurrentTransaction(), and save it back into CurrentResourceOwner once we are done in SnapBuildClearExportedSnapshot() as we need to rely on AbortTransaction() to do the static state cleanup if an error happens until the command after the replslot creation command shows up. -- Michael
Attachment
On Sat, Oct 16, 2021 at 9:13 AM Michael Paquier <michael@paquier.xyz> wrote: > > While double-checking this stuff, I have noticed something that's > wrong in the patch when a command that follows a > CREATE_REPLICATION_SLOT query resets SnapBuildClearExportedSnapshot(). > Once the slot is created, the WAL sender is in a TRANS_INPROGRESS > state, meaning that AbortCurrentTransaction() would call > AbortTransaction(), hence calling ResetSnapBuildExportSnapshotState() > and resetting SavedResourceOwnerDuringExport to NULL before we store a > NULL into CurrentResourceOwner :) Right, good catch! > One solution would be as simple as saving > SavedResourceOwnerDuringExport into a temporary variable before > calling AbortCurrentTransaction(), and save it back into > CurrentResourceOwner once we are done in > SnapBuildClearExportedSnapshot() as we need to rely on > AbortTransaction() to do the static state cleanup if an error happens > until the command after the replslot creation command shows up. Yeah, this idea looks fine to me. I have modified the patch. In addition to that I have removed calling ResetSnapBuildExportSnapshotState from the SnapBuildClearExportedSnapshot because that is anyway being called from the AbortTransaction. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Sat, Oct 16, 2021 at 3:10 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sat, Oct 16, 2021 at 9:13 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> While double-checking this stuff, I have noticed something that's
> wrong in the patch when a command that follows a
> CREATE_REPLICATION_SLOT query resets SnapBuildClearExportedSnapshot().
> Once the slot is created, the WAL sender is in a TRANS_INPROGRESS
> state, meaning that AbortCurrentTransaction() would call
> AbortTransaction(), hence calling ResetSnapBuildExportSnapshotState()
> and resetting SavedResourceOwnerDuringExport to NULL before we store a
> NULL into CurrentResourceOwner :)
Right, good catch!
> One solution would be as simple as saving
> SavedResourceOwnerDuringExport into a temporary variable before
> calling AbortCurrentTransaction(), and save it back into
> CurrentResourceOwner once we are done in
> SnapBuildClearExportedSnapshot() as we need to rely on
> AbortTransaction() to do the static state cleanup if an error happens
> until the command after the replslot creation command shows up.
Yeah, this idea looks fine to me. I have modified the patch. In
addition to that I have removed calling
ResetSnapBuildExportSnapshotState from the
SnapBuildClearExportedSnapshot because that is anyway being called
from the AbortTransaction.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Hi,
bq. While exporting a snapshot we set a temporary states which get
a temporary states -> temporary states
+extern void ResetSnapBuildExportSnapshotState(void);
ResetSnapBuildExportSnapshotState() is only called inside snapbuild.c
I wonder if the addition to snapbuild.h is needed.
Cheers
On Sat, Oct 16, 2021 at 08:31:36AM -0700, Zhihong Yu wrote:
> On Sat, Oct 16, 2021 at 3:10 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>> On Sat, Oct 16, 2021 at 9:13 AM Michael Paquier <michael@paquier.xyz>
>> wrote:
>>> One solution would be as simple as saving
>>> SavedResourceOwnerDuringExport into a temporary variable before
>>> calling AbortCurrentTransaction(), and save it back into
>>> CurrentResourceOwner once we are done in
>>> SnapBuildClearExportedSnapshot() as we need to rely on
>>> AbortTransaction() to do the static state cleanup if an error happens
>>> until the command after the replslot creation command shows up.
>>
>> Yeah, this idea looks fine to me. I have modified the patch. In
>> addition to that I have removed calling
>> ResetSnapBuildExportSnapshotState from the
>> SnapBuildClearExportedSnapshot because that is anyway being called
>> from the AbortTransaction.
That seems logically fine. I'll check that tomorrow.
> +extern void ResetSnapBuildExportSnapshotState(void);
>
> ResetSnapBuildExportSnapshotState() is only called inside snapbuild.c
> I wonder if the addition to snapbuild.h is needed.
As of xact.c in v2 of the patch, we have that:
@@ -2698,6 +2699,9 @@ AbortTransaction(void)
/* Reset logical streaming state. */
ResetLogicalStreamingState();
+ /* Reset snapshot export state. */
+ ResetSnapBuildExportSnapshotState();
--
Michael
Attachment
On Sun, Oct 17, 2021 at 09:33:48AM +0900, Michael Paquier wrote: > That seems logically fine. I'll check that tomorrow. And that looks indeed fine. I have adjusted a couple of things, and backpatched the fix. -- Michael
Attachment
On Mon, Oct 18, 2021 at 8:51 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Sun, Oct 17, 2021 at 09:33:48AM +0900, Michael Paquier wrote: > > That seems logically fine. I'll check that tomorrow. > > And that looks indeed fine. I have adjusted a couple of things, and > backpatched the fix. Thanks! -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com