On Tue, Jan 18, 2022 at 10:36 AM Greg Nancarrow <gregn4...@gmail.com> wrote: > > On Mon, Jan 17, 2022 at 5:18 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > I've attached an updated patch. Please review it. > > > > Some review comments for the v6 patch:
Thank you for the comments! > > > doc/src/sgml/logical-replication.sgml > > (1) Expanded output > > Since the view output is shown in "expanded output" mode, perhaps the > doc should say that, or alternatively add the following lines prior to > it, to make it clear: > > postgres=# \x > Expanded display is on. I'm not sure it's really necessary. A similar example would be perform.sgml but it doesn't say "\x". > > > (2) Message output in server log > > The actual CONTEXT text now just says "at ..." instead of "with commit > timestamp ...", so the doc needs to be updated as follows: > > BEFORE: > +CONTEXT: processing remote data during "INSERT" for replication > target relation "public.test" in transaction 716 with commit timestamp > 2021-09-29 15:52:45.165754+00 > AFTER: > +CONTEXT: processing remote data during "INSERT" for replication > target relation "public.test" in transaction 716 at 2021-09-29 > 15:52:45.165754+00 Will fix. > > (3) > The wording "the change" doesn't seem right here, so I suggest the > following update: > > BEFORE: > + Skipping the whole transaction includes skipping the change that > may not violate > AFTER: > + Skipping the whole transaction includes skipping changes that may > not violate > > > doc/src/sgml/ref/alter_subscription.sgml Will fix. > > (4) > I have a number of suggested wording improvements: > > BEFORE: > + Skips applying changes of the particular transaction. If incoming data > + violates any constraints the logical replication will stop until it is > + resolved. The resolution can be done either by changing data on the > + subscriber so that it doesn't conflict with incoming change or > by skipping > + the whole transaction. The logical replication worker skips all data > + modification changes within the specified transaction including > the changes > + that may not violate the constraint, so, it should only be used as a > last > + resort. This option has no effect on the transaction that is already > + prepared by enabling <literal>two_phase</literal> on subscriber. > > AFTER: > + Skips applying all changes of the specified transaction. If > incoming data > + violates any constraints, logical replication will stop until it is > + resolved. The resolution can be done either by changing data on the > + subscriber so that it doesn't conflict with incoming change or > by skipping > + the whole transaction. Using the SKIP option, the logical > replication worker skips all data > + modification changes within the specified transaction, including > changes > + that may not violate the constraint, so, it should only be used as a > last > + resort. This option has no effect on transactions that are already > + prepared by enabling <literal>two_phase</literal> on the subscriber. > Will fix. > > (5) > change -> changes > > BEFORE: > + subscriber so that it doesn't conflict with incoming change or > by skipping > AFTER: > + subscriber so that it doesn't conflict with incoming changes or > by skipping Will fix. > > > src/backend/replication/logical/worker.c > > (6) Missing word? > The following should say "worth doing" or "worth it"? > > + * doesn't seem to be worth since it's a very minor case. > WIll fix > > src/test/regress/sql/subscription.sql > > (7) Misleading test case > I think the following test case is misleading and should be removed, > because the "1.1" xid value is only regarded as invalid because "1" is > an invalid xid (and there's already a test case for a "1" xid) - the > fractional part gets thrown away, and doesn't affect the validity > here. > > +ALTER SUBSCRIPTION regress_testsub SKIP (xid = 1.1); > Good point. Will remove. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
