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

On Fri, Oct 9, 2020 at 5:45 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Thu, Oct 8, 2020 at 5:25 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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
>

I don't anticipate more combinations rather I am not yet sure whether
we want to distinguish these operations with flags or have separate
messages for each of these operations. I think for now we can go with
your proposal above.

--
With Regards,
Amit Kapila.