Re: WIP: Relaxing the constraints on numeric scale

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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