From: | Ildus Kurbangaliev <i(dot)kurbangaliev(at)postgrespro(dot)ru> |
---|---|
To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, "andres(at)anarazel(dot)de" <andres(at)anarazel(dot)de>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
Subject: | Re: [PATCH] Refactoring of LWLock tranches |
Date: | 2015-11-09 19:32:26 |
Message-ID: | 9DAEFD12-0BA7-404E-92CF-023006BD1C46@postgrespro.ru |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> On Nov 9, 2015, at 7:56 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
> 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 do not insist on concrete names or a case here, but I think that identifiers are more
useful when they don't contain spaces. For example that name will be exposed later
in other places and can be part of some longer string.
>
> Why do we have two per-buffer loops in SimpleLruInit? I mean, why not
> add the LWLockInitialize call to the second one?
Thanks. I didn't see that.
>
> 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?
I think pstrdup can be removed here.
>
>> /* 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?
It just means that we are allocating memory for an array of lwlocks,
i'll change it.
>
> 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 ...
Actually yes, that is a good idea.
----
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com <http://www.postgrespro.com/>
The Russian Postgres Company
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2015-11-09 19:47:34 | Re: eXtensible Transaction Manager API |
Previous Message | Alvaro Herrera | 2015-11-09 19:01:46 | Re: bootstrap pg_shseclabel in relcache initialization |