public inbox for [email protected]  

help / color / mirror / Atom feed

From: [email protected] <[email protected]>
To: 'vignesh C' <[email protected]>
To: Peter Smith <[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: Mon, 16 May 2022 03:02:26 +0000
Message-ID: <TYCPR01MB8373C3120C2B3112001ED6F1EDCF9@TYCPR01MB8373.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CALDaNm2-GJt2HsYTkLqQ=ecm=R-vOBw1=aM_d2EiYbz39x_cTQ@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>
	<CAHut+Pv_0DwyWoGQNMF+G2AGqMuJTzWQKRtmxaC+=zLTPL-Zkw@mail.gmail.com>
	<CALDaNm2-GJt2HsYTkLqQ=ecm=R-vOBw1=aM_d2EiYbz39x_cTQ@mail.gmail.com>

On Saturday, May 14, 2022 10:33 PM vignesh C <[email protected]> wrote:
> Thanks for the comments, the attached v5 patch has the changes for the same.
> Also I have made the changes for SKIP Table based on the new syntax, the
> changes for the same are available in
> v5-0002-Skip-publishing-the-tables-specified-in-EXCEPT-TA.patch.
Hi,


Thank you for updating the patch.
I'll share few minor review comments on v5-0001.


(1) doc/src/sgml/ref/alter_publication.sgml

@@ -73,12 +85,13 @@ ALTER PUBLICATION <replaceable class="parameter">name</replaceable> RENAME TO <r
    Adding a table to a publication additionally requires owning that table.
    The <literal>ADD ALL TABLES IN SCHEMA</literal> and
    <literal>SET ALL TABLES IN SCHEMA</literal> to a publication requires the
-   invoking user to be a superuser.  To alter the owner, you must also be a
-   direct or indirect member of the new owning role. The new owner must have
-   <literal>CREATE</literal> privilege on the database.  Also, the new owner
-   of a <literal>FOR ALL TABLES</literal> or <literal>FOR ALL TABLES IN
-   SCHEMA</literal> publication must be a superuser. However, a superuser can
-   change the ownership of a publication regardless of these restrictions.
+   invoking user to be a superuser.  <literal>RESET</literal> of publication
+   requires the invoking user to be a superuser. To alter the owner, you must
...


I suggest to combine the first part of your change with one existing sentence
before your change, to make our description concise.

FROM:
"The <literal>ADD ALL TABLES IN SCHEMA</literal> and
<literal>SET ALL TABLES IN SCHEMA</literal> to a publication requires the
invoking user to be a superuser.  <literal>RESET</literal> of publication
requires the invoking user to be a superuser."

TO:
"The <literal>ADD ALL TABLES IN SCHEMA</literal>,
<literal>SET ALL TABLES IN SCHEMA</literal> to a publication and
<literal>RESET</literal> of publication requires the invoking user to be a superuser."


(2) typo

+++ b/src/backend/commands/publicationcmds.c
@@ -53,6 +53,13 @@
 #include "utils/syscache.h"
 #include "utils/varlena.h"

+#define PUB_ATION_INSERT_DEFAULT true
+#define PUB_ACTION_UPDATE_DEFAULT true


Kindly change
FROM:
"PUB_ATION_INSERT_DEFAULT"
TO:
"PUB_ACTION_INSERT_DEFAULT"


(3) src/test/regress/expected/publication.out

+-- 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


We have "-- fail" for one case in this patch.
On the other hand, isn't better to add "-- ok" (or "-- success") for
other successful statements,
when we consider the entire tests description consistency ?


Best Regards,
	Takamichi Osumi

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], [email protected]
  Subject: RE: Skipping schema changes in publication
  In-Reply-To: <TYCPR01MB8373C3120C2B3112001ED6F1EDCF9@TYCPR01MB8373.jpnprd01.prod.outlook.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