On Thu, Oct 8, 2020 at 5:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > COMMENT
> > Line 177
> > +logicalrep_read_prepare(StringInfo in, LogicalRepPrepareData * prepare_data)
> >
> > prepare_data->prepare_type = flags;
> > This code may be OK but it does seem a bit of an abuse of the flags.
> >
> > e.g. Are they flags or are the really enum values?
> > e.g. And if they are effectively enums (it appears they are) then
> > seemed inconsistent that |= was used when they were previously
> > assigned.
> >
> > ;
>
> I don't understand this point. As far as I can see at the time of
> write (logicalrep_write_prepare()), the patch has used |=, and at the
> time of reading (logicalrep_read_prepare()) it has used assignment
> which seems correct from the code perspective. Do you have a better
> proposal?
OK. I will explain my thinking when I wrote that review comment.
I agree all is "correct" from a code perspective.
But IMO using bit arithmetic implies that different combinations are
also possible, whereas in current code they are not.
So code is kind of having a bet each-way - sometimes treating "flags"
as bit flags and sometimes as enums.
e.g. If these flags are not really bit flags at all then the
logicalrep_write_prepare() code might just as well be written as
below:
BEFORE
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;
/* Make sure exactly one of the expected flags is set. */
if (!PrepareFlagsAreValid(flags))
elog(ERROR, "unrecognized flags %u in prepare message", flags);
AFTER
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;
~
OTOH, if you really do want to anticipate having future flag bit
combinations then maybe the PrepareFlagsAreValid() macro ought to to
be tweaked accordingly, and the logicalrep_read_prepare() code maybe
should look more like below:
BEFORE
/* set the action (reuse the constants used for the flags) */
prepare_data->prepare_type = flags;
AFTER
/* set the action (reuse the constants used for the flags) */
prepare_data->prepare_type =
flags & LOGICALREP_IS_COMMIT_PREPARED ? LOGICALREP_IS_COMMIT_PREPARED :
flags & LOGICALREP_IS_ROLLBACK_PREPARED ? LOGICALREP_IS_ROLLBACK_PREPARED :
LOGICALREP_IS_PREPARE;
Kind Regards.
Peter Smith
Fujitsu Australia