Re: Skipping schema changes in publication

Hi Shlok. Some review comments for v29-0001 (ALTER PUBLICATION RESET)

======
doc/src/sgml/ref/alter_publication.sgml

Description:

1.
   <para>
    The command <command>ALTER PUBLICATION</command> can change the attributes
-   of a publication.
-  </para>
+   of a publication. There are several subforms described below.

IMO, you don't need to say that sentence "There are several subforms
described below," because it is obvious from the context.

(I guess you copied it from ALTER TABLE, but that doesn't change my opinion)

~~~

<General>

2.
The new description list entries all say:

"This form adds..."
"This form replaces..."
"This for removes..."
etc.

IMO, the words "This form" are all unnecessary here. Instead, just say
"Adds...", "Replaces...", "Removes...", etc.

(I guess you copied it from ALTER TABLE, but that doesn't change my opinion)

~~~

DROP:

3.
+   <varlistentry>
+    <term><literal>DROP <replaceable
class="parameter">publication_object</replaceable> [,
...]</literal></term>
+    <listitem>

The replacement name here should be "publication_drop_object".

~~~

SET:

4.
+     <para>
+      This form can change all of the publication properties specified in
+      <xref linkend="sql-createpublication"/>. Properties not mentioned in the
+      command retain their previous settings. It is not applicable to
+      sequences.
+     </para>

For "can change all of"; maybe that should be "can change any of".
Also, for "It is not applicable." -- saying "it" seemed awkward.

IMO the current master text (shown below) seemed OK as-is; At least I
thought it was better than the patch replacement text:
This clause alters publication parameters originally set by CREATE
PUBLICATION. See there for more information. This clause is not
applicable to sequences.

~~~

5.
+      <para>
+       This problem can be avoided by refraining from modifying partition leaf
+       tables after the <command>ALTER PUBLICATION ... SET</command> until the
+       <link linkend="sql-altersubscription"><command>ALTER
SUBSCRIPTION ... REFRESH PUBLICATION</command></link>
+       is executed and by only refreshing using the
<literal>copy_data = off</literal>
+       option.
+      </para>

This link would be better if it referenced the actual REFRESH
PUBLICATION. Currently, it just goes to the top of the ALTER
SUBSCRIPTION page.

~~~

6.
   <para>
    You must own the publication to use <command>ALTER PUBLICATION</command>.
    Adding a table to a publication additionally requires owning that table.
-   The <literal>ADD TABLES IN SCHEMA</literal> and
-   <literal>SET TABLES IN SCHEMA</literal> to a publication requires the
-   invoking user to be a superuser.
-   To alter the owner, you must be able to <literal>SET ROLE</literal> to the
-   new owning role, and that role must have <literal>CREATE</literal>
-   privilege on the database.
+   The <literal>ADD TABLES IN SCHEMA</literal>,
+   <literal>SET TABLES IN SCHEMA</literal> to a publication and
+   <literal>RESET</literal> of publication requires the invoking user to be a
+   superuser. To alter the owner, you must be able to
+   <literal>SET ROLE</literal> to the new owning role, and that role must have
+   <literal>CREATE</literal> privilege on the database.
    Also, the new owner of a
    <link linkend="sql-createpublication-params-for-tables-in-schema"><literal>FOR
TABLES IN SCHEMA</literal></link>
    or <link linkend="sql-createpublication-params-for-all-tables"><literal>FOR
ALL TABLES</literal></link>


I felt that those sentences:
"To alter the owner, you must ...", and
"Also, the new owner of a ..."
really belonged in the ALTER OWNER TO description part.

src/test/regress/sql/publication.sql
======

7.
+ALTER PUBLICATION testpub_reset SET (publish_via_partition_root = 'true');

It's a bit strange to say 'true' in quotes. Indeed, you shouldn't need
to give any value here -- just say "SET (publish_via_partition_root)"

======
Kind Regards,
Peter Smith.
Fujitsu Australia