Re: [Patch] ALTER SYSTEM READ ONLY

On Fri, Sep 10, 2021 at 12:20 PM Mark Dilger
 wrote:
> A better example may be found in ginmetapage.c:
>
>         needwal = RelationNeedsWAL(indexrel);
>         if (needwal)
>         {
>             CheckWALPermitted();
>             computeLeafRecompressWALData(leaf);
>         }
>
>         /* Apply changes to page */
>         START_CRIT_SECTION();

Yeah, that looks sketchy. Why not move CheckWALPermitted() down a line?

> Even if CheckWALPermitted is assumed to be close enough to atomic to not be a problem (I don't agree), that argument can't be made here, as computeLeafRecompressWALData is not trivial and signals could easily be processed while it is running.

I think the relevant question here is not "could a signal handler
fire?" but "can we hit a CHECK_FOR_INTERRUPTS()?". If the relevant
question is the former, then there's no hope of ever making it work
because there's always a race condition. But the signal handler is
only setting flags whose only effect is to make a subsequent
CHECK_FOR_INTERRUPTS() do something, so it doesn't really matter when
the signal handler can run, but when CHECK_FOR_INTERRUPTS() can call
ProcessInterrupts().

-- 
Robert Haas
EDB: http://www.enterprisedb.com