Re: Binary support for pgoutput plugin

On Fri, 10 Jul 2020 at 14:21, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

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

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