On Wed, Sep 13, 2023 at 1:47 PM Junwang Zhao <
zhjwpku@gmail.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