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 1wVKJW-001snT-0D for pgsql-hackers@arkaria.postgresql.org; Fri, 05 Jun 2026 02:29:38 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wVKJU-009dHm-1n for pgsql-hackers@arkaria.postgresql.org; Fri, 05 Jun 2026 02:29:36 +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 1wVKJU-009dHe-04 for pgsql-hackers@lists.postgresql.org; Fri, 05 Jun 2026 02:29:36 +0000 Received: from mail-qt1-x82d.google.com ([2607:f8b0:4864:20::82d]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wVKJS-00000001ALf-05th for pgsql-hackers@lists.postgresql.org; Fri, 05 Jun 2026 02:29:35 +0000 Received: by mail-qt1-x82d.google.com with SMTP id d75a77b69052e-5178a42caa3so12434891cf.1 for ; Thu, 04 Jun 2026 19:29:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1780626571; cv=none; d=google.com; s=arc-20240605; b=Fs+xfaj/zFVeUSabBNeZxahRrR0ubz/t/TRjXk6l7VvymiI/Sqd0hWmJV1kRiElJ7H iD3ZAysAiMNU0D49CBYxBj8+ImSzuA5NcoY2PXY3tPCZaJOE5gNTs8e+Qn7Yk+sFETBS HH+LurZbNDcNChtPutkJAKZsTLSrfICHBIwLrkYiQYDmKdYuV9CNgOXkNScW/SdRt5HB PaZ1OMjDIDu0g+QYzXsGFfLbLSF/rmIaXZ/v2WjSClvGBCXLSWob7j/StWhJkLqyn6fw 5qiL+uIxkLd94dh4YXKhriQ6y6kZdM5SFBkYBpYEmPpS/8/Pl/ksCV39Q3iaPyAVd6bf vrgw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=F4I3aB+OEcIWhDjbqWPZjB15SyQvRa7InWoPgvm7Pgo=; fh=NLFbHh7DpKTMLsVcC4lUXbKjUVVGXtv7P4xnhNO2+vQ=; b=CM0sWsaa9MMP4GDO4XM5I+1VkqcJ6Sztfk0DaHgG5hZGuo15gFaSleK8EDa8NU4gYx apM+JR157pmRs/kmuGM+A4agLaEKcw/iOHPDdodlI78eGKWCultCXkNYYqF3KBkI57Xr SH5BxmdHDOzKFHfvOPpYOa5bRW9qPWzj+u0wLkdnTziyh+YHdC6+4GEni7e0DjEOQ5Dn 8htGorhK6vIzVi0MwxqZH6oOBTptHju2eGgyUtGGUJ9tkfNpbz22qcU6DjAzp2JQxUMB fqG39Dd+E8QfSaLGpbZD/9YVQ2lWzUfbIjHvRtikz1Se38OCS+DCfCxnJPqd/ZQ/Z0Ba b6Xg==; 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=1780626571; x=1781231371; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=F4I3aB+OEcIWhDjbqWPZjB15SyQvRa7InWoPgvm7Pgo=; b=imhOtte5sYovfsDxlN4j8XqWYe0z/NT2EiPFDD37XwPYbQCv2yz5F3DPg69RpnxPtl jBQ4LgF0aqHLDgyU6kjdVhH/DMs8uNhL3xlAOq1O9DSngPAYI0wXJcxKTceor/hyiVpr J//D0ugUA1K081POd+wgr94mXepzDFkXjQ/Qa2WakBwey79bi62eAhPK1DjymjfjOQsS O2kSe1Yhj5HakHAQ1hF1B3Oze8EYXthbjnaeprxRGKQgCKIOQIrJhykpJt44xr8tWF3v YJeWzp0QEZrkjXXL7/t/gJ5blZmZi6d6lhtT+CaK4GY/uAXB2nfp95ZFOctWLj4ZOGt5 rCcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1780626571; x=1781231371; h=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=F4I3aB+OEcIWhDjbqWPZjB15SyQvRa7InWoPgvm7Pgo=; b=JXlPq+lZjsED3N9ElGTcjl4dHCeBINB45IQ/39ibGH71qwmelgswNTzCGVETRN03Mq lHLCLSpODTJR9tv6UIISLjYcETrBdF346oc2gOXAP7kBR4ImFCKwwKaErMknt24QVZO8 w37TEOjL5F3O0h1QOtGqEmof4Nmt6xHWPjrruWZ2G2+HsUmbWh3G+7Lo8wJL4SkC64zZ 79WpsSzRuU9UmPvk80pAjJMQ821qGcoHB3b7eiPoESe4tpzYLDgSW1pPQyqao3Q68Iuu OAXKVEbB5diXdRV32cxIxYF30XWtK9/Bd2MFc/+HmHSJ2aiZjmG0hjQsiSB0w0douesj g8+w== X-Forwarded-Encrypted: i=1; AFNElJ8g8S5JfWGiWlFTzVp3Kn4ukmqr/nWlVrG1kIWN2OxYUaweOLMVAdABfs5Z2Pebj4it5ezv+FvjgghZCDpw@lists.postgresql.org X-Gm-Message-State: AOJu0YxHdyP93n9uBq70NoUhKhD1mAqRsleUORrLwX/y8Aw/mseLankL GjquOpnhxGDQNdrUapYqg47gmbVvOyHTHlhLnc6tMX1mwIkieK7aTZEcAEPSQ4+1xw/LbUOrd6I 9yygWel3Fo2KdX+ZgFJBKl+RzI8eXM2Y= X-Gm-Gg: Acq92OGaKP9R7OhygKFX/l4NGAjctqU+Pn8vwRFkqUurHpnp1DHIO3IObaaG62rGrPt FdD0KUs89SGNtWKzPGEWy1ASvGhk1+dfLOXKqIVq/JtQIyf4G1aAejcyE+tqGB/XMyN3kB0vXKz xr4RbZdS71vt2ZfwO/sYS98fvV7Hz5EY0lk2OuVCDHGjCPCg+69CxTu5tzBhGnSB+MxJ1lh+Irs zg9ToSVOv3gqbsD/pb+ubX634rwQ1kyua5fW5kKPPiQSHFwQ8XBTMTeqZ2bntYr/FQw8B+Bz/gD d6aaNrlXqXpitWkFUA== X-Received: by 2002:a05:622a:c15:b0:517:87a9:6a89 with SMTP id d75a77b69052e-51798667679mr8202601cf.6.1780626571430; Thu, 04 Jun 2026 19:29:31 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Fri, 5 Jun 2026 12:29:04 +1000 X-Gm-Features: AVHnY4JXzf3gLxcqNYfz02j_Oh4rHp2HSjU1n9L8gVQjTD1Sjk5dOEi8WBYO8xY Message-ID: Subject: Re: Proposal: Conflict log history table for Logical Replication To: vignesh C Cc: Dilip Kumar , Nisha Moond , Amit Kapila , shveta malik , Masahiko Sawada , Bharath Rupireddy , PostgreSQL Hackers , shveta malik Content-Type: text/plain; charset="UTF-8" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk Hi Vignesh. Some review comments for the patch v45-0004. ====== src/bin/psql/describe.c describeSubscriptions: 1. + if (pset.sversion >= 190000) + { + appendPQExpBuffer(&buf, + ", (select srvname from pg_foreign_server where oid=subserver) AS \"%s\"\n", + gettext_noop("Server")); - /* Skip LSN is only supported in v15 and higher */ - if (pset.sversion >= 150000) - appendPQExpBuffer(&buf, - ", subskiplsn AS \"%s\"\n", - gettext_noop("Skip LSN")); + appendPQExpBuffer(&buf, + ", subretaindeadtuples AS \"%s\"\n", + gettext_noop("Retain dead tuples")); appendPQExpBuffer(&buf, - ", pg_catalog.obj_description(oid, 'pg_subscription') AS \"%s\"\n", - gettext_noop("Description")); + ", submaxretention AS \"%s\"\n", + gettext_noop("Max retention duration")); + + appendPQExpBuffer(&buf, + ", subretentionactive AS \"%s\"\n", + gettext_noop("Retention active")); + + ncols += 4; + } These can all be combined together, so there is just a single appendPQExpBuffer. Same as other single code fragments in the same function. ~~~ 2. + appendPQExpBuffer(&buf, + ", pg_catalog.obj_description(oid, 'pg_subscription') AS \"%s\"\n", + gettext_noop("Description")); + ncols++; + + /* Conflict log destination is supported in v19 and higher */ + if (pset.sversion >= 190000) + { + appendPQExpBuffer(&buf, + ", subconflictlogdest AS \"%s\"\n", + gettext_noop("Conflict log destination")); + ncols++; I think the "Description" should remain as the very last column of the table, same as it always has been. All those others are options so they belong together, but description is different. ~~~ 3. + printTableAddHeader(&cont, gettext_noop("Description"), true, align); + printTableAddCell(&cont, PQgetvalue(res, i, current_col++), false, false); + + if (pset.sversion >= 190000) + { + char *logdest; + + printTableAddHeader(&cont, gettext_noop("Conflict log destination"), + true, align); Ditto. Leave the description as the very last column. ~~~ 4. + if (pset.sversion >= 190000) + { + char *logdest; + + printTableAddHeader(&cont, gettext_noop("Conflict log destination"), + true, align); + + logdest = PQgetvalue(res, i, current_col++); + + printTableAddCell(&cont, logdest, false, false); + + if (strcmp(logdest, "table") == 0 || + strcmp(logdest, "all") == 0) + { + char conflictlogtable[NAMEDATALEN + 32]; + + snprintf(conflictlogtable, + sizeof(conflictlogtable), + "%s.pg_conflict_log_for_subid_%s", + conflict_schema, subid); + + printTableAddFooter(&cont, _("Conflict log table:")); + printTableAddFooter(&cont, psprintf(" %s", conflictlogtable)); + } + } + 4a. This 'snprintf' is yet another code fragment that is building the generated CLT name. This should be using some common function to do it. See my review comment for an earlier patch in this set [1, comment #2]. ~~~ 4b. + printTableAddFooter(&cont, _("Conflict log table:")); + printTableAddFooter(&cont, psprintf(" %s", conflictlogtable)); Normally in these describe commands, the footer part (e.g. "Except tables:" etc) puts the names in quotes. So, even though it may not be strictly necessary, this should do the same for consistency: e.g. BEFORE Conflict log table: pg_conflict.pg_conflict_log_for_subid_16392 AFTER Conflict log table: "pg_conflict.pg_conflict_log_for_subid_16392" ~~~ 4c. IMO there should be a separate function for handling the subscription footer/s, same as there is already a function addFooterToPublicationDesc. ====== src/test/regress/expected/subscription.out Some general comments about the describe testing: 5. AFAICT there this patch is not being tested properly because there are no tests where the conflict_log_destination is "table" or "all". ~~~ 6. Even though the \dRs code was changed a lot, it seems there were no impacted tests because they were all using \dRs+ and never \dRs. So I think there needs to be more "describe" tests that are using *both* forms of this command. Also consider using expanded form \x for some of the \dRs and \dRs+ tests, so that the expected output is easier to read. ====== [1] https://www.postgresql.org/message-id/CAHut%2BPt3%3DrZO%2ByJqj7o3xH0LcgrztFCvhoayiBJWbhmEio6teQ%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia