Fix ALTER SUBSCRIPTION ... SET PUBLICATION documentation

Started by japinalmost 5 years ago4 messages
#1japin
japinli@hotmail.com
1 attachment(s)

Hi,

When I read the documentation of ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (...),
it says "set_publication_option" only support "refresh" in documentation [1]https://www.postgresql.org/docs/devel/sql-altersubscription.html.
However, we can also supply the "copy_data" option, and the code is:

case ALTER_SUBSCRIPTION_PUBLICATION:
{
bool copy_data;
bool refresh;

parse_subscription_options(stmt->options,
NULL, /* no "connect" */
NULL, NULL, /* no "enabled" */
NULL, /* no "create_slot" */
NULL, NULL, /* no "slot_name" */
&copy_data,
NULL, /* no "synchronous_commit" */
&refresh,
NULL, NULL, /* no "binary" */
NULL, NULL); /* no "streaming" */
values[Anum_pg_subscription_subpublications - 1] =
publicationListToArray(stmt->publication);
replaces[Anum_pg_subscription_subpublications - 1] = true;

update_tuple = true;

/* Refresh if user asked us to. */
if (refresh)
{
if (!sub->enabled)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions"),
errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).")));

/* Make sure refresh sees the new list of publications. */
sub->publications = stmt->publication;

AlterSubscription_refresh(sub, copy_data);
}

break;
}

Should we fix the documentation or the code? I'd be inclined fix the documentation.

[1]: https://www.postgresql.org/docs/devel/sql-altersubscription.html

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Attachments:

fix-alter-subscription-set-publication-doc.patchtext/x-patchDownload
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 97c427e0f4..418e9a8ce1 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -101,6 +101,17 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
          </para>
         </listitem>
        </varlistentry>
+       <varlistentry>
+        <term><literal>copy_data</literal> (<type>boolean</type>)</term>
+        <listitem>
+         <para>
+          Specifies whether the existing data in the publications that are
+          being subscribed to should be copied once the replication starts.
+          The default is <literal>true</literal>.  (Previously subscribed
+          tables are not copied.)
+         </para>
+        </listitem>
+       </varlistentry>
       </variablelist>
 
       Additionally, refresh options as described
#2Amit Kapila
amit.kapila16@gmail.com
In reply to: japin (#1)
Re: Fix ALTER SUBSCRIPTION ... SET PUBLICATION documentation

On Tue, Jan 26, 2021 at 4:56 PM japin <japinli@hotmail.com> wrote:

Hi,

When I read the documentation of ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (...),
it says "set_publication_option" only support "refresh" in documentation [1].
However, we can also supply the "copy_data" option, and the code is:

I think there is a reference to the 'copy_data' option as well. There
is a sentence saying: "Additionally, refresh options as described
under REFRESH PUBLICATION may be specified." and then if you Refresh
option, there we do mention about 'copy_data', isn't that sufficient?

--
With Regards,
Amit Kapila.

#3Bharath Rupireddy
bharath.rupireddyforpostgres@gmail.com
In reply to: Amit Kapila (#2)
Re: Fix ALTER SUBSCRIPTION ... SET PUBLICATION documentation

On Wed, Jan 27, 2021 at 4:57 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jan 26, 2021 at 4:56 PM japin <japinli@hotmail.com> wrote:

Hi,

When I read the documentation of ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (...),
it says "set_publication_option" only support "refresh" in documentation [1].
However, we can also supply the "copy_data" option, and the code is:

I think there is a reference to the 'copy_data' option as well. There
is a sentence saying: "Additionally, refresh options as described
under REFRESH PUBLICATION may be specified." and then if you Refresh
option, there we do mention about 'copy_data', isn't that sufficient?

Right. It looks like the copy_option is indirectly mentioned with the
statement "Additionally, refresh options as described under REFRESH
PUBLICATION may be specified." under "set_publication_option". IMHO,
we can keep it that way.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

#4japin
japinli@hotmail.com
In reply to: Bharath Rupireddy (#3)
Re: Fix ALTER SUBSCRIPTION ... SET PUBLICATION documentation

On Wed, 27 Jan 2021 at 19:47, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Jan 27, 2021 at 4:57 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jan 26, 2021 at 4:56 PM japin <japinli@hotmail.com> wrote:

Hi,

When I read the documentation of ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (...),
it says "set_publication_option" only support "refresh" in documentation [1].
However, we can also supply the "copy_data" option, and the code is:

I think there is a reference to the 'copy_data' option as well. There
is a sentence saying: "Additionally, refresh options as described
under REFRESH PUBLICATION may be specified." and then if you Refresh
option, there we do mention about 'copy_data', isn't that sufficient?

Right. It looks like the copy_option is indirectly mentioned with the
statement "Additionally, refresh options as described under REFRESH
PUBLICATION may be specified." under "set_publication_option". IMHO,
we can keep it that way.

My bad. It may be sufficient. Sorry for noise.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.