Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

Started by houzj.fnst@fujitsu.comabout 3 years ago10 messages

about 3 years ago

1 attachment(s)

Hi,

While working on some logical replication related features.
I found the HINT message could be improved when I tried to add a publication to
a subscription which was disabled.

alter subscription sub add publication pub2;
--
ERROR: ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions
HINT: Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).
--

Because I was executing the ADD PUBLICATION command, I feel the hint should
also mention it instead of SET PUBLICATION.

Best regards,
Hou zj

Attachments:

0001-Improve-errhint-for-ALTER-SUBSCRIPTION-ADD-DROP-PUBL.patchapplication/octet-stream; name=0001-Improve-errhint-for-ALTER-SUBSCRIPTION-ADD-DROP-PUBL.patchDownload

From ecc5dd960c4994998b556409d7d548d582c8c629 Mon Sep 17 00:00:00 2001
From: "houzj.fnst" <houzj.fnst@cn.fujitsu.com>
Date: Mon, 17 Oct 2022 10:57:38 +0800
Subject: [PATCH] Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

---
 src/backend/commands/subscriptioncmds.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8fb89a9..2fe88a3 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1226,7 +1226,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 						ereport(ERROR,
 								(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 								 errmsg("ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions"),
-								 errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).")));
+								 errhint("Use ALTER SUBSCRIPTION ... %s PUBLICATION ... WITH (refresh = false).",
+										 isadd ? "ADD" : "DROP")));
 
 					/*
 					 * See ALTER_SUBSCRIPTION_REFRESH for details why this is
@@ -1236,8 +1237,9 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 						ereport(ERROR,
 								(errcode(ERRCODE_SYNTAX_ERROR),
 								 errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed when two_phase is enabled"),
-								 errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh = false, or with copy_data = false"
-										 ", or use DROP/CREATE SUBSCRIPTION.")));
+								 errhint("Use ALTER SUBSCRIPTION ... %s PUBLICATION with refresh = false, or with copy_data = false"
+										 ", or use DROP/CREATE SUBSCRIPTION.",
+										 isadd ? "ADD" : "DROP")));
 
 					PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with refresh");
 
-- 
2.7.2.windows.1

Amit Kapila

amit.kapila16@gmail.com

about 3 years ago

In reply to: houzj.fnst@fujitsu.com (#1)

Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

On Mon, Oct 17, 2022 at 8:39 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

While working on some logical replication related features.
I found the HINT message could be improved when I tried to add a publication to
a subscription which was disabled.

alter subscription sub add publication pub2;
--
ERROR: ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions
HINT: Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).
--

Because I was executing the ADD PUBLICATION command, I feel the hint should
also mention it instead of SET PUBLICATION.

+1. I haven't tested it yet but the changes look sane.

--
With Regards,
Amit Kapila.

Alvaro Herrera

alvherre@alvh.no-ip.org

about 3 years ago

In reply to: houzj.fnst@fujitsu.com (#1)

Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

Hello

On 2022-Oct-17, houzj.fnst@fujitsu.com wrote:

alter subscription sub add publication pub2;

Because I was executing the ADD PUBLICATION command, I feel the hint should
also mention it instead of SET PUBLICATION.

Hmm, ok. But:

@@ -1236,8 +1237,9 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed when two_phase is enabled"),
-								 errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh = false, or with copy_data = false"
-										 ", or use DROP/CREATE SUBSCRIPTION.")));
+								 errhint("Use ALTER SUBSCRIPTION ... %s PUBLICATION with refresh = false, or with copy_data = false"
+										 ", or use DROP/CREATE SUBSCRIPTION.",
+										 isadd ? "ADD" : "DROP")));

This looks confusing for translators. I propose to move the whole
command out of the message, not just one piece of it:

+    /*- translator: %s is an ALTER DDL command */
+    errhint("Use %s with refresh = false, or with copy_data = false, or use DROP/CREATE SUBSCRIPTION.",
             isadd ? "ALTER SUBSCRIPTION ... ADD PUBLICATION" : ALTER SUBSCRIPTION ... DROP PUBLICATION")

I'm not sure that ERRCODE_SYNTAX_ERROR is the right thing here; sounds
like ERRCODE_FEATURE_NOT_SUPPORTED might be more appropriate.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"You don't solve a bad join with SELECT DISTINCT" #CupsOfFail
https://twitter.com/connor_mc_d/status/1431240081726115845

Peter Smith

smithpb2250@gmail.com

about 3 years ago

In reply to: Alvaro Herrera (#3)

Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Hello

On 2022-Oct-17, houzj.fnst@fujitsu.com wrote:

alter subscription sub add publication pub2;

Because I was executing the ADD PUBLICATION command, I feel the hint should
also mention it instead of SET PUBLICATION.

Hmm, ok. But:

@@ -1236,8 +1237,9 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed when two_phase is enabled"),
-                                                              errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh = false, or with copy_data = false"
-                                                                              ", or use DROP/CREATE SUBSCRIPTION.")));
+                                                              errhint("Use ALTER SUBSCRIPTION ... %s PUBLICATION with refresh = false, or with copy_data = false"
+                                                                              ", or use DROP/CREATE SUBSCRIPTION.",
+                                                                              isadd ? "ADD" : "DROP")));

This looks confusing for translators. I propose to move the whole
command out of the message, not just one piece of it:

+    /*- translator: %s is an ALTER DDL command */
+    errhint("Use %s with refresh = false, or with copy_data = false, or use DROP/CREATE SUBSCRIPTION.",
isadd ? "ALTER SUBSCRIPTION ... ADD PUBLICATION" : ALTER SUBSCRIPTION ... DROP PUBLICATION")

I'm not sure that ERRCODE_SYNTAX_ERROR is the right thing here; sounds
like ERRCODE_FEATURE_NOT_SUPPORTED might be more appropriate.

I thought maybe ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE, which would
make it the same as similar messages in the same function when
incompatible parameters are specified.

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

Alvaro Herrera

alvherre@alvh.no-ip.org

about 3 years ago

In reply to: Peter Smith (#4)

Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

On 2022-Oct-17, Peter Smith wrote:

On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I'm not sure that ERRCODE_SYNTAX_ERROR is the right thing here; sounds
like ERRCODE_FEATURE_NOT_SUPPORTED might be more appropriate.

I thought maybe ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE, which would
make it the same as similar messages in the same function when
incompatible parameters are specified.

Hmm, yeah, I guess that's also a possibility.

Maybe we need a specific errcode, "incompatible logical replication
configuration", within that class ("object not in prerequisite state" is
a generic SQLSTATE class 55), given that the class itself is a mishmash
of completely unrelated things. I think I already mentioned this in
some other thread ... ah yes:

/messages/by-id/20220928084641.xecjrgym476fihtn@alvherre.pgsql
"incompatible publication definition" 55PR1 is what I suggested then.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Hay dos momentos en la vida de un hombre en los que no debería
especular: cuando puede permitírselo y cuando no puede" (Mark Twain)

Amit Kapila

amit.kapila16@gmail.com

about 3 years ago

In reply to: Alvaro Herrera (#5)

Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

On Mon, Oct 17, 2022 at 2:41 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2022-Oct-17, Peter Smith wrote:

On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I'm not sure that ERRCODE_SYNTAX_ERROR is the right thing here; sounds
like ERRCODE_FEATURE_NOT_SUPPORTED might be more appropriate.

I thought maybe ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE, which would
make it the same as similar messages in the same function when
incompatible parameters are specified.

Hmm, yeah, I guess that's also a possibility.

Right, ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE seems to suite better here.

Maybe we need a specific errcode, "incompatible logical replication
configuration", within that class ("object not in prerequisite state" is
a generic SQLSTATE class 55), given that the class itself is a mishmash
of completely unrelated things. I think I already mentioned this in
some other thread ... ah yes:

/messages/by-id/20220928084641.xecjrgym476fihtn@alvherre.pgsql
"incompatible publication definition" 55PR1 is what I suggested then.

Yeah, this is another way to deal with it. But, won't it be better to
survey all call sites of ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE and
then try to subdivide it instead of doing it for
subscription/publication cases? I know that is a much bigger ask and
we don't need to do it for this patch but that seems like a more
future-proof way if we can build a consensus for the same.

--
With Regards,
Amit Kapila.

houzj.fnst@fujitsu.com

about 3 years ago

In reply to: Amit Kapila (#6)

1 attachment(s)

RE: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

On Monday, October 17, 2022 6:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Oct 17, 2022 at 2:41 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

On 2022-Oct-17, Peter Smith wrote:

On Mon, Oct 17, 2022 at 6:43 PM Alvaro Herrera

<alvherre@alvh.no-ip.org> wrote:

I'm not sure that ERRCODE_SYNTAX_ERROR is the right thing here;
sounds like ERRCODE_FEATURE_NOT_SUPPORTED might be more

appropriate.

I thought maybe ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE, which
would make it the same as similar messages in the same function when
incompatible parameters are specified.

Hmm, yeah, I guess that's also a possibility.

Right, ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE seems to suite better
here.

Agreed. Here is new version patch which changed the error code and
moved the whole command out of the message according to Álvaro's comment.

Best regards,
Hou zj

Attachments:

v2-0001-Improve-errhint-for-ALTER-SUBSCRIPTION-ADD-DROP-PUBL.patchapplication/octet-stream; name=v2-0001-Improve-errhint-for-ALTER-SUBSCRIPTION-ADD-DROP-PUBL.patchDownload

From 9cbfe5ced8c650c7b43227c1b9f7a4da2a0d2c51 Mon Sep 17 00:00:00 2001
From: "houzj.fnst" <houzj.fnst@cn.fujitsu.com>
Date: Mon, 17 Oct 2022 10:57:38 +0800
Subject: [PATCH] Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

Improve the HINT message when adding or dropping a publication on disabled
subscription. While on it, also change the error code of related messages to a
more appropriate one.

---
 src/backend/commands/subscriptioncmds.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8fb89a9..2186d9e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1182,7 +1182,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 					 */
 					if (sub->twophasestate == LOGICALREP_TWOPHASE_STATE_ENABLED && opts.copy_data)
 						ereport(ERROR,
-								(errcode(ERRCODE_SYNTAX_ERROR),
+								(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 								 errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed when two_phase is enabled"),
 								 errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh = false, or with copy_data = false"
 										 ", or use DROP/CREATE SUBSCRIPTION.")));
@@ -1226,7 +1226,9 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 						ereport(ERROR,
 								(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 								 errmsg("ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions"),
-								 errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).")));
+						/* translator: %s is an SQL ALTER command */
+								 errhint("Use %s.", isadd ? "ALTER SUBSCRIPTION ... ADD PUBLICATION ... WITH (refresh = false)"
+										 : "ALTER SUBSCRIPTION ... DROP PUBLICATION ... WITH (refresh = false)")));
 
 					/*
 					 * See ALTER_SUBSCRIPTION_REFRESH for details why this is
@@ -1234,10 +1236,13 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 					 */
 					if (sub->twophasestate == LOGICALREP_TWOPHASE_STATE_ENABLED && opts.copy_data)
 						ereport(ERROR,
-								(errcode(ERRCODE_SYNTAX_ERROR),
+								(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 								 errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed when two_phase is enabled"),
-								 errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh = false, or with copy_data = false"
-										 ", or use DROP/CREATE SUBSCRIPTION.")));
+						/* translator: %s is an SQL ALTER command */
+								 errhint("Use %s with refresh = false, or with copy_data = false"
+										 ", or use DROP/CREATE SUBSCRIPTION.",
+										 isadd ? "ALTER SUBSCRIPTION ... ADD PUBLICATION"
+										 : "ALTER SUBSCRIPTION ... DROP PUBLICATION")));
 
 					PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with refresh");
 
-- 
2.7.2.windows.1

Japin Li

japinli@hotmail.com

about 3 years ago

In reply to: houzj.fnst@fujitsu.com (#7)

Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

On Tue, 18 Oct 2022 at 12:00, houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:

Agreed. Here is new version patch which changed the error code and
moved the whole command out of the message according to Álvaro's comment.

My bad! The patch looks good to me.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Alvaro Herrera

alvherre@alvh.no-ip.org

about 3 years ago

In reply to: Japin Li (#8)

Re: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

On 2022-Oct-18, Japin Li wrote:

On Tue, 18 Oct 2022 at 12:00, houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:

Agreed. Here is new version patch which changed the error code and
moved the whole command out of the message according to Álvaro's comment.

My bad! The patch looks good to me.

Thank you, I pushed it to both branches, because I realized we were
saying "SET PUBLICATION" when we meant "ADD/DROP"; that hint could be
quite damaging if anybody decides to actually follow it ISTM.

I noted that no test needed to be changed because of this, which is
perhaps somewhat concerning.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

#10

houzj.fnst@fujitsu.com

about 3 years ago

In reply to: Alvaro Herrera (#9)

RE: Improve errhint for ALTER SUBSCRIPTION ADD/DROP PUBLICATION

On Tuesday, October 18, 2022 5:50 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2022-Oct-18, Japin Li wrote:

On Tue, 18 Oct 2022 at 12:00, houzj.fnst@fujitsu.com

<houzj.fnst@fujitsu.com> wrote:

Agreed. Here is new version patch which changed the error code and
moved the whole command out of the message according to Álvaro's

comment.

My bad! The patch looks good to me.

Thank you, I pushed it to both branches, because I realized we were saying "SET
PUBLICATION" when we meant "ADD/DROP"; that hint could be quite
damaging if anybody decides to actually follow it ISTM.

Thanks for pushing!

Best regards,
Hou zj