Re: Fix doc bug in logical replication.

On Thu, Jun 27, 2019 at 01:46:47PM -0400, Dave Cramer wrote:
>On Thu, 27 Jun 2019 at 12:50, Tomas Vondra <tomas.vondra@2ndquadrant.com>
>wrote:
>
>> On Sun, Jun 23, 2019 at 10:26:47PM -0400, Robert Treat wrote:
>> >On Sun, Jun 23, 2019 at 1:25 PM Peter Eisentraut
>> ><peter.eisentraut@2ndquadrant.com> wrote:
>> >>
>> >> On 2019-04-12 19:52, Robert Treat wrote:
>> >> > It is clear to me that the docs are wrong, but I don't see anything
>> >> > inherently incorrect about the code itself. Do you have suggestions
>> >> > for how you would like to see the code comments improved?
>> >>
>> >> The question is perhaps whether we want to document that non-matching
>> >> data types do work.  It happens to work now, but do we always want to
>> >> guarantee that?  There is talk of a binary mode for example.
>> >>
>> >
>> >Whether we *want* to document that it works, documenting that it
>> >doesn't work when it does can't be the right answer. If you want to
>> >couch the language to leave the door open that we may not support this
>> >the same way in the future I wouldn't be opposed to that, but at this
>> >point we will have three releases with the current behavior in
>> >production, so if we decide to change the behavior, it is likely going
>> >to break certain use cases. That may be ok, but I'd expect a
>> >documentation update to accompany a change that would cause such a
>> >breaking change.
>> >
>>
>> I agree with that. We have this behavior for quite a bit of time, and
>> while technically we could change the behavior in the future (using the
>> "not supported" statement), IMO that'd be pretty annoying move. I always
>> despised systems that "fix" bugs by documenting that it does not work, and
>> this is a bit similar.
>>
>> FWIW I don't quite see why supporting binary mode would change this?
>> Surely we can't just enable binary mode blindly, there need to be some
>> sort of checks (alignment, type sizes, ...) with fallback to text mode.
>> And perhaps support only for built-in types.
>>
>
>The proposed implementation of binary only supports built-in types.
>The subscriber turns it on so presumably it can handle the binary data
>coming at it.
>

I don't recall that being discussed in the patch thread, but maybe it
should not be enabled merely based on what the subscriber requests. Maybe
the subscriber should indicate "interest" and the decision should be made
on the upstream, after some additional checks.

That's why pglogical does check_binary_compatibility() - see [1].

This is necessary, because the FE/BE protocol docs [2] say:

    Keep in mind that binary representations for complex data types might
    change across server versions; the text format is usually the more
    portable choice.

So you can't just assume the subscriber knows what it's doing, because
either of the sides might be upgraded.

Note: The pglogical code also does check additional stuff (like
sizeof(Datum) or endianess), but I'm not sure that's actually necessary -
I believe the binary protocol should be independent of that.


regards

[1] https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/pglogical_output_plugin.c#L107

[2] https://www.postgresql.org/docs/current/protocol-overview.html

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services