From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | "Peter(dot)B(dot)Smith(at)fujitsu(dot)com" <Peter(dot)B(dot)Smith(at)fujitsu(dot)com>, Ajin Cherian <itsajin(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [HACKERS] logical decoding of two-phase transactions |
Date: | 2020-10-09 00:14:24 |
Message-ID: | CAHut+Pub4K=Wca5ppVz5QSLWuM0RbpiXKRiaoZz-PdHnS01RFg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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 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
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2020-10-09 00:19:50 | Re: [PATCH] ecpg: fix progname memory leak |
Previous Message | Tom Lane | 2020-10-08 23:59:15 | Re: Assertion failure with LEFT JOINs among >500 relations |