Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> Attached is a more complete patch, with updated docs and tests.
I took a brief look at this and have a couple of quick suggestions:
* 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.
* 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.
* It might be advisable to write NUMERIC_MIN_SCALE with parens:
#define NUMERIC_MIN_SCALE (-1000)
to avoid any precedence gotchas.
* I'd be inclined to leave the num_typemod_test table in place,
rather than dropping it, so that it serves to exercise pg_dump
for these cases during the pg_upgrade test.
Haven't read the code in detail yet.
regards, tom lane
