Re: Atomics for heap_parallelscan_nextpage()

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_create(uint64 magic, void *address, Size nbytes)
>  	Assert(nbytes > offsetof(shm_toc, toc_entry));
>  	toc->toc_magic = magic;
>  	SpinLockInit(&toc->toc_mutex);
> -	toc->toc_total_bytes = nbytes;
> +	toc->toc_total_bytes = BUFFERALIGN_DOWN(nbytes);
>  	toc->toc_allocated_bytes = 0;
>  	toc->toc_nentry = 0;

Don't think we require BUFFERALIGN - MAXALIGN ought to be
sufficient. 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 avoid defining the new macro...


> @@ -252,7 +252,8 @@ shm_toc_lookup(shm_toc *toc, uint64 key, bool noError)
>  Size
>  shm_toc_estimate(shm_toc_estimator *e)
>  {
> -	return add_size(offsetof(shm_toc, toc_entry),
> -					add_size(mul_size(e->number_of_keys, sizeof(shm_toc_entry)),
> -							 e->space_for_chunks));
> +	return BUFFERALIGN(
> +		add_size(offsetof(shm_toc, toc_entry),
> +				 add_size(mul_size(e->number_of_keys, sizeof(shm_toc_entry)),
> +						  e->space_for_chunks)));
>  }

I think splitting this into separate statements would be better. I think
we also should rather *ALLIGN the size without e->space_for_chunks. The
latter is already aligned properly, and the important bit is that we
have enough space for e->space_for_chunks afterwards, and only padding
is in th eway of that...

Greetings,

Andres Freund