Re: Binary support for pgoutput plugin

Daniel Gustafsson <daniel(at)yesql(dot)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.

> 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.)

* 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.

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.

regards, tom lane