Dear Amit, Thank you for reviewing! PSA new version.
> > I think you need to set the new flag only when we are not skipping the
> > transaction or in other words when we decide to process the
> > transaction. Otherwise, how will you distinguish the case where the
> > xact is already decoded and sent to client?
Actually, I wondered what should be, but I followed it. Indeed, we should avoid
the case which the xact has already been sent. But I was not sure other
conditions
like transactions for another database - IIUC previous version regarded it as
not
acceptable.
Now, I reconsider these cases can be ignored because they would not be sent to
subscriber. The consistency between pub/sub would not be broken even if these
WALs are remained.
> In the attached patch atop your v47*, I have changed it to show you
> what I have in mind.
Thanks, was included.
> A few more comments:
> =================
> 1.
> +
> + /*
> + * Did the logical decoding context skip outputting any changes?
> + *
> + * This flag is used only when the context is in the silent mode.
> + */
> + bool output_skipped;
> } LogicalDecodingContext;
>
> This doesn't seem to convey the meaning to the caller. How about
> processing_required? BTW, I have made this change as well in the
> patch.
LGTM, changed like that.
> 2.
> @@ -295,7 +295,7 @@ xact_decode(LogicalDecodingContext *ctx,
> XLogRecordBuffer *buf)
> */
> if (TransactionIdIsValid(xid))
> {
> - if (!ctx->fast_forward)
> + if (ctx->decoding_mode != DECODING_MODE_FAST_FORWARD)
> ReorderBufferAddInvalidations(reorder, xid,
> buf->origptr,
> invals->nmsgs,
> @@ -303,7 +303,7 @@ xact_decode(LogicalDecodingContext *ctx,
> XLogRecordBuffer *buf)
> ReorderBufferXidSetCatalogChanges(ctx->reorder, xid,
> buf->origptr);
> }
> - else if ((!ctx->fast_forward))
> + else if (ctx->decoding_mode != DECODING_MODE_FAST_FORWARD)
> ReorderBufferImmediateInvalidation(ctx->reorder,
> invals->nmsgs,
> invals->msgs);
>
> We don't to execute the invalidations even in silent mode. Looking at
> this and other changes in the patch related to silent mode, I wonder
> whether we really need to introduce 'silent_mode'. Can't we simply set
> processing_required when 'fast_forward' mode is true and then let the
> caller decide whether it needs to further process the WAL?
After considering again, I agreed to remove silent mode. Initially, it was
introduced because did_process flag is set at XXX_cb_wrapper and reorderbuffer
layer. Now, the processing_required is set in
DecodeCommit()->DecodeTXNNeedSkip(),
which means that each records does not need to be decoded. Based on that,
I removed the silent mode and use fast-forwarding mode instead.
Also, some parts (mostly code comments) were modified.
Acknowledgement: Thanks Peter and Hou for discussing with me.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v48-0001-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
Description: v48-0001-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
