On Wed, Sep 13, 2023 at 3:46 PM Junwang Zhao <zhjw...@gmail.com> wrote: > > On Wed, Sep 13, 2023 at 4:22 PM John Naylor > <john.nay...@enterprisedb.com> wrote:
> > - 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. > > What do you mean by 'the non-key part of LogicalRepPartMapEntry will > never get new members'? I mean, if this struct: > typedef struct LogicalRepPartMapEntry > { > Oid partoid; /* LogicalRepPartMap's key */ > LogicalRepRelMapEntry relmapentry; > } LogicalRepPartMapEntry; ...gets a new member, it will not get memset when memsetting "relmapentry". > > 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. > > There are some code using assert for this sort, for example in > *ReorderBufferToastAppendChunk*: > and in *rebuild_database_list*, tom commented that the key has already > been filled, which I think > he was trying to tell people no need to assign the key again. Okay, we have examples of each. -- John Naylor EDB: http://www.enterprisedb.com