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

On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro <b.sadhu@gmail.com> wrote:

Hi All,

Publisher 'DateStyle' is set as "SQL, MDY", whereas in Subscriber as
"SQL, DMY", the logical replication is not working...

From Publisher:
postgres=# INSERT INTO calendar VALUES ('07-18-1036', '1'), ('05-15-1135', '1');
INSERT 0 2

Getting below error in the subscriber log file,
2021-10-14 00:59:23.067 PDT [38262] ERROR: date/time field value out
of range: "07/18/1036"
2021-10-14 00:59:23.067 PDT [38262] HINT: Perhaps you need a
different "datestyle" setting.

Is this an expected behavior?

Looks like a problem to me, I think for fixing this, on logical
replication connection always set subscriber's DateStlyle, with that
the walsender will always send the data in the same DateStyle that
worker understands and then we are good.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, 14 Oct 2021 at 19:49, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro <b.sadhu@gmail.com> wrote:

Hi All,

Publisher 'DateStyle' is set as "SQL, MDY", whereas in Subscriber as
"SQL, DMY", the logical replication is not working...

From Publisher:
postgres=# INSERT INTO calendar VALUES ('07-18-1036', '1'), ('05-15-1135', '1');
INSERT 0 2

Getting below error in the subscriber log file,
2021-10-14 00:59:23.067 PDT [38262] ERROR: date/time field value out
of range: "07/18/1036"
2021-10-14 00:59:23.067 PDT [38262] HINT: Perhaps you need a
different "datestyle" setting.

Is this an expected behavior?

Looks like a problem to me, I think for fixing this, on logical
replication connection always set subscriber's DateStlyle, with that
the walsender will always send the data in the same DateStyle that
worker understands and then we are good.

Right! Attached fix it.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Sat, 16 Oct 2021 at 22:42, Japin Li <japinli@hotmail.com> wrote:

On Thu, 14 Oct 2021 at 19:49, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro <b.sadhu@gmail.com> wrote:

Hi All,

Publisher 'DateStyle' is set as "SQL, MDY", whereas in Subscriber as
"SQL, DMY", the logical replication is not working...

From Publisher:
postgres=# INSERT INTO calendar VALUES ('07-18-1036', '1'), ('05-15-1135', '1');
INSERT 0 2

Getting below error in the subscriber log file,
2021-10-14 00:59:23.067 PDT [38262] ERROR: date/time field value out
of range: "07/18/1036"
2021-10-14 00:59:23.067 PDT [38262] HINT: Perhaps you need a
different "datestyle" setting.

Is this an expected behavior?

Looks like a problem to me, I think for fixing this, on logical
replication connection always set subscriber's DateStlyle, with that
the walsender will always send the data in the same DateStyle that
worker understands and then we are good.

Right! Attached fix it.

Add a test case in subscription/t/100_bugs.pl. Please consider the v2 patch
for review.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Add a test case in subscription/t/100_bugs.pl. Please consider the v2 patch
for review.

Reviewed and tested the patch, it works fine... There are some
trailing spaces present in the newly added code lines, which needs to
be corrected...
Doing some further testing with different datestyles, will update further...

Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com

On Thu, Oct 14, 2021 at 8:50 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro <b.sadhu@gmail.com> wrote:

Hi All,

Publisher 'DateStyle' is set as "SQL, MDY", whereas in Subscriber as
"SQL, DMY", the logical replication is not working...

From Publisher:
postgres=# INSERT INTO calendar VALUES ('07-18-1036', '1'), ('05-15-1135', '1');
INSERT 0 2

Getting below error in the subscriber log file,
2021-10-14 00:59:23.067 PDT [38262] ERROR: date/time field value out
of range: "07/18/1036"
2021-10-14 00:59:23.067 PDT [38262] HINT: Perhaps you need a
different "datestyle" setting.

Is this an expected behavior?

Looks like a problem to me, I think for fixing this, on logical
replication connection always set subscriber's DateStlyle, with that
the walsender will always send the data in the same DateStyle that
worker understands and then we are good.

+1

Probably the same is true for IntervalStyle? If the publisher sets
'sql_standard', the subscriber sets 'postgres', and an interval value
'-1 11:22:33' is inserted, these two nodes have different data:

* Publisher
=# set intervalstyle to 'postgres'; select * from test;
i
-------------------
-1 days -11:22:33
(1 row)

* Subscriber
=# set intervalstyle to 'postgres'; select * from test;
i
-------------------
-1 days +11:22:33
(1 row)

Regards,

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

Masahiko Sawada <sawada.mshk@gmail.com> writes:

On Thu, Oct 14, 2021 at 8:50 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Looks like a problem to me, I think for fixing this, on logical
replication connection always set subscriber's DateStlyle, with that
the walsender will always send the data in the same DateStyle that
worker understands and then we are good.

+1

An alternative that wouldn't require a network round trip is for the
publisher to set its own datestyle to ISO/YMD. I'm pretty sure that
will be interpreted correctly regardless of the receiver's datestyle.

Probably the same is true for IntervalStyle?

Not sure if an equivalent solution applies to intervals ...

regards, tom lane

I wrote:

An alternative that wouldn't require a network round trip is for the
publisher to set its own datestyle to ISO/YMD. I'm pretty sure that
will be interpreted correctly regardless of the receiver's datestyle.

Ah ... see postgres_fdw's set_transmission_modes(). I think we want
to copy that logic not invent some other way to do it.

regards, tom lane

On Sun, Oct 17, 2021 at 11:41:35PM -0400, Tom Lane wrote:

Ah ... see postgres_fdw's set_transmission_modes(). I think we want
to copy that logic not invent some other way to do it.

dblink.c has something similar as of applyRemoteGucs(), except that it
does not do extra_float_digits. It would be nice to avoid more
duplication for those things, at least on HEAD. On the top of my
head, don't we have something similar for parallel workers when
passing down GUCs from the leader?
--
Michael

On Mon, 18 Oct 2021 at 11:14, Sadhuprasad Patro <b.sadhu@gmail.com> wrote:

Add a test case in subscription/t/100_bugs.pl. Please consider the v2 patch
for review.

Reviewed and tested the patch, it works fine... There are some
trailing spaces present in the newly added code lines, which needs to
be corrected...
Doing some further testing with different datestyles, will update further...

Thanks for your review and test! As Tom Lane said, the postgres_fdw has the similar
things, I will update the patch later.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, 18 Oct 2021 at 11:59, Michael Paquier <michael@paquier.xyz> wrote:

On Sun, Oct 17, 2021 at 11:41:35PM -0400, Tom Lane wrote:

Ah ... see postgres_fdw's set_transmission_modes(). I think we want
to copy that logic not invent some other way to do it.

dblink.c has something similar as of applyRemoteGucs(), except that it
does not do extra_float_digits. It would be nice to avoid more
duplication for those things, at least on HEAD. On the top of my
head, don't we have something similar for parallel workers when
passing down GUCs from the leader?

Since it will be used in more than one places. IMO, we can implement it in core.
Any thoughts?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, 18 Oct 2021 at 11:41, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

An alternative that wouldn't require a network round trip is for the
publisher to set its own datestyle to ISO/YMD. I'm pretty sure that
will be interpreted correctly regardless of the receiver's datestyle.

Ah ... see postgres_fdw's set_transmission_modes(). I think we want
to copy that logic not invent some other way to do it.

Thanks for your remention. As Michael Paquier side, dblink also uses the
similar logical. I will read them then update the patch.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Japin Li <japinli@hotmail.com> writes:

On Mon, 18 Oct 2021 at 11:59, Michael Paquier <michael@paquier.xyz> wrote:

dblink.c has something similar as of applyRemoteGucs(), except that it
does not do extra_float_digits. It would be nice to avoid more
duplication for those things, at least on HEAD. On the top of my
head, don't we have something similar for parallel workers when
passing down GUCs from the leader?

Since it will be used in more than one places. IMO, we can implement it in core.
Any thoughts?

It's not going to be the same code everywhere. A logrep sender won't
have a need to save-and-restore the settings like postgres_fdw does,
AFAICS. Also, now that I look at it, dblink is doing the opposite
thing of absorbing the sender's values.

It would be good I guess to have some central notion of which
variables ought to be set to what, but I'm not sure how to
mechanize that given the need for different behaviors.

regards, tom lane

On Mon, 18 Oct 2021 at 12:17, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Japin Li <japinli@hotmail.com> writes:

On Mon, 18 Oct 2021 at 11:59, Michael Paquier <michael@paquier.xyz> wrote:

dblink.c has something similar as of applyRemoteGucs(), except that it
does not do extra_float_digits. It would be nice to avoid more
duplication for those things, at least on HEAD. On the top of my
head, don't we have something similar for parallel workers when
passing down GUCs from the leader?

Since it will be used in more than one places. IMO, we can implement it in core.
Any thoughts?

It's not going to be the same code everywhere. A logrep sender won't
have a need to save-and-restore the settings like postgres_fdw does,

Thanks for your explanation. Yeah, we do not need reset the settings in
logical replication.

AFAICS. Also, now that I look at it, dblink is doing the opposite
thing of absorbing the sender's values.

Sorry I misunderstand. You are right, the dblink applies the remote
server's settings to local server.

Attached v3 patch modify the settings on sender as you suggest.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, 18 Oct 2021 at 11:26, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Oct 14, 2021 at 8:50 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro <b.sadhu@gmail.com> wrote:

Hi All,

Publisher 'DateStyle' is set as "SQL, MDY", whereas in Subscriber as
"SQL, DMY", the logical replication is not working...

From Publisher:
postgres=# INSERT INTO calendar VALUES ('07-18-1036', '1'), ('05-15-1135', '1');
INSERT 0 2

Getting below error in the subscriber log file,
2021-10-14 00:59:23.067 PDT [38262] ERROR: date/time field value out
of range: "07/18/1036"
2021-10-14 00:59:23.067 PDT [38262] HINT: Perhaps you need a
different "datestyle" setting.

Is this an expected behavior?

Looks like a problem to me, I think for fixing this, on logical
replication connection always set subscriber's DateStlyle, with that
the walsender will always send the data in the same DateStyle that
worker understands and then we are good.

+1

Probably the same is true for IntervalStyle? If the publisher sets
'sql_standard', the subscriber sets 'postgres', and an interval value
'-1 11:22:33' is inserted, these two nodes have different data:

* Publisher
=# set intervalstyle to 'postgres'; select * from test;
i
-------------------
-1 days -11:22:33
(1 row)

* Subscriber
=# set intervalstyle to 'postgres'; select * from test;
i
-------------------
-1 days +11:22:33
(1 row)

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.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, Oct 18, 2021 at 1:41 PM Japin Li <japinli@hotmail.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.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, 18 Oct 2021 at 17:27, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Oct 18, 2021 at 1:41 PM Japin Li <japinli@hotmail.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.

Please consider the v4 patch to review.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Wed, Oct 20, 2021 at 8:12 PM Japin Li <japinli@hotmail.com> wrote:

On Mon, 18 Oct 2021 at 17:27, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Oct 18, 2021 at 1:41 PM Japin Li <japinli@hotmail.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.

Regards,

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

On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Oct 20, 2021 at 8:12 PM Japin Li <japinli@hotmail.com> wrote:

On Mon, 18 Oct 2021 at 17:27, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Oct 18, 2021 at 1:41 PM Japin Li <japinli@hotmail.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.

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?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, 21 Oct 2021 at 14:04, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Oct 20, 2021 at 8:12 PM Japin Li <japinli@hotmail.com> wrote:

On Mon, 18 Oct 2021 at 17:27, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Oct 18, 2021 at 1:41 PM Japin Li <japinli@hotmail.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.

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?

+1

I inclined to let walsender set the parameters.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, Oct 21, 2021 at 3:04 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Oct 20, 2021 at 8:12 PM Japin Li <japinli@hotmail.com> wrote:

On Mon, 18 Oct 2021 at 17:27, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Oct 18, 2021 at 1:41 PM Japin Li <japinli@hotmail.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/

On Thu, 21 Oct 2021 at 19:54, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Oct 21, 2021 at 3:04 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Oct 20, 2021 at 8:12 PM Japin Li <japinli@hotmail.com> wrote:

On Mon, 18 Oct 2021 at 17:27, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Oct 18, 2021 at 1:41 PM Japin Li <japinli@hotmail.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.

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?

Yes, we can. However, each client should be concerned the style for
datestyle, IMO it is boring.

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.

How it breaks? The user also can specify the "options" to get date data
in the style which they are wanted. Right?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Japin Li <japinli@hotmail.com> writes:

On Thu, 21 Oct 2021 at 19:54, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

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?

Yes, we can. However, each client should be concerned the style for
datestyle, IMO it is boring.

There's another issue here: the subscriber can run user-defined code
(in triggers), while AFAIK the sender cannot. People might be surprised
if their triggers run with a datestyle setting different from the
database's prevailing setting. So while I think it should be okay
to set-and-forget the datestyle on the sender side, we could not get
away with that in the subscriber. We'd have to set and unset for
each row, much as (e.g.) postgres_fdw has to do.

regards, tom lane

On Thu, 21 Oct 2021 at 22:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Japin Li <japinli@hotmail.com> writes:

On Thu, 21 Oct 2021 at 19:54, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

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?

Yes, we can. However, each client should be concerned the style for
datestyle, IMO it is boring.

There's another issue here: the subscriber can run user-defined code
(in triggers), while AFAIK the sender cannot.

Sorry, I'm not sure about this. Could you give me an example?

People might be surprised
if their triggers run with a datestyle setting different from the
database's prevailing setting. So while I think it should be okay
to set-and-forget the datestyle on the sender side, we could not get
away with that in the subscriber. We'd have to set and unset for
each row, much as (e.g.) postgres_fdw has to do.

Yeah! As Masahiko said, we can avoid the network round trips by specifying
the "options" parameter in the connection string.

If this way is more accepted, I'll update the patch later.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Japin Li <japinli@hotmail.com> writes:

On Thu, 21 Oct 2021 at 22:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:

There's another issue here: the subscriber can run user-defined code
(in triggers), while AFAIK the sender cannot.

Sorry, I'm not sure about this. Could you give me an example?

If you're doing logical replication into a table that has triggers,
the replication worker has to execute those triggers.

regards, tom lane

On Thu, 21 Oct 2021 at 23:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Japin Li <japinli@hotmail.com> writes:

On Thu, 21 Oct 2021 at 22:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:

There's another issue here: the subscriber can run user-defined code
(in triggers), while AFAIK the sender cannot.

Sorry, I'm not sure about this. Could you give me an example?

If you're doing logical replication into a table that has triggers,
the replication worker has to execute those triggers.

Does that mean we should use the subscriber's settings to set the
replication's parameter (e.g. datestyle)? If we do this, it might
loss precision (for example: extra_float_digits on publisher is 3
and on subscriber is -4), is this accpted?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, Oct 21, 2021 at 11:18 PM Japin Li <japinli@hotmail.com> wrote:

On Thu, 21 Oct 2021 at 19:54, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Oct 21, 2021 at 3:04 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Oct 20, 2021 at 8:12 PM Japin Li <japinli@hotmail.com> wrote:

On Mon, 18 Oct 2021 at 17:27, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Mon, Oct 18, 2021 at 1:41 PM Japin Li <japinli@hotmail.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.

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?

Yes, we can. However, each client should be concerned the style for
datestyle, IMO it is boring.

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.

How it breaks?

I don't know the real case but for example, if an application gets
changes via pg_recvlogical with a decoding plugin (say wal2json) from
the database whose DateStyle setting is "SQL, MDY", it expects that
the date values in the streamed data are in the style of "ISO, MDY".
But with this change, it will get date values in the style of "ISO"
which could lead to a parse error in the application.

The user also can specify the "options" to get date data
in the style which they are wanted. Right?

Right. But doesn't it mean breaking the compatibility?

Regards,

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

On Fri, 22 Oct 2021 at 08:26, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Oct 21, 2021 at 11:18 PM Japin Li <japinli@hotmail.com> wrote:

How it breaks?

I don't know the real case but for example, if an application gets
changes via pg_recvlogical with a decoding plugin (say wal2json) from
the database whose DateStyle setting is "SQL, MDY", it expects that
the date values in the streamed data are in the style of "ISO, MDY".
But with this change, it will get date values in the style of "ISO"
which could lead to a parse error in the application.

The user also can specify the "options" to get date data
in the style which they are wanted. Right?

Right. But doesn't it mean breaking the compatibility?

Yeah, it might be break the compatibility.

In conclusion, this bug has two ways to fix.

In conclusion, this bug has two ways to fix.

1. Set the parameters on publisher, this might be break the compatibility.
2. Set the parameters on subscriber. In my first patch, I try to set the
parameters after establish the connection, it will lead more network
round trips. We can set the parameters when connecting the walsender
using "options".

For the second way, should we set the parameters same as subscriber or
use the parameters (e.g. datestyle = "ISO") likes postgres_fdw
set_transmission_modes()?

Any thoughts?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Fri, Oct 22, 2021 at 8:07 AM Japin Li <japinli@hotmail.com> wrote:

On Fri, 22 Oct 2021 at 08:26, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Oct 21, 2021 at 11:18 PM Japin Li <japinli@hotmail.com> wrote:

How it breaks?

I don't know the real case but for example, if an application gets
changes via pg_recvlogical with a decoding plugin (say wal2json) from
the database whose DateStyle setting is "SQL, MDY", it expects that
the date values in the streamed data are in the style of "ISO, MDY".
But with this change, it will get date values in the style of "ISO"
which could lead to a parse error in the application.

The user also can specify the "options" to get date data
in the style which they are wanted. Right?

Right. But doesn't it mean breaking the compatibility?

Yeah, it might be break the compatibility.

In conclusion, this bug has two ways to fix.

In conclusion, this bug has two ways to fix.

1. Set the parameters on publisher, this might be break the compatibility.

Is it not possible to set the parameter on publisher as "ISO, MDY" or
"ISO, YMD", instead of only "ISO"?
DateStyle includes both, so we may set the parameter with date format...

2. Set the parameters on subscriber. In my first patch, I try to set the
parameters after establish the connection, it will lead more network
round trips. We can set the parameters when connecting the walsender
using "options".

For the second way, should we set the parameters same as subscriber or
use the parameters (e.g. datestyle = "ISO") likes postgres_fdw
set_transmission_modes()?

Any thoughts?

IMO, setting the parameter value same as the subscriber is better. It
is always possible that we can set any datestyle in the plugins
itself...

Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com

On Fri, 22 Oct 2021 at 15:00, Sadhuprasad Patro <b.sadhu@gmail.com> wrote:

On Fri, Oct 22, 2021 at 8:07 AM Japin Li <japinli@hotmail.com> wrote:

On Fri, 22 Oct 2021 at 08:26, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Oct 21, 2021 at 11:18 PM Japin Li <japinli@hotmail.com> wrote:

How it breaks?

I don't know the real case but for example, if an application gets
changes via pg_recvlogical with a decoding plugin (say wal2json) from
the database whose DateStyle setting is "SQL, MDY", it expects that
the date values in the streamed data are in the style of "ISO, MDY".
But with this change, it will get date values in the style of "ISO"
which could lead to a parse error in the application.

The user also can specify the "options" to get date data
in the style which they are wanted. Right?

Right. But doesn't it mean breaking the compatibility?

Yeah, it might be break the compatibility.

In conclusion, this bug has two ways to fix.

In conclusion, this bug has two ways to fix.

1. Set the parameters on publisher, this might be break the compatibility.

Is it not possible to set the parameter on publisher as "ISO, MDY" or
"ISO, YMD", instead of only "ISO"?
DateStyle includes both, so we may set the parameter with date format...

2. Set the parameters on subscriber. In my first patch, I try to set the
parameters after establish the connection, it will lead more network
round trips. We can set the parameters when connecting the walsender
using "options".

For the second way, should we set the parameters same as subscriber or
use the parameters (e.g. datestyle = "ISO") likes postgres_fdw
set_transmission_modes()?

Any thoughts?

IMO, setting the parameter value same as the subscriber is better. It
is always possible that we can set any datestyle in the plugins
itself...

Attach v5 patch. This patch set the datestyle, intervalstyle and
extra_float_digits parameters when we connect to publisher, this can
avoid the network round trips (compare with the first patch).

OTOH, the patch uses the subscriber's parameters as connecting parameters,
which is more complex. If we use the parameters likes postgres_fdw
set_transmission_mode(), the code will be easier [1]diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c index 5c6e56a5b2..0d03edd39f 100644 --- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c +++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c @@ -128,8 +128,8 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname, { WalReceiverConn *conn; PostgresPollingStatusType status; - const char *keys[5]; - const char *vals[5]; + const char *keys[6]; + const char *vals[6]; int i = 0;.

[1]
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 5c6e56a5b2..0d03edd39f 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -128,8 +128,8 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
 {
 	WalReceiverConn *conn;
 	PostgresPollingStatusType status;
-	const char *keys[5];
-	const char *vals[5];
+	const char *keys[6];
+	const char *vals[6];
 	int			i = 0;

 	/*
@@ -155,6 +155,8 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
 	{
 		keys[++i] = "client_encoding";
 		vals[i] = GetDatabaseEncodingName();
+		keys[++i] = "options";
+		vals[i] = "-c datestyle=ISO,\\ YMD -c intervalstyle=postgres extra_float_digits=3";
 	}
 	keys[++i] = NULL;
 	vals[i] = NULL;

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Japin Li <japinli@hotmail.com> writes:

Attach v5 patch. This patch set the datestyle, intervalstyle and
extra_float_digits parameters when we connect to publisher, this can
avoid the network round trips (compare with the first patch).

You could make it a little less confusing by not insisting on a
space in the datestyle. This should work fine:

vals[i] = "-c datestyle=ISO,YMD -c intervalstyle=postgres extra_float_digits=3";

Also, I think some comments would be appropriate.

I don't see any value whatsoever in the more complicated version
of the patch. It's just more code to maintain and more things
to go wrong. And not only at our level, but the DBA's too.
What if the subscriber and publisher are of different PG versions
and have different ideas of the valid values of these settings?

regards, tom lane

On Sat, 23 Oct 2021 at 00:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Japin Li <japinli@hotmail.com> writes:

Attach v5 patch. This patch set the datestyle, intervalstyle and
extra_float_digits parameters when we connect to publisher, this can
avoid the network round trips (compare with the first patch).

You could make it a little less confusing by not insisting on a
space in the datestyle. This should work fine:

vals[i] = "-c datestyle=ISO,YMD -c intervalstyle=postgres extra_float_digits=3";

Oh. My apologies. I try this style before, but find it see "ISO," is not valid,
so I add backslash, but it seems like that is my environment doesn't cleanup.

Fixed.

Also, I think some comments would be appropriate.

Add comments for it.

I don't see any value whatsoever in the more complicated version
of the patch. It's just more code to maintain and more things
to go wrong. And not only at our level, but the DBA's too.

Agreed.

What if the subscriber and publisher are of different PG versions
and have different ideas of the valid values of these settings?

Sorry, I'm a bit confused. Do you mean we should provide a choose for user
to set thoses parameters when establish logical replication?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Japin Li <japinli@hotmail.com> writes:

On Sat, 23 Oct 2021 at 00:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:

What if the subscriber and publisher are of different PG versions
and have different ideas of the valid values of these settings?

Sorry, I'm a bit confused. Do you mean we should provide a choose for user
to set thoses parameters when establish logical replication?

No, I'm just pointing out that pushing the subscriber's settings
to the publisher wouldn't be guaranteed to work. As long as we
use curated values that we know do what we want on all versions,
I think we're okay.

regards, tom lane

On Sat, 23 Oct 2021 at 02:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Japin Li <japinli@hotmail.com> writes:

On Sat, 23 Oct 2021 at 00:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:

What if the subscriber and publisher are of different PG versions
and have different ideas of the valid values of these settings?

Sorry, I'm a bit confused. Do you mean we should provide a choose for user
to set thoses parameters when establish logical replication?

No, I'm just pointing out that pushing the subscriber's settings
to the publisher wouldn't be guaranteed to work. As long as we
use curated values that we know do what we want on all versions,
I think we're okay.

Thanks for your clarification.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Pushed with some adjustment of the comments. I also simplified the
datestyle setting to just "ISO", because that's sufficient: that
DateStyle doesn't care about DateOrder. Since the settings are
supposed to match what pg_dump uses, it's just confusing if they don't.

Also, I didn't commit the test case. It was useful for development,
but it seemed entirely too expensive to keep forevermore compared to its
likely future value. It increased the runtime of 100_bugs.pl by about
a third, and I'm afraid the likely future value is nil. The most likely
bug in this area would be introducing some new GUC that we need to set
and forgetting to do so here; but this test case could not expose that.

regards, tom lane