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 1vljgN-003CWi-05 for pgsql-hackers@arkaria.postgresql.org; Fri, 30 Jan 2026 08:16:47 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vljgK-003hh8-37 for pgsql-hackers@arkaria.postgresql.org; Fri, 30 Jan 2026 08:16:45 +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 1vljgK-003hh0-23 for pgsql-hackers@lists.postgresql.org; Fri, 30 Jan 2026 08:16:45 +0000 Received: from mail-qt1-x82c.google.com ([2607:f8b0:4864:20::82c]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1vljgJ-00000000A8t-0uNG for pgsql-hackers@lists.postgresql.org; Fri, 30 Jan 2026 08:16:45 +0000 Received: by mail-qt1-x82c.google.com with SMTP id d75a77b69052e-5033a2c4b81so21187101cf.0 for ; Fri, 30 Jan 2026 00:16:42 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1769761001; cv=none; d=google.com; s=arc-20240605; b=KR5Y83WMtYNaOHWwi9k4b4PBs3uxT+6HOLTDcPQVhAs91nhdZpyz8Gpp+JDtXc2+yM vP+P4tjaLvoZS8bahWiDJ6vb8aG3VIt7Wtd9pMDOWI9tlJzsmzONfvwK7KWcVMZ/aWjw XlDvps+6JzRVq6hj9FASmZ+w8rpXaZjqWYJWqt76MYFKK3CNjOG+IfRT/7hzRIdbaE/2 wVuPH0P+XbQx0O+GUIrlgBhUo8LxdjJQYXUJzT0s3Ui2lTP8bDryyBe7Twc9FglOLqUi IBaP7Ygpb6b55zkdv56VXM8sm7UmXQVKdU67mFErWEjt4rndHzEGrqfIhbcS+uas7k/v 0cSw== 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=+juW+bg2fX/nnSw+A74Oq59wHvLsg/pXuA2Gg2Khnh0=; fh=4mNPF+QkhFZduAsj6Mg8MPH19wQqyE+VjryG5aB4Fs8=; b=N3K2fpXIbpfQLZdRwqW75ek8uKsq2Zjhu3I6De9CdlEEAxWLNRSbT1b0jz8+QdN4Fy UQF0XfgvceX4Z0LGBU63h72gZNyPNksy1rTFJ7KlmqgoVZoAJI1HZzq9rZ6r29P7L4ML BKQXQpF4FgMcoDG1csojc06ZnqpOsp9aZG1MXejuDNQdNwn+US+icvQfAnPr5DMFgaez BLuJIpQtwVfv2XSeuPPjo3S2Z5D03e82Iz2CoeFgXJNVUyUyicwIcJQNLfW0lFApK0bH PqozAVia+mtgXmlV5KNSNKsjL5Qk3MXIlwnVjnIgIY6KG1jaMZo8pxNJx2dWZ9Kgoj1j DKmw==; 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=20230601; t=1769761001; x=1770365801; 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=+juW+bg2fX/nnSw+A74Oq59wHvLsg/pXuA2Gg2Khnh0=; b=I1kbTEQ8mMJkD9icIiJO2iAPpRerQnQEpSLHeKIadsjjdORHnxZp0V7tmTCK51kARM x3RnXmHJ4p0XWyzCMm4qU0RGf28kePfmZPp6xM6Ac6L3/1pwDbRYjpMqBxEAdXwpK8jl +O3aCV/w/7Nv/8nfR6q36bcMcpjXHIPAC1gO85y7VeESHG81BCN8Wxlt95tmjBxoZcBj bqeYYGS6a12Q0CKz8SbrxX2YW3TjMtxq+rNQX0n2e5PVs6zMiqWHs+qkzJWk/q1beGBO 5OxBLSwO2a4uS8oYtkaNWq3rm4fkAPydvx0M14xLUjaETSfhz8jGbhPUg+GJKP4H1jm6 zWyw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1769761001; x=1770365801; 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=+juW+bg2fX/nnSw+A74Oq59wHvLsg/pXuA2Gg2Khnh0=; b=h0uhTX8iBX8KJj7yZfXuha9/s45iic+UTfAW5UGe9R3Lo+9Y6g4kosROhuIim0jokn KI4BkjtokqTzzzEAWyFP59oMkEtyJvmRhdpOhaAEbEawB7ciyhHjzEPArEMNvTFsuofK aXhe63WcYdEaQeKbAKzMXgAYITd3+LljWegcqDNM3u3Z8DCBw5LgwVS0i6qGRnyBizWz mxFv95qcwtOghQ2J0hk3Z0xLM1z9nXhW5225DXtQ+gir0i7dloUW583EkQ18lqUffIkd 9zzdglpGURyJFP2cZGnJ3Soxg7NwxBwH0sIjb5fmKb5pdW/I5Aab+l9rJaj+M48lp550 iyGw== X-Forwarded-Encrypted: i=1; AJvYcCVRE6n0eKj4Y9KGm9r7BwHobh4Gb0dR9eKXlL+D2HiIeq/xDnIMgQaXHT8ECm/77qPJYrV/66Fk2fSnqXIK@lists.postgresql.org X-Gm-Message-State: AOJu0YxgLMKQ/XPpphJJgOGbCaHAQcjaScuCsOyGpJAs51Z92N+AdYS1 Wm7FmttEBgkrT5WWWD1Usts1MSUZlXUgruRqHgl29u6jBZFJwE0DgotG3xRNVOsfFT41m4+5x9L Ylwg6DyWkiSM6mNZXlB/4/3gPz824xHk= X-Gm-Gg: AZuq6aJHzO9TGZ1aH7lRQAPQO8m7oYIHiHKZTK1e4noIMzHNTBA5uVEkFUfGqKqenAn 4r9PsmxVKE+1fRufCzLNxqzZfDRMbmhbsX9xcLI0u3RJjwdPvOlaoPar9l66+hX2oulNp4kIWx/ Q1RwJpYbVbw/kG7Vqiv31f7/blmIKTMh7pxIEdOUhZRJEnjbjYmsBHww3ahIccPoBBoadIGvCPT 6iYiSzbPczlUWEk3xgyj/XB3KbhjEbXwrGzAoMdntjSmaJstUhItFdmT8hwv2mkHtDmqOaUPVJ0 io5j97bxG+Khi5dio0jZVGxvvWUxVA== X-Received: by 2002:ac8:5dca:0:b0:501:b1be:c31b with SMTP id d75a77b69052e-505d217a4f0mr25174641cf.14.1769761000673; Fri, 30 Jan 2026 00:16:40 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Fri, 30 Jan 2026 19:16:13 +1100 X-Gm-Features: AZwV_QhTodv0IfH5bcmQEelAwoH-NhP784h6zgd86EieWqMAUbp1aaPveNm2qqQ Message-ID: Subject: Re: Skipping schema changes in publication To: vignesh C Cc: shveta malik , Dilip Kumar , Amit Kapila , Shlok Kyal , "Zhijie Hou (Fujitsu)" , YeXiu <1518981153@qq.com>, Ian Lawrence Barwick , Bharath Rupireddy , PostgreSQL Hackers 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 v38-0001. ====== 1. General. Patch structure This patch set structure seems muddled to me; I think patch 0001 requires proper handling for EXCEPT (partitions), otherwise, it cannot be independently committed. It's my understanding that the goal is to try approach #1, but if that proves too difficult, then the fallback would be approach #3. Yet AFAICT this patch 0001 is neither -- I have no idea anymore what patch 0001 does for partitions; IIUC it looks like just old partition logic from a few versions back (???). Personally, I felt it would be better to combine 0001 + 0002 (approach #3), then 0002 would be a patch that *replaces* approach #3 logic with approach #1 logic. That way, 0001 is self-contained, and 0002 is an evolution of the feature. OTOH if the plan is that 0001 and (one of the) 0002 *must* be merged prior to commit, then I would have expected that the 0001 has no EXCEPT(partition) logic and not tests at all -- just maybe a bunch of placeholders that patch 0002 would flesh out. Anyway, the following review comments are based on the patch set as it currently stands. ~~~ 2. Still unaddressed comments for 3 weeks? It seems most (all?) of my review comments from 8/January remain unaddressed in this 0001 patch. Multiple versions have come and gone, but the following review comments are still pending, according to my records: 8/1 #1 (code review) 8/1 #3 (code review) 8/1 #1 (internal review) 8/1 #2a (internal review) 8/1 #2b (internal review) 8/1 #C2 (cosmetic review) 8/1 #C3 (cosmetic review) 8/1 #C4 (cosmetic review) etc.. ====== Commit Message 3. The message needs to explain what this patch is doing regarding partition tables and partitions. Currently, I have no idea. It seems just old logic that is neither approach #1 nor approach #3. ====== doc/src/sgml/ref/create_publication.sgml 4. + + For partitioned tables, when publish_via_partition_root + is set to true, specifying a root partitioned table in + EXCEPT TABLE excludes it and all its partitions from + replication. Specifying a leaf partition has no effect, as its changes are + still replicated via the root partitioned table. When + publish_via_partition_root is set to + false, specifying a root partitioned table has no + effect, as changes are replicated via the leaf partitions. Specifying a + leaf partition excludes only that partition from replication. The optional + * has no meaning for partitioned tables. + 4a. Huh? This is not at all my understanding of how either approach #1 or approach #3 would work. ~ 4b. I thought the patch set is currently arranged to handle partitions in the alternate patch 0002s. So, I expected this doc fragment to be more like: For partitioned tables, TBA. Later, your subsequent patch 0002 should replace this "TBA" part according to approach #1 or #3 implementation. ====== src/backend/catalog/pg_publication.c publication_add_relation: 5. + /* + * Handle the case where a partition is excluded by EXCEPT TABLE while + * publish_via_partition_root = true. + */ + if (pub->alltables && pub->pubviaroot && pri->except && + targetrel->rd_rel->relispartition) + ereport(WARNING, + (errmsg("partition \"%s\" might be replicated as publish_via_partition_root is \"%s\"", + RelationGetRelationName(targetrel), "true"))); + + /* + * Handle the case where a partitioned table is excluded by EXCEPT TABLE + * while publish_via_partition_root = false. + */ + if (pub->alltables && !pub->pubviaroot && pri->except && + targetrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + ereport(WARNING, + (errmsg("partitioned table \"%s\" might be replicated as publish_via_partition_root is \"%s\"", + RelationGetRelationName(targetrel), "false"))); + Huh? IIUC, this is neither approach #1 nor approach #3. ====== src/backend/replication/pgoutput/pgoutput.c get_rel_sync_entry: 6. + * + * If this is a FOR ALL TABLES publication and it has an EXCEPT + * TABLE list: + * + * 1. If pubviaroot is set and the relation is a partition, check + * whether the partition root is included in the EXCEPT TABLE + * list. If so, do not publish the change. + * + * 2. If pubviaroot is not set, check whether the relation itself + * is included in the EXCEPT TABLE list. If so, do not publish the + * change. + * + * This is achieved by keeping the variable "publish" set to + * false. And eventually, entry->pubactions will remain all false + * for this publication. Again, this logic seems to describe neither approach #1 nor approach #3. So, what is it (???) ====== src/test/subscription/t/037_rep_changes_except_table.pl 7. IIUC, the patch set is arranged so that the 2 x patch 00002 implementations are for the alternate partition handling. So I was not expecting patch 0001 to have any partition tests at all. If 0001 is not implementing approach #1 or approach #3 logic, then what are these tests testing? ====== Kind Regards, Peter Smith. Fujitsu Australia