On Fri, Jul 1, 2022 at 6:42 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > - I might be missing something here, but this isn't actually making
> > the relfilenode 56 bits, is it? The reason to do that is to make the
> > BufferTag smaller, so I expected to see that BufferTag either used
> > bitfields like RelFileNumber relNumber:56 and ForkNumber forkNum:8, or
> > else that it just declared a single field for both as uint64 and used
> > accessor macros or static inlines to separate them out. But it doesn't
> > seem to do either of those things, which seems like it can't be right.
> > On a related note, I think it would be better to declare RelFileNumber
> > as an unsigned type even though we have no use for the high bit; we
> > have, equally, no use for negative values. It's easier to reason about
> > bit-shifting operations with unsigned types.
>
> Opps, somehow missed to merge that change in the patch. Changed that
> like below and adjusted the macros.
> typedef struct buftag
> {
> Oid spcOid; /* tablespace oid. */
> Oid dbOid; /* database oid. */
> uint32 relNumber_low; /* relfilenumber 32 lower bits */
> uint32 relNumber_hi:24; /* relfilenumber 24 high bits */
> uint32 forkNum:8; /* fork number */
> BlockNumber blockNum; /* blknum relative to begin of reln */
> } BufferTag;
>
> I think we need to break like this to keep the BufferTag 4 byte
> aligned otherwise the size of the structure will be increased.
Well, I guess you're right. That's a bummer. In that case I'm a little
unsure whether it's worth using bit fields at all. Maybe we should
just write uint32 something[2] and use macros after that.
Another approach could be to accept the padding and define a constant
SizeOfBufferTag and use that as the hash table element size, like we
do for the sizes of xlog records.
--
Robert Haas
EDB: http://www.enterprisedb.com
