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.94.2) (envelope-from ) id 1ue2HX-004FzK-MG for pgsql-hackers@arkaria.postgresql.org; Tue, 22 Jul 2025 01:59:04 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1ue2HV-004EHI-7h for pgsql-hackers@arkaria.postgresql.org; Tue, 22 Jul 2025 01:59: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.94.2) (envelope-from ) id 1ue2HU-004EH9-QU for pgsql-hackers@lists.postgresql.org; Tue, 22 Jul 2025 01:59:01 +0000 Received: from mail-qt1-x82b.google.com ([2607:f8b0:4864:20::82b]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1ue2HT-0007ZQ-06 for pgsql-hackers@lists.postgresql.org; Tue, 22 Jul 2025 01:59:00 +0000 Received: by mail-qt1-x82b.google.com with SMTP id d75a77b69052e-4abc006bcadso33761621cf.0 for ; Mon, 21 Jul 2025 18:58:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1753149538; x=1753754338; 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=RXOs/cumbgQcMQA+RDo32m6aHEE3ec1see5PKJRreUE=; b=Fq/J6o5eFpZW0Dt+FrNUSflD5gqziVFB6+P0i1ZgP1fg4VQLSB26MOamyk6g1g4SoI CKJDdUWSpvvp7kN/uNedH2dMOEoA1GIble/S2Zkf266BCIrroy3Qd8nrcDVBKy4xCa/Q Q4Q6RogkrX0sUotNNrQQFOJqqUUyfhzz/GnyojmtWrsjaOA5LTz1n2kgIt1fDtmaEl4Z JwSCTU58VK3TPuZQQUKouImCx/SytNgBc76yCtyOQhcYXnnu5peT5qyl7l8Fwo/DNVzz a1x9hfCyNnyWao2Nv5hrhEmKuaO6n3aqqb7AnUNk3OTWQyR/ptjGVhcp9XI89XSLHGob AkSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1753149538; x=1753754338; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=RXOs/cumbgQcMQA+RDo32m6aHEE3ec1see5PKJRreUE=; b=iImCzLvXx4EbpDCZTTF1onld14x1FJ0gpsNTOFE2KRqHv0FwigSj/BN6GlY7/QgIha LiUSnXkCjeg6OQcxoSVtu6fvTHw2pMFn+dSxVeMxv1mAr6ovZDykRT/5hSJz1qC7azJf e1PEZfTpxO77Mgb0820RMguxRnbC7LhS+I+8tR7T/k99HPT3+KwuCsPqyJJwPPe/8fpj qs2FPfAtBF7M2bWlWBbA5v/MnM+gBdmbi2HktPLjxKyxu49d/Z3ObbDeXm6MczAzoaYP P9chTPaOcA1qEFSvIrZW877MTmiwpddGijG3Ba6f1celoZN3qsZpNcz1TgMt2YH7MP5V SaLA== X-Forwarded-Encrypted: i=1; AJvYcCVS19Ghxxp1JGXVuTBtdwvOGQDrxaj84tqClKL++xLoDt5mtFCGr3hRb8KHZqx6d+EkW0MO1ovk8YGh6acx@lists.postgresql.org X-Gm-Message-State: AOJu0YxIbBsezhnFeQEJg5JB9y6YOQHHvft65cv1HOKBXUjbAor2Vkr8 uijJHAIu/Kcdg60K2KeROQP0yMBlGhYrYDVVTR3gXlpgFaDjUtQ/atoGIZImV+ldwx63UQlniaW zvgnL915htvWVhFOgRobB9cfd9xyUpXE= X-Gm-Gg: ASbGncu0Xh0QtY4aLvtwYhRj7FMlR2h7C8QfGVrAOMUOaRL5L7czxdFLAa4lOluD8pr ED+tczXZGNO84w4ESS9WtzWU0XuHrZOtV23z4OqMu+EVbCpEvnj44PDGRnb4vjYvw9FmdsTdmYI 7d9Iqg9XokxYnulYyRszkXgjix45eRe2TJIYUgOgr4156FGpD2KV5Zqf10NVdDCyKJGc/oqClXt AjiZ34bdXoeOKRZW3D3Bps3Ow/0FlhIYR4sjVYC7w== X-Google-Smtp-Source: AGHT+IG/AqB1vL0vhsIPHv2uhCB+Iu1T8nEzYkJK42Tp0go5UWJ/L2r6RMGyXy+wH1QEVmd2bKv+vIpo9LctpxbNq34= X-Received: by 2002:ac8:5a4f:0:b0:4ab:8202:bb6e with SMTP id d75a77b69052e-4abd9ab91damr74326891cf.33.1753149537959; Mon, 21 Jul 2025 18:58:57 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Tue, 22 Jul 2025 11:58:29 +1000 X-Gm-Features: Ac12FXwWdNJ2U8WPekS-_IGf7hUgzu7RrtdE6OIGXqnxysyIVSwrJvSNQTjpcFg Message-ID: Subject: Re: Skipping schema changes in publication To: Shlok Kyal Cc: Amit Kapila , "Zhijie Hou (Fujitsu)" , vignesh C , 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 Shlok, Some review comments for patch v17-0003. I also checked the TAP test this time. ====== doc/src/sgml/logical-replication.sgml 1. + publish_generated_columns. Specifying generated + columns in a column list using the EXCEPT clause excludes + the specified generated columns from being published, regardless of the + + publish_generated_columns setting. However, for I think that is not quite the same wording I had previously suggested. It sounds a bit odd/redundant saying "Specifying" and "specified" in the same sentence. ====== src/backend/parser/gram.y 2. check_except_collist I'm wondering if this checking should be done within the existing preprocess_pubobj_list() function, alongside all the other ERROR checking. Care needs to be taken to make sure the pubtable->except is referring to an EXCEPT (col-list), instead of the other kind of EXCEPT tables, but in general I think it is better to keep all the publication combinations checking errors like this in one place. ====== src/bin/psql/describe.c 3. addFooterToPublicationDesc - appendPQExpBuffer(&buf, " (%s)", - PQgetvalue(result, i, 2)); + { + if (!PQgetisnull(result, i, 3) && + strcmp(PQgetvalue(result, i, 3), "t") == 0) + appendPQExpBuffer(&buf, " EXCEPT (%s)", + PQgetvalue(result, i, 2)); + else + appendPQExpBuffer(&buf, " (%s)", + PQgetvalue(result, i, 2)); + } Do you really need to check !PQgetisnull(result, i, 3) here? (e.g. The comment does not say that this attribute can be NULL) ====== .../t/037_rep_changes_except_collist.pl 4. +# Copyright (c) 2021-2025, PostgreSQL Global Development Group + +# Logical replication tests for except table publications Comment is wrong. These tests are for EXCEPT (column-list) ~~~ 5. +# Test for except column publications +# Initial setup +$node_publisher->safe_psql('postgres', "CREATE SCHEMA sch1"); +$node_publisher->safe_psql('postgres', + "CREATE TABLE tab2 (a int, b int NOT NULL, c int)"); +$node_publisher->safe_psql('postgres', + "CREATE TABLE sch1.tab2 (a int, b int, c int)"); +$node_publisher->safe_psql('postgres', + "CREATE TABLE tab3 (a int, b int, c int)"); +$node_publisher->safe_psql('postgres', + "CREATE TABLE tab4 (a int, b int GENERATED ALWAYS AS (a * 2) STORED, c int GENERATED ALWAYS AS (a * 3) STORED)" +); +$node_publisher->safe_psql('postgres', + "CREATE TABLE tab5 (a int, b int GENERATED ALWAYS AS (a * 2) STORED, c int GENERATED ALWAYS AS (a * 3) STORED)" +); +$node_publisher->safe_psql('postgres', "INSERT INTO tab2 VALUES (1, 2, 3)"); +$node_publisher->safe_psql('postgres', + "INSERT INTO sch1.tab2 VALUES (1, 2, 3)"); +$node_publisher->safe_psql('postgres', + "CREATE PUBLICATION tap_pub_col FOR TABLE tab2 EXCEPT (a), sch1.tab2 EXCEPT (b, c)" +); 5a. I think you don't need to say "Test for except column publications", because that is the purpose of thie entire file. ~ 5b. You can combine multiple of these safe_psql calls together ~ 5c. It might help make tests easier to read if you named those generated columns 'b', 'c' cols as 'bgen', 'cgen' instead. ~ 5d. The table names are strange, because why does it start at tab2 when there is not a tab1? ~~~ 6. +$node_subscriber->safe_psql('postgres', "CREATE SCHEMA sch1"); +$node_subscriber->safe_psql('postgres', + "CREATE TABLE tab2 (a int, b int NOT NULL, c int)"); +$node_subscriber->safe_psql('postgres', + "CREATE TABLE sch1.tab2 (a int, b int, c int)"); +$node_subscriber->safe_psql('postgres', + "CREATE TABLE tab3 (a int, b int, c int)"); +$node_subscriber->safe_psql('postgres', + "CREATE TABLE tab4 (a int, b int, c int)"); +$node_subscriber->safe_psql('postgres', + "CREATE TABLE tab5 (a int, b int, c int)"); You can combine multiple of these safe_psql calls together ~~~ 7. +# Test initial sync +my $result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab2"); +is($result, qq(|2|3), + 'check that initial sync for except column publication'); The message seems strange. Do you mean "check initial sync for an 'EXCEPT (column-list)' publication" NOTE: There are many other messages where you wrote "for except column publication" but I think maybe all of those can be improved a bit like above. ~~~ 8. +$node_publisher->safe_psql('postgres', "INSERT INTO tab2 VALUES (4, 5, 6)"); +$node_publisher->safe_psql('postgres', + "INSERT INTO sch1.tab2 VALUES (4, 5, 6)"); +$node_publisher->wait_for_catchup('tap_sub_col'); 8a. You can combine multiple of these safe_psql calls together. NOTE: I won't keep repeating this review comment but I think maybe there are lots more places where the safe_psql can all be combined to expected multiple statements. ~ 8b. I felt all those commands should be under the "Test incremental changes" comment. ~~~ 9. +is($result, qq(1||3), 'check alter publication with EXCEPT'); Maybe that should've said with 'EXCEPT (column-list)' ~~~ 10. +# Test for publication created with publish_generated_columns as true on table +# with generated columns and column list specified with EXCEPT +$node_publisher->safe_psql('postgres', "INSERT INTO tab4 VALUES (1)"); +$node_publisher->safe_psql('postgres', + "ALTER PUBLICATION tap_pub_col SET (publish_generated_columns)"); +$node_publisher->safe_psql('postgres', + "ALTER PUBLICATION tap_pub_col SET TABLE tab4 EXCEPT(b)"); +$node_subscriber->safe_psql('postgres', + "ALTER SUBSCRIPTION tap_sub_col REFRESH PUBLICATION"); +$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub_col'); 10a. I felt the test comments for both those generated columns parameter test should give more explanation to say what is the expected result and why. ~ 10b. How does "ALTER PUBLICATION tap_pub_col SET (publish_generated_columns)" even work? I thought the "pubish_generated_columns" is an enum but you did not specify any enum value here (???) ~~~ 11. + 'check publication(publish_generated_columns as false) with generated columns and EXCEPT' Hmm. I thought there is no such thing as "publish_generated_columns as false", and also the EXCEPT should say 'EXCEPT (column-list)' ~~~ 12. I wonder if there should be another boundary condition test case as follows: - have some table with cols a,b,c. - create a publication 'EXCEPT (a,b,c)', so you don't publish anything at all. - then ALTER the TABLE to add a column 'd'. - now the publication should publish only 'd'. ====== Kind Regards, Peter Smith. Fujitsu Australia