On Wed, 21 Jul 2021 at 22:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I took a brief look at this and have a couple of quick suggestions:
>
Thanks for looking at this!
> * As you mention, keeping some spare bits in the typmod might come
> in handy some day, but as given this patch isn't really doing so.
> I think it might be advisable to mask the scale off at 11 bits,
> preserving the high 5 bits of the low-order half of the word for future
> use. The main objection to that I guess is that it would complicate
> doing sign extension in TYPMOD_SCALE(). But it doesn't seem like we
> use that logic in any really hot code paths, so another instruction
> or three probably is not much of a cost.
>
Yeah, that makes sense, and it's worth documenting where the spare bits are.
Interestingly, gcc recognised the bit hack I used for sign extension
and turned it into (x << 21) >> 21 using x86 shl and sar instructions,
though I didn't write it that way because apparently that's not
portable.
> * I agree with wrapping the typmod construction/extraction into macros
> (or maybe they should be inline functions?) but the names you chose
> seem generic enough to possibly confuse onlookers. I'd suggest
> changing TYPMOD to NUMERIC_TYPMOD or NUM_TYPMOD. The comment for them
> should probably also explicitly explain "For purely historical reasons,
> VARHDRSZ is added to the typmod value after these fields are combined",
> or words to that effect.
>
I've turned them into inline functions, since that makes them easier
to read, and debug if necessary.
All your other suggestions make sense too. Attached is a new version.
Regards,
Dean
