Small documentation improvement for ALTER SUBSCRIPTION
Hi all,
When reading the doc of ALTER SUBSCRIPTION I realized that 'refresh
options' in the following paragraph is not tagged:
---
Additionally, refresh options as described under REFRESH PUBLICATION
may be specified, except in the case of DROP PUBLICATION.
---
When I read it for the first time, I got confused because we actually
have the 'refresh' option and this description in the paragraph of the
'refresh' option. I think we can improve it by changing to
'<replaceable>refresh_option</replaceable>'. Thoughts?
The patch is attached.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Attachments:
alter_subscription_doc.patchapplication/octet-stream; name=alter_subscription_doc.patchDownload
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index b3d173179f..6c553637ff 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -128,7 +128,7 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
</varlistentry>
</variablelist>
- Additionally, refresh options as described
+ Additionally, <replaceable>refresh_option</replaceable> as described
under <literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.
</para>
On 8 Jul 2021, at 15:00, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I think we can improve it by changing to
'<replaceable>refresh_option</replaceable>'. Thoughts?
My first thought was that the existing wording is clearer, referring to
“options to refresh”. But thinking on it more, it’s easy to see someone
confusing the options part as referring to the (bool) “option” to refresh
rather than refresh_option. I think your version is an improvement.
--
Daniel Gustafsson https://vmware.com/
Hi,
On Thu, Jul 8, 2021 at 10:14 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 8 Jul 2021, at 15:00, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I think we can improve it by changing to
'<replaceable>refresh_option</replaceable>'. Thoughts?My first thought was that the existing wording is clearer, referring to
“options to refresh”. But thinking on it more, it’s easy to see someone
confusing the options part as referring to the (bool) “option” to refresh
rather than refresh_option. I think your version is an improvement.
Thanks for your comments!
I've added this patch to the next commitfest so as not to forget.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On Thu, Jul 8, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Hi all,
When reading the doc of ALTER SUBSCRIPTION I realized that 'refresh
options' in the following paragraph is not tagged:---
Additionally, refresh options as described under REFRESH PUBLICATION
may be specified, except in the case of DROP PUBLICATION.
---When I read it for the first time, I got confused because we actually
have the 'refresh' option and this description in the paragraph of the
'refresh' option. I think we can improve it by changing to
'<replaceable>refresh_option</replaceable>'. Thoughts?
I see that one can get confused but how about changing it to
"Additionally, refresh options as described under <literal>REFRESH
PUBLICATION</literal> (<replaceable>refresh_option</replaceable>) may
be specified,.."? I think keeping "refresh options" in the text would
be good because there could be multiple such options.
--
With Regards,
Amit Kapila.
On Sat, Aug 7, 2021 at 4:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jul 8, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Hi all,
When reading the doc of ALTER SUBSCRIPTION I realized that 'refresh
options' in the following paragraph is not tagged:---
Additionally, refresh options as described under REFRESH PUBLICATION
may be specified, except in the case of DROP PUBLICATION.
---When I read it for the first time, I got confused because we actually
have the 'refresh' option and this description in the paragraph of the
'refresh' option. I think we can improve it by changing to
'<replaceable>refresh_option</replaceable>'. Thoughts?I see that one can get confused but how about changing it to
"Additionally, refresh options as described under <literal>REFRESH
PUBLICATION</literal> (<replaceable>refresh_option</replaceable>) may
be specified,.."? I think keeping "refresh options" in the text would
be good because there could be multiple such options.
I feel like it would be better to reword it in some way that avoids
using parentheses because they look like part of the syntax instead of
just part of the sentence.
------
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Sun, Aug 8, 2021 at 10:21 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Sat, Aug 7, 2021 at 4:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jul 8, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Hi all,
When reading the doc of ALTER SUBSCRIPTION I realized that 'refresh
options' in the following paragraph is not tagged:---
Additionally, refresh options as described under REFRESH PUBLICATION
may be specified, except in the case of DROP PUBLICATION.
---When I read it for the first time, I got confused because we actually
have the 'refresh' option and this description in the paragraph of the
'refresh' option. I think we can improve it by changing to
'<replaceable>refresh_option</replaceable>'. Thoughts?I see that one can get confused but how about changing it to
"Additionally, refresh options as described under <literal>REFRESH
PUBLICATION</literal> (<replaceable>refresh_option</replaceable>) may
be specified,.."? I think keeping "refresh options" in the text would
be good because there could be multiple such options.I feel like it would be better to reword it in some way that avoids
using parentheses because they look like part of the syntax instead of
just part of the sentence.
Fair enough, feel free to propose if you find something better or if
you think the current text in the docs is good.
--
With Regards,
Amit Kapila.
On Mon, Aug 9, 2021 at 12:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sun, Aug 8, 2021 at 10:21 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Sat, Aug 7, 2021 at 4:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jul 8, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Hi all,
When reading the doc of ALTER SUBSCRIPTION I realized that 'refresh
options' in the following paragraph is not tagged:---
Additionally, refresh options as described under REFRESH PUBLICATION
may be specified, except in the case of DROP PUBLICATION.
---When I read it for the first time, I got confused because we actually
have the 'refresh' option and this description in the paragraph of the
'refresh' option. I think we can improve it by changing to
'<replaceable>refresh_option</replaceable>'. Thoughts?I see that one can get confused but how about changing it to
"Additionally, refresh options as described under <literal>REFRESH
PUBLICATION</literal> (<replaceable>refresh_option</replaceable>) may
be specified,.."? I think keeping "refresh options" in the text would
be good because there could be multiple such options.I feel like it would be better to reword it in some way that avoids
using parentheses because they look like part of the syntax instead of
just part of the sentence.Fair enough, feel free to propose if you find something better or if
you think the current text in the docs is good.
IMO just the same as your suggestion but without the parens would be good. e.g.
"Additionally, refresh options as described under <literal>REFRESH
PUBLICATION</literal> <replaceable>refresh_option</replaceable> may be
specified,.."
On Mon, Aug 9, 2021 at 1:01 PM Peter Smith <smithpb2250@gmail.com> wrote:
On Mon, Aug 9, 2021 at 12:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sun, Aug 8, 2021 at 10:21 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Sat, Aug 7, 2021 at 4:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jul 8, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Hi all,
When reading the doc of ALTER SUBSCRIPTION I realized that 'refresh
options' in the following paragraph is not tagged:---
Additionally, refresh options as described under REFRESH PUBLICATION
may be specified, except in the case of DROP PUBLICATION.
---When I read it for the first time, I got confused because we actually
have the 'refresh' option and this description in the paragraph of the
'refresh' option. I think we can improve it by changing to
'<replaceable>refresh_option</replaceable>'. Thoughts?I see that one can get confused but how about changing it to
"Additionally, refresh options as described under <literal>REFRESH
PUBLICATION</literal> (<replaceable>refresh_option</replaceable>) may
be specified,.."? I think keeping "refresh options" in the text would
be good because there could be multiple such options.I feel like it would be better to reword it in some way that avoids
using parentheses because they look like part of the syntax instead of
just part of the sentence.Fair enough, feel free to propose if you find something better or if
you think the current text in the docs is good.
Thank you for the comments!
IMO just the same as your suggestion but without the parens would be good. e.g.
"Additionally, refresh options as described under <literal>REFRESH
PUBLICATION</literal> <replaceable>refresh_option</replaceable> may be
specified,.."
But "REFRESH PUBLICATION refresh_option" seems wrong in terms of SQL
syntax, not?
Given there could be multiple options how about using
"<replaceable>refresh_options</replaceable>"? That is, the sentence
will be:
Additionally, <replaceable>refresh_options</replaceable> as described
under <literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On Tue, Aug 10, 2021 at 11:01 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Mon, Aug 9, 2021 at 1:01 PM Peter Smith <smithpb2250@gmail.com> wrote:
On Mon, Aug 9, 2021 at 12:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sun, Aug 8, 2021 at 10:21 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Sat, Aug 7, 2021 at 4:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jul 8, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Hi all,
When reading the doc of ALTER SUBSCRIPTION I realized that 'refresh
options' in the following paragraph is not tagged:---
Additionally, refresh options as described under REFRESH PUBLICATION
may be specified, except in the case of DROP PUBLICATION.
---When I read it for the first time, I got confused because we actually
have the 'refresh' option and this description in the paragraph of the
'refresh' option. I think we can improve it by changing to
'<replaceable>refresh_option</replaceable>'. Thoughts?I see that one can get confused but how about changing it to
"Additionally, refresh options as described under <literal>REFRESH
PUBLICATION</literal> (<replaceable>refresh_option</replaceable>) may
be specified,.."? I think keeping "refresh options" in the text would
be good because there could be multiple such options.I feel like it would be better to reword it in some way that avoids
using parentheses because they look like part of the syntax instead of
just part of the sentence.Fair enough, feel free to propose if you find something better or if
you think the current text in the docs is good.Thank you for the comments!
IMO just the same as your suggestion but without the parens would be good. e.g.
"Additionally, refresh options as described under <literal>REFRESH
PUBLICATION</literal> <replaceable>refresh_option</replaceable> may be
specified,.."But "REFRESH PUBLICATION refresh_option" seems wrong in terms of SQL
syntax, not?
Because the sentence says "... as described under ..." I thought it
was clear enough it was referring to the documentation below and not
the SQL syntax.
Given there could be multiple options how about using
"<replaceable>refresh_options</replaceable>"? That is, the sentence
will be:Additionally, <replaceable>refresh_options</replaceable> as described
under <literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.
+1 LGTM
------
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Tue, Aug 10, 2021 at 6:31 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Mon, Aug 9, 2021 at 1:01 PM Peter Smith <smithpb2250@gmail.com> wrote:
On Mon, Aug 9, 2021 at 12:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
But "REFRESH PUBLICATION refresh_option" seems wrong in terms of SQL
syntax, not?Given there could be multiple options how about using
"<replaceable>refresh_options</replaceable>"? That is, the sentence
will be:Additionally, <replaceable>refresh_options</replaceable> as described
under <literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.
Normally (at least on this doc page), we use this tag for some defined
option, syntax and as refresh_options is none of them, it would look a
bit awkward.
--
With Regards,
Amit Kapila.
On Tue, Aug 10, 2021 at 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 10, 2021 at 6:31 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Mon, Aug 9, 2021 at 1:01 PM Peter Smith <smithpb2250@gmail.com> wrote:
On Mon, Aug 9, 2021 at 12:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
But "REFRESH PUBLICATION refresh_option" seems wrong in terms of SQL
syntax, not?Given there could be multiple options how about using
"<replaceable>refresh_options</replaceable>"? That is, the sentence
will be:Additionally, <replaceable>refresh_options</replaceable> as described
under <literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.Normally (at least on this doc page), we use this tag for some defined
option, syntax and as refresh_options is none of them, it would look a
bit awkward.
Indeed.
Thinking more the idea proposed by Peter Smith, it looks unnatural to
me, especially the part of "REFRESH PUBLICATION refresh_option":
Additionally, refresh options as described
under <literal>REFRESH PUBLICATION</literal>
<replaceable>refresh_option</replaceable> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.
As an alternative idea, how about using the "refresh_option of REFRESH
PUBLICATION" instead ? That is,
Additionally, refresh options as described in
<replaceable>refresh_option</replaceable> of
<literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On 11 Aug 2021, at 09:57, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Additionally, refresh options as described in
<replaceable>refresh_option</replaceable> of
<literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.
Since this paragraph is under the literal option “refresh”, which takes a
value, I still find your original patch to be the clearest.
--
Daniel Gustafsson https://vmware.com/
On Wed, Aug 11, 2021 at 5:42 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 11 Aug 2021, at 09:57, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Additionally, refresh options as described in
<replaceable>refresh_option</replaceable> of
<literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.Since this paragraph is under the literal option “refresh”, which takes a
value, I still find your original patch to be the clearest.
Yeah, I prefer my original patch over this idea. On the other hand, I
can see the point of review comment on it that Amit pointed out[1]/messages/by-id/CAA4eK1KaWwUSkDEKPseVY-z00kQJfpfVFdJCXPv9_CrwVZPMhg@mail.gmail.com.
Regards,
[1]: /messages/by-id/CAA4eK1KaWwUSkDEKPseVY-z00kQJfpfVFdJCXPv9_CrwVZPMhg@mail.gmail.com
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On Thu, Aug 12, 2021 at 12:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Yeah, I prefer my original patch over this idea. On the other hand, I
can see the point of review comment on it that Amit pointed out[1].Regards,
[1] /messages/by-id/CAA4eK1KaWwUSkDEKPseVY-z00kQJfpfVFdJCXPv9_CrwVZPMhg@mail.gmail.com
Personally, I don't really think the wording that results from the
original patch is great, because it doesn't give the impression of
multiple options.
I prefer something like:
Additionally, refresh options may be specified, as described under
<literal>REFRESH PUBLICATION</literal> supported
<replaceable>refresh_option</replaceable> values, except in the case
of <literal>DROP PUBLICATION</literal>.
Regards,
Greg Nancarrow
Fujitsu Australia
On 12.08.21 04:52, Masahiko Sawada wrote:
On Wed, Aug 11, 2021 at 5:42 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 11 Aug 2021, at 09:57, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Additionally, refresh options as described in
<replaceable>refresh_option</replaceable> of
<literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.Since this paragraph is under the literal option “refresh”, which takes a
value, I still find your original patch to be the clearest.Yeah, I prefer my original patch over this idea. On the other hand, I
can see the point of review comment on it that Amit pointed out[1].
How about this:
- Additionally, refresh options as described
- under <literal>REFRESH PUBLICATION</literal> may be specified.
+ Additionally, the options described under <literal>REFRESH
+ PUBLICATION</literal> may be specified, to control the implicit
refresh
+ operation.On 7 Sep 2021, at 13:36, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
On 12.08.21 04:52, Masahiko Sawada wrote:
On Wed, Aug 11, 2021 at 5:42 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 11 Aug 2021, at 09:57, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Additionally, refresh options as described in
<replaceable>refresh_option</replaceable> of
<literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.Since this paragraph is under the literal option “refresh”, which takes a
value, I still find your original patch to be the clearest.Yeah, I prefer my original patch over this idea. On the other hand, I
can see the point of review comment on it that Amit pointed out[1].How about this:
- Additionally, refresh options as described - under <literal>REFRESH PUBLICATION</literal> may be specified. + Additionally, the options described under <literal>REFRESH + PUBLICATION</literal> may be specified, to control the implicit refresh + operation.
LGTM.
--
Daniel Gustafsson https://vmware.com/
On Tue, Sep 7, 2021 at 9:01 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 7 Sep 2021, at 13:36, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
On 12.08.21 04:52, Masahiko Sawada wrote:
On Wed, Aug 11, 2021 at 5:42 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 11 Aug 2021, at 09:57, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Additionally, refresh options as described in
<replaceable>refresh_option</replaceable> of
<literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.Since this paragraph is under the literal option “refresh”, which takes a
value, I still find your original patch to be the clearest.Yeah, I prefer my original patch over this idea. On the other hand, I
can see the point of review comment on it that Amit pointed out[1].How about this:
- Additionally, refresh options as described - under <literal>REFRESH PUBLICATION</literal> may be specified. + Additionally, the options described under <literal>REFRESH + PUBLICATION</literal> may be specified, to control the implicit refresh + operation.LGTM.
+1
Attached the patch.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Attachments:
alter_subscription_doc_v2.patchapplication/octet-stream; name=alter_subscription_doc_v2.patchDownload
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 835be0d2a4..937a680512 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -131,8 +131,9 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
</varlistentry>
</variablelist>
- Additionally, refresh options as described
- under <literal>REFRESH PUBLICATION</literal> may be specified.
+ Additionally, the options described under
+ <literal>REFRESH PUBLICATION</literal> may be specified, to control the
+ implicit refresh operation.
</para>
</listitem>
</varlistentry>
On Wed, Sep 8, 2021 at 5:11 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Sep 7, 2021 at 9:01 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 7 Sep 2021, at 13:36, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
On 12.08.21 04:52, Masahiko Sawada wrote:
On Wed, Aug 11, 2021 at 5:42 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 11 Aug 2021, at 09:57, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Additionally, refresh options as described in
<replaceable>refresh_option</replaceable> of
<literal>REFRESH PUBLICATION</literal> may be specified,
except in the case of <literal>DROP PUBLICATION</literal>.Since this paragraph is under the literal option “refresh”, which takes a
value, I still find your original patch to be the clearest.Yeah, I prefer my original patch over this idea. On the other hand, I
can see the point of review comment on it that Amit pointed out[1].How about this:
- Additionally, refresh options as described - under <literal>REFRESH PUBLICATION</literal> may be specified. + Additionally, the options described under <literal>REFRESH + PUBLICATION</literal> may be specified, to control the implicit refresh + operation.LGTM.
+1
Attached the patch.
LGTM as well. Peter E., Daniel, does any one of you is intending to
push this? If not, I can take care of this.
--
With Regards,
Amit Kapila.
On 14 Sep 2021, at 11:57, Amit Kapila <amit.kapila16@gmail.com> wrote:
LGTM as well. Peter E., Daniel, does any one of you is intending to
push this? If not, I can take care of this.
No worries, I can pick it up.
--
Daniel Gustafsson https://vmware.com/
On 14 Sep 2021, at 14:35, Daniel Gustafsson <daniel@yesql.se> wrote:
On 14 Sep 2021, at 11:57, Amit Kapila <amit.kapila16@gmail.com> wrote:
LGTM as well. Peter E., Daniel, does any one of you is intending to
push this? If not, I can take care of this.No worries, I can pick it up.
And done, thanks!
--
Daniel Gustafsson https://vmware.com/
On Wed, Sep 15, 2021 at 4:58 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 14 Sep 2021, at 14:35, Daniel Gustafsson <daniel@yesql.se> wrote:
On 14 Sep 2021, at 11:57, Amit Kapila <amit.kapila16@gmail.com> wrote:
LGTM as well. Peter E., Daniel, does any one of you is intending to
push this? If not, I can take care of this.No worries, I can pick it up.
Sorry for the late reply. I was on vacation.
And done, thanks!
Thanks!
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/