On Wed, 31 Aug 2022 at 01:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > I think the Assert is useful as if we were ever to add an enum member
> > with the value of 8 and forgot to adjust MEMORY_CONTEXT_METHODID_BITS
> > then bad things would happen inside MemoryChunkSetHdrMask() and
> > MemoryChunkSetHdrMaskExternal(). I think it's unlikely we'll ever get
> > that many MemoryContext types, but I don't know for sure and would
> > rather the person who adds the 9th one get alerted to the lack of bit
> > space in MemoryChunk as soon as possible.
>
> I think that's a weak argument, so I don't mind dropping this Assert.
> What would be far more useful is a comment inside the
> MemoryContextMethodID enum pointing out that we can support at most
> 8 values because XYZ.
I'd just sleep better knowing that we have some test coverage to
ensure that MemoryChunkSetHdrMask() and
MemoryChunkSetHdrMaskExternal() have some verification that we don't
end up with future code that will cause the hdrmask to be invalid. I
tried to make those functions as lightweight as possible. Without the
Assert, I just feel that there's a bit too much trust that none of the
bits overlap. I've no objections to adding a comment to the enum to
explain to future devs. My vote would be for that and add the (int)
cast as proposed by Robert.
David
