create subscription - improved warning message

Started by Peter Smithover 3 years ago33 messageshackers
Jump to latest
#1Peter Smith
smithpb2250@gmail.com

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

Attachments:

v1-0001-create-subscription-warning-message.patchapplication/octet-stream; name=v1-0001-create-subscription-warning-message.patchDownload+22-12
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Smith (#1)
Re: create subscription - improved warning message

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

#3Peter Smith
smithpb2250@gmail.com
In reply to: Tom Lane (#2)
Re: create subscription - improved warning message

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

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Smith (#3)
Re: create subscription - improved warning message

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

#5Amit Kapila
amit.kapila16@gmail.com
In reply to: Peter Smith (#3)
Re: create subscription - improved warning message

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.

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Amit Kapila (#5)
Re: create subscription - improved warning message

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

#7Amit Kapila
amit.kapila16@gmail.com
In reply to: Tom Lane (#6)
Re: create subscription - improved warning message

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]https://www.postgresql.org/docs/devel/logical-replication-subscription.html: "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.

#8Peter Smith
smithpb2250@gmail.com
In reply to: Amit Kapila (#7)
Re: create subscription - improved warning message

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

Attachments:

v2-0001-create-subscription-warning-message.patchapplication/octet-stream; name=v2-0001-create-subscription-warning-message.patchDownload+22-12
#9Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#6)
Re: create subscription - improved warning message

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

#10Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Smith (#8)
Re: create subscription - improved warning message

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

#11Amit Kapila
amit.kapila16@gmail.com
In reply to: Tom Lane (#10)
Re: create subscription - improved warning message

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]https://www.postgresql.org/docs/devel/sql-createsubscription.html? 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.

#12Peter Smith
smithpb2250@gmail.com
In reply to: Amit Kapila (#11)
Re: create subscription - improved warning message

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

Attachments:

v3-0002-create-subscription-pgdocs.patchapplication/octet-stream; name=v3-0002-create-subscription-pgdocs.patchDownload+3-6
v3-0001-create-subscription-warning-message.patchapplication/octet-stream; name=v3-0001-create-subscription-warning-message.patchDownload+22-14
#13Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Peter Smith (#8)
Re: create subscription - improved warning message

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)

#14Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Peter Smith (#12)
Re: create subscription - improved warning message

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/

#15Amit Kapila
amit.kapila16@gmail.com
In reply to: Alvaro Herrera (#13)
Re: create subscription - improved warning message

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.

#16Amit Kapila
amit.kapila16@gmail.com
In reply to: Alvaro Herrera (#14)
Re: create subscription - improved warning message

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]https://www.postgresql.org/docs/devel/logical-replication-subscription.html 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.

#17Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Amit Kapila (#16)
Re: create subscription - improved warning message

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"

#18Amit Kapila
amit.kapila16@gmail.com
In reply to: Alvaro Herrera (#17)
Re: create subscription - improved warning message

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]: /messages/by-id/CAHut+PtgkebavGYsGnROkY1=ULhJ5+yn4_i3Y9E9+yDeksqpwQ@mail.gmail.com
separate patch. What do you think?

[1]: /messages/by-id/CAHut+PtgkebavGYsGnROkY1=ULhJ5+yn4_i3Y9E9+yDeksqpwQ@mail.gmail.com

--
With Regards,
Amit Kapila.

#19Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Amit Kapila (#18)
Re: create subscription - improved warning message

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/

#20Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#19)
Re: create subscription - improved warning message

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

#21Peter Smith
smithpb2250@gmail.com
In reply to: Tom Lane (#20)
#22Amit Kapila
amit.kapila16@gmail.com
In reply to: Tom Lane (#20)
#23Peter Smith
smithpb2250@gmail.com
In reply to: Peter Smith (#21)
#24Amit Kapila
amit.kapila16@gmail.com
In reply to: Peter Smith (#23)
#25Peter Smith
smithpb2250@gmail.com
In reply to: Amit Kapila (#24)
#26shiy.fnst@fujitsu.com
shiy.fnst@fujitsu.com
In reply to: Peter Smith (#25)
#27Amit Kapila
amit.kapila16@gmail.com
In reply to: Peter Smith (#25)
#28Peter Smith
smithpb2250@gmail.com
In reply to: Amit Kapila (#27)
#29Peter Smith
smithpb2250@gmail.com
In reply to: shiy.fnst@fujitsu.com (#26)
#30shiy.fnst@fujitsu.com
shiy.fnst@fujitsu.com
In reply to: Peter Smith (#29)
#31Peter Smith
smithpb2250@gmail.com
In reply to: shiy.fnst@fujitsu.com (#30)
#32Amit Kapila
amit.kapila16@gmail.com
In reply to: Peter Smith (#31)
#33Peter Smith
smithpb2250@gmail.com
In reply to: Amit Kapila (#32)