On Friday, March 4, 2022 10:09 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > On Thu, Mar 3, 2022 at 10:02 PM Masahiko Sawada > <sawada.m...@gmail.com> wrote: > > > > > > I'm updating the patches and will submit them. > > Attached updated version patches. Thank you for sharing the patch v3.
Few minor comments. (1) v03-0001, apply_error_callback function - /* append transaction information */ - if (TransactionIdIsNormal(errarg->remote_xid)) + if (errarg->rel == NULL) { - appendStringInfo(&buf, _(" in transaction %u"), errarg->remote_xid); Should write !errarg->rel ? (2) v03-0002, doc/src/sgml/logical-replication.sgml + transaction that conflicts with the existing data. When a conflict produces + an error, it is shown in the subscriber's server logs as follows: +<screen> +ERROR: duplicate key value violates unique constraint "test_pkey" +DETAIL: Key (c)=(1) already exists. +CONTEXT: processing remote data during "INSERT" for replication target relation "public.test" in transaction 725 committed at LSN 0/14BFA88 +</screen> We should update the CONTEXT message by using the v3-0001. (3) v03-0002, doc/src/sgml/logical-replication.sgml + The LSN of the transaction that contains the change violating the constraint and + the replication origin name can be found from those outputs (LSN 0/14C0378 and + replication origin <literal>pg_16395</literal> in the above case). To skip the + transaction, the subscription needs to be disabled temporarily by + <command>ALTER SUBSCRIPTION ... DISABLE</command> first. Then, the transaction + can be skipped by calling the <link linkend="pg-replication-origin-advance"> The LSN(0/14C0378) is not same as the one in the above error context. It's recommended to check LSNs directly written in the documentation. (4) one confirmation We don't have a TAP test of pg_replication_origin_advance() for v3, that utilizes this new log in a logical replication setup. This is because existing tests for this function (in test_decoding) is only for permission check and argument validation, and we're just changing error message itself. Is this correct ? Best Regards, Takamichi Osumi