Re: Fix doc bug in logical replication.

On Thu, 27 Jun 2019 at 14:20, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:

> On Thu, Jun 27, 2019 at 01:46:47PM -0400, Dave Cramer wrote:
> >On Thu, 27 Jun 2019 at 12:50, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)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(dot)eisentraut(at)2ndquadrant(dot)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
>
>
Thanks for the pointer. I'll add that to the patch.

Dave Cramer

davec(at)postgresintl(dot)com
www.postgresintl.com

>
>