On Tue, Aug 13, 2024 at 9:48 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > 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()?
Please find v4 attached. Addressed comments in that.
Manual testing done on v4:
1) Error and Fatal case
2) Shutdown after replorigin_session_origin_lsn was set in
apply_handle_prepare() and before EndPrepare was called.
2a) with log_min_messages=debug5. This will result in processing
of shutdown signal by errfinish() before PREPARE is over.
2b) with default log_min_messages. This will result in processing
of shutdown signal by LogicalRepApplyLoop() after ongoing PREPARE is
over.
>
> 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?
>
Sure, will do next.
thanks
Shveta
v4-0001-Prevent-origin-progress-advancement-if-failed-to-.patch
Description: Binary data
