Re: [Patch] ALTER SYSTEM READ ONLY

On Tue, Sep 8, 2020 at 2:20 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> This pattern seems like it'll get unwieldy with more than one barrier
> type. And won't flag "unhandled" barrier types either (already the case,
> I know). We could go for something like:
>
> while (flags != 0)
> {
> barrier_bit = pg_rightmost_one_pos32(flags);
> barrier_type = 1 >> barrier_bit;
>
> switch (barrier_type)
> {
> case PROCSIGNAL_BARRIER_PLACEHOLDER:
> processed = ProcessBarrierPlaceholder();
> }
>
> if (processed)
> BARRIER_CLEAR_BIT(flags, barrier_type);
> }
>
> But perhaps that's too complicated?

I don't mind a loop, but that one looks broken. We have to clear the
bit before we call the function that processes that type of barrier.
Otherwise, if we succeed in absorbing the barrier but a new instance
of the same barrier arrives meanwhile, we'll fail to realize that we
need to absorb the new one.

> For this to be correct, wouldn't flags need to be volatile? Otherwise
> this might use a register value for flags, which might not contain the
> correct value at this point.

I think you're right.

> Perhaps a comment explaining why we have to clear bits first would be
> good?

Probably a good idea.

[ snipping assorted comments with which I agree ]

> It might be good to add a warning to WaitForProcSignalBarrier() or by
> pss_barrierCheckMask indicating that it's *not* OK to look at
> pss_barrierCheckMask when checking whether barriers have been processed.

Not sure I understand this one.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company