Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread David Rowley
On 17 August 2017 at 01:20, Heikki Linnakangas wrote: > Looks reasonable. I edited the comments and the variable names a bit, to my > liking, and committed. Thanks! Thanks for committing. I've just been catching up on all that went on while I was sleeping. Thanks for handling the cleanup too. I'

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Tom Lane
Andres Freund writes: > On August 16, 2017 3:09:27 PM PDT, Tom Lane wrote: >> I wonder whether it's sensible to have --enable-cassert have the effect >> of filling memory allocated by ShmemAlloc or the DSA code with junk (as >> palloc does) instead of leaving it at zeroes. It's not modeling the

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Andres Freund
On August 16, 2017 3:09:27 PM PDT, Tom Lane wrote: >Heikki Linnakangas writes: >> On 08/17/2017 12:20 AM, Tom Lane wrote: >>> Indeed, gaur fails with >>> 2017-08-16 17:09:38.315 EDT [13043:11] PANIC: stuck spinlock >detected at pg_atomic_compare_exchange_u64_impl, atomics.c:196 > >> I was able

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Tom Lane
Heikki Linnakangas writes: > On 08/17/2017 12:20 AM, Tom Lane wrote: >> Indeed, gaur fails with >> 2017-08-16 17:09:38.315 EDT [13043:11] PANIC: stuck spinlock detected at >> pg_atomic_compare_exchange_u64_impl, atomics.c:196 > I was able to reproduce this locally, with --disable-atomics, but o

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Heikki Linnakangas
On 08/17/2017 12:20 AM, Tom Lane wrote: Andres Freund writes: On 2017-08-16 16:20:28 +0300, Heikki Linnakangas wrote: + pg_atomic_write_u64(&target->phs_nallocated, 0); It's not ok to initialize an atomic with pg_atomic_write_u64 - works well enough for "plain" atomics, but the fallbac

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Tom Lane
Andres Freund writes: > On 2017-08-16 16:20:28 +0300, Heikki Linnakangas wrote: > + pg_atomic_write_u64(&target->phs_nallocated, 0); > It's not ok to initialize an atomic with pg_atomic_write_u64 - works > well enough for "plain" atomics, but the fallback implementation isn't > ok with it. Yo

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Tom Lane
Heikki Linnakangas writes: > On 08/16/2017 09:00 PM, Tom Lane wrote: >> I don't buy that argument. A caller might think "Why do I need >> shm_toc_estimate, when I can compute the *exact* size I need?". >> And it would have worked, up till this proposed patch. > Well, no. The size of the shm_toc

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Andres Freund
On 2017-08-16 16:20:28 +0300, Heikki Linnakangas wrote: > On 05/06/2017 04:57 PM, David Rowley wrote: > > Andres mentioned in [2] that it might be worth exploring using atomics > > to do the same job. So I went ahead and did that, and came up with the > > attached, which is a slight variation on wh

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Andres Freund
On 2017-08-16 14:09:08 -0400, Tom Lane wrote: > >> I'm not sure that that's good enough, and I'm damn sure that it > >> shouldn't be undocumented. > > > 8 byte alignment would be good enough, so BUFFERALIGN ought to be > > sufficient. But it'd be nicer to have a separate more descriptive knob. >

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Heikki Linnakangas
On 08/16/2017 09:00 PM, Tom Lane wrote: Robert Haas writes: On Wed, Aug 16, 2017 at 1:44 PM, Tom Lane wrote: I was feeling a bit uncomfortable with the BUFFERALIGN_DOWN() for a different reason: if the caller has specified the exact amount of space it needs, having shm_toc_create discard some

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Tom Lane
Andres Freund writes: > On 2017-08-16 13:40:09 -0400, Tom Lane wrote: >> I was wondering why the shm_toc code was using BUFFERALIGN and not >> MAXALIGN, and I now suspect that the answer is "it's an entirely >> undocumented kluge to make the atomics code not crash on 32-bit >> machines, so long as

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Tom Lane
Robert Haas writes: > On Wed, Aug 16, 2017 at 1:44 PM, Tom Lane wrote: >> I was feeling a bit uncomfortable with the BUFFERALIGN_DOWN() for a >> different reason: if the caller has specified the exact amount of space it >> needs, having shm_toc_create discard some could lead to an unexpected >> f

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Andres Freund
On August 16, 2017 10:47:23 AM PDT, Robert Haas wrote: >On Wed, Aug 16, 2017 at 1:40 PM, Tom Lane wrote: >> I was wondering why the shm_toc code was using BUFFERALIGN and not >> MAXALIGN, and I now suspect that the answer is "it's an entirely >> undocumented kluge to make the atomics code not c

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Robert Haas
On Wed, Aug 16, 2017 at 1:44 PM, Tom Lane wrote: > I was feeling a bit uncomfortable with the BUFFERALIGN_DOWN() for a > different reason: if the caller has specified the exact amount of space it > needs, having shm_toc_create discard some could lead to an unexpected > failure. Well, that's why H

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Andres Freund
On 2017-08-16 13:44:28 -0400, Tom Lane wrote: > Andres Freund writes: > > Don't think we require BUFFERALIGN - MAXALIGN ought to be > > sufficient. > > Uh, see my other message just now. Yup, you're right. > > The use of BUFFERALIGN presumably is to space out things > > into different cachelin

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Robert Haas
On Wed, Aug 16, 2017 at 1:40 PM, Tom Lane wrote: > I was wondering why the shm_toc code was using BUFFERALIGN and not > MAXALIGN, and I now suspect that the answer is "it's an entirely > undocumented kluge to make the atomics code not crash on 32-bit > machines, so long as nobody puts a pg_atomic_

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Andres Freund
On 2017-08-16 13:40:09 -0400, Tom Lane wrote: > I wrote: > > I can confirm that on dromedary, that regression test case is attempting > > to create a TOC with a not-well-aligned size: 93268 = 0x16c54 bytes. > > ... although, on closer look, it still seems like we have a fundamental > bit of schizo

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Tom Lane
Andres Freund writes: > Don't think we require BUFFERALIGN - MAXALIGN ought to be > sufficient. Uh, see my other message just now. > The use of BUFFERALIGN presumably is to space out things > into different cachelines, but that doesn't really seem to be important > with this. Then we can just a

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Tom Lane
I wrote: > I can confirm that on dromedary, that regression test case is attempting > to create a TOC with a not-well-aligned size: 93268 = 0x16c54 bytes. ... although, on closer look, it still seems like we have a fundamental bit of schizophrenia here, because on this machine $ grep ALIGN pg_con

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Tom Lane
Heikki Linnakangas writes: > On 08/16/2017 08:10 PM, Andres Freund wrote: >> Seems like we'd just have to add alignment of the total size to >> shm_toc_estimate()? > Yeah, that's the gist of it. I can confirm that on dromedary, that regression test case is attempting to create a TOC with a not-w

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Andres Freund
Hi, On 2017-08-16 20:19:29 +0300, Heikki Linnakangas wrote: > diff --git a/src/backend/storage/ipc/shm_toc.c > b/src/backend/storage/ipc/shm_toc.c > index 9f259441f0..121d5a1ec9 100644 > --- a/src/backend/storage/ipc/shm_toc.c > +++ b/src/backend/storage/ipc/shm_toc.c > @@ -44,7 +44,7 @@ shm_toc_

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Robert Haas
On Wed, Aug 16, 2017 at 1:19 PM, Heikki Linnakangas wrote: > Yeah, that's the gist of it. > > The attached patch seems to fix this. I'm not too familiar with this DSM > stuff, but this seems right to me. Unless someone has a better idea soon, > I'll commit this to make the buildfarm happy. Mmm, c

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Heikki Linnakangas
On 08/16/2017 08:10 PM, Andres Freund wrote: Afaict shm_create/shm_toc_allocate don't actually guarantee that the end of the toc's memory is suitably aligned. But I didn't yet have any coffee, so ... Robert, I'm not quite sure what the intended behaviour of shm_toc is wrt alignment. I see that

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Andres Freund
On 2017-08-16 09:57:35 -0700, Peter Geoghegan wrote: > On Wed, Aug 16, 2017 at 9:55 AM, Andres Freund wrote: > > On 2017-08-16 11:16:58 -0400, Tom Lane wrote: > >> Heikki Linnakangas writes: > >> > A couple of 32-bit x86 buildfarm members don't seem to be happy with > >> > this. I'll investigate,

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Andres Freund
Robert, Tom, On 2017-08-16 09:55:15 -0700, Andres Freund wrote: > > Not sure if this is your bug or if it's exposing a pre-existing > > deficiency in the atomics code, viz, failure to ensure that > > pg_atomic_uint64 is actually a 64-bit-aligned type. Andres? > I suspect it's the former. Suspe

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Peter Geoghegan
On Wed, Aug 16, 2017 at 9:55 AM, Andres Freund wrote: > On 2017-08-16 11:16:58 -0400, Tom Lane wrote: >> Heikki Linnakangas writes: >> > A couple of 32-bit x86 buildfarm members don't seem to be happy with >> > this. I'll investigate, but if anyone has a clue, I'm all ears... >> >> dromedary's is

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Andres Freund
On 2017-08-16 11:16:58 -0400, Tom Lane wrote: > Heikki Linnakangas writes: > > A couple of 32-bit x86 buildfarm members don't seem to be happy with > > this. I'll investigate, but if anyone has a clue, I'm all ears... > > dromedary's issue seems to be alignment: > > TRAP: UnalignedPointer("(((u

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Tom Lane
Heikki Linnakangas writes: > A couple of 32-bit x86 buildfarm members don't seem to be happy with > this. I'll investigate, but if anyone has a clue, I'm all ears... dromedary's issue seems to be alignment: TRAP: UnalignedPointer("(((uintptr_t) ((uintptr_t)(ptr)) + ((8) - 1)) & ~((uintptr_t) (

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Heikki Linnakangas
On 08/16/2017 04:20 PM, Heikki Linnakangas wrote: On 05/06/2017 04:57 PM, David Rowley wrote: Andres mentioned in [2] that it might be worth exploring using atomics to do the same job. So I went ahead and did that, and came up with the attached, which is a slight variation on what he mentioned i

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-08-16 Thread Heikki Linnakangas
On 05/06/2017 04:57 PM, David Rowley wrote: Andres mentioned in [2] that it might be worth exploring using atomics to do the same job. So I went ahead and did that, and came up with the attached, which is a slight variation on what he mentioned in the thread. To keep things a bit more simple, an

Re: [HACKERS] Atomics for heap_parallelscan_nextpage()

2017-05-08 Thread Amit Kapila
On Sat, May 6, 2017 at 7:27 PM, David Rowley wrote: > > I've also noticed that both the atomics patch and unpatched master do > something that looks a bit weird with synchronous seq-scans. If the > parallel seq-scan piggybacked on another scan, then subsequent > parallel scans will start at the sa

[HACKERS] Atomics for heap_parallelscan_nextpage()

2017-05-06 Thread David Rowley
Hi, A while back I did some benchmarking on a big 4 socket machine to explore a bit around the outer limits of parallel aggregates. I discovered along the way that, given enough workers, and a simple enough task, that seq-scan workers were held up waiting for the lock to be released in heap_paral