Re: Reset snapshot export state on the transaction abort

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