RE: BUG #18055: logical decoding core on AllocateSnapshotBuilder()

On Wednesday, August 23, 2023 4:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Mon, Aug 21, 2023 at 11:57 PM Andres Freund <andres@anarazel.de>
> wrote:
> >
> > On 2023-08-21 19:07:16 +0530, Amit Kapila wrote:
> > > On Mon, Aug 21, 2023 at 2:35 AM Andres Freund <andres@anarazel.de>
> wrote:
> > > >
> > > > On 2023-08-18 04:21:53 +0000, Zhijie Hou (Fujitsu) wrote:
> > > > > From ee1dfccc0306812c356c84bbd7e2558f27d7d348 Mon Sep 17
> > > > > 00:00:00 2001
> > > > > From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
> > > > > Date: Thu, 17 Aug 2023 19:29:34 +0800
> > > > > Subject: [PATCH v4] cleanup decoding context in error cases
> > > > >
> > > > > Some of the management functions for logical decoding didn't
> > > > > clean up the decoding context when an error occurs during
> > > > > decoding. This can result in accessing unfreed resources and
> > > > > assert failure the next time we call these functions. Fix it by
> > > > > cleaning up the decoding context and slots in error cases.
> > > >
> > > > I don't think this is the right fix - at all. We shouldn't run
> > > > arbitrary code after a failure, which we do by calling the shutdown
> callback.
> > > >
> > >
> >
> > > The other alternatives to fix are (a) Have some Reset* kind of
> > > function (similar to ResetReindexState) to reset the required
> > > variables and call at AbortTransaction time.
> >
> > -1. This makes things global concerns that shouldn't be.
> >
> > If we really need something to clean this up, I'd look at
> > MemoryContextRegisterResetCallback().
> >
> 
> +1. This sounds like a better idea.

Here is a small patch based on this idea.
I registered the callback under snapshot builder context
and the callback will reset the variables.

Best Regards,
Hou zj