Spinlocks and semaphores in 9.2 and 9.3
This rabbit hole keeps getting deeper and deeper :-(
I realized a couple days ago that it had been close to three years since
I last tried building the further-back branches on my ancient HPPA box.
Not so surprisingly, things were broken: commits 37de8de9e33606a0 et al
had introduced use of memory barriers into 9.2 and 9.3, which up to that
moment had had none, so we'd never bothered to make barrier.h actually
work in those branches. I back-patched some fixes in barrier.h, which
got things to compile, and then commit 44cd47c1d49655c5, which got things
to actually work ... on that box anyway. I now see from the buildfarm
(and can confirm locally) that 44cd47c1d49655c5 breaks builds with
--disable-spinlock, because it introduces an initialization of a spinlock
before the underlying PGSemaphore infrastructure can possibly get
initialized.
In 9.4 and up, this works because of daa7527afc227443, which decoupled
spinlocks from semaphores enough that you can do s_init_lock_sema()
long before you can actually use the spinlock. Come to think of it,
that commit also means that you can get away with using a spinlock
you've never done initialization on at all, which is not so good.
So at this point I'm not sure what to do. I could back out the back-patch
of 44cd47c1d49655c5, which would mean accepting that 9.2/9.3 are broken
and will never be fixed for HPPA, as well as any other architectures that
use the same fallback memory barrier implementation. The lack of
complaints from the field suggests that nobody would care. Or I could
push forward by back-patching daa7527afc227443 (and a couple of minor
follow-on cleanups). That doesn't seem particularly risky, now that
9.4's been out for awhile, but it's kind of a large back-patch to benefit
architectures that apparently no actual users care about.
Thoughts?
Independently of that, I think we should fix the --disable-spinlock
support so that a spinlock containing zero is rejected as not properly
initialized. A large part of the mess here comes from the fact that HPPA
is our only architecture in which zero is not the correct initialization
state for a spinlock. I'd like to have some more portable method of
catching failure to initialize a spinlock. I only propose doing this
part in HEAD, though.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
So at this point I'm not sure what to do. I could back out the back-patch
of 44cd47c1d49655c5, which would mean accepting that 9.2/9.3 are broken
and will never be fixed for HPPA, as well as any other architectures that
use the same fallback memory barrier implementation. The lack of
complaints from the field suggests that nobody would care. Or I could
push forward by back-patching daa7527afc227443 (and a couple of minor
follow-on cleanups). That doesn't seem particularly risky, now that
9.4's been out for awhile, but it's kind of a large back-patch to benefit
architectures that apparently no actual users care about.
I went ahead and prepared and tested such a patch; the version for 9.3
is attached. (9.2 is identical modulo some pgindent-induced whitespace
difference.) This doesn't look too hazardous to me, so I'm thinking
we should apply it.
regards, tom lane
Attachments:
backport-spinlock-sema-changes.patchtext/x-diff; charset=us-ascii; name=backport-spinlock-sema-changes.patchDownload+98-59
On April 16, 2016 6:02:39 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
So at this point I'm not sure what to do. I could back out the
back-patch
of 44cd47c1d49655c5, which would mean accepting that 9.2/9.3 are
broken
and will never be fixed for HPPA, as well as any other architectures
that
use the same fallback memory barrier implementation. The lack of
complaints from the field suggests that nobody would care. Or Icould
push forward by back-patching daa7527afc227443 (and a couple of minor
follow-on cleanups). That doesn't seem particularly risky, now that
9.4's been out for awhile, but it's kind of a large back-patch tobenefit
architectures that apparently no actual users care about.
I went ahead and prepared and tested such a patch; the version for 9.3
is attached. (9.2 is identical modulo some pgindent-induced whitespace
difference.) This doesn't look too hazardous to me, so I'm thinking
we should apply it.
I can't look at the patch just now, but the plan sounds good. Of you rather have somebody look art the patch before, I can do tomorrow morning.
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
On April 16, 2016 6:02:39 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I went ahead and prepared and tested such a patch; the version for 9.3
is attached. (9.2 is identical modulo some pgindent-induced whitespace
difference.) This doesn't look too hazardous to me, so I'm thinking
we should apply it.
I can't look at the patch just now, but the plan sounds good. Of you rather have somebody look art the patch before, I can do tomorrow morning.
Did you want to actually review this patch, or should I just push it?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-04-18 11:07:08 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On April 16, 2016 6:02:39 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I went ahead and prepared and tested such a patch; the version for 9.3
is attached. (9.2 is identical modulo some pgindent-induced whitespace
difference.) This doesn't look too hazardous to me, so I'm thinking
we should apply it.I can't look at the patch just now, but the plan sounds good. Of you rather have somebody look art the patch before, I can do tomorrow morning.
Did you want to actually review this patch, or should I just push it?
No, I'm good, you should push it. I did a quick scan of the patch, and
it looks sane. For a second I was concerned that there might be a
situation in which this patch increases the total number of semaphore
needed, which might make backpatching a bit problematic - but it appears
that that'd be a very absurd configuration.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
On 2016-04-18 11:07:08 -0400, Tom Lane wrote:
Did you want to actually review this patch, or should I just push it?
No, I'm good, you should push it. I did a quick scan of the patch, and
it looks sane. For a second I was concerned that there might be a
situation in which this patch increases the total number of semaphore
needed, which might make backpatching a bit problematic - but it appears
that that'd be a very absurd configuration.
I was actually wondering whether it'd make sense to cut the number of
underlying semaphores to 128 or 512 or thereabouts. But I think we had
that discussion when the daa7527afc227443 patch went in to begin with,
and convinced ourselves that 1024 was okay. Robert, do you recall the
reasoning?
(The thing that gave me pause about this was noticing that I could not
start two such postmasters concurrently on my RHEL6 box, without changing
the default system limits on number of SysV semaphores.)
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-04-18 11:24:07 -0400, Tom Lane wrote:
(The thing that gave me pause about this was noticing that I could not
start two such postmasters concurrently on my RHEL6 box, without changing
the default system limits on number of SysV semaphores.)
Hm, is that different before/after the patch? Because afaics at around
NBuffers = 1000, the number of semaphores should be lower after the
patch?
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
On 2016-04-18 11:24:07 -0400, Tom Lane wrote:
(The thing that gave me pause about this was noticing that I could not
start two such postmasters concurrently on my RHEL6 box, without changing
the default system limits on number of SysV semaphores.)
Hm, is that different before/after the patch? Because afaics at around
NBuffers = 1000, the number of semaphores should be lower after the
patch?
Yeah, that was probably the argument we used before. But it's true that a
postmaster built with --disable-spinlocks is harder to test than one built
without (because you're going from ~100 semas to ~1100 semas, at default
configuration). If we believe that the main real use-case for this switch
is testing, that's not a very nice property.
The bottom line IMO is that --disable-spinlocks is actually not that awful
for low-stress, low-concurrency scenarios; for example, it doesn't change
the runtime of make installcheck-parallel very much for me. On the other
hand, for high-concurrency scenarios you'd darn well better get a real
spinlock implementation. Given that sort of scope of use, it seems like
a hundred or so underlying semas should be plenty, and it'd be less likely
to cause operational problems than 1024.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Apr 18, 2016 at 11:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
On 2016-04-18 11:07:08 -0400, Tom Lane wrote:
Did you want to actually review this patch, or should I just push it?
No, I'm good, you should push it. I did a quick scan of the patch, and
it looks sane. For a second I was concerned that there might be a
situation in which this patch increases the total number of semaphore
needed, which might make backpatching a bit problematic - but it appears
that that'd be a very absurd configuration.I was actually wondering whether it'd make sense to cut the number of
underlying semaphores to 128 or 512 or thereabouts. But I think we had
that discussion when the daa7527afc227443 patch went in to begin with,
and convinced ourselves that 1024 was okay. Robert, do you recall the
reasoning?
I don't recall a specific discussion about it, but the number we would
have needed before was gigantic - one per lwlock, which is typically
going to be far more than 1024. I mean, at shared_buffers=32MB, there
are 4096 buffers using two lwlocks each... never mind everything else
that uses lwlocks.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Apr 18, 2016 at 11:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
On 2016-04-18 11:24:07 -0400, Tom Lane wrote:
(The thing that gave me pause about this was noticing that I could not
start two such postmasters concurrently on my RHEL6 box, without changing
the default system limits on number of SysV semaphores.)Hm, is that different before/after the patch? Because afaics at around
NBuffers = 1000, the number of semaphores should be lower after the
patch?Yeah, that was probably the argument we used before. But it's true that a
postmaster built with --disable-spinlocks is harder to test than one built
without (because you're going from ~100 semas to ~1100 semas, at default
configuration). If we believe that the main real use-case for this switch
is testing, that's not a very nice property.The bottom line IMO is that --disable-spinlocks is actually not that awful
for low-stress, low-concurrency scenarios; for example, it doesn't change
the runtime of make installcheck-parallel very much for me. On the other
hand, for high-concurrency scenarios you'd darn well better get a real
spinlock implementation. Given that sort of scope of use, it seems like
a hundred or so underlying semas should be plenty, and it'd be less likely
to cause operational problems than 1024.
I suppose that's probably true. I thought surely any system worth its
salt wouldn't have a problem with 1024 semaphores, but a quick Google
search for "hp-ux semmns" turned up this page:
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Apr 18, 2016 at 11:53 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Apr 18, 2016 at 11:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
On 2016-04-18 11:24:07 -0400, Tom Lane wrote:
(The thing that gave me pause about this was noticing that I could not
start two such postmasters concurrently on my RHEL6 box, without changing
the default system limits on number of SysV semaphores.)Hm, is that different before/after the patch? Because afaics at around
NBuffers = 1000, the number of semaphores should be lower after the
patch?Yeah, that was probably the argument we used before. But it's true that a
postmaster built with --disable-spinlocks is harder to test than one built
without (because you're going from ~100 semas to ~1100 semas, at default
configuration). If we believe that the main real use-case for this switch
is testing, that's not a very nice property.The bottom line IMO is that --disable-spinlocks is actually not that awful
for low-stress, low-concurrency scenarios; for example, it doesn't change
the runtime of make installcheck-parallel very much for me. On the other
hand, for high-concurrency scenarios you'd darn well better get a real
spinlock implementation. Given that sort of scope of use, it seems like
a hundred or so underlying semas should be plenty, and it'd be less likely
to cause operational problems than 1024.I suppose that's probably true. I thought surely any system worth its
salt wouldn't have a problem with 1024 semaphores, but a quick Google
search for "hp-ux semmns" turned up this page:
Oops, hit send too soon.
http://docstore.mik.ua/manuals/hp-ux/en/B2355-60130/semmni.5.html
That's from 2007 and indicates a default maximum of 2048. So it would
be fairly easy to hit the limit.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers