On Fri, Jul 16, 2010 at 2:39 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>> I'm not entirely happy with the way I handled the variable-length
>> struct, although I don't think it's horrible, either. I'm willing to
>> rework it if someone has a better idea.
>
> I don't like the way you did that either (specifically, not the kluge
> in NUMERIC_DIGITS()). It would probably work better if you declared
> two different structs, or a union of same, to represent the two layout
> cases.
>
> A couple of other thoughts:
>
> n_sign_dscale is now pretty inappropriately named, probably better to
> change the field name. This will also help to catch anything that's
> not using the macros. (Renaming the n_weight field, or at least burying
> it in an extra level of struct, would be helpful for the same reason.)
I'm not sure what you have in mind here. If we create a union of two
structs, we'll still have to pick one of them to use to check the high
bits of the first word, so I'm not sure we'll be adding all that much
in terms of clarity. One possibility would be to name the fields
something like n_header1 and n_header2, or even just n_header[], but
I'm not sure if that's any better. If it is I'm happy to do it.
> It seems like you've handled the NAN case a bit awkwardly. Since the
> weight is uninteresting for a NAN, it's okay to not store the weight
> field, so I think what you should do is consider that the dscale field
> is still full-width, ie the format of the first word remains old-style
> not new-style. I don't remember whether dscale is meaningful for a NAN,
> but if it is, your approach is constraining what is possible to store,
> and is also breaking compatibility with old databases.
There is only one NaN value. Neither weight or dscale is meaningful.
I think if the high two bits of the first word are 11 we never examine
anything else - do you see somewhere that we're doing otherwise?
> Also, I wonder whether you can do anything with depending on the actual
> bit values of the flag bits --- specifically, it's short header format
> iff first bit is set. The NUMERIC_HEADER_SIZE macro in particular could
> be made more efficient with that.
Right, OK.
> The sign extension code in the NUMERIC_WEIGHT() macro seems a bit
> awkward; I wonder if there's a better way. One solution might be to
> offset the value (ie, add or subtract NUMERIC_SHORT_WEIGHT_MIN) rather
> than try to sign-extend per se.
Hmm... so, if the weight is X we store the value
X-NUMERIC_SHORT_WEIGHT_MIN as an unsigned integer? That's kind of a
funny representation - I *think* it works out to sign extension with
the high bit flipped. I guess we could do it that way, but it might
make it harder/more confusing to do bit arithmetic with the weight
sign bit later on.
> Please do NOT commit this:
>
> (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> ! errmsg("value overflows numeric format %x
> w=%d s=%u",
> ! result->n_sign_dscale,
> ! NUMERIC_WEIGHT(result),
> NUMERIC_DSCALE(result))));
>
> or at least hide it in "#ifdef DEBUG_NUMERIC" or some such.
Woopsie. That's debugging leftovers, sorry.
> Other than that the code changes look pretty clean, I'm mostly just
> dissatisfied with the access macros.
Thanks for the review.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
