> On Nov 9, 2015, at 7:56 PM, Alvaro Herrera <alvhe...@2ndquadrant.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
