Re: Atomics for heap_parallelscan_nextpage()

From: Andres Freund <andres(at)anarazel(dot)de>
To: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: Atomics for heap_parallelscan_nextpage()
Date: 2017-08-16 17:26:23
Message-ID: 20170816172623.g7s37dk4islrmsq6@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-08-16 17:32:44 Re: Atomics for heap_parallelscan_nextpage()
Previous Message Robert Haas 2017-08-16 17:24:36 Re: Atomics for heap_parallelscan_nextpage()