Thanks for the updated patch On Tue, 29 Jun 2021 at 22:29, Dean Rasheed <dean.a.rash...@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
