Re: global / super barriers (for checksums)

On Mon, Dec 9, 2019 at 7:37 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> I've read through the patchset and played around with it to try and break it
> and understand it (not in that order).  Being a bit out of my comfort zone, I
> can't offer the deep insights that Andres has done; but in reading the code it
> all makes sense, and it works as I expect it to.

Stellar. If nobody objects in the meantime, I plan to commit 0001-0003
next week.

> The patch has a tiny typo though, s/procesess/processes/:

But I'll fix that first. Thanks for the review.

> Something which can be observed in order to hook up a test for it, but which
> has no sideeffects?  A NOOP barrier which only does a debug elog?

It would be a bit hard to hook up a test for that, because a normal
test doesn't see the logs, and a TAP test could, but in this case what
you want to check for is that there are no stragglers who should have
gotten the memo and didn't, which supposes you have a list of everyone
who ought to get the memo. You could manually list out all the
background processes but that seems like it would be at risk of
becoming obsolete. You could check that everything in pg_stat_activity
emitted a message except for a known list of exceptions, which would
be less likely to become obsolete, but would also be complex. Also, I
think it's a crock to have something like that in there long term just
for testing purposes.

> I sort of like the callback idea conceptually, but Andres is making a good
> point about the extensibility actually making it harder to reason about.

That objection doesn't hold any water for me, because this is open
source. People can always patch the core. If we don't add hooks, then
we make life easier for fork maintainers (including my employer) and
harder for extension authors. I think that's *definitely* the wrong
bet for the community to be making; we should be trying very hard to
help extension authors and minimize the need for forks. If you install
an extension that uses a hook, any hook, and it breaks things, then
you get to keep both pieces.

And that approach would let us do spiffy testing without having to
leave cruft in core.

> FWIW I've rebased the online checksums patch on top of this patchset instead of
> Andres' previous patch and everything seems to work fine.  All tests pass, and
> I've been unable to trigger anything unexpected.  I'll post the new version of
> said patch shortly.

Cool.

> > and for the thing that got me
> > motivated to work on this, which is ALTER SYSTEM READ ONLY.
>
> Aha, thats a pretty neat usecase.

Thanks.

> In order to play around with and try to understand the patchset I wrote a PoC
> PGC_PROCSIGNALBARRIER class for GUCs and converted a few PGC_SIGHUP GUCs to use
> a barrier rather than a HUP.  The idea was to test interacting with the API a
> bit more than the online checksum patch does, as its needs are pretty basic.
> In hacking this up I didn't really come across anything in particular that I
> lacked, and the result worked fine (insofar as a PoC can be considered
> working).

Cool.

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