Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Sat, 23 Oct 2021 at 02:00, Tom Lane wrote: > Japin Li writes: >> On Sat, 23 Oct 2021 at 00:55, Tom Lane 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.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
Japin Li writes: > On Sat, 23 Oct 2021 at 00:55, Tom Lane 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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Sat, 23 Oct 2021 at 00:55, Tom Lane wrote:
> Japin Li 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.
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 5c6e56a5b2..2c1a598300 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,13 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
{
keys[++i] = "client_encoding";
vals[i] = GetDatabaseEncodingName();
+
+ /*
+ * Force assorted GUC parameters to settings that ensure that we'll output
+ * data values in a form that is unambiguous to the publisher.
+ */
+ keys[++i] = "options";
+ vals[i] = "-c datestyle=ISO,YMD -c intervalstyle=postgres -c extra_float_digits=3";
}
keys[++i] = NULL;
vals[i] = NULL;
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index baa4a90771..d92ab60d86 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -6,7 +6,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 5;
+use Test::More tests => 7;
# Bug #15114
@@ -224,3 +224,69 @@ $node_sub->safe_psql('postgres', "DROP TABLE tab1");
$node_pub->stop('fast');
$node_pub_sub->stop('fast');
$node_sub->stop('fast');
+
+# Verify different datestyle between publisher and subscriber.
+$node_publisher = PostgresNode->new('datestyle_publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+ "datestyle = 'SQL, MDY'");
+$node_publisher->append_conf('postgresql.conf',
+ "extra_float_digits = '-4'");
+$node_publisher->start;
+
+$node_subscriber = PostgresNode->new('datestyle_subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->append_conf('postgresql.conf',
+ "datestyle = 'SQL, DMY'");
+$node_subscriber->start;
+
+# Table for datestyle
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+
+# Table for extra_float_digits
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE flt_rep(a real, d double precision)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE flt_rep(a real, d double precision)");
+
+# Setup logical replication
+my $node_publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tab_pub FOR ALL TABLES");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION tab_sub CONNECTION '$node_publisher_connstr' PUBLICATION tab_pub");
+
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_rep VALUES ('07-18-2021'), ('05-15-2018')");
+
+$node_publisher->wait_for_catchup('tab_sub');
+
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM tab_rep");
+is($result, qq(2), 'failed to replication date from different datestyle');
+
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO flt_rep VALUES (1.2121323, 32.32321232132434)");
+
+$node_publisher->wait_for_catchup('tab_sub');
+
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT a, d FROM flt_rep");
+is($result, qq(1.2121323|32.32321232132434),
+ 'failed to replication floating-point values');
+
+# Clean up the tables on both publisher and subscriber as we don't need them
+$node_publisher->safe_psql('postgres', 'DROP TABLE tab_rep');
+$node_subscr
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
Japin Li 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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Fri, 22 Oct 2021 at 15:00, Sadhuprasad Patro wrote:
> On Fri, Oct 22, 2021 at 8:07 AM Japin Li wrote:
>>
>>
>> On Fri, 22 Oct 2021 at 08:26, Masahiko Sawada wrote:
>> > On Thu, Oct 21, 2021 at 11:18 PM Japin Li 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].
[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.
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 5c6e56a5b2..c1fb4fd3d4 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -30,6 +30,7 @@
#include "pqexpbuffer.h"
#include "replication/walreceiver.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/memutils.h"
#include "utils/pg_lsn.h"
#include "utils/tuplestore.h"
@@ -82,6 +83,8 @@ static WalRcvExecResult *libpqrcv_exec(WalReceiverConn *conn,
const int nRetTypes,
const Oid *retTypes);
static void libpqrcv_disconnect(WalReceiverConn *conn);
+static char *add_backslash(const char *value);
+static char *get_transmission_modes(void);
static WalReceiverFunctionsType PQWalReceiverFunctions = {
libpqrcv_connect,
@@ -128,9 +131,10 @@ 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;
+ char *options;
/*
* We use the expand_dbname parameter to process the connection string (or
@@ -155,6 +159,10 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
{
keys[++i] = "client_encoding";
vals[i] = GetDatabaseEncodingName();
+
+ options = get_transmission_modes();
+ keys[++i] = "options";
+ vals[i] = options;
}
keys[++i] = NULL;
vals[i] = NULL;
@@ -164,6 +172
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Fri, Oct 22, 2021 at 8:07 AM Japin Li wrote: > > > On Fri, 22 Oct 2021 at 08:26, Masahiko Sawada wrote: > > On Thu, Oct 21, 2021 at 11:18 PM Japin Li 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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Fri, 22 Oct 2021 at 08:26, Masahiko Sawada wrote: > On Thu, Oct 21, 2021 at 11:18 PM Japin Li 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.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, Oct 21, 2021 at 11:18 PM Japin Li wrote: > > > On Thu, 21 Oct 2021 at 19:54, Masahiko Sawada wrote: > > On Thu, Oct 21, 2021 at 3:04 PM Dilip Kumar wrote: > >> > >> On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada > >> wrote: > >> > > >> > On Wed, Oct 20, 2021 at 8:12 PM Japin Li wrote: > >> > > > >> > > > >> > > On Mon, 18 Oct 2021 at 17:27, Dilip Kumar > >> > > wrote: > >> > > > On Mon, Oct 18, 2021 at 1:41 PM Japin Li 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/
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, 21 Oct 2021 at 23:10, Tom Lane wrote: > Japin Li writes: >> On Thu, 21 Oct 2021 at 22:46, Tom Lane 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.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
Japin Li writes: > On Thu, 21 Oct 2021 at 22:46, Tom Lane 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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, 21 Oct 2021 at 22:46, Tom Lane wrote: > Japin Li writes: >> On Thu, 21 Oct 2021 at 19:54, Masahiko Sawada 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.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
Japin Li writes: > On Thu, 21 Oct 2021 at 19:54, Masahiko Sawada 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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, 21 Oct 2021 at 19:54, Masahiko Sawada wrote: > On Thu, Oct 21, 2021 at 3:04 PM Dilip Kumar wrote: >> >> On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada >> wrote: >> > >> > On Wed, Oct 20, 2021 at 8:12 PM Japin Li wrote: >> > > >> > > >> > > On Mon, 18 Oct 2021 at 17:27, Dilip Kumar wrote: >> > > > On Mon, Oct 18, 2021 at 1:41 PM Japin Li 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.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, Oct 21, 2021 at 3:04 PM Dilip Kumar wrote:
>
> On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada
> wrote:
> >
> > On Wed, Oct 20, 2021 at 8:12 PM Japin Li wrote:
> > >
> > >
> > > On Mon, 18 Oct 2021 at 17:27, Dilip Kumar wrote:
> > > > On Mon, Oct 18, 2021 at 1:41 PM Japin Li 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/
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, 21 Oct 2021 at 14:04, Dilip Kumar wrote:
> On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada
> wrote:
>>
>> On Wed, Oct 20, 2021 at 8:12 PM Japin Li wrote:
>> >
>> >
>> > On Mon, 18 Oct 2021 at 17:27, Dilip Kumar wrote:
>> > > On Mon, Oct 18, 2021 at 1:41 PM Japin Li 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.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, Oct 21, 2021 at 11:16 AM Masahiko Sawada wrote:
>
> On Wed, Oct 20, 2021 at 8:12 PM Japin Li wrote:
> >
> >
> > On Mon, 18 Oct 2021 at 17:27, Dilip Kumar wrote:
> > > On Mon, Oct 18, 2021 at 1:41 PM Japin Li 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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Wed, Oct 20, 2021 at 8:12 PM Japin Li wrote:
>
>
> On Mon, 18 Oct 2021 at 17:27, Dilip Kumar wrote:
> > On Mon, Oct 18, 2021 at 1:41 PM Japin Li 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/
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 17:27, Dilip Kumar wrote:
> On Mon, Oct 18, 2021 at 1:41 PM Japin Li 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.
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e2a76ba055..7ae0c7bff2 100644
--- 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"));
}
else if (!parse_bool(valptr, &am_walsender))
ereport(FATAL,
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index baa4a90771..d92ab60d86 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -6,7 +6,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 5;
+use Test::More tests => 7;
# Bug #15114
@@ -224,3 +224,69 @@ $node_sub->safe_psql('postgres', "DROP TABLE tab1");
$node_pub->stop('fast');
$node_pub_sub->stop('fast');
$node_sub->stop('fast');
+
+# Verify different datestyle between publisher and subscriber.
+$node_publisher = PostgresNode->new('datestyle_publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+ "datestyle = 'SQL, MDY'");
+$node_publisher->append_conf('postgresql.conf',
+ "extra_float_digits = '-4'");
+$node_publisher->start;
+
+$node_subscriber = PostgresNode->new('datestyle_subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->append_conf('postgresql.conf',
+ "datestyle = 'SQL, DMY'");
+$node_subscriber->start;
+
+# Table for datestyle
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+
+# Table for extra_float_digits
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE flt_rep(a real, d double precision)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE flt_rep(a real, d double precision)");
+
+# Setup logical replication
+my $node_publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tab_pub FOR ALL TABLES");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION tab_sub CONNECTION '$node_publisher_connstr' PUBLICATION tab_pub");
+
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_rep VALUES ('07-18-2021'), ('05-15-2018')");
+
+$node_publisher->wait_for_catchup('tab_sub');
+
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM tab_rep");
+is($result, qq(2), 'failed to replication date from different datestyle');
+
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO flt_rep VALUES (1.2121323, 32.32321232132434)");
+
+$node_publisher->wait_for_catchup('tab_sub');
+
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT a, d FROM flt_rep");
+is($result, qq(1.2121323|32.32321232132434),
+ 'failed to replication floating-point values');
+
+# Clean up the tables on both publisher and subscriber as we don't need them
+$node_publisher->safe_psql('postgres', 'DROP TABLE tab_rep');
+$node_subscriber->safe_psql('postgres', 'DROP TABLE tab_rep');
+$node_publisher->safe_psql('postgres', 'DROP TABLE flt_rep');
+$node_subscriber->safe_psql('postgres', 'DROP TABLE flt_rep');
+
+# Drop subscription/publication as we don't need anymore
+$node_subscriber->safe_psql('postgres', 'DROP SUBSCRIPTION tab_sub');
+$node_publisher->safe_psql('postgres', 'DROP PUBLICATION tab_pub');
+
+$node_publisher->stop('fast');
+$node_subscri
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, Oct 18, 2021 at 1:41 PM Japin Li 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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 11:26, Masahiko Sawada wrote:
> On Thu, Oct 14, 2021 at 8:50 PM Dilip Kumar wrote:
>>
>> On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro 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.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 12:17, Tom Lane wrote:
> Japin Li writes:
>> On Mon, 18 Oct 2021 at 11:59, Michael Paquier 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.
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e2a76ba055..3c1b2fa85e 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2223,6 +2223,20 @@ 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"));
}
else if (!parse_bool(valptr, &am_walsender))
ereport(FATAL,
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index baa4a90771..a88a61df41 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -6,7 +6,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 5;
+use Test::More tests => 6;
# Bug #15114
@@ -224,3 +224,44 @@ $node_sub->safe_psql('postgres', "DROP TABLE tab1");
$node_pub->stop('fast');
$node_pub_sub->stop('fast');
$node_sub->stop('fast');
+
+# Verify different datestyle between publisher and subscriber.
+$node_publisher = PostgresNode->new('datestyle_publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+ "datestyle = 'SQL, MDY'");
+$node_publisher->start;
+
+$node_subscriber = PostgresNode->new('datestyle_subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->append_conf('postgresql.conf',
+ "datestyle = 'SQL, DMY'");
+$node_subscriber->start;
+
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_rep VALUES ('07-18-2021'), ('05-15-2018')");
+
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+
+# Setup logical replication
+my $node_publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tab_pub FOR ALL TABLES");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION tab_sub CONNECTION '$node_publisher_connstr' PUBLICATION tab_pub");
+
+$node_publisher->wait_for_catchup('tab_sub');
+
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM tab_rep");
+is($result, qq(2), 'failed to replication date from different datestyle');
+
+# Clean up the tables on both publisher and subscriber as we don't need them
+$node_publisher->safe_psql('postgres', 'DROP TABLE tab_rep');
+$node_subscriber->safe_psql('postgres', 'DROP TABLE tab_rep');
+
+$node_publisher->stop('fast');
+$node_subscriber->stop('fast');
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
Japin Li writes: > On Mon, 18 Oct 2021 at 11:59, Michael Paquier 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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 11:41, Tom Lane 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.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 11:59, Michael Paquier 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.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Mon, 18 Oct 2021 at 11:14, Sadhuprasad Patro 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.
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
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 signature.asc Description: PGP signature
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
Masahiko Sawada writes: > On Thu, Oct 14, 2021 at 8:50 PM Dilip Kumar 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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, Oct 14, 2021 at 8:50 PM Dilip Kumar wrote:
>
> On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro 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/
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
> 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
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Sat, 16 Oct 2021 at 22:42, Japin Li wrote:
> On Thu, 14 Oct 2021 at 19:49, Dilip Kumar wrote:
>> On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro 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.
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 5c6e56a5b2..81cf9e30d7 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -30,6 +30,7 @@
#include "pqexpbuffer.h"
#include "replication/walreceiver.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/memutils.h"
#include "utils/pg_lsn.h"
#include "utils/tuplestore.h"
@@ -218,6 +219,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
if (logical)
{
PGresult *res;
+ const char *datestyle;
res = libpqrcv_PQexec(conn->streamConn,
ALWAYS_SECURE_SEARCH_PATH_SQL);
@@ -229,6 +231,23 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
pchomp(PQerrorMessage(conn->streamConn);
}
PQclear(res);
+
+ datestyle = GetConfigOption("datestyle", true, true);
+ if (datestyle != NULL) {
+ char *sql;
+
+ sql = psprintf("SELECT pg_catalog.set_config('datestyle', '%s', false);", datestyle);
+ res = libpqrcv_PQexec(conn->streamConn, sql);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+PQclear(res);
+ereport(ERROR,
+ (errmsg("could not set datestyle: %s",
+pchomp(PQerrorMessage(conn->streamConn);
+ }
+ PQclear(res);
+ pfree(sql);
+ }
}
conn->logical = logical;
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index baa4a90771..a88a61df41 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -6,7 +6,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 5;
+use Test::More tests => 6;
# Bug #15114
@@ -224,3 +224,44 @@ $node_sub->safe_psql('postgres', "DROP TABLE tab1");
$node_pub->stop('fast');
$node_pub_sub->stop('fast');
$node_sub->stop('fast');
+
+# Verify different datestyle between publisher and subscriber.
+$node_publisher = PostgresNode->new('datestyle_publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+ "datestyle = 'SQL, MDY'");
+$node_publisher->start;
+
+$node_subscriber = PostgresNode->new('datestyle_subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->append_conf('postgresql.conf',
+ "datestyle = 'SQL, DMY'");
+$node_subscriber->start;
+
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_rep VALUES ('07-18-2021'), ('05-15-2018')");
+
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE tab_rep(a date)");
+
+# Setup logical replication
+my $node_publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tab_pub FOR ALL TABLES");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION tab_sub CONNECTION '$node_publisher_connstr' PUBLICATION tab_pub");
+
+$node_publisher->wait_for_catchup('tab_sub');
+
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM tab_rep");
+is($result, qq(2), 'failed to replication date from different datestyle');
+
+# Clean up the tables on both publisher and subscriber as we don't need them
+$node_publisher->safe_psql('postgres', 'DROP TABLE tab_rep');
+$node_subscriber->safe_psql('postgres', 'DROP TABLE tab_rep');
+
+$node_publisher->stop('fast');
+$node_subscriber->stop('fast');
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, 14 Oct 2021 at 19:49, Dilip Kumar wrote:
> On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro 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.
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 5c6e56a5b2..81cf9e30d7 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -30,6 +30,7 @@
#include "pqexpbuffer.h"
#include "replication/walreceiver.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/memutils.h"
#include "utils/pg_lsn.h"
#include "utils/tuplestore.h"
@@ -218,6 +219,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
if (logical)
{
PGresult *res;
+ const char *datestyle;
res = libpqrcv_PQexec(conn->streamConn,
ALWAYS_SECURE_SEARCH_PATH_SQL);
@@ -229,6 +231,23 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
pchomp(PQerrorMessage(conn->streamConn);
}
PQclear(res);
+
+ datestyle = GetConfigOption("datestyle", true, true);
+ if (datestyle != NULL) {
+ char *sql;
+
+ sql = psprintf("SELECT pg_catalog.set_config('datestyle', '%s', false);", datestyle);
+ res = libpqrcv_PQexec(conn->streamConn, sql);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+PQclear(res);
+ereport(ERROR,
+ (errmsg("could not set datestyle: %s",
+pchomp(PQerrorMessage(conn->streamConn);
+ }
+ PQclear(res);
+ pfree(sql);
+ }
}
conn->logical = logical;
Re: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
On Thu, Oct 14, 2021 at 3:48 PM Sadhuprasad Patro 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
Fwd: [Bug] Logical Replication failing if the DateStyle is different in Publisher & Subscriber
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?
Thanks & Regards
SadhuPrasad
http://www.EnterpriseDB.com/
