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 what he mentioned in the
> > thread.
> >
> > To keep things a bit more simple, and streamline, I ended up pulling
> > out the logic for setting the startblock into another function, which
> > we only call once before the first call to
> > heap_parallelscan_nextpage(). I also ended up changing phs_cblock and
> > replacing it with a counter that always starts at zero. The actual
> > block is calculated based on that + the startblock modulo nblocks.
> > This makes things a good bit more simple for detecting when we've
> > allocated all the blocks to the workers, and also works nicely when
> > wrapping back to the start of a relation when we started somewhere in
> > the middle due to piggybacking with a synchronous scan.
> Looks reasonable. I edited the comments and the variable names a bit, to my
> liking, and committed. Thanks!
Brief post-commit review:
+ * phs_nallocated tracks how many pages have been allocated to workers
+ * already. When phs_nallocated >= rs_nblocks, all blocks have been
+ * allocated.
allocated seems a bit of a confusing terminology.
@@ -1635,8 +1637,8 @@ heap_parallelscan_initialize(ParallelHeapScanDesc target, Relation relation,
!RelationUsesLocalBuffers(relation)&& target->phs_nblocks > NBuffers / 4; SpinLockInit(&target->phs_mutex);
- target->phs_cblock = InvalidBlockNumber; target->phs_startblock = InvalidBlockNumber;
+ pg_atomic_write_u64(&target->phs_nallocated, 0); SerializeSnapshot(snapshot, target->phs_snapshot_data);}
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. You're probably going to get a failure on the respective
buildfarm animal soon.
- Andres