On Mon, Nov 14, 2022 8:08 PM Melih Mutlu wrote:
>
> Attached patch with updated version of this patch.
Thanks for your patch.
I tried to do a performance test for this patch, the result looks good to me.
(The steps are similar to what Melih shared [1].)
The time to synchronize about 1GB data in
Hi,
Thanks for your review.
shiy.f...@fujitsu.com , 11 Oca 2023 Çar, 11:56
tarihinde şunu yazdı:
> On Mon, Nov 14, 2022 8:08 PM Melih Mutlu wrote:
> 1.
> +# Binary enabled subscription should fail
> +$node_subscriber_binary->wait_for_log("ERROR: insufficient data left in
> message");
>
> Shoul
On Wed, 11 Jan 2023 at 16:14, Melih Mutlu wrote:
>
> Hi,
>
> Thanks for your review.
>
> shiy.f...@fujitsu.com , 11 Oca 2023 Çar, 11:56
> tarihinde şunu yazdı:
>>
>> On Mon, Nov 14, 2022 8:08 PM Melih Mutlu wrote:
>> 1.
>> +# Binary enabled subscription should fail
>> +$node_subscriber_binary->w
On Wednesday, January 11, 2023 7:45 PM Melih Mutlu
wrote:
> Thanks for your review.
> Done.
Hi, minor comments on v5.
(1) publisher's version check
+ /* If the publisher is v16 or later, copy in binary format.*/
+ server_version = walrcv_server_version(LogRepWorkerWalRcvConn);
+
Hi,
Thanks for your reviews.
Takamichi Osumi (Fujitsu) , 12 Oca 2023 Per,
06:07 tarihinde şunu yazdı:
> On Wednesday, January 11, 2023 7:45 PM Melih Mutlu
> wrote:
> (1-1) There is no need to log the version and the query by elog here.
> (1-2) Also, I suggest we can remove the server_version va
On Thu, Jan 12, 2023 at 1:53 PM Melih Mutlu wrote:
>
> Hi,
>
> Thanks for your reviews.
Thanks. I have some comments:
1. The performance numbers posted upthread [1] look impressive for the
use-case tried, that's a 2.25X improvement or 55.6% reduction in
execution times. However, it'll be great t
Hi Bharath,
Thanks for reviewing.
Bharath Rupireddy , 18 Oca 2023
Çar, 10:17 tarihinde şunu yazdı:
> On Thu, Jan 12, 2023 at 1:53 PM Melih Mutlu
> wrote:
> 1. The performance numbers posted upthread [1] look impressive for the
> use-case tried, that's a 2.25X improvement or 55.6% reduction in
>
On Mon, Jan 30, 2023 at 4:19 PM Melih Mutlu wrote:
>
Thanks for providing an updated patch.
>> On Thu, Jan 12, 2023 at 1:53 PM Melih Mutlu wrote:
>> 1. The performance numbers posted upthread [1] look impressive for the
>> use-case tried, that's a 2.25X improvement or 55.6% reduction in
>> exec
Hi Takamichi,
Thanks for your reviews.
I addressed your reviews, please find the attached patch.
osumi.takami...@fujitsu.com , 16 Eyl 2022 Cum,
16:51 tarihinde şunu yazdı:
> (1) whitespace issues
>
Fixed
(2) Suggestion to update another general description about the subscription
>
> Kindly ha
Hi,
On Monday, October 3, 2022 8:50 PM Melih Mutlu wrote:
> (4) Error message of the binary format copy
>
> I've gotten below message from data type contradiction (between
> integer and bigint).
> Probably, this is unclear for the users to understand the direct cause
> a
Hello,
osumi.takami...@fujitsu.com , 12 Eki 2022 Çar,
04:36 tarihinde şunu yazdı:
> > I agree with the direction to support binary copy for v16 and
> later.
> >
> > IIUC, the binary format replication with different data types
> fails even
> > during apply phase on HEAD.
> > I t
On Monday, January 30, 2023 7:50 PM Melih Mutlu wrote:
> Thanks for reviewing.
...
> Please see [1] and you'll get the following error in your case:
> "ERROR: incorrect binary data format in logical replication column 1"
Hi, thanks for sharing v7.
(1) general comment
I wondered if the addition
Hi, Melih
On Monday, January 30, 2023 7:50 PM Melih Mutlu wrote:
> Thanks for reviewing.
...
> Well, with this patch, it will begin to fail in the table copy phase...
The latest patch doesn't get updated for more than two weeks
after some review comments. If you don't have time,
I would like to
Hi,
Please see the attached patch for following changes.
Bharath Rupireddy , 30 Oca 2023
Pzt, 15:34 tarihinde şunu yazdı:
> On Mon, Jan 30, 2023 at 4:19 PM Melih Mutlu
> wrote:
It'd be better and clearer to have a separate TAP test file IMO since
> the focus of the feature here isn't to just t
Hi,
Thanks for reviewing. Please see the v8 here [1]
Takamichi Osumi (Fujitsu) , 1 Şub 2023 Çar,
06:05 tarihinde şunu yazdı:
> (1) general comment
>
> I wondered if the addition of the new option/parameter can introduce some
> confusion to the users.
>
> case 1. When binary = true and copy_forma
On Mon, Jan 30, 2023 at 4:19 PM Melih Mutlu wrote:
>
> Hi Bharath,
>
> Thanks for reviewing.
>
> Bharath Rupireddy , 18 Oca 2023 Çar,
> 10:17 tarihinde şunu yazdı:
>>
>> On Thu, Jan 12, 2023 at 1:53 PM Melih Mutlu wrote:
>> 1. The performance numbers posted upthread [1] look impressive for the
>
Dear Melih,
Thank you for updating the patch. Before reviewing, I found that
cfbot have not accepted v8 patch [1].
IIUC src/psql/describe.c has been modified in v8, but
src/test/regress/expected/subscription.out
has not been changed accordingly.
[1]: https://cirrus-ci.com/github/postgresql-cfbo
On Thu, Feb 16, 2023 8:48 PM Amit Kapila wrote:
>
> On Mon, Jan 30, 2023 at 4:19 PM Melih Mutlu
> wrote:
> >
> > Logical replication between different types like int and smallint is
> > already not
> working properly on HEAD too.
> > Yes, the scenario you shared looks like working. But you didn
Hi,
Hayato Kuroda (Fujitsu) , 20 Şub 2023 Pzt, 10:12
tarihinde şunu yazdı:
> Dear Melih,
>
> Thank you for updating the patch. Before reviewing, I found that
> cfbot have not accepted v8 patch [1].
>
Thanks for letting me know.
Attached the fixed version of the patch.
Best,
--
Melih Mutlu
Micr
Amit Kapila , 16 Şub 2023 Per, 15:47 tarihinde
şunu yazdı:
> On Mon, Jan 30, 2023 at 4:19 PM Melih Mutlu
> wrote:
> >> 8. Note that the COPY binary format isn't portable across platforms
> >> (Windows to Linux for instance) or major versions
> >> https://www.postgresql.org/docs/devel/sql-copy.htm
On Mon, Feb 20, 2023 at 5:17 PM Melih Mutlu wrote:
>
> Thanks for letting me know.
> Attached the fixed version of the patch.
Thanks. I have few comments on v9 patch:
1.
+/* Do not allow binary = false with copy_format = binary */
+if (!opts.binary &&
+
Dear Melih,
Thank you for updating the patch! Followings are my comments.
01. catalogs.sgml
```
If true, the subscription will request that the publisher send data
- in binary format
```
I'm not clear here. subbinary does not directly mean that whether the worker
requests to send
> If in future version the general data type is added, the copy command in
> binary
> format will not work well, right? It is because the inferior version does not
> have
> recv/send functions for added type. It means that there is a possibility that
> replication between different versions may b
Dear Jelte,
> I don't think it's necessary to check versions. Yes, there are
> situations where binary will fail across major versions. But in many
> cases it does not. To me it seems the responsibility of the operator
> to evaluate this risk. And if the operator chooses wrong and uses
> binary co
On Thu, Feb 23, 2023 12:40 PM Kuroda, Hayato/黒田 隼人
wrote:
>
> > > I'm not sure the combination of "copy_format = binary" and "copy_data =
> false"
> > > should be accepted or not. How do you think?
> >
> > It seems quite useless indeed to specify the format of a copy that won't
> happen.
>
> I
shiy.f...@fujitsu.com , 23 Şub 2023 Per, 12:29
tarihinde şunu yazdı:
> On Thu, Feb 23, 2023 12:40 PM Kuroda, Hayato/黒田 隼人 <
> kuroda.hay...@fujitsu.com> wrote:
> >
> > > > I'm not sure the combination of "copy_format = binary" and
> "copy_data =
> > false"
> > > > should be accepted or not. How do
> This is because copy_data is not something stored in pg_subscription
> or another catalog. But this is not an issue for copy_fornat since its
> value will be stored in the catalog. This can allow users to set the
> format even if copy_data=false and no initial sync is needed at that
> moment.
On
Here is my summary of this thread so far, plus some other thoughts.
(I wrote this mostly for my own internal notes/understanding, but
maybe it is a helpful summary for others so I am posting it here too).
~~
1. Initial Goal
--
Currently, the CREATE SUBSCRIPTION ... WITH(binary=t
On Monday, February 20, 2023 8:47 PM Melih Mutlu wrote:
> Thanks for letting me know.
> Attached the fixed version of the patch.
Hi, Melih
Thanks for updating the patch. Minor comments on v9.
(1) commit message
"The patch introduces a new parameter, copy_format, to CREATE SUBSCRIPTION to
all
On Mon, Feb 20, 2023 at 3:37 PM shiy.f...@fujitsu.com
wrote:
>
> On Thu, Feb 16, 2023 8:48 PM Amit Kapila wrote:
> >
> > So, doesn't this mean that there is no separate failure mode during
> > the initial copy? I am clarifying this to see if the patch really
> > needs a separate copy_format optio
On Fri, Feb 24, 2023 at 8:32 AM Peter Smith wrote:
>
> Here is my summary of this thread so far, plus some other thoughts.
Thanks.
> 1. Initial Goal
> --
>
> Currently, the CREATE SUBSCRIPTION ... WITH(binary=true) mode does
> data replication in binary mode, but tablesync COPY p
On Tue, Feb 21, 2023 at 7:18 PM Bharath Rupireddy
wrote:
>
> On Mon, Feb 20, 2023 at 5:17 PM Melih Mutlu wrote:
> >
> > Thanks for letting me know.
> > Attached the fixed version of the patch.
>
> Thanks. I have few comments on v9 patch:
>
> 1.
> +/* Do not allow binary = fals
Hi,
Thanks for all of your reviews!
So, I made some changes in the v10 according to your comments.
1- copy_format is changed to a boolean parameter copy_binary.
It sounds easier to use a boolean to enable/disable binary copy. Default
value is false, so nothing changes in the current behaviour if
On Tue, 28 Feb 2023 at 01:22, Melih Mutlu wrote:
>
> Hi,
>
> Thanks for all of your reviews!
>
> So, I made some changes in the v10 according to your comments.
>
> 1- copy_format is changed to a boolean parameter copy_binary.
> It sounds easier to use a boolean to enable/disable binary copy. Defau
On Tue, Feb 28, 2023 at 1:22 AM Melih Mutlu wrote:
>
> Hi,
>
> Thanks for all of your reviews!
>
> So, I made some changes in the v10 according to your comments.
Thanks. Some quick comments on v10:
1.
+
+ If true, initial data synchronization will be performed in binary format
+
> 3. I think the newly added tests must verify if the binary COPY is
> picked up when enabled. Perhaps, looking at the publisher's server log
> for 'COPY ... WITH BINARY format'? Maybe it's an overkill, otherwise,
> we have no way of testing that the option took effect.
Another way to test that BI
Hi,
Attached v11.
vignesh C , 28 Şub 2023 Sal, 13:59 tarihinde şunu
yazdı:
> Thanks for the patch, Few comments:
> 1) Are primary key required for the tables, if not required we could
> remove the primary key which will speed up the test by not creating
> the indexes and inserting in the indexes
On Mon, Feb 27, 2023 at 2:31 PM Amit Kapila wrote:
>
> On Mon, Feb 20, 2023 at 3:37 PM shiy.f...@fujitsu.com
> wrote:
> >
> > On Thu, Feb 16, 2023 8:48 PM Amit Kapila wrote:
> > >
> > > So, doesn't this mean that there is no separate failure mode during
> > > the initial copy? I am clarifying th
On Wed, Mar 1, 2023 at 4:47 PM Dilip Kumar wrote:
>
> > > walsender ERROR: no binary output function available for type
> > > public.myvarchar
> > > walsender STATEMENT: COPY public.tbl1 (a) TO STDOUT WITH (FORMAT binary)
> > >
> >
> > Thanks for sharing the example. I think to address this us
Hi,
Bharath Rupireddy , 1 Mar 2023 Çar,
15:02 tarihinde şunu yazdı:
> On Wed, Mar 1, 2023 at 4:47 PM Dilip Kumar wrote:
> > I agree with this thought, basically adding an extra option will
> > always complicate things for the user. And logically it doesn't make
> > much sense to copy data in te
Dear Melih,
If we do not have to treat the case Shi pointed out[1] as code-level, I agreed
to
same option binary because it is simpler. I read the use-cases addressed by
Bharath[2]
and I cannot find advantage for case 1 and 3 expect the case that binary
functions
are not implemented.
Previously
Hi,
Hayato Kuroda (Fujitsu) , 1 Mar 2023 Çar, 18:40
tarihinde şunu yazdı:
> Dear Melih,
>
> If we do not have to treat the case Shi pointed out[1] as code-level, I
> agreed to
> same option binary because it is simpler.
How is this an issue if we let the binary option do binary copy and not an
On Thu, Mar 2, 2023 at 5:10 AM Melih Mutlu wrote:
>
> Hi,
>
> Hayato Kuroda (Fujitsu) , 1 Mar 2023 Çar, 18:40
> tarihinde şunu yazdı:
>>
>> Dear Melih,
>>
>> If we do not have to treat the case Shi pointed out[1] as code-level, I
>> agreed to
>> same option binary because it is simpler.
>
>
> Ho
On Thu, Mar 2, 2023 at 7:27 AM Peter Smith wrote:
>
> On Thu, Mar 2, 2023 at 5:10 AM Melih Mutlu wrote:
> >
> > Hi,
> >
> > Hayato Kuroda (Fujitsu) , 1 Mar 2023 Çar, 18:40
> > tarihinde şunu yazdı:
> >>
> >> Dear Melih,
> >>
> >> If we do not have to treat the case Shi pointed out[1] as code-lev
On Thu, Mar 2, 2023 at 10:30 AM Amit Kapila wrote:
>
> > TBH I am not sure anymore if the complications justify the patch.
> >
> > It seems we have to choose from 2 bad choices:
> > - separate options = this works but would be more confusing for the user
> > - unified option = this would be simple
On Thu, Mar 2, 2023 at 4:00 PM Amit Kapila wrote:
>
> On Thu, Mar 2, 2023 at 7:27 AM Peter Smith wrote:
> >
...
> > IIUC most people seem to be coming down in favour of there being a
> > single unified option (the existing 'binary==true/false) which would
> > apply to both the COPY and the data r
On Wed, Mar 1, 2023 at 7:58 PM Melih Mutlu wrote:
>
> Bharath Rupireddy , 1 Mar 2023 Çar,
> 15:02 tarihinde şunu yazdı:
>>
>
> That was my intention in the beginning with this patch. Then the new option
> also made some sense at some point, and I added copy_binary option according
> to reviews.
Dear Melih,
>> I think we should add description to doc that it is more likely happen to
>> fail
>> the initial copy user should enable binary format after synchronization if
>> tables have original datatype.
>
> I tried to explain when binary copy can cause failures in the doc. What
> exactly
>
Hi,
On 7 Mar 2023 Tue at 04:10 Amit Kapila wrote:
> As per what I could read in this thread, most people prefer to use the
> existing binary option rather than inventing a new way (option) to
> binary copy in the initial sync phase. Do you agree?
I agree.
What do you think about the version ch
On Wed, Mar 8, 2023 at 6:17 PM Melih Mutlu wrote:
>
> On 7 Mar 2023 Tue at 04:10 Amit Kapila wrote:
>>
>> As per what I could read in this thread, most people prefer to use the
>> existing binary option rather than inventing a new way (option) to
>> binary copy in the initial sync phase. Do you a
On Wed, Aug 10, 2022, at 12:03 PM, Melih Mutlu wrote:
> I see that logical replication subscriptions have an option to enable binary
> [1].
> When it's enabled, subscription requests publisher to send data in binary
> format.
> But this is only the case for apply phase. In tablesync, tables are
On Thu, Aug 11, 2022 at 7:34 AM Euler Taveira wrote:
>
> On Wed, Aug 10, 2022, at 12:03 PM, Melih Mutlu wrote:
>
> I see that logical replication subscriptions have an option to enable binary
> [1].
> When it's enabled, subscription requests publisher to send data in binary
> format.
> But this
On Thu, Aug 11, 2022, at 8:04 AM, Amit Kapila wrote:
> On Thu, Aug 11, 2022 at 7:34 AM Euler Taveira wrote:
> >
> > The reason to use text format is that it is error prone. There are
> > restrictions
> > while using the binary format. For example, if your schema has different
> > data
> > types
Euler Taveira , 11 Ağu 2022 Per, 16:27 tarihinde şunu
yazdı:
> On Thu, Aug 11, 2022, at 8:04 AM, Amit Kapila wrote:
>
> On Thu, Aug 11, 2022 at 7:34 AM Euler Taveira wrote:
> >
> > The reason to use text format is that it is error prone. There are
> restrictions
> > while using the binary format.
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 signifi
Euler Taveira , 11 Ağu 2022 Per, 20:16 tarihinde şunu
yazdı:
> My main concern is to break a scenario that was previously working (14 ->
> 15) but after a subscriber upgrade
> it won't (14 -> 16).
>
Fair concern. Some cases that might break the logical replication with
version upgrade would be:
1-
Hi,
On 2022-08-10 18:03:56 +0300, Melih Mutlu wrote:
> To copy tables, COPY command is used and that command supports copying in
> binary. So it seemed to me possible to copy in binary for tablesync too.
> I'm not sure if there is a reason to always copy tables in text format.
It'd be good to col
Hello,
Andres Freund , 2 Eyl 2022 Cum, 01:25 tarihinde şunu
yazdı:
> It'd be good to collect some performance numbers justifying this. I'd
> expect
> decent gains if there's e.g. a bytea or timestamptz column involved.
Experimented the binary copy with a quick setup.
- Created a "temp" table w
Hi hackers,
I just wanted to gently ping to hear what you all think about this patch.
Appreciate any feedback/thougths.
Thanks,
Melih
On Tuesday, August 16, 2022 2:04 AM Melih Mutlu wrote:
> Attached patch also includes some additions to the doc along with the tests.
Hi, thank you for updating the patch. Minor review comments for the v2.
(1) whitespace issues
Please fix below whitespace errors.
$ git apply v2-0001-Allow-log
Hi
Few more minor comments.
On Tuesday, August 16, 2022 2:04 AM Melih Mutlu wrote:
>
>
> My main concern is to break a scenario that was previously working (14
> -> 15) but after a subscriber upgrade
> it won't (14 -> 16).
>
> Fair concern. Some cases that might break the logical
Hi,
Attached v12 with a unified option.
Setting binary = true now allows the initial sync to happen in binary
format.
Thanks,
--
Melih Mutlu
Microsoft
v12-0001-Allow-logical-replication-to-copy-table-in-binary.patch
Description: Binary data
Here are some review comments for patch v12-0001
==
General
1.
There is no new test code. Are we sure that there are already
sufficient TAP tests doing binary testing with/without copy_data and
covering all the necessary combinations?
==
Commit Message
2.
Without this patch, table are c
On Tue, Mar 14, 2023 at 11:06 AM Peter Smith wrote:
>
> Here are some review comments for patch v12-0001
>
> ==
> General
>
> 1.
> There is no new test code. Are we sure that there are already
> sufficient TAP tests doing binary testing with/without copy_data and
> covering all the necessary c
Here are some review comments for patch v12-0001 (test code only)
==
src/test/subscription/t/014_binary.pl
# Check the synced data on subscribers
~
There are a couple of comments like the above that say: "on
subscribers" instead of "on subscriber".
~~~
I wondered if it might be useful to
On Tue, Mar 14, 2023 at 6:18 AM Peter Smith wrote:
>
> On Tue, Mar 14, 2023 at 11:06 AM Peter Smith wrote:
> >
> > Here are some review comments for patch v12-0001
> >
> > ==
> > General
> >
> > 1.
> > There is no new test code. Are we sure that there are already
> > sufficient TAP tests doin
Hi,
Attached v13.
Peter Smith , 14 Mar 2023 Sal, 03:07 tarihinde şunu
yazdı:
> Here are some review comments for patch v12-0001
>
Thanks for reviewing. I tried to make explanations in docs better
according to your comments.
What do you think?
Amit Kapila , 14 Mar 2023 Sal, 06:17 tarihinde
şun
On Tuesday, March 14, 2023 8:02 PM Melih Mutlu wrote:
> Attached v13.
Hi,
Thanks for sharing v13. Few minor review comments.
(1) create_subscription.sgml
+ column types as opposed to text format. Even when this option is
enabled,
+ only data types having binary send and recei
On Tue, Mar 14, 2023 at 8:50 PM Takamichi Osumi (Fujitsu)
wrote:
>
> On Tuesday, March 14, 2023 8:02 PM Melih Mutlu wrote:
> (3) copy_table()
>
> + /*
> +* If the publisher version is earlier than v14, it COPY command
> doesn't
> +* support the binary option.
> +*/
Here are some review comments for v13-0001
==
doc/src/sgml/logical-replication.sgml
1.
@@ -241,10 +241,13 @@
types of the columns do not need to match, as long as the text
representation of the data can be converted to the target type. For
example, you can replicate from a column
On Wed, Mar 15, 2023 at 11:52 AM Peter Smith wrote:
>
> ==
> src/backend/replication/logical/tablesync.c
>
> 5.
> +
> + /*
> + * If the publisher version is earlier than v14, it COPY command doesn't
> + * support the binary option.
> + */
> + if (walrcv_server_version(LogRepWorkerWalRcvConn) >
Hi,
On Wednesday, March 15, 2023 2:34 PM Amit Kapila
wrote:
> On Tue, Mar 14, 2023 at 8:50 PM Takamichi Osumi (Fujitsu)
> wrote:
> >
> > On Tuesday, March 14, 2023 8:02 PM Melih Mutlu
> wrote:
> > (3) copy_table()
> >
> > + /*
> > +* If the publisher version is earlier than v14,
On Tue, Mar 14, 2023 at 4:32 PM Melih Mutlu wrote:
>
> Attached v13.
>
I have a question related to the below test in the patch:
+# Setting binary to false should allow syncing
+$node_subscriber->safe_psql(
+'postgres', qq(
+ALTER SUBSCRIPTION tsub SET (binary = false);));
+
+# Ensure th
Hi,
Please see the attached patch.
Takamichi Osumi (Fujitsu) , 14 Mar 2023 Sal,
18:20 tarihinde şunu yazdı:
> (1) create_subscription.sgml
>
> + column types as opposed to text format. Even when this option
> is enabled,
> + only data types having binary send and receive functi
Hi,
Please see v14 [1].
Peter Smith , 15 Mar 2023 Çar, 09:22 tarihinde şunu
yazdı:
> Here are some review comments for v13-0001
>
> ==
> doc/src/sgml/logical-replication.sgml
>
> 1.
> That's why in the previous v12 review [1] (comment #3) I suggested
> that this page should just say somethin
Amit Kapila , 15 Mar 2023 Çar, 12:31 tarihinde
şunu yazdı:
> On Tue, Mar 14, 2023 at 4:32 PM Melih Mutlu
> wrote:
>
> What purpose does this test serve w.r.t this patch? Before checking
> the sync for different column orders, the patch has already changed
> binary to false, so it doesn't seem t
On Wed, 15 Mar 2023 at 15:30, Melih Mutlu wrote:
>
> Hi,
>
> Please see the attached patch.
One comment:
1) There might be a chance the result order of select may vary as
"ORDER BY" is not specified, Should we add "ORDER BY" as the table
has multiple rows:
+# Check the synced data on the subscri
Hi,
vignesh C , 15 Mar 2023 Çar, 18:12 tarihinde şunu
yazdı:
> One comment:
> 1) There might be a chance the result order of select may vary as
> "ORDER BY" is not specified, Should we add "ORDER BY" as the table
> has multiple rows:
> +# Check the synced data on the subscriber
> +$result = $nod
Here are some review comments for v15-0001
==
doc/src/sgml/logical-replication.sgml
1.
+ target table. However, logical replication in binary format is
more restrictive,
+ see binary option of
+ CREATE
SUBSCRIPTION
+ for more details.
IMO (and Chat-GPT agrees) the new text should be
On Wed, Mar 15, 2023 at 5:31 PM Amit Kapila wrote:
>
> On Wed, Mar 15, 2023 at 11:52 AM Peter Smith wrote:
> >
> > ==
> > src/backend/replication/logical/tablesync.c
> >
> > 5.
> > +
> > + /*
> > + * If the publisher version is earlier than v14, it COPY command doesn't
> > + * support the bin
On Thu, Mar 16, 2023 2:26 AM Melih Mutlu wrote:
>
> Right, it needs to be ordered. Fixed.
>
Hi,
Thanks for updating the patch. I tested some cases like toast data, combination
of row filter and column lists, and it works well.
Here is a comment:
+# Ensure the COPY command is executed in bina
On Thu, Mar 16, 2023 at 5:59 AM Peter Smith wrote:
>
> On Wed, Mar 15, 2023 at 5:31 PM Amit Kapila wrote:
> >
> > On Wed, Mar 15, 2023 at 11:52 AM Peter Smith wrote:
> > >
> > > ==
> > > src/backend/replication/logical/tablesync.c
> > >
> > > 5.
> > > +
> > > + /*
> > > + * If the publisher
Hi,
Thanks for updating the patch, I think it is a useful feature.
I looked at the v15 patch and the patch looks mostly good to me.
Here are few comments:
1.
+ {
+ appendStringInfo(&cmd, " WITH (FORMAT binary)");
We could use appendStringInfoString here.
2.
+# It should fa
On Wed, Mar 15, 2023 at 3:33 PM Melih Mutlu wrote:
>
> Amit Kapila , 15 Mar 2023 Çar, 12:31 tarihinde şunu
> yazdı:
>>
>> On Tue, Mar 14, 2023 at 4:32 PM Melih Mutlu wrote:
>
>
>>
>> What purpose does this test serve w.r.t this patch? Before checking
>> the sync for different column orders, the
On Wed, Mar 8, 2023, at 11:50 PM, Amit Kapila wrote:
> On Wed, Mar 8, 2023 at 6:17 PM Melih Mutlu wrote:
> >
> > On 7 Mar 2023 Tue at 04:10 Amit Kapila wrote:
> >>
> >> As per what I could read in this thread, most people prefer to use the
> >> existing binary option rather than inventing a new w
On Thu, Mar 16, 2023 at 8:27 AM Euler Taveira wrote:
>
> On Wed, Mar 8, 2023, at 11:50 PM, Amit Kapila wrote:
>
> It is not clear to me which version check you wanted to add because we
> seem to have a binary option in COPY from the time prior to logical
> replication. I feel we need a publisher v
Hi,
Please see the attached v16.
Peter Smith , 16 Mar 2023 Per, 03:03 tarihinde şunu
yazdı:
> Here are some review comments for v15-0001
>
I applied your comments in the updated patch.
shiy.f...@fujitsu.com , 16 Mar 2023 Per, 05:35
tarihinde şunu yazdı:
> On Thu, Mar 16, 2023 2:26 AM Melih Mu
Amit Kapila , 16 Mar 2023 Per, 06:25 tarihinde
şunu yazdı:
> On Thu, Mar 16, 2023 at 8:27 AM Euler Taveira wrote:
> >
> > On Wed, Mar 8, 2023, at 11:50 PM, Amit Kapila wrote:
> >
> > It is not clear to me which version check you wanted to add because we
> > seem to have a binary option in COPY fr
On Thu, Mar 16, 2023 at 6:59 PM Melih Mutlu wrote:
>
> Amit Kapila , 16 Mar 2023 Per, 06:25 tarihinde şunu
> yazdı:
>>
>> On Thu, Mar 16, 2023 at 8:27 AM Euler Taveira wrote:
>> >
>> > On Wed, Mar 8, 2023, at 11:50 PM, Amit Kapila wrote:
>> >
>> > It is not clear to me which version check you wa
On Thu, Mar 16, 2023 at 1:55 PM Amit Kapila wrote:
>
> On Wed, Mar 15, 2023 at 3:33 PM Melih Mutlu wrote:
> >
> > Amit Kapila , 15 Mar 2023 Çar, 12:31 tarihinde
> > şunu yazdı:
> >>
> >> On Tue, Mar 14, 2023 at 4:32 PM Melih Mutlu wrote:
> >
> >
> >>
> >> What purpose does this test serve w.r.t
On Fri, Mar 17, 2023 at 12:20 AM Melih Mutlu wrote:
>
> Hi,
>
> Please see the attached v16.
>
> Peter Smith , 16 Mar 2023 Per, 03:03 tarihinde şunu
> yazdı:
>>
>> Here are some review comments for v15-0001
>
>
> I applied your comments in the updated patch.
Thanks.
I checked patchv16-0001 and
On Thu, Mar 16, 2023 9:20 PM Melih Mutlu wrote:
>
> Hi,
>
> Please see the attached v16.
>
Thanks for updating the patch.
+# Cannot sync due to type mismatch
+$node_subscriber->wait_for_log(qr/ERROR: ( [A-Z0-9]+:)? incorrect binary data
format/);
+# Ensure the COPY command is executed in t
On Fri, Mar 17, 2023 at 6:42 AM Peter Smith wrote:
>
> On Thu, Mar 16, 2023 at 1:55 PM Amit Kapila wrote:
> >
> > On Wed, Mar 15, 2023 at 3:33 PM Melih Mutlu wrote:
> > >
> > > Amit Kapila , 15 Mar 2023 Çar, 12:31 tarihinde
> > > şunu yazdı:
> > >>
> > >> On Tue, Mar 14, 2023 at 4:32 PM Melih M
Hi,
Sharing v17.
Amit Kapila , 17 Mar 2023 Cum, 03:02 tarihinde
şunu yazdı:
> I think to reduce the risk of breakage, let's change the check to
> >=v16. Also, accordingly, update the doc and commit message.
>
Done.
Peter Smith , 17 Mar 2023 Cum, 04:58 tarihinde şunu
yazdı:
> IMO the sentence
On Fri, 17 Mar 2023 at 17:55, Melih Mutlu wrote:
>
> Hi,
>
> Sharing v17.
>
> Amit Kapila , 17 Mar 2023 Cum, 03:02 tarihinde şunu
> yazdı:
>>
>> I think to reduce the risk of breakage, let's change the check to
>> >=v16. Also, accordingly, update the doc and commit message.
>
>
> Done.
>
> Peter
On Fri, Mar 17, 2023 at 11:25 PM Melih Mutlu wrote:
>
> Hi,
>
> Sharing v17.
>
> Amit Kapila , 17 Mar 2023 Cum, 03:02 tarihinde şunu
> yazdı:
>>
>> I think to reduce the risk of breakage, let's change the check to
>> >=v16. Also, accordingly, update the doc and commit message.
>
>
> Done.
>
Here
On Sat, Mar 18, 2023 at 3:11 PM Peter Smith wrote:
>
> On Fri, Mar 17, 2023 at 11:25 PM Melih Mutlu wrote:
> ==
> src/backend/replication/logical/tablesync.c
>
> 2.
> @@ -1168,6 +1170,15 @@ copy_table(Relation rel)
>
> appendStringInfoString(&cmd, ") TO STDOUT");
> }
> +
> + /* The binary
There are a couple of TAP tests where the copy binary is expected to
fail. And when it fails, you do binary=false (changing the format back
to 'text') so the test is then expected to be able to proceed.
I don't know if this happens in practice, but IIUC in theory, if the
timing is extremely bad, t
On Mon, Mar 20, 2023 at 3:37 AM Peter Smith wrote:
>
> There are a couple of TAP tests where the copy binary is expected to
> fail. And when it fails, you do binary=false (changing the format back
> to 'text') so the test is then expected to be able to proceed.
>
> I don't know if this happens in
Dear Melih,
Thank you for updating the patch.
I checked your added description about initial data sync and I think it's OK.
Few minor comments:
01. copy_table
```
+ List *options = NIL;
```
I found a unnecessary blank just after "List". You can remove it and align
definition.
02.
1 - 100 of 113 matches
Mail list logo