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

On Wed, Sep 13, 2023 at 5:28 PM John Naylor
<john(dot)naylor(at)enterprisedb(dot)com> wrote:
>
>
> On Wed, Sep 13, 2023 at 3:46 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
> >
> > On Wed, Sep 13, 2023 at 4:22 PM John Naylor
> > <john(dot)naylor(at)enterprisedb(dot)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".

ok, I see. I will leave this case as it was.

>
> > > 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

Add a v2 with some change to fix warnings about unused-parameter.

I will add this to Commit Fest.

--
Regards
Junwang Zhao