public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: vignesh C <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Skipping schema changes in publication
Date: Fri, 13 May 2022 14:07:17 +1000
Message-ID: <CAHut+Pv_0DwyWoGQNMF+G2AGqMuJTzWQKRtmxaC+=zLTPL-Zkw@mail.gmail.com> (raw)
In-Reply-To: <CALDaNm0k_0Ccj47wzJzzPFwgQB7w=R5+Q2_nSqYrmMmjhmcRUw@mail.gmail.com>
References: <CALDaNm3=JrucjhiiwsYQw5-PGtBHFONa6F7hhWCXMsGvh=tamA@mail.gmail.com>
<CALj2ACVOzhs+BD+abFV2x4oKJdsDNd6SgsE7r8UjnZDCKGEckA@mail.gmail.com>
<CAA4eK1K6Kr88d2S0zFdHRMyuoaZeNh+ktU+oigmCuD09_x_-+g@mail.gmail.com>
<CAHut+PsvC-NezO3MJkdyEz=G1QRje2LntjwhQiEeVbmhOQuBMA@mail.gmail.com>
<CALDaNm18VH2j8cTqfELHQ=0ZNognbGBhbHPteJenWQC6C2dueQ@mail.gmail.com>
<CALDaNm0k_0Ccj47wzJzzPFwgQB7w=R5+Q2_nSqYrmMmjhmcRUw@mail.gmail.com>
On Thu, May 12, 2022 at 2:24 PM vignesh C <[email protected]> wrote:
>
...
> The attached patch has the implementation for "ALTER PUBLICATION
> pubname RESET". This command will reset the publication to default
> state which includes resetting the publication options, setting ALL
> TABLES option to false and dropping the relations and schemas that are
> associated with the publication.
>
Please see below my review comments for the v1-0001 (RESET) patch
======
1. Commit message
This patch adds a new RESET option to ALTER PUBLICATION which
Wording: "RESET option" -> "RESET clause"
~~~
2. doc/src/sgml/ref/alter_publication.sgml
+ <para>
+ The <literal>RESET</literal> clause will reset the publication to default
+ state which includes resetting the publication options, setting
+ <literal>ALL TABLES</literal> option to <literal>false</literal>
and drop the
+ relations and schemas that are associated with the publication.
</para>
2a. Wording: "to default state" -> "to the default state"
2b. Wording: "and drop the relations..." -> "and dropping all relations..."
~~~
3. doc/src/sgml/ref/alter_publication.sgml
+ invoking user to be a superuser. <literal>RESET</literal> of publication
+ requires invoking user to be a superuser. To alter the owner, you must also
Wording: "requires invoking user" -> "requires the invoking user"
~~~
4. doc/src/sgml/ref/alter_publication.sgml - Example
@@ -207,6 +220,12 @@ ALTER PUBLICATION sales_publication ADD ALL
TABLES IN SCHEMA marketing, sales;
<structname>production_publication</structname>:
<programlisting>
ALTER PUBLICATION production_publication ADD TABLE users,
departments, ALL TABLES IN SCHEMA production;
+</programlisting></para>
+
+ <para>
+ Resetting the publication <structname>production_publication</structname>:
+<programlisting>
+ALTER PUBLICATION production_publication RESET;
Wording: "Resetting the publication" -> "Reset the publication"
~~~
5. src/backend/commands/publicationcmds.c
+ /* Check and reset the options */
IMO the code can just reset all these options unconditionally. I did
not see the point to check for existing option values first. I feel
the simpler code outweighs any negligible performance difference in
this case.
~~~
6. src/backend/commands/publicationcmds.c
+ /* Check and reset the options */
Somehow it seemed a pity having to hardcode all these default values
true/false in multiple places; e.g. the same is already hardcoded in
the parse_publication_options function.
To avoid multiple hard coded bools you could just call the
parse_publication_options with an empty options list. That would set
the defaults which you can then use:
values[Anum_pg_publication_pubinsert - 1] = BoolGetDatum(pubactiondefs->insert);
Alternatively, maybe there should be #defines to use instead of having
the scattered hardcoded bool defaults:
#define PUBACTION_DEFAULT_INSERT true
#define PUBACTION_DEFAULT_UPDATE true
etc
~~~
7. src/include/nodes/parsenodes.h
@@ -4033,7 +4033,8 @@ typedef enum AlterPublicationAction
{
AP_AddObjects, /* add objects to publication */
AP_DropObjects, /* remove objects from publication */
- AP_SetObjects /* set list of objects */
+ AP_SetObjects, /* set list of objects */
+ AP_ReSetPublication /* reset the publication */
} AlterPublicationAction;
Unusual case: "AP_ReSetPublication" -> "AP_ResetPublication"
~~~
8. src/test/regress/sql/publication.sql
8a.
+-- Test for RESET PUBLICATION
SUGGESTED
+-- Tests for ALTER PUBLICATION ... RESET
8b.
+-- Verify that 'ALL TABLES' option is reset
SUGGESTED:
+-- Verify that 'ALL TABLES' flag is reset
8c.
+-- Verify that publish option and publish via root option is reset
SUGGESTED:
+-- Verify that publish options and publish_via_partition_root option are reset
8d.
+-- Verify that only superuser can execute RESET publication
SUGGESTED
+-- Verify that only superuser can reset a publication
------
Kind Regards,
Peter Smith.
Fujitsu Australia
view thread (377+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Skipping schema changes in publication
In-Reply-To: <CAHut+Pv_0DwyWoGQNMF+G2AGqMuJTzWQKRtmxaC+=zLTPL-Zkw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox