Feedback-ID: i0c21471d:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-811-gb808317eab-fm-20220801.001-gb808317e
Mime-Version: 1.0
Message-Id: <f1d58324-8df4-4bb5-a546-8c741c2e6fa8@www.fastmail.com>
In-Reply-To: 
 <CAGPVpCR15YfE=4hthY6S2OC5G0nj0QM4oE2bV769+FANM_UvwQ@mail.gmail.com>
References: 
 <CAGPVpCQvAziCLknEnygY0v1-KBtg+Om-9JHJYZOnNPKFJPompw@mail.gmail.com>
 <2ebc7ea8-3c46-474e-aea7-5d73ff6165fb@www.fastmail.com>
 <CAA4eK1KxYj=3rYHbPMQvNSFytHdyS5=q0D3A1T=uerC7HMFCtg@mail.gmail.com>
 <7cf14e66-8cea-406f-9607-607a397292f6@www.fastmail.com>
 <CAGPVpCR15YfE=4hthY6S2OC5G0nj0QM4oE2bV769+FANM_UvwQ@mail.gmail.com>
Date: Thu, 11 Aug 2022 14:15:17 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: "Melih Mutlu" <m.melihmutlu@gmail.com>
Cc: "Amit Kapila" <amit.kapila16@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Allow logical replication to copy tables in binary format
Content-Type: multipart/alternative;
 boundary=889885ee166a49f3ba6ece1d8af98042
Archived-At: 
 <https://www.postgresql.org/message-id/f1d58324-8df4-4bb5-a546-8c741c2e6fa8%40www.fastmail.com>
Precedence: bulk

--889885ee166a49f3ba6ece1d8af98042
Content-Type: text/plain

On Thu, Aug 11, 2022, at 10:46 AM, Melih Mutlu wrote:
> If such restrictions are already the case for replication phase after initial table sync, then it shouldn't prevent us from enabling binary option for table sync. Right?
I didn't carefully examine the COPY code but I won't expect significant
differences (related to text vs binary mode) from the logical replication
protocol. After inspecting the git history, I took my argument back after
checking the commit 670c0a1d474. The initial commit 9de77b54531 imposes some
restrictions (user-defined arrays and composite types) as mentioned in the
commit message but it was removed in 670c0a1d474. My main concern is to break a
scenario that was previously working (14 -> 15) but after a subscriber upgrade
it won't (14 -> 16). I would say that you should test some scenarios:
014_binary.pl and also custom data types, same column with different data
types, etc.

> How is any issue that might occur due to version mismatch being handled right now in repliaction after table sync?
> What I understand from the documentation is if replication can fail due to using different pg versions, it just fails. So binary option cannot be used. [1]
> Do you think that this is more serious for table sync and we need to restrict binary option with different publisher and subscriber versions? But not for replication?
It is a conservative argument. If we didn't allow a publisher to run COPY in
binary mode while using previous Postgres versions, we know that it works. (At
least there aren't bug reports for logical replication using binary option.)
Since one of the main use cases for logical replication is migration, I'm
concerned that it may not work (even if the binary option defaults to false,
someone can decide to use it for performance reasons).

I did a quick test and the failure while using binary mode is not clear. Since
you are modifying this code, you could probably provide additional patch(es) to
make it clear that there is an error (due to some documented restriction).


--
Euler Taveira
EDB   https://www.enterprisedb.com/

--889885ee166a49f3ba6ece1d8af98042
Content-Type: text/html
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><head><title></title><style type=3D"text/css">p.Mso=
Normal,p.MsoNoSpacing{margin:0}</style></head><body><div>On Thu, Aug 11,=
 2022, at 10:46 AM, Melih Mutlu wrote:<br></div><blockquote type=3D"cite=
" id=3D"qt" style=3D""><div dir=3D"ltr"><div dir=3D"ltr"><div dir=3D"ltr=
"><div dir=3D"ltr">If such restrictions are already the case for replica=
tion phase after initial table sync, then it shouldn't prevent us from e=
nabling binary option for table sync. Right?<br></div></div></div></div>=
</blockquote><div>I didn't carefully examine the COPY code but I won't e=
xpect significant<br></div><div>differences (related to text vs binary m=
ode) from the logical replication<br></div><div>protocol. After inspecti=
ng the git history, I took my argument back after<br></div><div>checking=
 the commit 670c0a1d474. The initial commit 9de77b54531 imposes some<br>=
</div><div>restrictions (user-defined arrays and composite types) as men=
tioned in the<br></div><div>commit message but it was removed in 670c0a1=
d474. My main concern is to break a<br></div><div>scenario that was prev=
iously working (14 -&gt; 15) but after a subscriber upgrade<br></div><di=
v>it won't (14 -&gt; 16). I would say that you should test some scenario=
s:<br></div><div>014_binary.pl and also custom data types, same column w=
ith different data<br></div><div>types, etc.<br></div><div><br></div><bl=
ockquote type=3D"cite" id=3D"qt" style=3D""><div dir=3D"ltr"><div dir=3D=
"ltr"><div dir=3D"ltr"><div class=3D"qt-gmail_quote"><div><div dir=3D"lt=
r" class=3D"qt-gmail_attr">How is any issue that might occur due to vers=
ion mismatch being handled right now in repliaction&nbsp;after table syn=
c?<br></div><div>What I understand from the documentation is if replicat=
ion can fail due to using different pg versions, it just fails. So binar=
y option cannot be used. [1]<br></div><div>Do you think that this is mor=
e serious for table sync and we need to restrict binary option with diff=
erent publisher and subscriber versions? But not for replication?<br></d=
iv></div></div></div></div></div></blockquote><div>It is a conservative =
argument. If we didn't allow a publisher to run COPY in<br></div><div>bi=
nary mode while using previous Postgres versions, we know that it works.=
 (At<br></div><div>least there aren't bug reports for logical replicatio=
n using binary option.)<br></div><div>Since one of the main use cases fo=
r logical replication is migration, I'm<br></div><div>concerned that it =
may not work (even if the binary option defaults to false,<br></div><div=
>someone can decide to use it for performance reasons).<br></div><div><b=
r></div><div>I did a quick test and the failure while using binary mode =
is not clear. Since<br></div><div>you are modifying this code, you could=
 probably provide additional patch(es) to<br></div><div>make it clear th=
at there is an error (due to some documented restriction).</div><div><br=
></div><div><br></div><div id=3D"sig112409080"><div class=3D"signature">=
--<br></div><div class=3D"signature">Euler Taveira<br></div><div class=3D=
"signature">EDB&nbsp;&nbsp; <a href=3D"https://www.enterprisedb.com/">ht=
tps://www.enterprisedb.com/</a><br></div></div><div><br></div></body></h=
tml>
--889885ee166a49f3ba6ece1d8af98042--