On Fri, 10 Jul 2020 at 14:21, Tom Lane <t...@sss.pgh.pa.us> wrote: > Daniel Gustafsson <dan...@yesql.se> writes: > > Thanks for the update! Do note that my patch included a new file which > is > > missing from this patchset: > > src/test/subscription/t/014_binary.pl > > This is, IMO, the most interesting test of this feature so it would be > good to > > be included. It's a basic test and can no doubt be extended to be even > more > > relevant, but it's a start. > > I was about to complain that the latest patchset includes no meaningful > test cases, but I assume that this missing file contains those. > > >> I did not do the macro for updated, inserted, deleted, will give that a > go tomorrow. > > > This might not be a blocker, but personally I think it would make the > > code more readable. Anyone else have an opinion on this? > > +1 for using macros. >
Got it, will add. > > > 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 ? > > * Please also remove debugging hacks such as enabling WAL_DEBUG. > > * It'd likely be wise for the documentation to point out that binary > mode only works if all types to be transferred have send/receive > functions. > will do > > > BTW, while it's not the job of this patch to fix it, I find it quite > distressing that we're apparently repeating the lookups of the type > I/O functions for every row transferred. > > I'll set this back to WoA, but I concur with Daniel's opinion that > it doesn't seem that far from committability. > Thanks for looking at this Dave Cramer
