Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber

On Thu, Oct 21, 2021 at 3:04 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Wed, Oct 20, 2021 at 8:12 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
> > >
> > >
> > > On Mon, 18 Oct 2021 at 17:27, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > > On Mon, Oct 18, 2021 at 1:41 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
> > > >
> > > >> I attached v3 patch that set IntervalStyle to 'postgres' when the
> > > >> server backend is walsender, and this problem has gone.
> > > >
> > > >> I test that set IntervalStyle to 'sql_standard' on publisher and
> > > >> 'iso_8601' on subscriber, it works fine.
> > > >
> > > >> Please try v3 patch and let me know if they work as unexpected.
> > > >> Thanks in advance.
> > > >
> > > > I think the idea of setting the standard DateStyle and the
> > > > IntervalStyle on the walsender process looks fine to me. As this will
> > > > avoid extra network round trips as Tom mentioned.
> > >
> > > After some test, I find we also should set the extra_float_digits to avoid
> > > precision lossing.
> >
> > Thank you for the patch!
> >
> > --- a/src/backend/postmaster/postmaster.c
> > +++ b/src/backend/postmaster/postmaster.c
> > @@ -2223,6 +2223,24 @@ retry1:
> > {
> > am_walsender = true;
> > am_db_walsender = true;
> > +
> > + /*
> > + * Force assorted GUC
> > parameters to settings that ensure
> > + * that we'll output data
> > values in a form that is
> > + * unambiguous to the walreceiver.
> > + */
> > + port->guc_options =
> > lappend(port->guc_options,
> > +
> > pstrdup("datestyle"));
> > + port->guc_options =
> > lappend(port->guc_options,
> > +
> > pstrdup("ISO"));
> > + port->guc_options =
> > lappend(port->guc_options,
> > +
> > pstrdup("intervalstyle"));
> > + port->guc_options =
> > lappend(port->guc_options,
> > +
> > pstrdup("postgres"));
> > + port->guc_options =
> > lappend(port->guc_options,
> > +
> > pstrdup("extra_float_digits"));
> > + port->guc_options =
> > lappend(port->guc_options,
> > +
> > pstrdup("3"));
> > }
> >
> > I'm concerned that it sets parameters too early since wal senders end
> > up setting the parameters regardless of logical decoding plugins. It
> > might be better to force the parameters within the plugin for logical
> > replication, pgoutput, in order to avoid affecting other plugins? On
> > the other hand, if we do so, we will need to handle table sync worker
> > cases separately since they copy data via COPY executed by the wal
> > sender process. For example, we can have table sync workers set the
> > parameters.
>
> You mean table sync worker to set over the replication connection
> right? I think that was the first solution where normal workers, as
> well as table sync workers, were setting over the replication
> connection, but Tom suggested that setting on the walsender is a
> better option as we can avoid the network round trip.

Right.

BTW I think we can set the parameters from the subscriber side without
additional network round trips by specifying the "options" parameter
in the connection string, no?

> If we want to set it over the replication connection then do it for
> both as Japin's first patch is doing, otherwise, I am not seeing any
> big issue in setting it early in the walsender also. I think it is
> good to let walsender always send in the standard format which can be
> understood by other node, no?

Yeah, probably the change on HEAD is fine but I'm a bit concerned
about possible issues on back branches like if the user expects to get
date data in the style of DateStyle setting on the server via
pg_recvlogical, this change could break it.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/