Hi, On 11/07/2020 14:14, Dave Cramer wrote:
On Fri, 10 Jul 2020 at 14:21, Tom Lane <t...@sss.pgh.pa.us <mailto:t...@sss.pgh.pa.us>> wrote:> Reading through the new patch, and running the tests, I'm marking this as Ready > for Committer. It does need some cosmetic TLC, quite possibly just from > pg_indent but I didn't validate if it will take care of everything, and comment > touchups (there is still a TODO comment around wording that needs to be > resolved). However, I think it's in good enough shape for consideration at > this point. I took a quick look through the patch and had some concerns: * Please strip out the PG_VERSION_NUM and USE_INTEGER_DATETIMES checks. Those are quite dead so far as a patch for HEAD is concerned --- in fact, since USE_INTEGER_DATETIMES hasn't even been defined since v10 or so, the patch is actively doing the wrong thing there. Not that it matters. This code will never appear in any branch where float timestamps could be a thing. * I doubt that the checks on USE_FLOAT4/8_BYVAL, sizeof(int), endiannness, etc, make any sense either. Those surely do not affect the on-the-wire representation of values --- or if they do, we've blown it somewhere else. I'd just take out all those checks and assume that the binary representation is sufficiently portable. (If it's not, it's more or less the user's problem, just as in binary COPY.) So is there any point in having them as options then ?
I am guessing this is copied from pglogical, right? We have them there because it can optionally send data in the on-disk format (not the network binary format) and there this matters, but for network binary format they do not matter as Tom says.
-- Petr Jelinek 2ndQuadrant - PostgreSQL Solutions for the Enterprise https://www.2ndQuadrant.com/
