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 1vLy60-00FHGg-03 for pgsql-hackers@arkaria.postgresql.org; Thu, 20 Nov 2025 06:24:44 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vLy5y-00HVrw-1y for pgsql-hackers@arkaria.postgresql.org; Thu, 20 Nov 2025 06:24:42 +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 1vLy5y-00HVrm-0k for pgsql-hackers@lists.postgresql.org; Thu, 20 Nov 2025 06:24:42 +0000 Received: from mail-io1-xd30.google.com ([2607:f8b0:4864:20::d30]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vLy5w-000T3m-0J for pgsql-hackers@lists.postgresql.org; Thu, 20 Nov 2025 06:24:41 +0000 Received: by mail-io1-xd30.google.com with SMTP id ca18e2360f4ac-94905c3e2a4so71629339f.1 for ; Wed, 19 Nov 2025 22:24:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1763619880; x=1764224680; 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=CaE9VjkpYH3werA5VpkGsMJCvhg6iHOCiceGJA5WYXs=; b=UXuPCXKZu0gq0VG3G5UhzwxVRqrs+qhorOVwq8MqiO+aNMk1usKfUtrSHKFf/NPBxV x3Bw3KU8ojDS3SD8cg8wNurhpkWyaY9t/Ruhb4zL4749XhByKZNxvJ3E76CgbteDHKJs O7KAoQhxFQd6VaGd2bPb8wGglLAL27JXGKDEaHFwKcgJZYFAX1udkSpfkw43iEznkrPS Vm9azRCBfH8P4KCBaKD3leOtNziDhRAW1jQQdD5/FKRZtZXhNMaknvhMT3L1FhlrPRuG BKY3nZ9AcpNw8u9Aw4zGyT5CHPdaeEcRybju0PPW46ZZenvk+3YbljSX9s2cOxVEOWFX HDRA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763619880; x=1764224680; 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=CaE9VjkpYH3werA5VpkGsMJCvhg6iHOCiceGJA5WYXs=; b=VqKpyXVhBayFKjALmVw5VYjvz8HqF0NlzDF8EilyOZdNCwbkWd6Lp9qzLlHtYh+A6I htxANfvzV7hubuMCfzrHbxK/CQKzCU/r8yFaVb0F7CmXG3k4IZNC4E8fewdcIdcQzLNW N7tz26EOJwKPYkzCqYxf7V22KTK2gvdDcJvUGYoTCDbY4FvsoJyHDYZgh3U1/lK7LZPJ 8ftfd7CyC47sipC4VZmr7PMMRZgDpLhFjDt3u1AZ20PWXKWvxnDaSDvEWwdz8/SJUbxY fRr9Nt5pOnjBi6uehNzE3zTE0HvWhX0QeP4dFPzLGeKQQbdrz+2XfngGcW051uD8hWTT LUMA== X-Forwarded-Encrypted: i=1; AJvYcCUU5p07FV0YXCvt3SflcPkj4uS5SExDkPJuj1SKg1w9cAc4pq7/FAHJ1VXTw5rU7tb0MogWe9g+EJXjKqm5@lists.postgresql.org X-Gm-Message-State: AOJu0YxPK2ezgOsfNRkAoomBG4/72icZuFlxfB9yAvvIacYvt8sde8mg ePgSYDFhVWxdkWX4+EfSymo5VJt0PRH1cvM7XxMXaGuv4aS43Nr4J8TxxSZFAH/6N2PdJiv4Jkt sOvZVg2tkVsK499E8Z1QqrTFxsFGYCek= X-Gm-Gg: ASbGncuFAMOaJ416DNJlDZw0acP7M1OOkIE1bZOPaBWWBF1k5kndjM84/LGqkjC2XaT hH5k1cpWuAG50WeeMUo2FiEqdvxZ+DjS9Y1GmZuocAJNXT8+9nSkjJIK4l/B6/GOrFainoM/sSg EHYsvikeVEwrJwexK6uAxnmolKlTAoHtHZPXppYkgUGxRL1cGJeU0JGhutFTHnR5kc+XmnWXIix GqeZGzjS7roZA1a62PhdZZWZuJMlyk+L7OXGlwv8Sm9YdEpYKP+abnooyN8+DWpxRzWLtugXcs+ w6LSYcsMsZhSPzBXO3gHXMWDUYBWb/tM4PEK4f4x X-Google-Smtp-Source: AGHT+IFOqzGbJ2CztrXQ0fNtl+hp8cXsde8GTbArsG7+HwNKP6Qs9JV/QAC1r8LNLiAixdv/PIHDL3uMqh4QlQKjGaM= X-Received: by 2002:a05:6638:a605:b0:5b4:a0e5:a3d9 with SMTP id 8926c6da1cb9f-5b95677afa6mr1160909173.3.1763619880069; Wed, 19 Nov 2025 22:24:40 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Thu, 20 Nov 2025 17:24:12 +1100 X-Gm-Features: AWmQ_blrqgq088QkzPKQJJhU0UpY-q0kefhVjeN2q0CdHnC2KRDbYDpLk1GQKGg Message-ID: Subject: Re: Skipping schema changes in publication To: Shlok Kyal Cc: vignesh C , Amit Kapila , "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 Shlok. Thanks for splitting the patches. Here are some review comments for the new patch v28-0002 (ADD ALL TABLES). ====== Commit Message 1. This patch adds support for using ADD ALL TABLES in ALTER PUBLICATION, allowing an existing publication to be changed into an ALL TABLES publication. This command is permitted only when the publication is in its default state, meaning it has no tables or schemas added, its ALL TABLES and ALL SEQUENCES flags are not set, and publication options such as publish_via_root_partition, publish_generated_columns, and publish are at their default values. ~ IMO, the restrictions for this new command are too severe: e.g. If I already have a FOR ALL SEQUENCES publication, then I expected it should be possible to ADD ALL TABLES to that as well, right? Likewise, why are we enforcing that the publication parameters must be defaults? IOW, why is (i) below disallowed, but (ii) is allowed? (i) ALTER PUBLICATION pub SET (publish_generated_columns=stored); ALTER PUBLICATION pub ADD ALL TABLES; (ii) ALTER PUBLICATION pub ADD ALL TABLES; ALTER PUBLICATION pub SET (publish_generated_columns=stored); ====== doc/src/sgml/ref/alter_publication.sgml Description: 2. The "Description" part of this page is confusing because it was referring to "The first three variants" and later "The fourth variant". Now that the "ADD ALL TABLES" variant has been added, I have lost track of what "variants" this description is talking about. Those words should be replaced by something clearer. This could be an ongoing issue if it is not worded differently because the same problem will happen again, e.g. when more syntax gets added for ALL SEQUENCES, etc. ~~~ 3. Note also that DROP TABLES IN SCHEMA will not drop any schema tables that were specified using FOR TABLE/ ADD TABLE. ~ That sentence (above) is from the docs. Does that also need updating now that there is ADD ALL TABLES? ====== src/backend/commands/publicationcmds.c CheckPublicationDefValues: 4. Is this function needed? ~~~ AlterPublication: 5. + if (stmt->for_all_tables) + { + bool isdefault = CheckPublicationDefValues(tup); + + if (!isdefault) + ereport(ERROR, + errcode(ERRCODE_INVALID_OBJECT_DEFINITION), + errmsg("adding ALL TABLES requires the publication to have default publication parameter values"), + errdetail("ALL TABLES or ALL SEQUENCES flag should not be set and no tables/schemas should be associated."), + errhint("Use ALTER PUBLICATION ... RESET to reset the publication")); + + AlterPublicationSetAllTables(rel, tup); + } + Why do we need this self-imposed restriction? ====== src/include/nodes/parsenodes.h 6. List *pubobjects; /* Optional list of publication objects */ + bool for_all_tables; /* Special publication for all tables in db */ AlterPublicationAction action; /* What action to perform with the given * objects */ } AlterPublicationStmt; There is no such "FOR" syntax like ALTER PUBLICATION ... FOR ALL TABLES, so I felt just 'puballtables' might be a better member name. ====== src/test/regress/sql/publication.sql 7. Don't uppercase any of the publication parameters because they never appear in the docs/examples like that. ~ 8. So that the last command is the one being tested, I felt that all the test cases should be doing RESET *first* instead of last. ~~~ 9. You don't always need to use RESET. There should also be some tests using an "empty" publication just to be sure it works. e.g CREATE PUBLICATION pub_empty; ALTER PUBLICATION pub_empty ADD ALL TABLES; ~~~ 10. As commented earlier, I felt the rules were too restrictive. So I think some test cases can be removed. ~~~ 11. +-- Tests for ALTER PUBLICATION ... ADD ALL TABLES ~ I noticed there is a "-- ======================================================" separator between the major groups of tests. 11a. Should use this separator in patch 0001 for the RESET group of tests 11b. Should use this separator in patch 0002 for the ADD ALL TABLES groups of tests ~~~ 12. +-- Can't add ALL TABLES to 'ALL TABLES' publication +ALTER PUBLICATION testpub_reset ADD ALL TABLES; + This test case seems to belong earlier, near the 'FOR TABLE' and the 'TABLES IN SCHEMA' tests. ====== Kind Regards, Peter Smith. Fujitsu Australia