Thread: 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/
Attachment
> 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]. Regards, [1] https://www.postgresql.org/message-id/CAA4eK1KaWwUSkDEKPseVY-z00kQJfpfVFdJCXPv9_CrwVZPMhg%40mail.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] https://www.postgresql.org/message-id/CAA4eK1KaWwUSkDEKPseVY-z00kQJfpfVFdJCXPv9_CrwVZPMhg%40mail.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/
Attachment
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/