Re: Improve monitoring of shared memory allocations

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