On Mon, Nov 2, 2020 at 9:40 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Oct 28, 2020 at 10:50 AM Peter Smith <smithpb2...@gmail.com> wrote: > > > > Hi Ajin. > > > > I have re-checked the v13 patches for how my remaining review comments > > have been addressed. > > > > On Tue, Oct 27, 2020 at 8:55 PM Ajin Cherian <itsa...@gmail.com> wrote: > > > > > > > ==================== > > > > v12-0002. File: src/backend/replication/logical/reorderbuffer.c > > > > ==================== > > > > > > > > COMMENT > > > > Line 2401 > > > > /* > > > > * We are here due to one of the 3 scenarios: > > > > * 1. As part of streaming in-progress transactions > > > > * 2. Prepare of a two-phase commit > > > > * 3. Commit of a transaction. > > > > * > > > > * If we are streaming the in-progress transaction then discard the > > > > * changes that we just streamed, and mark the transactions as > > > > * streamed (if they contained changes), set prepared flag as false. > > > > * If part of a prepare of a two-phase commit set the prepared flag > > > > * as true so that we can discard changes and cleanup tuplecids. > > > > * Otherwise, remove all the > > > > * changes and deallocate the ReorderBufferTXN. > > > > */ > > > > ~ > > > > The above comment is beyond my understanding. Anything you could do to > > > > simplify it would be good. > > > > > > > > For example, when viewing this function in isolation I have never > > > > understood why the streaming flag and rbtxn_prepared(txn) flag are not > > > > possible to be set at the same time? > > > > > > > > Perhaps the code is relying on just internal knowledge of how this > > > > helper function gets called? And if it is just that, then IMO there > > > > really should be some Asserts in the code to give more assurance about > > > > that. (Or maybe use completely different flags to represent those 3 > > > > scenarios instead of bending the meanings of the existing flags) > > > > > > > > > > Left this for now, probably re-look at this at a later review. > > > But just to explain; this function is what does the main decoding of > > > changes of a transaction. > > > At what point this decoding happens is what this feature and the > > > streaming in-progress feature is about. As of PG13, this decoding only > > > happens at commit time. With the streaming of in-progress txn feature, > > > this began to happen (if streaming enabled) at the time when the > > > memory limit for decoding transactions was crossed. This 2PC feature > > > is supporting decoding at the time of a PREPARE transaction. > > > Now, if streaming is enabled and streaming has started as a result of > > > crossing the memory threshold, then there is no need to > > > again begin streaming at a PREPARE transaction as the transaction that > > > is being prepared has already been streamed. > > > > > I don't think this is true, think of a case where we need to send the > last set of changes along with PREPARE. In that case we need to stream > those changes at the time of PREPARE. If I am correct then as pointed > by Peter you need to change some comments and some of the assumptions > related to this you have in the patch.
I have changed the asserts and the comments to reflect this.
>
> Few more comments on the latest patch
> (v15-0002-Support-2PC-txn-backend-and-tests)
> =========================================================================
> 1.
> @@ -274,6 +296,23 @@ DecodeXactOp(LogicalDecodingContext *ctx,
> XLogRecordBuffer *buf)
> DecodeAbort(ctx, buf, &parsed, xid);
> break;
> }
> + case XLOG_XACT_ABORT_PREPARED:
> + {
>
> ..
> +
> + if (!TransactionIdIsValid(parsed.twophase_xid))
> + xid = XLogRecGetXid(r);
> + else
> + xid = parsed.twophase_xid;
>
> I think we don't need this 'if' check here because you must have a
> valid value of parsed.twophase_xid;. But, I think this will be moot if
> you address the review comment in my previous email such that the
> handling of XLOG_XACT_ABORT_PREPARED and XLOG_XACT_ABORT will be
> combined as it is there without the patch.
>
> 2.
> +DecodePrepare(LogicalDecodingContext *ctx, XLogRecordBuffer *buf,
> + xl_xact_parsed_prepare * parsed)
> +{
> ..
> + if (SnapBuildXactNeedsSkip(ctx->snapshot_builder, buf->origptr) ||
> + (parsed->dbId != InvalidOid && parsed->dbId != ctx->slot->data.database) ||
> + ctx->fast_forward || FilterByOrigin(ctx, origin_id))
> + return;
> +
>
> I think this check is the same as the check in DecodeCommit, so you
> can write some comments to indicate the same and also why we don't
> need to call ReorderBufferForget here. One more thing is to note is
> even if we don't need to call ReorderBufferForget here but still we
> need to execute invalidations (which are present in top-level txn) for
> the reasons mentioned in ReorderBufferForget. Also, if we do this,
> don't forget to update the comment atop
> ReorderBufferImmediateInvalidation.
I have updated the comments. I wasn't sure of when to execute
invalidations. Should I only
execute invalidations if this was for another database than what was
being decoded or should
I execute invalidation every time we skip? I will also have to create
a new function in reorderbuffer,c similar to ReorderBufferForget
as the txn is not available in decode.c.
>
> 3.
> + /* This is a PREPARED transaction, part of a two-phase commit.
> + * The full cleanup will happen as part of the COMMIT PREPAREDs, so now
> + * just truncate txn by removing changes and tuple_cids
> + */
> + ReorderBufferTruncateTXN(rb, txn, true);
>
> The first line in the multi-line comment should be empty.
Updated.
regards,
Ajin Cherian
Fujitsu Australia
v16-0002-Support-2PC-txn-backend.patch
Description: Binary data
v16-0001-Support-2PC-txn-base.patch
Description: Binary data
v16-0003-Support-2PC-test-cases-for-test_decoding.patch
Description: Binary data
