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.
