On Fri, Sep 21, 2018 at 3:21 PM Andres Freund <andres@anarazel.de> wrote:
> I'm quite suspicious of the logic around:
>
> /*
> * If we received a query cancel or termination signal, we will have
> * EINTR set here. If the caller said that errors are OK here, check
> * for interrupts immediately.
> */
> if (errno == EINTR && elevel >= ERROR)
> CHECK_FOR_INTERRUPTS();
>
> because it seems far from guaranteed to do anything meaningful as I
> don't see a guarantee that interrupts are active at that point (e.g. it
> seems quite reasonable to hold an lwlock while resizing).
Right, in that case CFI does nothing and you get the following
ereport() instead, so the user sees an unsightly "Interrupt system
call" message (or however your strerror() spells it). That would
actually happen in the dsa.c case for example (something that should
be improved especially now that dsm_create() is so slow on Linux,
independently of all this). That's probably not a great user
experience, but I'm not sure if the fact that it "works around" the
suppression of interrupts while LWLocks are held by converting them
into errors is a more serious problem or not. The caller has to be
ready for errors to be raised here in any case.
> Afaict that might cause problems at a later stage, because at that point
> we've not adjusted the actual mapping, but *have* ftruncate()ed it. If
> there's actual data in the mapping, that certainly could cause trouble.
>
> In fact, while this commit has expanded the size of the problem, I fail
> to see how the error handling for resizing is correct. It's fine to fail
> in the ftruncate() itself - at that point no changes have been made -,
> but I don't think it's currently ok for posix_fallocate() to ever error
> out.
Right, independently of this commit, dsm_resize() might have done the
actual truncation when the error is reported. That's not good if the
caller plans to do anything other than abandon ship. None of this
applies to dsm_create() though, which uses the same code path but
knows how to clean up. There may be ways to fix the dsm_resize() path
based on the observation that you don't need to fallocate() if you
made the mapping smaller, and if you made it bigger then you could
always undo that on error (or not) and you haven't thrown away any
data. Hmm... I note that there are actually no callers of
dsm_resize(), and it's not implemented on Windows or SystemV.
--
Thomas Munro
http://www.enterprisedb.com