Hi Tomas,
> I did a review on v3 of the patch. I see there's been some minor changes > in v4 - I haven't tried to adjust my review, but I assume most of my > comments still apply. > > Thank you for your interest and review. > 1) I don't quite understand why hash_get_shared_size() got renamed to > hash_get_init_size()? Why is that needed? Does it cover only some > initial allocation, and there's additional memory allocated later? And > what's the point of the added const? > Earlier, this function was used to calculate the size for shared hash tables only. Now, it also calculates the size for a non-shared hash table. Hence the change of name. If I don't change the argument to const, I get a compilation error as follows: "passing argument 1 of ‘hash_get_init_size’ discards ‘const’ qualifier from pointer target type." This is due to this function being called from hash_create which considers HASHCTL to be a const. > > 5) Isn't it wrong that PredicateLockShmemInit() removes the ShmemAlloc() > along with calculating the per-element requestSize, but then still does > > memset(PredXact->element, 0, requestSize); > > and > > memset(RWConflictPool->element, 0, requestSize); > > with requestSize for the whole allocation? I haven't seen any crashes, > but this seems wrong to me. I believe we can simply zero the whole > allocation right after ShmemInitStruct(), there's no need to do this for > each individual element. > Good catch. Thanks for updating. > > 5) InitProcGlobal is another example of hard-to-read code. Admittedly, > it wasn't particularly readable before, but making the lines even longer > does not make it much better ... > > I guess adding a couple macros similar to hash_create() would be one way > to improve this (and there's even a review comment in that sense), but I > chose to just do maintain a simple pointer. But if you think it should > be done differently, feel free to do so. > > LGTM, long lines have been reduced by the ptr approach. > 6) I moved the PGProcShmemSize() to be right after ProcGlobalShmemSize() > which seems to be doing a very similar thing, and to not require the > prototype. Minor detail, feel free to undo. > > LGTM. > > 7) I think the PG_CACHE_LINE_SIZE is entirely unrelated to the rest of > the patch, and I see no reason to do it in the same commit. So 0005 > removes this change, and 0006 reintroduces it separately. > > OK. > FWIW I see no justification for why the cache line padding is useful, > and it seems quite unlikely this would benefit anything, consiering it > adds padding between fairly long arrays. What kind of contention can we > get there? I don't get it. > I have done that to address a review comment upthread by Andres and the because comment above that code block also mentions need for padding. > Also, why is the patch adding padding after statusFlags (the last array > allocated in InitProcGlobal) and not between allProcs and xids? > I agree it makes more sense this way, so changing accordingly. > * > + * XXX can we rely on this matching the calculation in > hash_get_shared_size? > + * or could/should we add some asserts? Can we have at > least some sanity checks > + * on nbuckets/nsegs? > > Both places rely on compute_buckets_and_segs() to calculate nbuckets and nsegs, hence the probability of mismatch is low. I am open to adding some asserts to verify this. Do you have any suggestions in mind? Please find attached updated patches after merging all your review comments except a few discussed above. Thank you, Rahila Syed
v5-0003-Add-cacheline-padding-between-heavily-accessed-array.patch
Description: Binary data
v5-0001-Account-for-initial-shared-memory-allocated-by-hash_.patch
Description: Binary data
v5-0002-Replace-ShmemAlloc-calls-by-ShmemInitStruct.patch
Description: Binary data
