On Fri, Oct 9, 2020 at 5:45 AM Peter Smith <smithpb2...@gmail.com> wrote: > > On Thu, Oct 8, 2020 at 5:25 PM Amit Kapila <amit.kapil...@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 >
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.
