On Fri, Apr 9, 2021 at 12:33 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, Dec 14, 2020 at 8:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > 2.
> > + /*
> > + * Flags are determined from the state of the transaction. We know we
> > + * always get PREPARE first and then [COMMIT|ROLLBACK] PREPARED, so if
> > + * it's already marked as committed then it has to be COMMIT PREPARED (and
> > + * likewise for abort / ROLLBACK PREPARED).
> > + */
> > + if (rbtxn_commit_prepared(txn))
> > + flags = LOGICALREP_IS_COMMIT_PREPARED;
> > + else if (rbtxn_rollback_prepared(txn))
> > + flags = LOGICALREP_IS_ROLLBACK_PREPARED;
> > + else
> > + flags = LOGICALREP_IS_PREPARE;
> >
> > I don't like clubbing three different operations under one message
> > LOGICAL_REP_MSG_PREPARE. It looks awkward to use new flags
> > RBTXN_COMMIT_PREPARED and RBTXN_ROLLBACK_PREPARED in ReordeBuffer so
> > that we can recognize these operations in corresponding callbacks. I
> > think setting any flag in ReorderBuffer should not dictate the
> > behavior in callbacks. Then also there are few things that are not
> > common to those APIs like the patch has an Assert to say that the txn
> > is marked with prepare flag for all three operations which I think is
> > not true for Rollback Prepared after the restart. We don't ensure to
> > set the Prepare flag if the Rollback Prepare happens after the
> > restart. Then, we have to introduce separate flags to distinguish
> > prepare/commit prepared/rollback prepared to distinguish multiple
> > operations sent as protocol messages. Also, all these operations are
> > mutually exclusive so it will be better to send separate messages for
> > each of these and I have changed it accordingly in the attached patch.
> >
>
> While looking at the two-phase protocol messages (with a view to
> documenting them) I noticed that the messages for
> LOGICAL_REP_MSG_PREPARE, LOGICAL_REP_MSG_COMMIT_PREPARED,
> LOGICAL_REP_MSG_ROLLBACK_PREPARED are all sending and receiving flag
> bytes which *always* has a value 0.
>
> ----------
> e.g.
> uint8 flags = 0;
> pq_sendbyte(out, flags);
>
> and
> /* read flags */
> uint8 flags = pq_getmsgbyte(in);
> if (flags != 0)
> elog(ERROR, "unrecognized flags %u in commit prepare message", flags);
> ----------
>
> I think this patch version v31 is where the flags became redundant.
>
I think this has been kept for future use similar to how we have in
logicalrep_write_commit. So, I think we can keep them unused for now.
We can document it similar commit message ('C') [1].
[1] - https://www.postgresql.org/docs/devel/protocol-logicalrep-message-formats.html
--
With Regards,
Amit Kapila.
