On 08/16/2017 09:00 PM, Tom Lane wrote:
Robert Haas <robertmh...@gmail.com> writes:
On Wed, Aug 16, 2017 at 1:44 PM, Tom Lane <t...@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

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

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to