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 1wV1ad-001cqD-2Q for pgsql-hackers@arkaria.postgresql.org; Thu, 04 Jun 2026 06:30:04 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wV1ab-005AAI-1X for pgsql-hackers@arkaria.postgresql.org; Thu, 04 Jun 2026 06:30:01 +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 1wV1aa-005AA9-2i for pgsql-hackers@lists.postgresql.org; Thu, 04 Jun 2026 06:30:01 +0000 Received: from mail-qt1-x82e.google.com ([2607:f8b0:4864:20::82e]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wV1aY-000000011pu-3l7G for pgsql-hackers@lists.postgresql.org; Thu, 04 Jun 2026 06:29:59 +0000 Received: by mail-qt1-x82e.google.com with SMTP id d75a77b69052e-516e1525aa3so4598761cf.3 for ; Wed, 03 Jun 2026 23:29:58 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1780554598; cv=none; d=google.com; s=arc-20240605; b=QAFe/AJVV5/kW1qIgT52OTTWLikeziOyPVGpJQ/JQ/lzynQrbSla0fikj4aC3pQJtG jf+zB2SOa96vFqmjDDXT+D6os7IXC9HE1MLCrjN8vmFwosB0ayuj1rC2SkG5YQJ3na2L ZT21duPr7uChj73T9f2F1WcT+5KX1epH6gj0cZGkMZUgrChYjxMbgOSAJ1ID+zI6zrPb PK7VgEIUzSoOSZSZjnUDtI7vPUc9vzUGpdqtV15mmdbsDUFpWek17kchNJLU6T6cRGNL jJLdDrsJKIJ4GPQl3txIb4fpb1lvKBWNDtLzryZope/Q3UwsVkFa8z+hLG6eps9JAJ+y +gcQ== 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=aOgS/rZnLvj5a+TCmdbXUnb9KtzkONg6+bVeOPe7Yfk=; fh=WElFtF+483gdFGHbM8W9WS8NIyDfuZYOTWNdLpHUkAE=; b=dskYwHGzgoWM1VUPwWaG2nL37aAFx3d0w4B/kuja+ewxq6sicUjYOBOnMRei/A1un7 /QKtboB+foZybIA/9JJYTN6hwgsIaQ+yPQOnlKFzCTRPgNmbw9gtoAaZVvcUFhLYtsx/ 245gdL6YII3a7v6b/xc8onua1+U3huMLkfA33Q/D1QPykn3lNSF103mlBrPt8vjkY5KS YSxKgcN/fDlpy/RRx3G9P31yW3/H9CntANAhEKR4nV0w+MC45yQGvie5hXJ8bOQxqXAA 31p0KHECaU5wNgAkmZXpP1E6iW9rhiV63JdP7E5S62ECKJZySTen+O/frImFNlx6WzXz cZcw==; 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=1780554598; x=1781159398; 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=aOgS/rZnLvj5a+TCmdbXUnb9KtzkONg6+bVeOPe7Yfk=; b=ELgeJ/hl3I3I6ukDYPvaMkcEunH29wy0ezOuJhYnyezzYmYEJSX7G8EonIxjW29VsK WOB0ro8V0GEAqlDO6ezEWZNOrxwoKX9vkxAc2LC8DA0wNLUyp6xYDobE1dd/IEQE0ZGy zHSmuyogCpYMxDP8qzTcNvI8xMvnKVWBqeV44ep+4pF6FQMNuw4L0wrGL8c5zXXvQ6/C BbpywBUBZEKKW0/h/SW1mB0pAfBqRSviNn2/Ew95zF0v6RBakjcnYi6m9PUVxZDrO2lo aEMox0BdCVJclI5xT3QPWp7EKu9/Ka5/4/54M+91wiwFS5n+I7/5ZlLNwEY4KfCHJr23 nn5A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1780554598; x=1781159398; 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=aOgS/rZnLvj5a+TCmdbXUnb9KtzkONg6+bVeOPe7Yfk=; b=b1YRjE82A3tL/kBTUmTn5pjEPC9ba2aognwcCucjtGJGLeeo7D893o3PEHqPVmq2ys m5uhck/U+iaKmdGIiKmQh0V8SQcDKiNGgfSmaI6z9wywoOep9/xIKXVRvicgxn4BSQM6 n5uW7bRvzZyAWYDjYDDneKdhKv8xdVXuFB3hrECbh9L5mXTlDOKZvLaaDK8gO0mkuYcQ xQFogcaBgbzJ1nGLJl95+vkLamuYaYbN2MN0v95k3RceO3mC2b8bwM4n6ezNSK9GXd37 qLQlj5KVyCcZODIsKcVB2PTvpaRdKQvs7a4Noxq6wAEcohN0dyrtT/w/d6npBMqJWXZU C2DA== X-Forwarded-Encrypted: i=1; AFNElJ9CKUZuYUTnaWUuPy1m1vee/bVeJmTQQDp8owrWGFh+znBheuAzFQPLz4yHEpNlk9cjafIimjixBxQalU8y@lists.postgresql.org X-Gm-Message-State: AOJu0YzGHaNxkWUaIEDpbjCArsMCRfpavQY+8XBk/ho6pM0E618yKuBS 4tza39yS4m7ARd1CcDKQIEjPPDFFrkaH7EHVXkvFi9cuulm8W/U2AP2AMiwBXH9FgWPwzKqzRwf 2RXDQM0lqIEXlswqOnOJCTWFgN/Gby6g= X-Gm-Gg: Acq92OHpo/GiCBxnxtUaaBftXeS53A7igSXxFgVRQCClJfR5G2nvB9vWRLvutRkOoqP yEAFVr7GtjkZ+BdawRbq2KJajCsXNyOzIyHcT6A2L92OmUCnundaB+durMJBu2H+zC4zjymYMyj BE7oe3xtsw9VyK+c7X8YdlYy3GZXPRvEud0pfZ8+ICnqm26WAmsYmjX0OMYW6PFllnQTrdnNqzv kxkAARIkAC1zYpr09QbfwOvQCPNypSEnBwyR2nwSjSG7tnGeFxTKrcNWtvMPd2OcvoJouWIuGcN Lc0+GN08fr2h58t1Sg== X-Received: by 2002:a05:622a:178e:b0:517:57f5:e21a with SMTP id d75a77b69052e-5177878462fmr101744241cf.41.1780554597928; Wed, 03 Jun 2026 23:29:57 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Thu, 4 Jun 2026 16:29:31 +1000 X-Gm-Features: AVHnY4IG6ThXUevNxymjYkENee0__JCEd14ckb7p_nvP9eu1Kx2jUIuJ2fTjCok 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 Some review comments for patch v45-0003. ====== src/backend/catalog/heap.c heap_create: 1. + * Allow creation of conflict table in binary-upgrade mode. /of conflict table/of the conflict log table/ ====== src/backend/commands/subscriptioncmds.c alter_sub_conflictlogdestination: 2. + char relname[NAMEDATALEN]; - relid = create_conflict_log_table(sub->oid, sub->name, sub->owner); - update_relid = true; + snprintf(relname, NAMEDATALEN, "pg_conflict_log_%u", sub->oid); Currently, we are generating the CLT name here and also in create_conflict_log_table. I think there should be only *one* place that does the CLT name generation. e.g. suggest making a new common function like get_conflict_log_table_name(sub->oid, &relname, NAMEDATALEN); ~~~ 3. + relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE); + if (OidIsValid(relid)) + { + /* Existing table from upgrade */ + Assert(IsBinaryUpgrade); + } + else Should this be the other way around? if (IsBinaryUpgrade) { relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE); Assert(OidIsValid(relid)); } ~~~ 4. + /* + * Establish an internal dependency between the conflict log + * table and the subscription. For details refer comments in * CreateSubscription function. */ SUGGESTION Refer to comments in the CreateSubscription function for details. ====== src/bin/pg_dump/pg_dump.c selectDumpableTable: 5. + if (strcmp(tbinfo->dobj.namespace->dobj.name, "pg_conflict") == 0) + { + if (dopt->binary_upgrade) + { + /* + * Dump pg_conflict tables only during binary upgrade. The schema + * is assumed to already exist. + */ + tbinfo->dobj.dump = DUMP_COMPONENT_DEFINITION; + + /* + * Suppress the "ALTER TABLE ... OWNER TO ..." command for this + * table. This prevents pg_dump from outputting the owner change. + */ + tbinfo->rolname = NULL; + } + else + tbinfo->dobj.dump = DUMP_COMPONENT_NONE; Doesn't the comment "Dump pg_conflict tables only during binary upgrade..." really belong above the "if (dopt->binary_upgrade)". ~~~ 6. + if (fout->remoteVersion >= 190000) + appendPQExpBufferStr(query, + " s.subconflictlogrelid, s.subconflictlogdest\n"); else - appendPQExpBufferStr(query, " NULL AS subservername\n"); + appendPQExpBufferStr(query, + " NULL AS subconflictlogrelid, NULL AS subconflictlogdest\n"); It seems more natural to think of the 'subconflictlogdest' before the subconflictlogrelid. Perhaps the SQL should swap those around. ~~~ 7. + if (PQgetisnull(res, i, i_subconflictlogrelid)) + subinfo[i].subconflictlogrelid = InvalidOid; + else + { + TableInfo *tableInfo; + + subinfo[i].subconflictlogrelid = + atooid(PQgetvalue(res, i, i_subconflictlogrelid)); + + if (subinfo[i].subconflictlogrelid) + { + tableInfo = findTableByOid(subinfo[i].subconflictlogrelid); + if (!tableInfo) + pg_fatal("could not find conflict log table with OID %u", + subinfo[i].subconflictlogrelid); + + addObjectDependency(&subinfo[i].dobj, tableInfo->dobj.dumpId); + } + } + + if (PQgetisnull(res, i, i_sublogdestination)) + subinfo[i].subconflictlogdest = NULL; + else + subinfo[i].subconflictlogdest = + pg_strdup(PQgetvalue(res, i, i_sublogdestination)); + 7a. Those new attributes come as a pair -- e.g. the relid only makes sense depending on the destination value; maybe only look for the CLT relid is the destination is not LOG/NULL Knowing the destination also means you can also do integrity checks for PQgetisnull(res, i, i_subconflictlogrelid) -- e.g. must be valid Oid, or must be invalid Oid. ~ 7b. The 'tableInfo' can be declared/assigned in the if block. ~~~ dumpSubscription: 8. + if (subinfo->subconflictlogdest && + (pg_strcasecmp(subinfo->subconflictlogdest, "log") != 0)) + appendPQExpBuffer(query, + "\n\nALTER SUBSCRIPTION %s SET (conflict_log_destination = %s);\n", + qsubname, + subinfo->subconflictlogdest); + 8a. Why was this implemented as a separate ALTER SUBSCRIPTION instead of just another CREATE SUBSCRIPTION option like all the others? If there is some good reason, these should be a comment here. ~~~ 8b. Not sure why this has a double \n\n prefix instead of just \n. I didn't see that pattern elsewhere in this file. ====== Kind Regards, Peter Smith. Fujitsu Australia