Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber

On Mon, Aug 12, 2024 at 3:37 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Fri, Aug 9, 2024 at 2:39 PM Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> >
> > > + /*
> > > + * Register a callback to reset the origin state before aborting the
> > > + * transaction in ShutdownPostgres(). This is to prevent the advancement
> > > + * of origin progress if the transaction failed to apply.
> > > + */
> > > + before_shmem_exit(replorigin_reset, (Datum) 0);
> > >
> > > I think we need this despite resetting the origin-related variables in
> > > PG_CATCH block to handle FATAL error cases, right? If so, can we use
> > > PG_ENSURE_ERROR_CLEANUP() instead of PG_CATCH()?
> >
> > There are two reasons to add a shmem-exit callback. One is to support a FATAL,
> > another one is to support the case that user does the shutdown request while
> > applying changes. In this case, I think ShutdownPostgres() can be called so that
> > the session origin may advance.
>
> Agree that we need the 'reset' during shutdown flow as well. Details at [1]
>

Thanks for the detailed analysis. I agree with your analysis that we
need to reset the origin information for the shutdown path to avoid it
being advanced incorrectly. However, the patch doesn't have sufficient
comments to explain why we need to reset it for both the ERROR and
Shutdown paths. Can we improve the comments in the patch?

Also, for the ERROR path, can we reset the origin information in
apply_error_callback()?

BTW, this needs to be backpatched till 16 when it has been introduced
by the parallel apply feature as part of commit 216a7848. So, can we
test this patch in back-branches as well?

--
With Regards,
Amit Kapila.