Re: [Patch] ALTER SYSTEM READ ONLY - Mailing list pgsql-hackers

Hi,

On 2020-08-28 15:53:29 -0400, Robert Haas wrote:
> On Wed, Aug 19, 2020 at 6:28 AM Amul Sul <sulamul@gmail.com> wrote:
> > Attached is a rebased on top of the latest master head (# 3e98c0bafb2).
>
> Does anyone, especially anyone named Andres Freund, have comments on
> 0001? That work is somewhat independent of the rest of this patch set
> from a theoretical point of view, and it seems like if nobody sees a
> problem with the line of attack there, it would make sense to go ahead
> and commit that part.

It'd be easier to review the proposed commit if it included reasoning
about the change...

In particular, it looks to me like the commit actually implements two
different changes:
1) Allow a barrier function to "reject" a set barrier, because it can't
   be set in that moment
2) Allow barrier functions to raise errors

and there's not much of an explanation as to why (probably somewhere
upthread, but ...)



 /*
  * ProcSignalShmemSize
@@ -486,17 +490,59 @@ ProcessProcSignalBarrier(void)
     flags = pg_atomic_exchange_u32(&MyProcSignalSlot->pss_barrierCheckMask, 0);

     /*
-     * Process each type of barrier. It's important that nothing we call from
-     * here throws an error, because pss_barrierCheckMask has already been
-     * cleared. If we jumped out of here before processing all barrier types,
-     * then we'd forget about the need to do so later.
-     *
-     * NB: It ought to be OK to call the barrier-processing functions
-     * unconditionally, but it's more efficient to call only the ones that
-     * might need us to do something based on the flags.
+     * If there are no flags set, then we can skip doing any real work.
+     * Otherwise, establish a PG_TRY block, so that we don't lose track of
+     * which types of barrier processing are needed if an ERROR occurs.
      */
-    if (BARRIER_SHOULD_CHECK(flags, PROCSIGNAL_BARRIER_PLACEHOLDER))
-        ProcessBarrierPlaceholder();
+    if (flags != 0)
+    {
+        PG_TRY();
+        {
+            /*
+             * Process each type of barrier. The barrier-processing functions
+             * should normally return true, but may return false if the barrier
+             * can't be absorbed at the current time. This should be rare,
+             * because it's pretty expensive.  Every single
+             * CHECK_FOR_INTERRUPTS() will return here until we manage to
+             * absorb the barrier, and that cost will add up in a hurry.
+             *
+             * NB: It ought to be OK to call the barrier-processing functions
+             * unconditionally, but it's more efficient to call only the ones
+             * that might need us to do something based on the flags.
+             */
+            if (BARRIER_SHOULD_CHECK(flags, PROCSIGNAL_BARRIER_PLACEHOLDER)
+                && ProcessBarrierPlaceholder())
+                BARRIER_CLEAR_BIT(flags, PROCSIGNAL_BARRIER_PLACEHOLDER);

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?

+        }
+        PG_CATCH();
+        {
+            /*
+             * If an ERROR occurred, add any flags that weren't yet handled
+             * back into pss_barrierCheckMask, and reset the global variables
+             * so that we try again the next time we check for interrupts.
+             */
+            pg_atomic_fetch_or_u32(&MyProcSignalSlot->pss_barrierCheckMask,
+                                   flags);

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.

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

+            ProcSignalBarrierPending = true;
+            InterruptPending = true;
+
+            PG_RE_THROW();
+        }
+        PG_END_TRY();


+        /*
+         * If some barrier was not successfully absorbed, we will have to try
+         * again later.
+         */
+        if (flags != 0)
+        {
+            pg_atomic_fetch_or_u32(&MyProcSignalSlot->pss_barrierCheckMask,
+                                   flags);
+            ProcSignalBarrierPending = true;
+            InterruptPending = true;
+            return;
+        }
+    }

I wish there were a way we could combine the PG_CATCH and this instance
of the same code. I'd probably just move into a helper.


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.


> Considering that this global barrier stuff is
> new and that I'm not sure how well we really understand the problems
> yet, there's a possibility that we might end up revising these details
> again. I understand that most people, including me, are somewhat
> reluctant to see experimental code get committed, in this case that
> ship has basically sailed already, since neither of the patches that
> we thought would use the barrier mechanism end up making it into v13.
> I don't think it's really making things any worse to try to improve
> the mechanism.

Yea, I have no problem with this.


Greetings,

Andres Freund