Horton

RE: Skipping schema changes in publication

Takamichi Osumi (Fujitsu) <osumi.takamichi@fujitsu.com>

View thread
From: "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>
To: 'vignesh C' <vignesh21@gmail.com>, Peter Smith <smithpb2250@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-05-16T03:02:26Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix miscellaneous issues in EXCEPT publication clause.

  2. Change syntax of EXCEPT TABLE clause in publication commands.

  3. Add support for EXCEPT TABLE in ALTER PUBLICATION.

  4. Allow table exclusions in publications via EXCEPT TABLE.

  5. Add wait_for_subscription_sync for TAP tests.

On Saturday, May 14, 2022 10:33 PM vignesh C <vignesh21@gmail.com> 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