Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wbwsP-002yCQ-1P for pgsql-hackers@arkaria.postgresql.org; Tue, 23 Jun 2026 08:53:01 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wbwsO-00AgCc-1I for pgsql-hackers@arkaria.postgresql.org; Tue, 23 Jun 2026 08:53:00 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wbwsO-00AgCQ-09 for pgsql-hackers@lists.postgresql.org; Tue, 23 Jun 2026 08:53:00 +0000 Received: from mail-yx1-xb131.google.com ([2607:f8b0:4864:20::b131]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wbwsM-00000001tRr-02aE for pgsql-hackers@lists.postgresql.org; Tue, 23 Jun 2026 08:52:59 +0000 Received: by mail-yx1-xb131.google.com with SMTP id 956f58d0204a3-66043ecf6b3so4920806d50.0 for ; Tue, 23 Jun 2026 01:52:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1782204776; cv=none; d=google.com; s=arc-20240605; b=YMALRgIF/quLk4gj86DK2QsL7AjNnscK4TWsAnC3qYhQoFFeWmBbhcwqq65JsLvH8t tqF8gmIpygib1tZWKRKWDrQ1vS9Wz7vDhPBwl2HkqzTI5IKmW8JKXW7Vrq6wTpdm57N3 gsZWvMYH4K2wwTMLgBGjdhU4l3/CUFaAWN03f/IgtrLNDlDzJxav88t2gAEzmj7pdR6q hKDAAlVwiG9DwVWqrpfe6V4h1sRFpWYmsl9hog8H9u8VVFhn/6hgOWtmhW49D1oQ+S2t jfIYkGaUqEcp9fJWx4/ipxyb8xf6RhNEZ8hvc5pdm9/LsoYSeG9Uss27SlpNiebobEPZ njXg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=GbvbFRbSkNFL84id/JRCJeAiCX4YrOqr8CSL5g2zuAI=; fh=WgjTfsS3BGGollgQnQqrH+JW52O6cpTkMaDhl/LEMXw=; b=LxAim9iNtZyfrzG8gaOQu+VgHJI8VHgHvP30nsTr6hhrRBlPebV5Gn65bOxZDM4TQr ah2gUxQznyqSJBL94CYxJH/jGCYW6HDSh31g3kDzoufATUCqTz6ozF09ELRg3f/P+4zT hyDunQNxGpVuR0q2LTXuVeS/NRtsJBnt2Lp7U+OImxMYJ6UEN+bU0MnygYcTplM5GYHH 2517m+7c2dNtlTFOi7luC5wWMkW5i1FSrI3YCzqUc2yGAEHoeJs4JA2S3h1+GNGhsN5g YVbOP4pZXkvPUnQKPoCkuP9pqCDDBGLBVnlBB9VmJVbhdQqC/roWV/evqKBv9+EV4q7O 9eCw==; darn=lists.postgresql.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1782204776; x=1782809576; darn=lists.postgresql.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=GbvbFRbSkNFL84id/JRCJeAiCX4YrOqr8CSL5g2zuAI=; b=DoudhyOrCY3RlRFjqX4eXZWh2/322ENhl3FRYdAaguHC26EL9RjHZNn0z1gCZgwK1Q fxQgx4kgf0xlQlFO9C25DsXCU+OtiSzuTm7LTH0OkCSMtssJbJ6/t0/eNp4+ASIpeuim A+N/vWI3zIzKfzzcdNVH7UUAnx9SgpdsXtjNJxdVI9+/49jvEN+GwaqjmouLeVjafx53 pMotBN5PlZ1UU55tG2V6g4V6hslMvRDDf1nJyG2HG7XCysIKJnlPgEYHtmuFHpWK+Nb9 peLXsoxLajjdhtd4MmTW1QVsQsb1+xqrwVjPqp82V0NQYGyRMCFU11m3DV/bmVyApjY/ J7YQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782204776; x=1782809576; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=GbvbFRbSkNFL84id/JRCJeAiCX4YrOqr8CSL5g2zuAI=; b=H/GBL2yfMUoRqQUosyhc8AffJYoWjl9D+10yFj7UQHbBSx0anNrlPTSF3L4OuEVCbo Hi/cP559kRzCrXjXyo6+TZWfwYXHkumZ3tQdb4CuXVFUtEVki+QI6oodQACj9ZrXzP2H 8If7hmVvwuCx58S1LWOkgitiOf96WLO1z7scykuBTfGAnei4Mfi2pSrVql1WjNghicFb xFqo7hi16BvKHFQMFlkqdIHhvxh1TB4uut9yLzYKq2V0y6M3azOKFgo88n5jWOjg6GtY U9z10XbBpAYZhQQJgqgW56ls85yZNTlz90NAiUIABmbYFv0PhR4u01qtRMfFbV4/J+Kr YMog== X-Forwarded-Encrypted: i=1; AHgh+RpYJQgpMizo9wm4NS/WaxMJFZbeAUcK97iXoRFoqeyt96buHxntDrG9i8DUvWWiEIpyqvGPhIo8sD1R2y6y@lists.postgresql.org X-Gm-Message-State: AOJu0YyK93qQsiiIpXykie9jL4bCaEr+A+MciV0i7TJ2Uo+9sWMcD2N1 Dwyo4dIK3gnHsvZb89xhBg5UPknH1oQT5XadNJMouUeJ8xSeeIwPHWR0VSiKBMDdIYpzshJ4iO8 Yf2toAq0XRYUW7kJUTRGKlZZ5Z6eDf5g= X-Gm-Gg: AfdE7ckyQ7dNLe8067MliQFtEX0i+skVPWimcRye/8FaF/y3e4+N7nkOYTcUo49pRFS X15YncT/i2np+aw+8F2B+OMUEIm9x9WpldzlJi63+1ITxHIvsjo7WIOZ0uQHCswtRCz120rdMEs NVm09eqDew/U8qWcAY0rAnc0oQlVjTEflaA1dpLCaAhzGncPV7k8lACBsp0iBw0LQ4SraBZkh/8 +JXvUvzaYxVikOtueCfWJbMGeQrnAq5iJ7nHOgEdAmf1EJY29Dxl3WTYHMleNzY5NV3aiJJJAQ= X-Received: by 2002:a05:690e:140b:b0:662:bd61:f80a with SMTP id 956f58d0204a3-6635a37118bmr1633035d50.48.1782204776140; Tue, 23 Jun 2026 01:52:56 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: vignesh C Date: Tue, 23 Jun 2026 14:22:41 +0530 X-Gm-Features: AVVi8CcQtSf7mNdoZIncuo_KSIkzZWDnkVcT9YSIL1jOBcMKdSOV0iNBt0EiIWw Message-ID: Subject: Re: Proposal: Conflict log history table for Logical Replication To: Dilip Kumar Cc: Nisha Moond , Amit Kapila , shveta malik , Peter Smith , Masahiko Sawada , Bharath Rupireddy , PostgreSQL Hackers Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk On Mon, 22 Jun 2026 at 20:52, Dilip Kumar wrote: > > On Mon, Jun 22, 2026 at 4:32=E2=80=AFPM Nisha Moond wrote: > > > > On Sun, Jun 21, 2026 at 7:19=E2=80=AFPM Amit Kapila wrote: > > > > > > On Thu, Jun 18, 2026 at 9:33=E2=80=AFAM shveta malik wrote: > > > > > > > > On Tue, Jun 16, 2026 at 6:54=E2=80=AFPM Dilip Kumar wrote: > > > > > > > > > > IMHO we should just log WARNING and continue the apply worker on > > > > > conflict insertion failure, lets see what other thinks on this. > > > > > > > > > > > > > I have the same opinion. Allowing CLT to block the apply worker wou= ld > > > > be undesirable; CLT is a history/logs collection feature and should > > > > not interrupt core logical replication work. > > > > > > > > > > I think the insert can fail in rare cases like disk getting full whil= e > > > writing WAL or some internal memory ERROR and the ERROR could be > > > persistent which means the LOG will be filled with the same WARNING i= f > > > there are many conflicts. Also, users may not like missing out on > > > conflict information. So, we can ERROR out and let users fix the > > > situation. Additionally, the nested try-catch to downgrade ERROR to > > > WARNING also looks ugly and a source of future bugs and maintenance > > > burden. The attached patch tries to fix this by ERRORing out on > > > insertion failure and attaching the required conflict info as a > > > context of ERROR. The patch also improved the ReportApplyConflict() > > > non-ERROR paths by displaying the conflict information in server LOGs > > > before inserting the same into CLT so that if insertion fails, the > > > complete conflict info can be present in server LOGs. See > > > v52-1-amit.Improve-error-handling-for-conflict-log-table-ins. > > > > > > Additionally, there is another problem with 0003 where when a paralle= l > > > apply worker hits an ERROR-level conflict it logs the conflict to the > > > conflict log table in a new transaction in its error path, after > > > aborting the failed apply transaction. But the leader detects worker > > > failure in pa_wait_for_xact_finish() by waiting on the worker's > > > transaction lock, and AbortOutOfAnyTransaction() releases that lock: > > > the leader unblocks, sees a non-finished state, raises "lost > > > connection to the logical replication parallel apply worker", and > > > tears the worker down -- which can SIGTERM it mid-insert and lose the > > > conflict log row, besides being a misleading message. The attached > > > top-up patch v52-2-amit.fix_parallel_apply_logging fixes that by > > > introducing PARALLEL_TRANS_ERROR state. > > > > I reproduced the above issue and verified the fix for it in > > v52-2-amit.fix_parallel_apply_logging. Here is a TAP test for the > > same. > > The attached top-up patch applies on top of the latest v53-0005 patch. > > > I have merged Amit's patch and Nisha's tap test patch and tested all > the cases discussed, and those are working fine. Few comments: 1) Currently we are storing these in shared memory. Looking at the implementation, these fields are purely worker-private state used to ferry data across the error boundary from prepare_conflict_log_tuple() (inside the PG_TRY block) to ProcessPendingConflictLogTuple() (inside the PG_CATCH block).If it is not required by another process, should it be moved out of shared memory. + /* A conflict log tuple that is prepared but not yet inserted. */ + HeapTuple conflict_log_tuple; + + /* + * Error-context string describing the conflict above, used to annotate any + * error raised while inserting conflict_log_tuple into the conflic= t log + * table. Allocated, like conflict_log_tuple, in ApplyContext. + */ + char *conflict_log_errcontext; 2) Should conflict_log_errcontext be initialized in logicalrep_worker_launch like other members: +++ b/src/include/replication/worker_internal.h @@ -100,6 +100,16 @@ typedef struct LogicalRepWorker */ TransactionId oldest_nonremovable_xid; + /* A conflict log tuple that is prepared but not yet inserted. */ + HeapTuple conflict_log_tuple; + + /* + * Error-context string describing the conflict above, used to annotate any + * error raised while inserting conflict_log_tuple into the conflic= t log + * table. Allocated, like conflict_log_tuple, in ApplyContext. + */ + char *conflict_log_errcontext; 3) Is the usage of heap_insert intentional here, shouldn't it be using generic table access method API table_tuple_insert? +InsertConflictLogTuple(Relation conflictlogrel) +{ + ErrorContextCallback errcallback; + + /* A valid tuple must be prepared and stored in MyLogicalRepWorker.= */ + Assert(MyLogicalRepWorker->conflict_log_tuple !=3D NULL); + + /* + * Set up an error context so that a failure to insert (e.g. the conflict + * log table was dropped, or an out-of-space error) carries informa= tion + * identifying the conflict we were trying to log. + */ + errcallback.callback =3D conflict_log_insert_errcontext; + errcallback.arg =3D MyLogicalRepWorker->conflict_log_errcontext; + errcallback.previous =3D error_context_stack; + error_context_stack =3D &errcallback; + + heap_insert(conflictlogrel, MyLogicalRepWorker->conflict_log_tuple, + GetCurrentCommandId(true), HEAP_INSERT_NO_LOGICAL, NULL); 4) Is the condition remote_commit_ts > 0 done intentionally? + if (remote_commit_ts > 0) + values[attno++] =3D TimestampTzGetDatum(remote_commit_ts); + else + nulls[attno++] =3D true; As I had seen some negative values for certain timestamps. Shouldn't the check be !=3D 0? SELECT extract(epoch FROM '1969-12-31 23:59:59+00'::timestamptz); extract ----------- -1.000000 (1 row) Regards, Vignesh