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 1vC6LS-0090RJ-6k for pgsql-hackers@arkaria.postgresql.org; Fri, 24 Oct 2025 01:11:53 +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 1vC6LP-00CxNE-Oj for pgsql-hackers@arkaria.postgresql.org; Fri, 24 Oct 2025 01:11:50 +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 1vC6LP-00CxN6-Ai for pgsql-hackers@lists.postgresql.org; Fri, 24 Oct 2025 01:11:50 +0000 Received: from mail-qv1-xf36.google.com ([2607:f8b0:4864:20::f36]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vC6LM-003S3B-1p for pgsql-hackers@lists.postgresql.org; Fri, 24 Oct 2025 01:11:49 +0000 Received: by mail-qv1-xf36.google.com with SMTP id 6a1803df08f44-87dfd3c24ddso20034406d6.0 for ; Thu, 23 Oct 2025 18:11:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1761268307; x=1761873107; 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=ZI21necx889MHyhySwFT4v9LrADDV2RsZMkDJ2bu5UI=; b=E7WRm0m7n5MQkbVXx140aaeJBtrS7Dh0BoppeSNrHB4/FwlxvXwifG3uJJA2u/O0Hf /uc7mpqAoWMpzvRPmlRsmu67UTu4oca5t5Q4DMh44y+NATXaVaY9aifdaFlcpTgT9gvg 7swP7HOLxzN38WEzzajw4VcbD1qwRvhy9wZ3QGS4/NbJFFvA1XikzpTKi8b8etc+Xtwa /JXtIkHRuyP5WE8KXqDNaHYeEqENoC8SrHAE04YDkIgboLsQecsNL88tRh8hUlHLd0SM QMyVxZ5v3rmVrrHrGkxnXrXxgkJw6l0YtpaisfZXYlwrEoGJWuZYIES9y2ilDt6nuu9T rhpg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1761268307; x=1761873107; 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=ZI21necx889MHyhySwFT4v9LrADDV2RsZMkDJ2bu5UI=; b=kRPGxBIfr/OUQNeAfkEnT3TKumUQhcOWeSXxGInLvI6TxqS3Cu2y29eDWvIC4pDdyn tI1Vdjn3fyvzc4UryTGU1XgJa1jMnoozoMieiOpNEYNDaTQuCta3wVjOVm0Bemmw3vF4 6LGftlyKydc27f23ojarTZD0CE1xrlsGMowqIDh6mi390fwh4woi+Gj/OLe/4sXTo4M2 3/T8fdSEWa60z5j3oaDXU7WstoXhBFCAZCcZKLpO6TTuIrckYG1DQeNeF60VyRemWQ1u wpadjMCPEzfK1/HHw0lrpF4pv9d7jLgR+lQxJTjpIX0HGlxth1XypHZ+B5c9VPZP/Scc Bjjw== X-Forwarded-Encrypted: i=1; AJvYcCUsB7O72h0DTHMY4t5wl+zC1KooypjJhlbCUxzA4fWRu0jfNdPBHAhvun068xMBk4GkYAV/L0JbLep6aeGd@lists.postgresql.org X-Gm-Message-State: AOJu0YwLZ+ExZa2YyPY30VnE5bvtItLUgg2EDkmwvE/mX9Esp3zdivcn tefBsGAaH42nwtWo6a00gjcvCHSsPOeMhBFjE+epH/ZZELwhDTg80TARl4cKCq+zgASsFe+zKqw 6D7M0ompFlGbVAO0sC/aWjE4vmQTtDeU= X-Gm-Gg: ASbGncvpOPb0R8pYgN56IVJoug2IPuCeMDQrxpoVba/QnndRzAM16tEpE+K8C/Kw7Zu vI9gRGe1tHQO4nZqyPpTZRvlVUEf5rOHFRJIrD7ig5aEzadAywos7n0c5e969VSLcOAY8pmadsz UU/e2i/zjfq+0diwp3khoxDF18o5Vd+tR4era+gjXinD5CuEAr8jvqf1ClsSUKulUlrOBmbcCML FxoSShm9IZ/EbxhGlZh0legjOH/Gu6iXqJUcTQPBHEbCV57m1i21Z9slWuDuKcelczzArs= X-Google-Smtp-Source: AGHT+IFPph+NpNL840fEJpalSqneA+2rUCYYyzwibZqZX79OvJH8TeHGM8USvTzCpoJRpw83lu09nL4zhIJq0w8jvYQ= X-Received: by 2002:a05:622a:138a:b0:4e8:947e:16ef with SMTP id d75a77b69052e-4e89d265f9fmr342928981cf.21.1761268307515; Thu, 23 Oct 2025 18:11:47 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Fri, 24 Oct 2025 12:11:20 +1100 X-Gm-Features: AS18NWD7OCWOQJar8LbBoSXKDK2xCpEO_Ed44fKnV5zP3mLEP6QZFSvMSBrU0Ww 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 Vignesh, I had a look at patch v24-0001. FYI -- a rebase is needed [postgres@CentOS7-x64 oss_postgres_misc]$ git apply ../patches_misc/v24-0001-Add-RESET-clause-to-Alter-Publication-which-will.patch error: patch failed: doc/src/sgml/ref/alter_publication.sgml:69 error: doc/src/sgml/ref/alter_publication.sgml: patch does not apply Here are some other review comments: ====== 1. There seems to be some basic omission of the ALTER PUBLICATION in that it does not support "ALL TABLES" as a publication_object. Therefore, if you have: CREATE PUBLICATION mypub FOR ALL TABLES; and then you do: ALTER PUBLICATION mypub RESET; There seems to be no way to restore mpub to be an ALL TABLES publication again! ~~~ I think if you are going to implement a RESET, then you also need a way to get back to what you had before you did the reset. So you'll also need to implement the ALTER PUBLICATION mypub SET ALL TABLES; IMO, supporting "SET ALL TABLES" should be your new 0001 patch because AFAIK, this situation already exists if the user had created an "empty" publication: CREATE PUBLICATION myemptypub; ====== doc/src/sgml/ref/alter_publication.sgml 2. Probably need to mention ALL SEQUENCES now too? ====== src/backend/commands/publicationcmds.c 3. +/* CREATE PUBLICATION default values for flags and publication parameters */ +#define PUB_DEFAULT_ACTION_INSERT true +#define PUB_DEFAULT_ACTION_UPDATE true +#define PUB_DEFAULT_ACTION_DELETE true +#define PUB_DEFAULT_ACTION_TRUNCATE true +#define PUB_DEFAULT_VIA_ROOT false +#define PUB_DEFAULT_ALL_TABLES false +#define PUB_DEFAULT_GENCOLS PUBLISH_GENCOLS_NONE + Is it better to put all these in the catalog/pg_publication.h where the catalog was defined? ~~~ AlterPublicationReset: 4. + /* Set ALL TABLES flag to false */ + if (pubform->puballtables) + { + values[Anum_pg_publication_puballtables - 1] = BoolGetDatum(PUB_DEFAULT_ALL_TABLES); + replaces[Anum_pg_publication_puballtables - 1] = true; + CacheInvalidateRelcacheAll(); + } Why not just do this anyway without the condition? ====== src/backend/parser/gram.y 6. It would be nicer if all these grammar productions were coded in the same order as the comment above them. ====== src/include/nodes/parsenodes.h 7. AP_AddObjects, /* add objects to publication */ AP_DropObjects, /* remove objects from publication */ AP_SetObjects, /* set list of objects */ + AP_ResetPublication, /* reset the publication */ } AlterPublicationAction; It is already called "AlterPublicationAction", so maybe the enum value only needs to be AP_Reset instead of AP_ResetPublication. ====== src/test/regress/expected/publication.out 8. Expected output all needs rebasing now that there is a new "All sequences" column. ====== src/test/regress/sql/publication.sql 9. +ALTER PUBLICATION testpub_reset ADD TABLE pub_sch1.tbl1; + +-- Verify that associated tables are removed from the publication after RESET +\dRp+ testpub_reset +ALTER PUBLICATION testpub_reset RESET; +\dRp+ testpub_reset + I felt the ADD TABLE should be after the comment. And ditto for all the other test cases -- should be that same pattern too. # comment about test ALTER .. do something \dRp+ pub ALTER .. RESET \dRp+ pub ~~~ 10. +-- Verify that associated schemas are reomved from the publication after RESET typo: /reomved/removed/ ~~~ 11. +-- Verify that only superuser can reset a publication +ALTER PUBLICATION testpub_reset OWNER TO regress_publication_user2; +SET ROLE regress_publication_user2; +ALTER PUBLICATION testpub_reset RESET; -- fail - must be superuser +SET ROLE regress_publication_user; + Perhaps this should be the first test? ====== Kind Regards, Peter Smith. Fujitsu Australia