Re: Atomics for heap_parallelscan_nextpage()

On 08/16/2017 09:00 PM, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Aug 16, 2017 at 1:44 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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