Hi, Attached v11.
vignesh C <vignes...@gmail.com>, 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. Even if required just create > for one of the tables: > I think that having a primary key in tables for logical replication tests is good for checking if log. rep. duplicates any row. Other tests also have primary keys in almost all tables. Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com>, 28 Şub 2023 Sal, 15:27 tarihinde şunu yazdı: > 1. > + <para> > + If true, initial data synchronization will be performed in binary > format > + </para></entry> > It's not just the initial table sync right? The table sync can happen > at any other point of time when ALTER SUBSCRIPTION ... REFRESH > PUBLICATION WITH (copy = true) is run. > How about - "If true, the subscriber requests publication for > pre-existing data in binary format"? > I changed it as you suggested. I sometimes feel like the phrase "initial sync" is used for initial sync of a table, not a subscription. Or table syncs triggered by ALTER SUBSCRIPTION are ignored in some places where "initial sync" is used. 2. > Perhaps, this should cover the recommended cases for enabling this new > option - something like below (may not need to have exact wording, but > the recommended cases?): > "It is recommended to enable this option only when 1) the column data > types have appropriate binary send/receive functions, 2) not > replicating between different major versions or different platforms, > 3) both publisher and subscriber tables have the exact same column > types (not when replicating from smallint to int or numeric to int8 > and so on), 4) both publisher and subscriber supports COPY with binary > option, otherwise the table copy can fail." > I added a line stating that binary format is less portable across machine architectures and versions as stated in COPY [1]. I don't think we should add line saying "recommended", but state the restrictions clearly instead. It's also similar in COPY docs as well. I think the explanation now covers all your points, right? Jelte Fennema <postg...@jeltef.nl>, 28 Şub 2023 Sal, 16:25 tarihinde şunu yazdı: > > 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 BINARY is enabled could be to trigger one > of the failure cases. Yes, there is already a failure case for binary copy which resolves with swithcing binary_copy to false. But I also added checks for publisher logs now too. [1] https://www.postgresql.org/docs/devel/sql-copy.html Thanks, -- Melih Mutlu Microsoft
v11-0001-Allow-logical-replication-to-copy-table-in-binary.patch
Description: Binary data
