Documentation patch for ALTER SUBSCRIPTION

Started by David Christensenalmost 6 years ago7 messages
#1David Christensen
david@endpoint.com
1 attachment(s)

Greetings,

Enclosed find a documentation patch that clarifies the behavior of ALTER SUBSCRIPTION … REFRESH PUBLICATION with new tables; I ran into a situation today where the docs were not clear that existing tables would not be re-copied, so remedying this situation.

Should apply back to Pg 10.

Best,

David

Attachments:

0001-Be-explicit-about-the-behavior-of-REFRESH-PUBLICATIO.patchapplication/octet-stream; name=0001-Be-explicit-about-the-behavior-of-REFRESH-PUBLICATIO.patch; x-unix-mode=0644Download
From ce2c2ba18b1dafd3ec8a383b2939cbeb0980fe58 Mon Sep 17 00:00:00 2001
From: David Christensen <david@endpoint.com>
Date: Thu, 23 Jan 2020 14:28:02 -0600
Subject: [PATCH] Be explicit about the behavior of REFRESH PUBLICATION's
 copy_data

The docs were ambiguous as to which tables would be copied over when the copy_data parameter was
true in an ALTER SUBSCRIPTION ... REFRESH PUBLICATION, so make it clear that it only applied to
tables which were new in the publication.

Should be applied back to version 10 where the feature was introduced.
---
 doc/src/sgml/ref/alter_subscription.sgml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 6dfb2e4d3e..bfa83577c4 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -128,7 +128,9 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
          <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>.
+          The default is <literal>true</literal>.  This option only applies to
+          new tables in the publication which were not part of the local
+          subscription; it does not copy data for already known tables.
          </para>
         </listitem>
        </varlistentry>
-- 
2.21.1 (Apple Git-122.3)

#2Amit Kapila
amit.kapila16@gmail.com
In reply to: David Christensen (#1)
Re: Documentation patch for ALTER SUBSCRIPTION

On Fri, Jan 24, 2020 at 2:05 AM David Christensen <david@endpoint.com> wrote:

Greetings,

Enclosed find a documentation patch that clarifies the behavior of ALTER SUBSCRIPTION … REFRESH PUBLICATION with new tables; I ran into a situation today where the docs were not clear that existing tables would not be re-copied, so remedying this situation.

It seems this is already covered in REFRESH PUBLICATION, see "This
will start replication of tables that were added to the subscribed-to
publications since the last invocation of REFRESH PUBLICATION or since
CREATE SUBSCRIPTION.". As far as I understand, this text explains the
situation you were facing. Can you explain why the text quoted by me
is not sufficient?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

#3David Christensen
david@endpoint.com
In reply to: Amit Kapila (#2)
Re: Documentation patch for ALTER SUBSCRIPTION

On Feb 4, 2020, at 8:45 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Jan 24, 2020 at 2:05 AM David Christensen <david@endpoint.com> wrote:
Greetings,
Enclosed find a documentation patch that clarifies the behavior of ALTER SUBSCRIPTION … REFRESH PUBLICATION with new tables; I ran into a situation today where the docs were not clear that existing tables would not be re-copied, so remedying this situation.

It seems this is already covered in REFRESH PUBLICATION, see "This
will start replication of tables that were added to the subscribed-to
publications since the last invocation of REFRESH PUBLICATION or since
CREATE SUBSCRIPTION.". As far as I understand, this text explains the
situation you were facing. Can you explain why the text quoted by me
is not sufficient?

Hi Amit,

From several reads of the text it was not explicitly clear to me that when you issued the copy_data that it would not effectively recopy existing tables in the existing publication, which I had been trying to confirm was not the case prior to running a refresh operation. I had to resort to reviewing the source code to get the answer I was looking for.

If you are already familiar with the operation under the hood I am sure the ambiguity is not there but since I was recently confused by this I wanted to be more explicit in a way that would have helped me answer my original question.

Best,

David

#4Amit Kapila
amit.kapila16@gmail.com
In reply to: David Christensen (#3)
Re: Documentation patch for ALTER SUBSCRIPTION

On Wed, Feb 5, 2020 at 8:44 AM David Christensen <david@endpoint.com> wrote:

On Feb 4, 2020, at 8:45 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Jan 24, 2020 at 2:05 AM David Christensen <david@endpoint.com> wrote:
Greetings,
Enclosed find a documentation patch that clarifies the behavior of ALTER SUBSCRIPTION … REFRESH PUBLICATION with new tables; I ran into a situation today where the docs were not clear that existing tables would not be re-copied, so remedying this situation.

It seems this is already covered in REFRESH PUBLICATION, see "This
will start replication of tables that were added to the subscribed-to
publications since the last invocation of REFRESH PUBLICATION or since
CREATE SUBSCRIPTION.". As far as I understand, this text explains the
situation you were facing. Can you explain why the text quoted by me
is not sufficient?

Hi Amit,

From several reads of the text it was not explicitly clear to me that when you issued the copy_data that it would not effectively recopy existing tables in the existing publication, which I had been trying to confirm was not the case prior to running a refresh operation. I had to resort to reviewing the source code to get the answer I was looking for.

If you are already familiar with the operation under the hood I am sure the ambiguity is not there but since I was recently confused by this I wanted to be more explicit in a way that would have helped me answer my original question.

It is possible that one might not understand how this option works by
reading the already existing text in docs, but I think writing in a
different language the same thing also doesn't seem advisable. I
think if we want to explain it better, then maybe a succinct example
at the end of the page might be helpful.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

#5Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Amit Kapila (#4)
Re: Documentation patch for ALTER SUBSCRIPTION

On 2020-Feb-05, Amit Kapila wrote:

It is possible that one might not understand how this option works by
reading the already existing text in docs, but I think writing in a
different language the same thing also doesn't seem advisable. I
think if we want to explain it better, then maybe a succinct example
at the end of the page might be helpful.

For reference, the complete varlistentry is:

<term><literal>REFRESH PUBLICATION</literal></term>
<listitem>
<para>
Fetch missing table information from publisher. This will start
replication of tables that were added to the subscribed-to publications
since the last invocation of <command>REFRESH PUBLICATION</command> or
since <command>CREATE SUBSCRIPTION</command>. <!-- [2] -->
</para>

<para>
<replaceable>refresh_option</replaceable> specifies additional options for the
refresh operation. The supported options are:

<variablelist>
<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>. <!-- [1] -->
</para>
</listitem>
</varlistentry>
</variablelist>
</para>
</listitem>

I tend to agree with David that this is ambiguous enough to warrant a
few words. Maybe his proposed wording is too verbose; how about just
adding "(Previously subscribed tables are not copied.)" where the [1]
appears? Alternatively, we could add "Tables that were already present
in the subscription are not modified in any way." where [2] appears, but
that seems less clear to me.

An example would not be bad if it showed that existing data is not
copied. But examples are actually just syntactical examples, so you'd
have to resort to a comment explaining that existing tables are not
copied by the shown syntax. You might as well just add the words in the
reference docs ...

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#6David Christensen
david@endpoint.com
In reply to: Alvaro Herrera (#5)
Re: Documentation patch for ALTER SUBSCRIPTION

On Feb 5, 2020, at 7:56 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2020-Feb-05, Amit Kapila wrote:

It is possible that one might not understand how this option works by
reading the already existing text in docs, but I think writing in a
different language the same thing also doesn't seem advisable. I
think if we want to explain it better, then maybe a succinct example
at the end of the page might be helpful.

For reference, the complete varlistentry is:

<term><literal>REFRESH PUBLICATION</literal></term>
<listitem>
<para>
Fetch missing table information from publisher. This will start
replication of tables that were added to the subscribed-to publications
since the last invocation of <command>REFRESH PUBLICATION</command> or
since <command>CREATE SUBSCRIPTION</command>. <!-- [2] -->
</para>

<para>
<replaceable>refresh_option</replaceable> specifies additional options for the
refresh operation. The supported options are:

<variablelist>
<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>. <!-- [1] -->
</para>
</listitem>
</varlistentry>
</variablelist>
</para>
</listitem>

I tend to agree with David that this is ambiguous enough to warrant a
few words. Maybe his proposed wording is too verbose; how about just
adding "(Previously subscribed tables are not copied.)" where the [1]
appears? Alternatively, we could add "Tables that were already present
in the subscription are not modified in any way." where [2] appears, but
that seems less clear to me.

An example would not be bad if it showed that existing data is not
copied. But examples are actually just syntactical examples, so you'd
have to resort to a comment explaining that existing tables are not
copied by the shown syntax. You might as well just add the words in the
reference docs …

I would be happy with the suggestion [1]; it would have clarified my specific question.

Thanks,

David

#7Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: David Christensen (#6)
Re: Documentation patch for ALTER SUBSCRIPTION

OK, pushed that.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services