On 05/10/2020 17:25, Álvaro Herrera wrote:
On 2020-Oct-05, Heikki Linnakangas wrote:
The code in sendFile() in basebackup.c seems suspicious in that regard. It
calls DataChecksumsNeedVerify() once before starting to read the file. Isn't
it possible for the checksums flag to change while it's reading the file and
sending it to the client? I hope there are CHECK_FOR_INTERRUPTS() calls
buried somewhere in the loop, because it could take minutes to send the
whole file.
I would feel better if the state transition of the "checksums" flag could
only happen in a few safe places, or there were some other safeguards for
this. I think that's what Andres was trying to say earlier in the thread on
ProcSignalBarriers. I'm not sure what the interface to that should be. It
could be something like HOLD/RESUME_INTERRUPTS(), where normally all
procsignals are handled on CHECK_FOR_INTERRUPTS(), but you could "hold off"
some if needed. Or something else. Or maybe we can just use
HOLD/RESUME_INTERRUPTS() for this. It's more coarse-grained than necessary,
but probably doesn't matter in practice.
I hope you're not suggesting that interrupts would be held for the whole
transmission of a file, which you say could take minutes. If we do have
an interrupt holdoff, then it has to be pretty short; users (and
systemd) despair if service shutdown is delayed more than a few seconds.
I'm not suggesting that, sorry I wasn't clear. That would indeed be
horrible.
sendFile() needs a different solution, but all the other places where
DataChecksumsNeedWrite/Verify() is called need to be inspected to make
sure that they hold interrupts, or ensure some other way that an
interrupt doesn't change the local checksums flag between the
DataChecksumsNeedWrite/Verify() call and the read/write.
I think sendFile() needs to re-check the local checksums state before
each read. It also needs to ensure that an interrupt doesn't occur and
change the local checksums state between read and the
DataChecksumsNeedVerify() calls, but that's a very short period if you
call DataChecksumsNeedVerify() again for each block.
- Heikki
