Dear Amit, Shveta, Hou, Thanks for giving many comments! I've updated the patch.
> @@ -4409,6 +4409,14 @@ start_apply(XLogRecPtr origin_startpos)
> }
> PG_CATCH();
> {
> + /*
> + * Reset the origin data to prevent the advancement of origin progress
> + * if the transaction failed to apply.
> + */
> + replorigin_session_origin = InvalidRepOriginId;
> + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> + replorigin_session_origin_timestamp = 0;
>
> Can't we call replorigin_reset() instead here?
I didn't use the function because arguments of calling function looked strange,
but ideally I can. Fixed.
> + /*
> + * 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.
However, I think we cannot use
PG_ENSURE_ERROR_CLEANUP()/PG_END_ENSURE_ERROR_CLEANUP
macros here. According to codes, it assumes that any before-shmem callbacks are
not registered within the block because the cleanup function is registered and
canceled
within the macro. LogicalRepApplyLoop() can register the function when
it handles COMMIT PREPARED message so it breaks the rule.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
v3-0001-Prevent-origin-progress-advancement-if-failed-to-.patch
Description: v3-0001-Prevent-origin-progress-advancement-if-failed-to-.patch
