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

On Mon, Mar 15, 2021 at 6:14 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Mon, Mar 15, 2021 at 2:04 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> I think something on these lines should be much
>> easier than the spool-file implementation unless we see any problem
>> with this idea.
>>
>
> Here's a new patch-set that implements this new solution proposed by Amit.

Thanks for the updated patch.
Few comments:
1) These are no longer needed as it has been removed with the new changes.
@@ -1959,6 +1962,8 @@ ProtocolVersion
PrsStorage
PruneState
PruneStepResult
+PsfFile
+PsfHashEntry

2) "Binary mode and streaming and two_phase" should be "Binary mode,
streaming and two_phase" in the below code:
@@ -6097,13 +6097,15 @@ describeSubscriptions(const char *pattern, bool verbose)

if (verbose)
{
- /* Binary mode and streaming are only supported in v14
and higher */
+ /* Binary mode and streaming and two_phase are only
supported in v14 and higher */
if (pset.sversion >= 140000)
appendPQExpBuffer(&buf,

3) We have some reference to psf spoolfile, this should be removed.
Also check if the assert should be <= or ==.
+ /*
+ * Normally, prepare_lsn == remote_final_lsn, but if this
prepare message
+ * was dispatched via the psf spoolfile replay then the remote_final_lsn
+ * is set to commit lsn instead. Hence the <= instead of == check below.
+ */
+ Assert(prepare_data.prepare_lsn <= remote_final_lsn);

4) Similarly in below code:
+ /*
+ * It is possible that we haven't received prepare because it occurred
+ * before walsender reached a consistent point in which case we need to
+ * skip rollback prepared.
+ *
+ * And we also skip the FinishPreparedTransaction if we're using the
+ * Prepare Spoolfile (using_psf) because in that case there is
no matching
+ * PrepareTransactionBlock done yet.
+ */
+ if (LookupGXact(rollback_data.gid, rollback_data.prepare_end_lsn,
+ rollback_data.preparetime))
+ {

5) Should this be present:
+#if 1
+ /* This is just debugging, for confirmation the update worked. */
+ {
+ Subscription *new_s;
+
+ StartTransactionCommand();
+ new_s = GetSubscription(MySubscription->oid, false);
+ CommitTransactionCommand();
+ }
+#endif

Regards,
Vignesh