On 20/04/16 23:44, Tom Lane wrote:
> Over in <17456.1460832307@sss.pgh.pa.us> I speculated about whether
> we should be enforcing that WAL insertion happen only inside critical
> sections. We don't currently, and a survey of the backend says that
> there are quite a few calls that aren't inside critical sections.
> But there are at least two good reasons why we should, IMO:
>
> 1. It's not very clear that XLogInsert will recover cleanly if it's
> invoked outside a critical section and hits a failure. Certainly,
> if we allow such usage, then every potential error inside that code
> has to be analyzed under both critical-section and normal rules.
It was certainly designed to recover from errors gracefully.
XLogInsertRecord(), which does the low-level work of inserting the
record to the WAL buffer, has a critical section of its own inside it.
The code in xloginsert.c, for constructing the record to insert,
operates on backend-private buffers only.
> 2. With no such check, it's quite easy for calling code to forget to
> create a critical section around code stanzas where one is *necessary*
> (because you're changing shared-buffer contents).
Yeah, that is very true.
> Both of these points represent pretty clear hazards for introduction
> of future bugs, whether or not there are any such bugs today.
>
> As against this, it could be argued that adding critical sections where
> they're not absolutely necessary must make crashing failures more probable
> than they need to be. But first you'd have to prove that they're not
> absolutely necessary, which I'm unsure about because of point #1.
One option would be to put the must-be-in-critical-section check in
XLogRegisterBlock(). A WAL record that is associated with a shared
buffer almost certainly needs a critical section, but many of the others
are safe without it.
- Heikki
