Thread: Strange failure in LWLock on skink in REL9_5_STABLE
Hello, Andres pinged me off-list to point out this failure after my commit fb389498be: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2018-09-20%2005%3A24%3A34 Change Set for this build: fb389498be Tue Sep 18 11:19:22 2018 UTC Allow DSM allocation to be interrupted. The failure looks like this: ! FATAL: semop(id=332464133) failed: Invalid argument ! CONTEXT: SQL statement "CREATE TEMP TABLE brin_result (cid tid)" ! PL/pgSQL function inline_code_block line 22 at SQL statement ! PANIC: queueing for lock while waiting on another one ! server closed the connection unexpectedly ! This probably means the server terminated abnormally ! before or while processing the request. ! connection to server was lost I don't immediately see any connection between that particular commit, which relates to the treatment of signals while allocating a DSM segment, and the location of the first failure, which is in a statement that is creating a temporary table. On the other hand skink has been very stable lately. I'm also not sure how the FATAL error and the PANIC are related (LWLockQueueSelf() has discovered that MyProc->lwWaiting is already set). Though it's possible that the root problem was something happening in any of the other parallel tests running, I don't see how any of those (lock security_label tablesample object_address rowsecurity collate spgist privileges matview replica_identity brin gin gist groupingsets) would reach code touched by that commit in 9.5, but I don't currently have any other ideas about what happened here. -- Thomas Munro http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Andres pinged me off-list to point out this failure after my commit fb389498be:
> ! FATAL: semop(id=332464133) failed: Invalid argument
I was just looking at that, and my guess is that it was caused by
something doing an ipcrm or equivalent, and is unrelated to your patch.
Especially since skink has succeeded with that patch in several other
branches.
If it's repeatable, then it would be time to get excited.
regards, tom lane
On Fri, Sep 21, 2018 at 2:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.munro@enterprisedb.com> writes: > > Andres pinged me off-list to point out this failure after my commit fb389498be: > > > ! FATAL: semop(id=332464133) failed: Invalid argument > > I was just looking at that, and my guess is that it was caused by > something doing an ipcrm or equivalent, and is unrelated to your patch. > Especially since skink has succeeded with that patch in several other > branches. > > If it's repeatable, then it would be time to get excited. I found this case from January: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2018-01-05%2000%3A10%3A03 ! FATAL: semop(id=1313210374) failed: Invalid argument ! LINE 1: CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relk... EINVAL The semaphore set doesn't exist, or semid is less than zero, or nsops has a nonpositive value. -- Thomas Munro http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes: > On Fri, Sep 21, 2018 at 2:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I was just looking at that, and my guess is that it was caused by >> something doing an ipcrm or equivalent, and is unrelated to your patch. >> Especially since skink has succeeded with that patch in several other >> branches. > I found this case from January: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2018-01-05%2000%3A10%3A03 > ! FATAL: semop(id=1313210374) failed: Invalid argument Uh-huh. https://www.postgresql.org/docs/devel/static/kernel-resources.html#SYSTEMD-REMOVEIPC regards, tom lane
On 2018-09-20 22:59:29 -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
> > Andres pinged me off-list to point out this failure after my commit fb389498be:
>
> > ! FATAL: semop(id=332464133) failed: Invalid argument
>
> I was just looking at that, and my guess is that it was caused by
> something doing an ipcrm or equivalent, and is unrelated to your patch.
> Especially since skink has succeeded with that patch in several other
> branches.
I'm (hopefully) the only person with access to that machine, and I
certainly didn't do so. Nor are there script I know of that'd do
so. There's not been a lot of instability on skink, so it's certainly
quite weird.
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).
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.
It's not clear to me how that'd be problematic in 9.5 of all releases
however.
> If it's repeatable, then it would be time to get excited.
Yea, I guess we'll have to wait :/.
Greetings,
Andres Freund
On 2018-09-20 23:15:45 -0400, Tom Lane wrote: > Thomas Munro <thomas.munro@enterprisedb.com> writes: > > On Fri, Sep 21, 2018 at 2:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> I was just looking at that, and my guess is that it was caused by > >> something doing an ipcrm or equivalent, and is unrelated to your patch. > >> Especially since skink has succeeded with that patch in several other > >> branches. > > > I found this case from January: > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2018-01-05%2000%3A10%3A03 > > ! FATAL: semop(id=1313210374) failed: Invalid argument > Uh-huh. > > https://www.postgresql.org/docs/devel/static/kernel-resources.html#SYSTEMD-REMOVEIPC That shouldn't be relevant here - I'm not running the buildfarm from an interactive session and then logging out. So that code shouldn't trigger. I've made sure that the setting is off now however, I'm not trusting the related logic very much... Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2018-09-20 23:15:45 -0400, Tom Lane wrote: >> https://www.postgresql.org/docs/devel/static/kernel-resources.html#SYSTEMD-REMOVEIPC > That shouldn't be relevant here - I'm not running the buildfarm from an > interactive session and then logging out. So that code shouldn't > trigger. Well, the reason that systemd behavior is so brain-dead is exactly that it will trigger against processes that you didn't launch interactively. IIUC, you just have to log in and out of that account, and anything that was running in the background (eg cron jobs) under the same userID gets clobbered. > I've made sure that the setting is off now however, I'm not > trusting the related logic very much... Was it on before? regards, tom lane
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
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> ... 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.
Why would we fix it rather than just removing it?
regards, tom lane
On 2018-09-20 23:46:54 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > I've made sure that the setting is off now however, I'm not > > trusting the related logic very much... > > Was it on before? Yes.
On Fri, Sep 21, 2018 at 4:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.munro@enterprisedb.com> writes: > > ... 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. Erm, actually you probably only need to do ftruncate() *or* posix_fallocate(), depending on the direction of the resize. Doing both is redundant and introduces this theoretical hazard (in practice I'd be surprised if fallocate() really can fail after you shrank a file that was already fully allocated). > Why would we fix it rather than just removing it? I assumed we wouldn't remove an extern C function extension code somewhere might use. Though admittedly I'd be surprised if anyone used this one. -- Thomas Munro http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Fri, Sep 21, 2018 at 4:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Why would we fix it rather than just removing it?
> I assumed we wouldn't remove an extern C function extension code
> somewhere might use. Though admittedly I'd be surprised if anyone
> used this one.
Unless it looks practical to support this behavior in the Windows
and SysV cases, I think we should get rid of it rather than expend
effort on supporting it for just some platforms.
regards, tom lane
On Fri, Sep 21, 2018 at 4:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.munro@enterprisedb.com> writes: > > On Fri, Sep 21, 2018 at 4:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Why would we fix it rather than just removing it? > > > I assumed we wouldn't remove an extern C function extension code > > somewhere might use. Though admittedly I'd be surprised if anyone > > used this one. > > Unless it looks practical to support this behavior in the Windows > and SysV cases, I think we should get rid of it rather than expend > effort on supporting it for just some platforms. We can remove it in back-branches without breaking API compatibility: 1. Change dsm_impl_can_resize() to return false unconditionally (I suppose client code is supposed to check this before using dsm_resize(), though I'm not sure why it has an "impl" in its name if it's part of the public interface of this module). 2. Change dsm_resize() and dsm_remap() to raise an error conditionally. 3. Rip out the DSM_OP_RESIZE cases from various places. Then in master, remove all of those functions completely. However, I'd feel like a bit of a vandal. Robert and Amit probably had plans for that code...? -- Thomas Munro http://www.enterprisedb.com
On 2018-09-22 08:54:57 +1200, Thomas Munro wrote: > On Fri, Sep 21, 2018 at 4:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Thomas Munro <thomas.munro@enterprisedb.com> writes: > > > On Fri, Sep 21, 2018 at 4:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > >> Why would we fix it rather than just removing it? > > > > > I assumed we wouldn't remove an extern C function extension code > > > somewhere might use. Though admittedly I'd be surprised if anyone > > > used this one. > > > > Unless it looks practical to support this behavior in the Windows > > and SysV cases, I think we should get rid of it rather than expend > > effort on supporting it for just some platforms. > > We can remove it in back-branches without breaking API compatibility: > > 1. Change dsm_impl_can_resize() to return false unconditionally (I > suppose client code is supposed to check this before using > dsm_resize(), though I'm not sure why it has an "impl" in its name if > it's part of the public interface of this module). > 2. Change dsm_resize() and dsm_remap() to raise an error conditionally. > 3. Rip out the DSM_OP_RESIZE cases from various places. > > Then in master, remove all of those functions completely. However, > I'd feel like a bit of a vandal. Robert and Amit probably had plans > for that code...? Robert, Amit: ^ Greetings, Andres Freund
On Sat, Sep 22, 2018 at 2:28 AM Andres Freund <andres@anarazel.de> wrote: > > On 2018-09-22 08:54:57 +1200, Thomas Munro wrote: > > On Fri, Sep 21, 2018 at 4:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > Thomas Munro <thomas.munro@enterprisedb.com> writes: > > > > On Fri, Sep 21, 2018 at 4:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > >> Why would we fix it rather than just removing it? > > > > > > > I assumed we wouldn't remove an extern C function extension code > > > > somewhere might use. Though admittedly I'd be surprised if anyone > > > > used this one. > > > > > > Unless it looks practical to support this behavior in the Windows > > > and SysV cases, I think we should get rid of it rather than expend > > > effort on supporting it for just some platforms. > > > > We can remove it in back-branches without breaking API compatibility: > > > > 1. Change dsm_impl_can_resize() to return false unconditionally (I > > suppose client code is supposed to check this before using > > dsm_resize(), though I'm not sure why it has an "impl" in its name if > > it's part of the public interface of this module). > > 2. Change dsm_resize() and dsm_remap() to raise an error conditionally. > > 3. Rip out the DSM_OP_RESIZE cases from various places. > > > > Then in master, remove all of those functions completely. However, > > I'd feel like a bit of a vandal. Robert and Amit probably had plans > > for that code...? > > Robert, Amit: ^ > I went through and check the original proposal [1] to see if any use case is mentioned there, but nothing related has been discussed. I couldn't think of much use of this facility except maybe for something like parallelizing correalated sub-queries where the size of outer var can change across executions and we might need to resize the initially allocated memory. This is just a wild thought, I don't have any concrete idea about this. Having said that, I don't object to removing this especially because the implementation doesn't seem to be complete. In future, if someone needs such a facility, they can first develop a complete version of this API. [1] - https://www.postgresql.org/message-id/CA%2BTgmoaDqDUgt%3D4Zs_QPOnBt%3DEstEaVNP%2B5t%2Bm%3DFPNWshiPR3A%40mail.gmail.com -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Sat, Sep 22, 2018 at 4:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Sat, Sep 22, 2018 at 2:28 AM Andres Freund <andres@anarazel.de> wrote: > > On 2018-09-22 08:54:57 +1200, Thomas Munro wrote: > > > On Fri, Sep 21, 2018 at 4:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Unless it looks practical to support this behavior in the Windows > > > > and SysV cases, I think we should get rid of it rather than expend > > > > effort on supporting it for just some platforms. > > > > > > We can remove it in back-branches without breaking API compatibility: > > > > > > 1. Change dsm_impl_can_resize() to return false unconditionally (I > > > suppose client code is supposed to check this before using > > > dsm_resize(), though I'm not sure why it has an "impl" in its name if > > > it's part of the public interface of this module). > > > 2. Change dsm_resize() and dsm_remap() to raise an error conditionally. > > > 3. Rip out the DSM_OP_RESIZE cases from various places. > > > > > > Then in master, remove all of those functions completely. However, > > > I'd feel like a bit of a vandal. Robert and Amit probably had plans > > > for that code...? > > > > Robert, Amit: ^ > > I went through and check the original proposal [1] to see if any use > case is mentioned there, but nothing related has been discussed. I > couldn't think of much use of this facility except maybe for something > like parallelizing correalated sub-queries where the size of outer var > can change across executions and we might need to resize the initially > allocated memory. This is just a wild thought, I don't have any > concrete idea about this. Having said that, I don't object to > removing this especially because the implementation doesn't seem to be > complete. In future, if someone needs such a facility, they can first > develop a complete version of this API. Thanks for looking into that. Here's a pair of draft patches to disable and then remove dsm_resize() and dsm_map(). -- Thomas Munro http://www.enterprisedb.com
Attachment
On Tue, Sep 25, 2018 at 06:22:19PM +1200, Thomas Munro wrote: > On Sat, Sep 22, 2018 at 4:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote: >> I went through and check the original proposal [1] to see if any use >> case is mentioned there, but nothing related has been discussed. I >> couldn't think of much use of this facility except maybe for something >> like parallelizing correalated sub-queries where the size of outer var >> can change across executions and we might need to resize the initially >> allocated memory. This is just a wild thought, I don't have any >> concrete idea about this. Having said that, I don't object to >> removing this especially because the implementation doesn't seem to be >> complete. In future, if someone needs such a facility, they can first >> develop a complete version of this API. > > Thanks for looking into that. Here's a pair of draft patches to > disable and then remove dsm_resize() and dsm_map(). Hm. Don't we need to worry about anybody potentially using these APIs in a custom module on platforms where it was actually working? I imagine that their reaction is not going be nice if any code breaks suddenly after a minor release. No issues with removing the interface on HEAD of course. -- Michael
Attachment
On Mon, Nov 5, 2018 at 2:28 AM Michael Paquier <michael@paquier.xyz> wrote: > Hm. Don't we need to worry about anybody potentially using these APIs > in a custom module on platforms where it was actually working? I > imagine that their reaction is not going be nice if any code breaks > suddenly after a minor release. No issues with removing the interface > on HEAD of course. +1. The fact that the code exists there at all is my fault. I thought it might be useful someday, but now I don't think so any more. Thomas's solution -- in the DSA machinery -- of allocating entirely new segments seems like a better approach for now, and in the long run, I think we should convert the whole backend to use threads, nonwithstanding the TODO entry that says otherwise. Even if we never do that, extending a segment in place is pretty difficult to make practical, since it may involve remapping the segment, which invalidates cached pointers to anything in the segment. And then there are the portability problems on top of that. So I'm not very optimistic about this any more. But ripping it out in the back branches seems unnecessary. I'd just leave the bugs unfixed there. Most likely nobody is using that stuff, but if they are, let's not break it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Nov 6, 2018 at 10:18 AM Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Nov 5, 2018 at 2:28 AM Michael Paquier <michael@paquier.xyz> wrote: > > Hm. Don't we need to worry about anybody potentially using these APIs > > in a custom module on platforms where it was actually working? I > > imagine that their reaction is not going be nice if any code breaks > > suddenly after a minor release. No issues with removing the interface > > on HEAD of course. > > +1. > > The fact that the code exists there at all is my fault. I thought it > might be useful someday, but now I don't think so any more. Thomas's > solution -- in the DSA machinery -- of allocating entirely new > segments seems like a better approach for now, and in the long run, I > think we should convert the whole backend to use threads, > nonwithstanding the TODO entry that says otherwise. Even if we never > do that, extending a segment in place is pretty difficult to make > practical, since it may involve remapping the segment, which > invalidates cached pointers to anything in the segment. And then > there are the portability problems on top of that. So I'm not very > optimistic about this any more. > > But ripping it out in the back branches seems unnecessary. I'd just > leave the bugs unfixed there. Most likely nobody is using that stuff, > but if they are, let's not break it. Thanks. Pushed to master only. -- Thomas Munro http://www.enterprisedb.com
On Tue, Nov 06, 2018 at 05:29:36PM +1300, Thomas Munro wrote: > Thanks. Pushed to master only. Just a wild idea while this thread is hot: could we add in the description of the broken APIs or in their headers more information about how to not use them so as users are warned if trying to use them in certain circumstances? This idea is just for the stable branches. -- Michael
Attachment
On Tue, Nov 6, 2018 at 6:15 PM Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Nov 06, 2018 at 05:29:36PM +1300, Thomas Munro wrote: > > Thanks. Pushed to master only. > > Just a wild idea while this thread is hot: could we add in the > description of the broken APIs or in their headers more information > about how to not use them so as users are warned if trying to use them > in certain circumstances? This idea is just for the stable branches. Like this? -- Thomas Munro http://www.enterprisedb.com
Attachment
On Tue, Nov 06, 2018 at 06:45:02PM +1300, Thomas Munro wrote: > Like this? Ah, my mistake. I thought that the limitations with dsm_resize were not documented but those actually return an error if trying to use DSM_OP_RESIZE and I did not notice that, so we may be fine without a comment or such in back-branches. Sorry for the noise. -- Michael