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