Re: [PATCH] Refactoring of LWLock tranches

Ildus Kurbangaliev wrote:

> Thanks for the review. I've attached a new version of SLRU patch. I've
> removed add_postfix and fixed EXEC_BACKEND case.

Thanks.

Please do not use "committs" in commit_ts.c; people didn't like the
abbreviated name without the underscore. But then, why are we
abbreviating here? We could keep it complete and with a space instead
of underscore, so why not use just "commit timestamp", because it's just
a string, right?

In multixact.c, is there a reason to have underscore in the strings? We
could substitute it with a space and it'd look prettier; but really, we
could also keep those names parallel to subdirectory names by using the
already existing string parameter as name here, and not add another one.

I also imagined that the Slru's ControlLock itself would be part of the
tranche, and not just the per-buffer locks. That requires a bit more
churn, but seems reasonable.

Why do we have two per-buffer loops in SimpleLruInit? I mean, why not
add the LWLockInitialize call to the second one?

I'm up to speed on how the LWLockTranche API works -- does assigning to
tranche_name a pstrdup string work okay? Is the pstrdup really
necessary?

> /* Initialize our shared state struct */
> diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
> index 90c7cf5..868b35a 100644
> --- a/src/backend/access/transam/slru.c
> +++ b/src/backend/access/transam/slru.c
> @@ -157,6 +157,8 @@ SimpleLruShmemSize(int nslots, int nlsns)
> if (nlsns > 0)
> sz += MAXALIGN(nslots * nlsns * sizeof(XLogRecPtr)); /* group_lsn[] */
>
> + sz += MAXALIGN(nslots * sizeof(LWLockPadded)); /* lwlocks[] */
> +
> return BUFFERALIGN(sz) + BLCKSZ * nslots;
> }

What is the "lwlocks[]" comment supposed to mean? I don't think there's
a struct member with that name, is there?

Uhm, actually, why do we keep buffer_locks[] at all? This arrangement
seems pretty odd, where if I understand correctly we have one array
which is the tranche and another array which points to each item in the
tranche ...

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services