Thread: create subscription - improved warning message
WARNING: tables were not subscribed, you will have to run ALTER SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables ~ When I first encountered the above CREATE SUBSCRIPTION warning message I thought it was dubious-looking English... On closer inspection I think the message has some other things that could be improved: a) it is quite long which IIUC is generally frowned upon b) IMO most of the text it is more like a "hint" about what to do ~ PSA a patch which modifies this warning as follows: BEFORE test_sub=# create subscription sub1 connection 'host=localhost port=test_pub' publication pub1 with (connect = false); WARNING: tables were not subscribed, you will have to run ALTER SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables CREATE SUBSCRIPTION AFTER test_sub=# create subscription sub1 connection 'host=localhost port=test_pub' publication pub1 with (connect = false); WARNING: tables were not subscribed HINT: You will have to run ALTER SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables. CREATE SUBSCRIPTION ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
Peter Smith <smithpb2250@gmail.com> writes:
> WARNING: tables were not subscribed, you will have to run ALTER
> SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables
> When I first encountered the above CREATE SUBSCRIPTION warning message
> I thought it was dubious-looking English...
> On closer inspection I think the message has some other things that
> could be improved:
> a) it is quite long which IIUC is generally frowned upon
> b) IMO most of the text it is more like a "hint" about what to do
You're quite right about both of those points, but I think there's
even more to criticize: "tables were not subscribed" is a basically
useless message, and probably not even conceptually accurate.
Looking at the code, I think the situation being complained of is that
we have created the subscription's main catalog entries locally, but
since we were told not to connect to the publisher, we don't know what
tables the subscription is supposed to be reading. I'm not sure what
the consequences of that are: do we not read any data at all yet, or
what?
I think maybe a better message would be along the lines of
WARNING: subscription was created, but is not up-to-date
HINT: You should now run %s to initiate collection of data.
Thoughts?
regards, tom lane
On Sat, Oct 8, 2022 at 2:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Smith <smithpb2250@gmail.com> writes: > > WARNING: tables were not subscribed, you will have to run ALTER > > SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables > > > When I first encountered the above CREATE SUBSCRIPTION warning message > > I thought it was dubious-looking English... > > > On closer inspection I think the message has some other things that > > could be improved: > > a) it is quite long which IIUC is generally frowned upon > > b) IMO most of the text it is more like a "hint" about what to do > > You're quite right about both of those points, but I think there's > even more to criticize: "tables were not subscribed" is a basically > useless message, and probably not even conceptually accurate. > Looking at the code, I think the situation being complained of is that > we have created the subscription's main catalog entries locally, but > since we were told not to connect to the publisher, we don't know what > tables the subscription is supposed to be reading. I'm not sure what > the consequences of that are: do we not read any data at all yet, or > what? > > I think maybe a better message would be along the lines of > > WARNING: subscription was created, but is not up-to-date > HINT: You should now run %s to initiate collection of data. > > Thoughts? Yes, IMO it's better to change the message more radically as you did. But if it's OK to do that then: - maybe it should mention the connection since the connect=false was what caused this warning. - maybe saying 'replication' instead of 'collection of data' would be more consistent with the pgdocs for CREATE SUBSCRIPTION e.g. WARNING: subscription was created, but is not connected HINT: You should run %s to initiate replication. (I can update the patch when the final text is decided) ------ Kind Regards, Peter Smith. Fujitsu Australia
Peter Smith <smithpb2250@gmail.com> writes:
> On Sat, Oct 8, 2022 at 2:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think maybe a better message would be along the lines of
>> WARNING: subscription was created, but is not up-to-date
>> HINT: You should now run %s to initiate collection of data.
> [ how about ]
> WARNING: subscription was created, but is not connected
> HINT: You should run %s to initiate replication.
OK by me; anybody else want to weigh in?
regards, tom lane
On Mon, Oct 10, 2022 at 4:40 AM Peter Smith <smithpb2250@gmail.com> wrote: > > But if it's OK to do that then: > - maybe it should mention the connection since the connect=false was > what caused this warning. > - maybe saying 'replication' instead of 'collection of data' would be > more consistent with the pgdocs for CREATE SUBSCRIPTION > > e.g. > > WARNING: subscription was created, but is not connected > HINT: You should run %s to initiate replication. > Yeah, this message looks better than the current one. However, when I tried to do what HINT says, it doesn't initiate replication. It gives me the below error: postgres=# Alter subscription sub1 refresh publication; ERROR: ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled subscriptions Then, I enabled the subscription and again tried as below: postgres=# Alter subscription sub1 enable; ALTER SUBSCRIPTION postgres=# Alter subscription sub1 refresh publication; ALTER SUBSCRIPTION Even after the above replication is not initiated. I see "ERROR: replication slot "sub1" does not exist" in subscriber logs. Then, I manually created this slot (by using pg_create_logical_replication_slot) on the publisher. After that, replication started to work. If I am not missing something, don't you think we need a somewhat more elaborative HINT, or may be just give the WARNING? -- With Regards, Amit Kapila.
Amit Kapila <amit.kapila16@gmail.com> writes:
> Yeah, this message looks better than the current one. However, when I
> tried to do what HINT says, it doesn't initiate replication. It gives
> me the below error:
> postgres=# Alter subscription sub1 refresh publication;
> ERROR: ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled subscriptions
Geez ... is there *anything* that's not broken about this message?
I'm beginning to question the entire premise here. That is,
rather than tweaking this message until it's within hailing
distance of sanity, why do we allow the no-connect case at all?
It's completely obvious that nobody uses this option, or we'd
already have heard complaints about the advice being a lie.
What's the real-world use case?
regards, tom lane
On Mon, Oct 10, 2022 at 10:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Kapila <amit.kapila16@gmail.com> writes: > > Yeah, this message looks better than the current one. However, when I > > tried to do what HINT says, it doesn't initiate replication. It gives > > me the below error: > > > postgres=# Alter subscription sub1 refresh publication; > > ERROR: ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled subscriptions > > Geez ... is there *anything* that's not broken about this message? > > I'm beginning to question the entire premise here. That is, > rather than tweaking this message until it's within hailing > distance of sanity, why do we allow the no-connect case at all? > The docs say [1]: "When creating a subscription, the remote host is not reachable or in an unclear state. In that case, the subscription can be created using the connect = false option. The remote host will then not be contacted at all. This is what pg_dump uses. The remote replication slot will then have to be created manually before the subscription can be activated." I think the below gives accurate information: WARNING: subscription was created, but is not connected HINT: You should create the slot manually, enable the subscription, and run %s to initiate replication. [1] - https://www.postgresql.org/docs/devel/logical-replication-subscription.html -- With Regards, Amit Kapila.
On Mon, Oct 10, 2022 at 4:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Mon, Oct 10, 2022 at 10:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Amit Kapila <amit.kapila16@gmail.com> writes: ... > The docs say [1]: "When creating a subscription, the remote host is > not reachable or in an unclear state. In that case, the subscription > can be created using the connect = false option. The remote host will > then not be contacted at all. This is what pg_dump uses. The remote > replication slot will then have to be created manually before the > subscription can be activated." > > I think the below gives accurate information: > WARNING: subscription was created, but is not connected > HINT: You should create the slot manually, enable the subscription, > and run %s to initiate replication. > +1 PSA patch v2 worded like that. ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Mon, Oct 10, 2022 at 12:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I'm beginning to question the entire premise here. That is, > rather than tweaking this message until it's within hailing > distance of sanity, why do we allow the no-connect case at all? That sounds pretty nuts to me, because of the pg_dump use case if nothing else. I don't think it's reasonable to say "oh, if you execute this DDL on your system, it will instantaneously and automatically begin to create outbound network connections, and there's no way to turn that off." It ought to be possible to set up a configuration first and then only later turn it on. And it definitely ought to be possible, if things aren't working out, to turn it back off, too. -- Robert Haas EDB: http://www.enterprisedb.com
Peter Smith <smithpb2250@gmail.com> writes:
> On Mon, Oct 10, 2022 at 4:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> I think the below gives accurate information:
>> WARNING: subscription was created, but is not connected
>> HINT: You should create the slot manually, enable the subscription,
>> and run %s to initiate replication.
> +1
It feels a little strange to me that we describe two steps rather vaguely
and then give exact SQL for the third step. How about, say,
HINT: To initiate replication, you must manually create the replication
slot, enable the subscription, and refresh the subscription.
Another idea is
HINT: To initiate replication, create the replication slot on the
publisher, then run ALTER SUBSCRIPTION ... ENABLE and ALTER
SUBSCRIPTION ... REFRESH PUBLICATION.
regards, tom lane
On Mon, Oct 10, 2022 at 8:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Smith <smithpb2250@gmail.com> writes: > > On Mon, Oct 10, 2022 at 4:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > >> I think the below gives accurate information: > >> WARNING: subscription was created, but is not connected > >> HINT: You should create the slot manually, enable the subscription, > >> and run %s to initiate replication. > > > +1 > > It feels a little strange to me that we describe two steps rather vaguely > and then give exact SQL for the third step. How about, say, > > HINT: To initiate replication, you must manually create the replication > slot, enable the subscription, and refresh the subscription. > LGTM. BTW, do we want to slightly adjust the documentation for the connect option on CREATE SUBSCRIPTION page [1]? It says: "Since no connection is made when this option is false, no tables are subscribed, and so after you enable the subscription nothing will be replicated. You will need to then run ALTER SUBSCRIPTION ... REFRESH PUBLICATION for tables to be subscribed." It doesn't say anything about manually creating the slot and probably the wording can be made similar. [1] - https://www.postgresql.org/docs/devel/sql-createsubscription.html -- With Regards, Amit Kapila.
On Tue, Oct 11, 2022 at 2:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Mon, Oct 10, 2022 at 8:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Peter Smith <smithpb2250@gmail.com> writes: > > > On Mon, Oct 10, 2022 at 4:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > >> I think the below gives accurate information: > > >> WARNING: subscription was created, but is not connected > > >> HINT: You should create the slot manually, enable the subscription, > > >> and run %s to initiate replication. > > > > > +1 > > > > It feels a little strange to me that we describe two steps rather vaguely > > and then give exact SQL for the third step. How about, say, > > > > HINT: To initiate replication, you must manually create the replication > > slot, enable the subscription, and refresh the subscription. > > > > LGTM. PSA patch v3-0001 where the message/hint is worded as suggested above > BTW, do we want to slightly adjust the documentation for the > connect option on CREATE SUBSCRIPTION page [1]? It says: "Since no > connection is made when this option is false, no tables are > subscribed, and so after you enable the subscription nothing will be > replicated. You will need to then run ALTER SUBSCRIPTION ... REFRESH > PUBLICATION for tables to be subscribed." > > It doesn't say anything about manually creating the slot and probably > the wording can be made similar. > PSA patch v3-0002 which changes CREATE SUBSCRIPTION pgdocs to use the same wording as in the HINT message. ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On 2022-Oct-10, Peter Smith wrote: > On Mon, Oct 10, 2022 at 4:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > I think the below gives accurate information: > > WARNING: subscription was created, but is not connected > > HINT: You should create the slot manually, enable the subscription, > > and run %s to initiate replication. I guess this is reasonable, but how do I know what slot name do I have to create? Maybe it'd be better to be explicit about that: HINT: You should create slot \"%s\" manually, enable the subscription, and run %s to initiate replication. though this still leaves a lot unexplained about that slot creation (which options do they have to use). If this sounds like too much for a HINT, perhaps we need a documentation subsection that explains exactly what to do, and have this HINT reference the documentation? I don't think we do that anywhere else, though. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Industry suffers from the managerial dogma that for the sake of stability and continuity, the company should be independent of the competence of individual employees." (E. Dijkstra)
On 2022-Oct-11, Peter Smith wrote: > On Tue, Oct 11, 2022 at 2:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Mon, Oct 10, 2022 at 8:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > It feels a little strange to me that we describe two steps rather > > > vaguely and then give exact SQL for the third step. How about, > > > say, > > > > > > HINT: To initiate replication, you must manually create the > > > replication slot, enable the subscription, and refresh the > > > subscription. > > > > LGTM. > > PSA patch v3-0001 where the message/hint is worded as suggested above LGTM. > > BTW, do we want to slightly adjust the documentation for the > > connect option on CREATE SUBSCRIPTION page [1]? It says: "Since no > > connection is made when this option is false, no tables are > > subscribed, and so after you enable the subscription nothing will be > > replicated. You will need to then run ALTER SUBSCRIPTION ... REFRESH > > PUBLICATION for tables to be subscribed." > > > > It doesn't say anything about manually creating the slot and probably > > the wording can be made similar. > > PSA patch v3-0002 which changes CREATE SUBSCRIPTION pgdocs to use the > same wording as in the HINT message. I think we want the documentation to explain in much more detail what is meant. Users are going to need some help in determining which commands to run for each of the step mentioned in the hint, so I don't think we want the docs to say the same thing as the hint. How does the user know the name of the slot, what options to use, what are the commands to run afterwards. So I think we should aim to *expand* that text, not reduce it. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Tue, Oct 11, 2022 at 4:27 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2022-Oct-10, Peter Smith wrote: > > > On Mon, Oct 10, 2022 at 4:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > I think the below gives accurate information: > > > WARNING: subscription was created, but is not connected > > > HINT: You should create the slot manually, enable the subscription, > > > and run %s to initiate replication. > > I guess this is reasonable, but how do I know what slot name do I have > to create? Maybe it'd be better to be explicit about that: > > HINT: You should create slot \"%s\" manually, enable the subscription, and run %s to initiate replication. > I am not so sure about including a slot name because users can create a slot with a name of their choice and set it via Alter Subscription. -- With Regards, Amit Kapila.
On Wed, Oct 12, 2022 at 12:34 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2022-Oct-11, Peter Smith wrote: > > > > BTW, do we want to slightly adjust the documentation for the > > > connect option on CREATE SUBSCRIPTION page [1]? It says: "Since no > > > connection is made when this option is false, no tables are > > > subscribed, and so after you enable the subscription nothing will be > > > replicated. You will need to then run ALTER SUBSCRIPTION ... REFRESH > > > PUBLICATION for tables to be subscribed." > > > > > > It doesn't say anything about manually creating the slot and probably > > > the wording can be made similar. > > > > PSA patch v3-0002 which changes CREATE SUBSCRIPTION pgdocs to use the > > same wording as in the HINT message. > > I think we want the documentation to explain in much more detail what is > meant. Users are going to need some help in determining which commands > to run for each of the step mentioned in the hint, so I don't think we > want the docs to say the same thing as the hint. How does the user know > the name of the slot, what options to use, what are the commands to run > afterwards. > I think it is a good idea to expand the docs for this but note that there are multiple places that use a similar description. For example, see the description slot_name option: "When slot_name is set to NONE, there will be no replication slot associated with the subscription. This can be used if the replication slot will be created later manually. Such subscriptions must also have both enabled and create_slot set to false.". Then, we have a few places in the logical replication docs [1] that talk about creating the slot manually but didn't explain in detail the name or options to use. We might want to write a slightly bigger doc patch so that we can write the description in one place and give reference to the same at other places. [1] - https://www.postgresql.org/docs/devel/logical-replication-subscription.html -- With Regards, Amit Kapila.
On 2022-Oct-12, Amit Kapila wrote: > I think it is a good idea to expand the docs for this but note that > there are multiple places that use a similar description. For example, > see the description slot_name option: "When slot_name is set to NONE, > there will be no replication slot associated with the subscription. > This can be used if the replication slot will be created later > manually. Such subscriptions must also have both enabled and > create_slot set to false.". Then, we have a few places in the logical > replication docs [1] that talk about creating the slot manually but > didn't explain in detail the name or options to use. We might want to > write a slightly bigger doc patch so that we can write the description > in one place and give reference to the same at other places. +1 -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Para tener más hay que desear menos"
On Wed, Oct 12, 2022 at 2:08 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2022-Oct-12, Amit Kapila wrote: > > > I think it is a good idea to expand the docs for this but note that > > there are multiple places that use a similar description. For example, > > see the description slot_name option: "When slot_name is set to NONE, > > there will be no replication slot associated with the subscription. > > This can be used if the replication slot will be created later > > manually. Such subscriptions must also have both enabled and > > create_slot set to false.". Then, we have a few places in the logical > > replication docs [1] that talk about creating the slot manually but > > didn't explain in detail the name or options to use. We might want to > > write a slightly bigger doc patch so that we can write the description > > in one place and give reference to the same at other places. > > +1 > Okay, then I think we can commit the last error message patch of Peter [1] as we have an agreement on the same, and then work on this as a separate patch. What do you think? [1] - https://www.postgresql.org/message-id/CAHut%2BPtgkebavGYsGnROkY1%3DULhJ5%2Byn4_i3Y9E9%2ByDeksqpwQ%40mail.gmail.com -- With Regards, Amit Kapila.
On 2022-Oct-12, Amit Kapila wrote: > Okay, then I think we can commit the last error message patch of Peter > [1] as we have an agreement on the same, and then work on this as a > separate patch. What do you think? No objection. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2022-Oct-12, Amit Kapila wrote:
>> Okay, then I think we can commit the last error message patch of Peter
>> [1] as we have an agreement on the same, and then work on this as a
>> separate patch. What do you think?
> No objection.
Yeah, the v3-0001 patch is fine. I agree that the docs change needs
more work.
regards, tom lane
On Thu, Oct 13, 2022 at 2:01 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > On 2022-Oct-12, Amit Kapila wrote: > >> Okay, then I think we can commit the last error message patch of Peter > >> [1] as we have an agreement on the same, and then work on this as a > >> separate patch. What do you think? > > > No objection. > > Yeah, the v3-0001 patch is fine. I agree that the docs change needs > more work. Thanks to everybody for the feedback/suggestions. I will work on improving the documentation for this and post something in a day or so. ------ Kind Regards, Peter Smith. Fujitsu Australia.
On Wed, Oct 12, 2022 at 8:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > On 2022-Oct-12, Amit Kapila wrote: > >> Okay, then I think we can commit the last error message patch of Peter > >> [1] as we have an agreement on the same, and then work on this as a > >> separate patch. What do you think? > > > No objection. > > Yeah, the v3-0001 patch is fine. > Pushed this one. -- With Regards, Amit Kapila.
On Thu, Oct 13, 2022 at 9:07 AM Peter Smith <smithpb2250@gmail.com> wrote: > > On Thu, Oct 13, 2022 at 2:01 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > On 2022-Oct-12, Amit Kapila wrote: > > >> Okay, then I think we can commit the last error message patch of Peter > > >> [1] as we have an agreement on the same, and then work on this as a > > >> separate patch. What do you think? > > > > > No objection. > > > > Yeah, the v3-0001 patch is fine. I agree that the docs change needs > > more work. > > Thanks to everybody for the feedback/suggestions. I will work on > improving the documentation for this and post something in a day or > so. PSA a patch for adding examples of how to activate a subscription that was originally created in a disconnected mode. The new docs are added as part of the "Logical Replication - Subscription" section 31.2. The CREATE SUBSCRIPTION reference page was also updated to include links to the new docs. Feedback welcome. ------ Kind Regards, Peter Smith. Fujitsu Australia.
Attachment
On Fri, Oct 14, 2022 at 8:22 AM Peter Smith <smithpb2250@gmail.com> wrote: > > On Thu, Oct 13, 2022 at 9:07 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > On Thu, Oct 13, 2022 at 2:01 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > > On 2022-Oct-12, Amit Kapila wrote: > > > >> Okay, then I think we can commit the last error message patch of Peter > > > >> [1] as we have an agreement on the same, and then work on this as a > > > >> separate patch. What do you think? > > > > > > > No objection. > > > > > > Yeah, the v3-0001 patch is fine. I agree that the docs change needs > > > more work. > > > > Thanks to everybody for the feedback/suggestions. I will work on > > improving the documentation for this and post something in a day or > > so. > > > PSA a patch for adding examples of how to activate a subscription that > was originally created in a disconnected mode. > > The new docs are added as part of the "Logical Replication - > Subscription" section 31.2. > > The CREATE SUBSCRIPTION reference page was also updated to include > links to the new docs. > You have used 'pgoutput' as plugin name in the examples. Shall we mention in some way that this is a default plugin used for built-in logical replication and it is required to use only this one to enable logical replication? -- With Regards, Amit Kapila.
On Sun, Oct 16, 2022 at 12:14 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Oct 14, 2022 at 8:22 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > On Thu, Oct 13, 2022 at 9:07 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > ... > > PSA a patch for adding examples of how to activate a subscription that > > was originally created in a disconnected mode. > > > > The new docs are added as part of the "Logical Replication - > > Subscription" section 31.2. > > > > The CREATE SUBSCRIPTION reference page was also updated to include > > links to the new docs. > > > > You have used 'pgoutput' as plugin name in the examples. Shall we > mention in some way that this is a default plugin used for built-in > logical replication and it is required to use only this one to enable > logical replication? > Updated as sugggested. PSA v5. ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Mon, Oct 17, 2022 9:47 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Sun, Oct 16, 2022 at 12:14 AM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Fri, Oct 14, 2022 at 8:22 AM Peter Smith <smithpb2250@gmail.com>
> wrote:
> > >
> > > On Thu, Oct 13, 2022 at 9:07 AM Peter Smith <smithpb2250@gmail.com>
> wrote:
> > > >
> ...
> > > PSA a patch for adding examples of how to activate a subscription that
> > > was originally created in a disconnected mode.
> > >
> > > The new docs are added as part of the "Logical Replication -
> > > Subscription" section 31.2.
> > >
> > > The CREATE SUBSCRIPTION reference page was also updated to include
> > > links to the new docs.
> > >
> >
> > You have used 'pgoutput' as plugin name in the examples. Shall we
> > mention in some way that this is a default plugin used for built-in
> > logical replication and it is required to use only this one to enable
> > logical replication?
> >
>
> Updated as sugggested.
>
> PSA v5.
>
Thanks for your patch. Here are some comments.
In Example 2, the returned slot_name should be "myslot".
+test_pub=# SELECT * FROM pg_create_logical_replication_slot('myslot', 'pgoutput');
+ slot_name | lsn
+-----------+-----------
+ sub1 | 0/19059A0
+(1 row)
Besides, I am thinking is it possible to slightly simplify the example. For
example, merge example 1 and 2, keep the steps of example 2 and in the step of
creating slot, mention what should we do if slot_name is not specified when
creating subscription.
Regards,
Shi yu
On Mon, Oct 17, 2022 at 7:17 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > Updated as sugggested. > + <para> + Sometimes, either by choice (e.g. <literal>create_slot = false</literal>), + or by necessity (e.g. <literal>connect = false</literal>), the remote + replication slot is not created automatically during + <literal>CREATE SUBSCRIPTION</literal>. In these cases the user will have + to create the slot manually before the subscription can be activated. + </para> This part looks a bit odd when in the previous section we have explained the same thing in different words. I think it may be better if we start with something like: "As mentioned in the previous section, there are cases where we need to create the slot manually before the subscription can be activated.". I think you can even combine the next para in the patch with this one. Also, it looks odd that the patch uses examples to demonstrate how to manually create a slot, and then we have a separate section whose title is Examples. I am not sure what is the best way to arrange docs here but maybe we can consider renaming the Examples section to something more specific. -- With Regards, Amit Kapila.
Thanks for the feedback. On Mon, Oct 17, 2022 at 10:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Mon, Oct 17, 2022 at 7:17 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > > > Updated as sugggested. > > > > + <para> > + Sometimes, either by choice (e.g. <literal>create_slot = false</literal>), > + or by necessity (e.g. <literal>connect = false</literal>), the remote > + replication slot is not created automatically during > + <literal>CREATE SUBSCRIPTION</literal>. In these cases the user will have > + to create the slot manually before the subscription can be activated. > + </para> > > This part looks a bit odd when in the previous section we have > explained the same thing in different words. I think it may be better > if we start with something like: "As mentioned in the previous > section, there are cases where we need to create the slot manually > before the subscription can be activated.". I think you can even > combine the next para in the patch with this one. Modified the text and combined the paragraphs as suggested. > > Also, it looks odd that the patch uses examples to demonstrate how to > manually create a slot, and then we have a separate section whose > title is Examples. I am not sure what is the best way to arrange docs > here but maybe we can consider renaming the Examples section to > something more specific. > Renamed the examples sections to make their purpose clearer. ~~~ PSA patch v6 with the above changes + one correction from Shi-san [1] ------ [1] https://www.postgresql.org/message-id/OSZPR01MB631051BA9AAA728CAA8CBD88FD299%40OSZPR01MB6310.jpnprd01.prod.outlook.com Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Mon, Oct 17, 2022 at 7:11 PM shiy.fnst@fujitsu.com
<shiy.fnst@fujitsu.com> wrote:
>
...
>
> Thanks for your patch. Here are some comments.
>
> In Example 2, the returned slot_name should be "myslot".
>
> +test_pub=# SELECT * FROM pg_create_logical_replication_slot('myslot', 'pgoutput');
> + slot_name | lsn
> +-----------+-----------
> + sub1 | 0/19059A0
> +(1 row)
>
Oops. Sorry for my cut/paste error. Fixed in patch v6.
> Besides, I am thinking is it possible to slightly simplify the example. For
> example, merge example 1 and 2, keep the steps of example 2 and in the step of
> creating slot, mention what should we do if slot_name is not specified when
> creating subscription.
>
Sure, it might be a bit shorter to combine the examples, but I thought
it’s just simpler not to do it that way because the combined example
will then need additional bullets/notes to say – “if there is no
slot_name do this…” and “if there is a slot_name do that…”. It’s
really only the activation part which is identical for both.
-----
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Oct 18, 2022 5:44 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, Oct 17, 2022 at 7:11 PM shiy.fnst@fujitsu.com
> <shiy.fnst@fujitsu.com> wrote:
> >
> ...
> >
> > Thanks for your patch. Here are some comments.
> >
> > In Example 2, the returned slot_name should be "myslot".
> >
> > +test_pub=# SELECT * FROM pg_create_logical_replication_slot('myslot',
> 'pgoutput');
> > + slot_name | lsn
> > +-----------+-----------
> > + sub1 | 0/19059A0
> > +(1 row)
> >
>
> Oops. Sorry for my cut/paste error. Fixed in patch v6.
>
>
> > Besides, I am thinking is it possible to slightly simplify the example. For
> > example, merge example 1 and 2, keep the steps of example 2 and in the
> step of
> > creating slot, mention what should we do if slot_name is not specified when
> > creating subscription.
> >
>
> Sure, it might be a bit shorter to combine the examples, but I thought
> it’s just simpler not to do it that way because the combined example
> will then need additional bullets/notes to say – “if there is no
> slot_name do this…” and “if there is a slot_name do that…”. It’s
> really only the activation part which is identical for both.
>
Thanks for updating the patch.
+test_sub=# CREATE SUBSCRIPTION sub1
+test_sub-# CONNECTION 'host=localhost dbname=test_pub'
+test_sub-# PUBLICATION pub1
+test_sub-# WITH (slot_name=NONE, enabled=false, create_slot=false);
+WARNING: subscription was created, but is not connected
+HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and refresh
thesubscription.
+CREATE SUBSCRIPTION
In example 3, there is actually no such warning message when creating
subscription because "connect=false" is not specified.
I have tested the examples in the patch and didn't see any problem other than
the one above.
Regards,
Shi yu
On Wed, Oct 19, 2022 at 2:44 PM shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com> wrote: > ... > > +test_sub=# CREATE SUBSCRIPTION sub1 > +test_sub-# CONNECTION 'host=localhost dbname=test_pub' > +test_sub-# PUBLICATION pub1 > +test_sub-# WITH (slot_name=NONE, enabled=false, create_slot=false); > +WARNING: subscription was created, but is not connected > +HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and refresh thesubscription. > +CREATE SUBSCRIPTION > > In example 3, there is actually no such warning message when creating > subscription because "connect=false" is not specified. > Oh, thanks for finding and reporting that. Sorry for my cut/paste errors. Fixed in v7. PSA, > I have tested the examples in the patch and didn't see any problem other than > the one above. > Thanks for your testing. ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Wed, Oct 19, 2022 at 10:10 AM Peter Smith <smithpb2250@gmail.com> wrote: > > Thanks for your testing. > LGTM so pushed with a minor change in one of the titles in the Examples section. -- With Regards, Amit Kapila.
On Wed, Nov 2, 2022 at 9:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Oct 19, 2022 at 10:10 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > Thanks for your testing. > > > > LGTM so pushed with a minor change in one of the titles in the Examples section. > Thanks for pushing. ------ Kind Regards, Peter Smith. Fujitsu Australia