|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|
|Views:||Raw Message | Whole Thread | Download mbox|
On 11/09/2015 10:32 PM, Ildus Kurbangaliev wrote:
>> 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.
>> 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
> 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.
Attached a new version of the patch that moves SLRU tranches and LWLocks
to SLRU control structs.
`buffer_locks` field now contains LWLocks itself, so we have some
economy of the memory here. `pstrdup` removed in SimpleLruInit. I didn't
change names from the previous patch yet, but I don't mind if they'll
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
|Next Message||Craig Ringer||2015-11-11 13:07:46||Re: can we add SKIP LOCKED to UPDATE?|
|Previous Message||Dean Rasheed||2015-11-11 11:45:26||Re: Proposal: Trigonometric functions in degrees|