On Thu, 1 Jul 2021 at 02:00, David Rowley <dgrowle...@gmail.com> wrote: > > 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.
OK, on further reflection, I think it's best not to use the send/recv names because those names suggest that these are the internal implementations of the numeric_send/recv() functions, which they're not. > 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. I have to disagree with that. pq_sendint() is marked as deprecated and almost all callers of it have been removed. It looks like the original motivation for that was performance (see 1de09ad8eb), but I prefer it that way because it makes changing the binary format for sending data more of a conscious choice. That implies we should use pq_getmsgint(buf, sizeof(int16)) to read NumericDigit's, which I've done in numericvar_deserialize(), but I've left numeric_recv() as it is -- these 2 functions have already diverged now, and this patch is meant to be about fixing overflow errors, so I don't want to add more scope-creep. Perhaps a follow-on patch could introduce pq_getmsgint8/16/32() functions, deprecate pq_getmsgint(), and convert callers to use the new functions. > 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. OK, fair enough. That makes it more compact and efficient. I'll post an update in a while. Thanks for the review. Regards, Dean