Re: BUG #12053: Strange behavior for numeric types with unspecified precision-scale - Mailing list pgsql-bugs

I wrote:
> So the question is whether it's worth adding logic to numeric_recv
> to guard against bogus dscale values.  I think that detecting this
> case would probably add a noticeable number of cycles to numeric_in.
> (We can't just count the number of digits received, since (a) trailing
> zeroes past dscale are OK, and (b) what we have at this point is
> base-10000 digits not base-10 digits...)  I guess we're usually willing
> to expend cycles to guard against client error, so maybe we should
> do it here too.

I had originally been thinking of throwing an error if the presented
dscale was too small for the number of digits sent, but after some
reflection it seems like it'd be safer to just silently truncate the
extra digits away.  If we throw an error it's likely to break applications
that are dependent on this buggy data adapter, and I'm not sure that the
users will thank us for that.  Truncating the extra digits will make the
value actually match what it would've printed as, and if we grant that the
Devart folk did any testing of their code at all, they probably looked at
what was printed and thought that that was what they intended.  That is,
I'm assuming that dscale = 2 means they only want 2 decimal places in
the value.

So I propose the attached patch, which requires only a minimal amount
of new code and is about as fast as we're going to get if we want to
check this issue at all.  (Note: the apparently new error condition
for out-of-range dscale doesn't create a backwards compatibility hazard,
because make_result would've barfed anyway.  This is just a more
to-the-point error message.)

            regards, tom lane

diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index d61af92..c73f9bc 100644
*** a/src/backend/utils/adt/numeric.c
--- b/src/backend/utils/adt/numeric.c
*************** numeric_recv(PG_FUNCTION_ARGS)
*** 717,722 ****
--- 717,724 ----
      alloc_var(&value, len);

      value.weight = (int16) pq_getmsgint(buf, sizeof(int16));
+     /* we allow any int16 for weight --- OK? */
+
      value.sign = (uint16) pq_getmsgint(buf, sizeof(uint16));
      if (!(value.sign == NUMERIC_POS ||
            value.sign == NUMERIC_NEG ||
*************** numeric_recv(PG_FUNCTION_ARGS)
*** 726,731 ****
--- 728,738 ----
                   errmsg("invalid sign in external \"numeric\" value")));

      value.dscale = (uint16) pq_getmsgint(buf, sizeof(uint16));
+     if ((value.dscale & NUMERIC_DSCALE_MASK) != value.dscale)
+         ereport(ERROR,
+                 (errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
+                  errmsg("invalid scale in external \"numeric\" value")));
+
      for (i = 0; i < len; i++)
      {
          NumericDigit d = pq_getmsgint(buf, sizeof(NumericDigit));
*************** numeric_recv(PG_FUNCTION_ARGS)
*** 737,742 ****
--- 744,757 ----
          value.digits[i] = d;
      }

+     /*
+      * If the given dscale would hide any digits, truncate those digits away.
+      * We could alternatively throw an error, but that would take a bunch of
+      * extra code (about as much as trunc_var involves), and it might cause
+      * client compatibility issues.
+      */
+     trunc_var(&value, value.dscale);
+
      apply_typmod(&value, typmod);

      res = make_result(&value);