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 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
