Re: replication cleanup code incorrect way to use of HTAB HASH_REMOVE ?

Hi,

On 2021-03-22 11:20:37 +1300, Thomas Munro wrote:
> On Mon, Mar 22, 2021 at 10:50 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > The real problem isn't the Assert. It's all those other usages of ent
> > disobeying the API rule: "(NB: in the case of the REMOVE action, the
> > result is a dangling pointer that shouldn't be dereferenced!)"

Right that's clearly not ok.

> I suppose the HASH_REMOVE case could clobber the object with 0x7f if
> CLOBBER_FREED_MEMORY is defined (typically assertion builds)

Yea. Plus VALGRIND_MAKE_MEM_NOACCESS() (requiring a
VALGRIND_MAKE_MEM_UNDEFINED() when reusing) or at least a
VALGRIND_MAKE_MEM_UNDEFINED().

>or alternatively return some other non-NULL but poisoned pointer, so
>that problems of this ilk blow up in early testing.

IMO it's just a bad API to combine the different use cases into
hash_search(). It's kinda defensible to have FIND/ENTER/ENTER_NULL go
through the same function (although I do think it makes code harder to
read), but having HASH_REMOVE is just wrong. The only reason for
returning a dangling pointer is that that's the obvious way to check if
something was found.

If they weren't combined we could tell newer compilers that the memory
shouldn't be accessed after the HASH_REMOVE anymore. And it'd remove
some unnecessary branches in performance critical code...

But I guess making dynahash not terrible from that POV basically means
replacing all of dynahash. Having all those branches for partitioned
hashes, actions are really not great.

Greetings,

Andres Freund