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
