Re: [dynahash] do not refill the hashkey after hash_search

On Wed, Sep 13, 2023 at 1:47 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> Hi hackers,
>
> It's not necessary to fill the key field for most cases, since
> hash_search has already done that for you. For developer that
> using memset to zero the entry structure after enter it, fill the
> key field is a must, but IMHO that is not good coding style, we
> really should not touch the key field after insert it into the
> dynahash.

- memset(part_entry, 0, sizeof(LogicalRepPartMapEntry));
- part_entry->partoid = partOid;
+ Assert(part_entry->partoid == partOid);
+ memset(entry, 0, sizeof(LogicalRepRelMapEntry));

This is making an assumption that the non-key part of
LogicalRepPartMapEntry will never get new members. Without knowing much
about this code, it seems like a risk in the abstract.

> This patch fixed some most abnormal ones, instead of refilling the
> key field of primitive types, adding some assert might be a better
> choice.

Taking a quick look, I didn't happen to see any existing asserts of this
sort, so the patch doesn't seem to be making things more "normal". I did
see a few instances of /* hash_search already filled in the key */, so if
we do anything at all here, we might prefer that.

- hash_search(uncommitted_enums, serialized++, HASH_ENTER, NULL);
+ (void) hash_search(uncommitted_enums, serialized++, HASH_ENTER, NULL);

I prefer explicit (void) for new code, but others may disagree. I don't
think we have a preferred style for this, so changing current usage will
just cause unnecessary code churn.

--
John Naylor
EDB: http://www.enterprisedb.com