Re: Atomics for heap_parallelscan_nextpage()

On 08/16/2017 09:00 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, Aug 16, 2017 at 1:44 PM, Tom Lane <tgl@sss.pgh.pa.us> 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 Heikki also patched shm_toc_estimate.  With the
>> patch, if the size being used in shm_toc_create comes from
>> shm_toc_estimate, it will always be aligned and nothing bad will
>> happen.
> 
> Sure.  So the point is entirely about what should happen if someone
> doesn't use shm_toc_estimate.
>
>> If the user invents another size out of whole cloth, then
>> they might get a few bytes less than they expect, but that's what you
>> get for not using shm_toc_estimate().
> 
> 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 struct is subtracted from the size 
that you give to shm_toc_create. As well as the sizes of the TOC 
entries. And those sizes are private to shm_toc.c, so a caller has no 
way to know what size it should pass to shm_toc_create(), in order to 
have N bytes of space actually usable. You really need to use 
shm_toc_estimate() if you want any guarantees on what will fit.

I've pushed the patch to fix this, with some extra comments and 
reformatting shm_toc_estimate.

>> 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.
> 
> What I meant by possibly not good enough is that pg_atomic_uint64 used
> in other places isn't going to be very safe.
> 
> We might be effectively all right as long as we have a coding rule that
> pg_atomic_uint64 can only be placed in memory handed out by ShmemAlloc
> or shm_toc_allocate, which both have bigger-than-MAXALIGN alignment
> practices.  But this needs to be documented.

Yeah. We are implicitly also relying on ShmemAlloc() to return 
sufficiently-aligned memory. Palloc() too, although you probably 
wouldn't use atomic ops on a palloc'd struct. I think we should 
introduce a new ALIGNOF macro for that, and document that those 
functions return memory with enough alignment. GENUINEMAX_ALIGNOF? 
MAXSTRUCT_ALIGNOF?

- Heikki