On Thu, 1 Jul 2021 at 02:00, David Rowley <dgrowleyml@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
