Re: [HACKERS] logical decoding of two-phase transactions

On Tue, Nov 24, 2020 at 3:29 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Mon, Nov 23, 2020 at 10:35 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > For the first two, as the xact is still not visible to others so we
> > don't need to make it behave like a committed txn. To make the (DDL)
> > changes visible to the current txn, the message
> > REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID copies the snapshot which
> > fills the subxip array. This will be sufficient to make the changes
> > visible to the current txn. For the third, I have checked the code
> > that whenever we have any change message the base snapshot gets set
> > via SnapBuildProcessChange. It is possible that I have missed
> > something but I don't want to call SnapbuildCommittedTxn in
> > DecodePrepare unless we have a clear reason for the same so leaving it
> > for now. Can you or someone see any reason for the same?
>
> I reviewed and tested this and like you said, SnapBuildProcessChange
> sets the base snapshot for every change.
> I did various tests using DDL updates and haven't seen any issues so
> far. I agree with your analysis.
>

Thanks, attached is a further revised version of the patch series.

Changes in v27-0001-Extend-the-output-plugin-API-to-allow-decoding-p
a. Removed the includes which are not required by this patch.
b. Moved the 'check_xid_aborted' parameter to 0004.
c. Added Assert(!ctx->fast_forward); in callback wrappers, because we
won't load the output plugin when fast_forward is set so there is no
chance that we call output plugin APIs. This is why we have this
Assert in all the existing APIs.
d. Adjusted the order of various callback APIs to make the code look consistent.
e. Added/Edited comments and doc updates at various places. Changed
error messages to make them consistent with other similar messages.
f. Some other cosmetic changes like the removal of spurious new lines
and fixed white-space issues.
g. Updated commit message.

Changes in v27-0002-Allow-decoding-at-prepare-time-in-ReorderBuffer
a. Move the check to whether a particular txn can be skipped into a
separate function as the same code for it was repeated at three
different places.
b. ReorderBufferPrepare has a parameter name as commit_lsn whereas it
should be preapre_lsn. Similar changes has been made at various places
in the patch.
c. filter_prepare_cb callback existence is checked in both decode.c
and in filter_prepare_cb_wrapper. Fixed by removing it from decode.c.
d. Fixed miscellaneous comments and some cosmetic changes.
e. Moved the special elog in ReorderBufferProcessTxn to test
concurrent aborts in 0004 patch.
f. Moved the changes related to flags RBTXN_COMMIT_PREPARED and
RBTXN_ROLLBACK_PREPARED to patch 0006 as those are used only in that
patch.
g. Updated commit message.

One problem with this patch is: What if we have assembled a consistent
snapshot after prepare and before commit prepared. In that case, it
will currently just send commit prepared record which would be a bad
idea. It should decode the entire transaction for such cases at commit
prepared time. This same problem is noticed by Arseny Sher, see
problem-2 in email [1].

One idea to fix this could be to check if the snapshot is consistent
to decide whether to skip the prepare and if we skip due to that
reason, then during commit we need to decode the entire transaction.
We can do that by setting a flag in txn->txn_flags such that during
prepare we can set a flag when we skip the prepare because the
snapshot is still not consistent and then used it during commit to see
if we need to decode the entire transaction. But here we need to think
about what would happen after restart? Basically, if it is possible
that after restart the snapshot is consistent for the same transaction
at prepare time and it got skipped due to start_decoding_at (which
moved ahead after restart) then such a solution won't work. Any
thoughts on this?

v27-0004-Support-2PC-txn-tests-for-concurrent-aborts
a. Moved the changes related to testing of concurrent aborts in this
patch from other patches.

v27-0006-Support-2PC-txn-pgoutput
a. Moved the changes related to flags RBTXN_COMMIT_PREPARED and
RBTXN_ROLLBACK_PREPARED from other patch.
b. Included headers required by this patch, previously it seems to be
dependent on other patches for this.

The other patches remain unchanged.

Let me know what you think about these changes?

[1] - https://www.postgresql.org/message-id/877el38j56.fsf%40ars-thinkpad

--
With Regards,
Amit Kapila.