Thanks for the updated patch
On Tue, 29 Jun 2021 at 22:29, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> I'm not a fan of the *serialize() function names in numeric.c though
> (e.g., the name numeric_serialize() seems quite misleading for what it
> actually does). So rather than adding to those, I've kept the original
> names. In the context where they're used, those names seem more
> natural.
I've only looked at the numeric-agg-sumX2-overflow-v2.patch one and it
all looks mostly ok.
I kinda disagree with the send/recv naming since all you're using them
for is to serialise/deserialise the NumericVar. Functions named
*send() and recv() I more expect to return a bytea so they can be used
for a type's send/recv function. I just don't have the same
expectations for functions named serialize/deserialize. That's all
pretty special to aggregates with internal states.
One other thing I can think of to mention is that the recv function
fetches the digits with pq_getmsgint(buf, sizeof(NumericDigit))
whereas, the send function sends them with pq_sendint16(buf,
var->digits[i]). I understand you've just copied numeric_send/recv,
but I disagree with that too and think that both send functions should
be using pq_sendint. This would save having weird issues if someone
was to change the type of the NumericDigit. Perhaps that would cause
other problems, but I don't think it's a good idea to bake those
problems in any further.
I also wonder if numericvar_recv() really needs all the validation
code? We don't do any other validation during deserialisation. I see
the logic in doing this for a recv function since a client could send
us corrupt data e.g. during a binary copy. There are currently no
external factors to account for with serial/deserial.
I'm also fine for that patch to go in as-is. I'm just pointing out
what I noted down when looking over it. I'll let you choose if you
want to make any changes based on the above.
David
