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 1vfqL1-005QOn-2j for pgsql-hackers@arkaria.postgresql.org; Wed, 14 Jan 2026 02:10:24 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vfqL1-007SgX-0B for pgsql-hackers@arkaria.postgresql.org; Wed, 14 Jan 2026 02:10:23 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vfqL0-007SgP-1x for pgsql-hackers@lists.postgresql.org; Wed, 14 Jan 2026 02:10:22 +0000 Received: from mail-qt1-x833.google.com ([2607:f8b0:4864:20::833]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vfqKy-000IjQ-1M for pgsql-hackers@lists.postgresql.org; Wed, 14 Jan 2026 02:10:21 +0000 Received: by mail-qt1-x833.google.com with SMTP id d75a77b69052e-50145cede6eso6192551cf.2 for ; Tue, 13 Jan 2026 18:10:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1768356620; x=1768961420; 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=iizOqnLkuRBSHxZQYzR7zvPfOoYF9qZI3h4wH11iCeI=; b=L3JwZu4pftuKb1VGAdAmuY63S/0KpObEvD3SAehvGxq/83+ObFO3FRBQpVkjBlTM5s nKy+iOU3YeDDFtLJdhAgqOQI93wtTfoNjgY9AsSKFR12Gpq78T4Gx3VWRyfwkCPeMnJf rYd9Fl1x+WhZUYrYok+VG8lxnht/My9JCZAN59uZOzmut7sf4BqAuTp8VkeEjljqS9ev Y3fg1mYXhdDhqu1kQb1A9N8DyE4AG08BcOKn4RxBMPt9OG8qdO2iWHMk6Mqa41aWzNXG 5H0mOUv3nrrbiaiCKqFuncd1zUxbWmLXebLsBIC1lt9MBmJ4rmFzX8Z9LR3Xo0tPapTM nYwg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1768356620; x=1768961420; 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=iizOqnLkuRBSHxZQYzR7zvPfOoYF9qZI3h4wH11iCeI=; b=WRzgFS/ok08qotE9LbHwkJUHDPtpYlB2ZI6oIqFCWMm6J2LgZc7GJHySCp2+mwx4qf +IpHL5IN/REdtdVhu9NbIClyL/6CBSqwI/toZ6VcNR5SnXNNK/R3Jbz2tqHYau9ZmgB+ fzphjopWb66jkqe7Z0m9icvqIAsgTWgP1o2bmtoWvLth8/S/BM4bRlxrzQbYepch6jm6 9aPyAAffxMAeBPc6LPwyCex4/TuZpwUxVZ2740lH+OMPAx1lzfeBBajMGUamM8lrMK41 nttSFVBy6WSjoFhVY94Fnzd/McXVfLl6y8krlqqczNmNjXfIjcHGgsL33/h+w6aqHzAK cJqA== X-Forwarded-Encrypted: i=1; AJvYcCWhepsnuuOCYbhDhndqOzprnfRk3qOrKzKuc2mACk7UyeqhrAwkJEG6i/gVw7R1R0dRayc/nMl6YfWDaHTh@lists.postgresql.org X-Gm-Message-State: AOJu0YwRLWunExDswWmiS1y4J2Sq/cUFXC6MYvAu7VWW+rKh1R5SonTH HR9DDkQ0rEiRSdvOz+im3aO6He1/mNgFryDF+aV2YjD6dsFuWCFxEwEkLU9/5gCB1FlhVjcgq9L TmpGdMFCZRRta1B/iPxbKaf2sEZZD6Pk= X-Gm-Gg: AY/fxX4ot+dgvM3f+CpwRmgYe1IdNYMHWYflUshgDl/gIf427DWdHUhEN+CgZJkOdy0 ab1Wb9CXbfbLaELqftZ1FuzSALYD1fpi+rvbDqrcQA4dq01B75InKq53CjehDOsMeAPT8LbhjLU e/M7CEj3LoK043XtS6U4hI/HYkQOqHyU4eSNCfA6WgLcuwufpAgGWMM5UHS2pIOzLi17ucU9soP bb91lppyZprgOawghPpun5sj9BnLBNq21c80Ig+P2lVvsMt2YmjQP5eHY5ilnCNwGfVl7taDUMl 3Fssfl5xDkTGl4X1cuiDEesUlvNF3g== X-Received: by 2002:ac8:5e4a:0:b0:4ee:1563:2837 with SMTP id d75a77b69052e-50148495945mr17400421cf.67.1768356620274; Tue, 13 Jan 2026 18:10:20 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Wed, 14 Jan 2026 13:09:53 +1100 X-Gm-Features: AZwV_QjzmmYZrk10rbfzpaVHjEBsLA5ZI6e62DRSjn0zd8UdlTa8Ki06rH_ybds Message-ID: Subject: Re: Proposal: Conflict log history table for Logical Replication To: Dilip Kumar Cc: vignesh C , shveta malik , Amit Kapila , 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 Tue, Jan 13, 2026 at 9:28=E2=80=AFPM Dilip Kumar = wrote: > ... > I am not sure does it makes sense to check the tuple from clt for all > the test cases in 035_conflict.pl or just checking in a few of them is > sufficient? Thoughts? > I felt just having some was OK, not everything. ////////// Some review comments for patch v21-0002. =3D=3D=3D=3D=3D=3D Commit message 1. Handling Multi-row Conflicts: A single remote tuple may conflict with multi= ple local tuples (e.g., in the case of multiple_unique_conflicts). To handle th= is, the infrastructure creates a single row in the conflict log table for each remote tuple. The details of all conflicting local rows are aggregated into= a single JSON array in the local_conflicts column. The JSON array uses the following structured format: [ { "xid": "1001", "commit_ts": "2025-12-25 10:00:00+05:30", "origin": "nod= e_1", "key": {"id": 1}, "tuple": {"id": 1, "val": "old_data"} }, ... ] ~ As mentioned in the patch 0001 review comments, there is some ambiguity regarding JSON versus JSONB. Here, the commit message is calling it a "JSON array"; later in the patch, the same field is called a "JSONB array". Also, the code refers to type JSONOID in places where I thought maybe it should be JSONBOID (assuming the JSONB array comment was right). So, which one is it meant to be? All the code/comments need to be consistent. Meanwhile, according to my AI... "Only use JSON if you absolutely need to preserve exact formatting or if your workload is heavily write-biased with almost no reads." Since the CLT is basically write-only, and human-readable would be nice too, maybe it's JSON that we want here. =3D=3D=3D=3D=3D=3D src/backend/replication/logical/conflict.c 2. + /* + * Get both the conflict log destination and the opened conflict log + * relation for insertion. + */ + conflictlogrel =3D GetConflictLogDestAndTable(&dest); It' not obvious from that comment that the conflictlogrel could be NULL her= e. SUGGESTION Get the conflict log destination. Also, (if there is one) return the CLT relation already opened and ready for insertion. ~~~ 3. + errdetail("Conflict details logged to internal table with OID %u.", + MySubscription->conflictlogrelid)); Instead of "internal table", it should be called the "conflict log table". Also, why not give the real name of the CLT instead of making the user have to figure it out from the relid? SUGGESTION "Conflict details are logged to the conflict log table: %s" (It might be nicer if you have a common util function for returning the CLT name "pg_conflict_%u" given the subid) ~~~ 4. +/* + * GetConflictLogDestAndTable + * + * Fetches conflict logging metadata from the cached MySubscription pointe= r. + * Sets the destination enum in *log_dest and, if applicable, opens and + * returns the relation handle for the internal log table. + */ +Relation +GetConflictLogDestAndTable(ConflictLogDest *log_dest) /for the internal log table./for the conflict log table./ =3D=3D=3D=3D=3D=3D src/test/subscription/t/035_conflicts.pl 5. +my $json_query =3D qq[ + SELECT string_agg((unnested.j::json)->'key'->>'a', ',') + FROM ( + SELECT unnest(local_conflicts) AS j + FROM $clt + ) AS unnested; +]; + +my $all_keys =3D $node_subscriber->safe_psql('postgres', $json_query); + +# Verify that '2' is present in the resulting string +like($all_keys, qr/\b2\b/, 'Verified that key 2 exists in the local_conflicts log'); I noticed you are jumping through some hoops to extract fields from the JSON before checking their content. If the 'local_conflicts' really is stored as human-readable JSON (not JSONB), then maybe that extraction work is not necessary. e.g. consider just matching against a portion of the JSON string, instead of having to unnest/extract anything at all. =3D=3D=3D=3D=3D=3D Kind Regards, Peter Smith. Fujitsu Australia