On Mon, Aug 12, 2024 at 3:37 PM shveta malik <shveta.ma...@gmail.com> wrote: > > On Fri, Aug 9, 2024 at 2:39 PM Hayato Kuroda (Fujitsu) > <kuroda.hay...@fujitsu.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.
