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

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.
>

But OTOH, it can prevent freeing some global memory like in the case
of pgoutput_shutdown which frees some memory allocated in
CacheMemoryContext. Also, as pointed out by Hou-San[1], it can lead to
unwanted behavior as next time we can access some invalid entries.

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. The minor risk could be
that if any extension uses the same function name as ours but not sure
if it is worth worrying about. (b) Initialize the required variables
in AllocateSnapshotBuilder().

[1] -
https://www.postgresql.org/message-id/OS0PR01MB57167C62D7DA4A8EBBC92B0A941BA%40OS0PR01MB5716.jpnprd01.prod.outlook.com

--
With Regards,
Amit Kapila.