error message when subscription target is a partitioned table

Hi,

Could we improve the error message that's output when the subscription
target relation is a partitioned table? Currently, we get:

ERROR: logical replication target relation "public.foo" is not a table

I think it'd be more helpful to get:

ERROR: "public.foo" is a partitioned table
DETAIL: Partitioned tables are not supported as logical replication targets

Thoughts?

+1.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Mon, Dec 3, 2018 at 7:39 AM Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

Hi,

Could we improve the error message that's output when the subscription
target relation is a partitioned table? Currently, we get:

ERROR: logical replication target relation "public.foo" is not a table

I think it'd be more helpful to get:

ERROR: "public.foo" is a partitioned table
DETAIL: Partitioned tables are not supported as logical replication

targets

Thoughts?

+1

+1 as well. That is definitely confusing -- to most people, that is still a
table...

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On 2018/12/03 17:51, Magnus Hagander wrote:

On Mon, Dec 3, 2018 at 7:39 AM Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

Could we improve the error message that's output when the subscription
target relation is a partitioned table? Currently, we get:

ERROR: logical replication target relation "public.foo" is not a table

I think it'd be more helpful to get:

ERROR: "public.foo" is a partitioned table
DETAIL: Partitioned tables are not supported as logical replication

targets

Thoughts?

+1

+1 as well. That is definitely confusing -- to most people, that is still a
table...

Okay, here is a patch. I didn't find any tests in subscription.sql
related to unsupported relkinds, so didn't bother adding one for this case
either.

Thanks,
Amit

On Tue, Dec 04, 2018 at 10:51:40AM +0900, Amit Langote wrote:

Okay, here is a patch. I didn't find any tests in subscription.sql
related to unsupported relkinds, so didn't bother adding one for this case
either.

Should we care about other relkinds as well?
--
Michael

On 2018/12/04 11:23, Michael Paquier wrote:

On Tue, Dec 04, 2018 at 10:51:40AM +0900, Amit Langote wrote:

Okay, here is a patch. I didn't find any tests in subscription.sql
related to unsupported relkinds, so didn't bother adding one for this case
either.

Should we care about other relkinds as well?

Maybe, foreign tables? I'd think that code wouldn't care about explictly
handling indexes, view, sequences, toast tables, etc.

Thanks,
Amit

On Tue, Dec 4, 2018 at 3:38 AM Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
wrote:

On 2018/12/04 11:23, Michael Paquier wrote:

On Tue, Dec 04, 2018 at 10:51:40AM +0900, Amit Langote wrote:

Okay, here is a patch. I didn't find any tests in subscription.sql
related to unsupported relkinds, so didn't bother adding one for this

case

either.

Should we care about other relkinds as well?

Maybe, foreign tables? I'd think that code wouldn't care about explictly
handling indexes, view, sequences, toast tables, etc.

I think more people would directly understand the "is not a table" for a
foreign table than a partitioned one (for example, it does now show up in
\dt or under tables in pgadmin, but partitioned ones do). That said, if
it's not too complicated, I think including foreign tables as well would
definitely be useful, because it has table in the name. For the other
types, I agree they don't need to be special-cased, they are fine the way
they are.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On Tue, Dec 04, 2018 at 09:25:09AM +0100, Magnus Hagander wrote:

I think more people would directly understand the "is not a table" for a
foreign table than a partitioned one (for example, it does now show up in
\dt or under tables in pgadmin, but partitioned ones do). That said, if
it's not too complicated, I think including foreign tables as well would
definitely be useful, because it has table in the name. For the other
types, I agree they don't need to be special-cased, they are fine the way
they are.

relkind is directly available in this code path, so it is not that hard
to add. As you suggest, foreign tables make sense to add as those are
actually *tables*. And it seems to me that we should also add toast
tables for clarity for the same reason.
--
Michael

On 2018/12/05 10:20, Michael Paquier wrote:

On Tue, Dec 04, 2018 at 09:25:09AM +0100, Magnus Hagander wrote:

I think more people would directly understand the "is not a table" for a
foreign table than a partitioned one (for example, it does now show up in
\dt or under tables in pgadmin, but partitioned ones do). That said, if
it's not too complicated, I think including foreign tables as well would
definitely be useful, because it has table in the name. For the other
types, I agree they don't need to be special-cased, they are fine the way
they are.

relkind is directly available in this code path, so it is not that hard
to add. As you suggest, foreign tables make sense to add as those are
actually *tables*. And it seems to me that we should also add toast
tables for clarity for the same reason.

Considering toast tables here seems like a stretch to me, because they're
not user defined. Chances of users adding a table to a publication whose
name matches that of a toast table's on the subscription side seems thin
too. Partitioned tables and foreign tables are user-defined and something
they'd expect to be handled appropriately.

Thanks,
Amit

On 2018/12/05 10:28, Amit Langote wrote:

On 2018/12/05 10:20, Michael Paquier wrote:

On Tue, Dec 04, 2018 at 09:25:09AM +0100, Magnus Hagander wrote:

I think more people would directly understand the "is not a table" for a
foreign table than a partitioned one (for example, it does now show up in
\dt or under tables in pgadmin, but partitioned ones do). That said, if
it's not too complicated, I think including foreign tables as well would
definitely be useful, because it has table in the name. For the other
types, I agree they don't need to be special-cased, they are fine the way
they are.

relkind is directly available in this code path, so it is not that hard
to add. As you suggest, foreign tables make sense to add as those are
actually *tables*. And it seems to me that we should also add toast
tables for clarity for the same reason.

Considering toast tables here seems like a stretch to me, because they're
not user defined. Chances of users adding a table to a publication whose
name matches that of a toast table's on the subscription side seems thin
too. Partitioned tables and foreign tables are user-defined and something
they'd expect to be handled appropriately.

Attached updated patch that adds the check for foreign tables.

Thanks,
Amit

On 2018/12/06 11:28, Amit Langote wrote:

On 2018/12/05 10:28, Amit Langote wrote:

On 2018/12/05 10:20, Michael Paquier wrote:

On Tue, Dec 04, 2018 at 09:25:09AM +0100, Magnus Hagander wrote:

I think more people would directly understand the "is not a table" for a
foreign table than a partitioned one (for example, it does now show up in
\dt or under tables in pgadmin, but partitioned ones do). That said, if
it's not too complicated, I think including foreign tables as well would
definitely be useful, because it has table in the name. For the other
types, I agree they don't need to be special-cased, they are fine the way
they are.

relkind is directly available in this code path, so it is not that hard
to add. As you suggest, foreign tables make sense to add as those are
actually *tables*. And it seems to me that we should also add toast
tables for clarity for the same reason.

Considering toast tables here seems like a stretch to me, because they're
not user defined. Chances of users adding a table to a publication whose
name matches that of a toast table's on the subscription side seems thin
too. Partitioned tables and foreign tables are user-defined and something
they'd expect to be handled appropriately.

Attached updated patch that adds the check for foreign tables.

Adding to January CF.

Thanks,
Amit

On Thu, Dec 06, 2018 at 11:34:19AM +0900, Amit Langote wrote:

Adding to January CF.

Okay, that looks good to me based on your arguments upthread. A
small-ish comment I have is that you could use a set of if/else if
conditions instead of separate ifs.
--
Michael

On 2018/12/06 13:19, Michael Paquier wrote:

On Thu, Dec 06, 2018 at 11:34:19AM +0900, Amit Langote wrote:

Adding to January CF.

Okay, that looks good to me based on your arguments upthread.

Thanks for looking.

A
small-ish comment I have is that you could use a set of if/else if
conditions instead of separate ifs.

Okay, no problem. Updated patch attached.

Thanks,
Amit

On 06/12/2018 05:46, Amit Langote wrote:

/*
* We currently only support writing to regular tables.
*/

I think that comment should stay above the code you are adding.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Thanks for reviewing.

On 2018/12/31 20:23, Peter Eisentraut wrote:

On 06/12/2018 05:46, Amit Langote wrote:

/*
* We currently only support writing to regular tables.
*/

I think that comment should stay above the code you are adding.

Do you mean to keep it at the top and expand it to mention the point about
partitioned and foreign tables, like this:

     /*
-     * We currently only support writing to regular tables.
+     * We currently only support writing to regular tables.  However, give
+     * a more specific error for partitioned and foreign tables.
      */
+    if (relkind == RELKIND_PARTITIONED_TABLE)

If so, that makes sense. I've updated the patch like that.

Thanks,
Amit

On Mon, Jan 07, 2019 at 01:49:49PM +0900, Amit Langote wrote:

{
/*
-	 * We currently only support writing to regular tables.
+	 * We currently only support writing to regular tables.  However, give
+	 * a more specific error for partitioned and foreign tables.
*/
+	if (relkind == RELKIND_PARTITIONED_TABLE)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s.%s\" is a partitioned table",
+						nspname, relname),
+				 errdetail("Partitioned tables are not
supported as logical replication targets.")));

Could it be possible to avoid a full sentence in the primary error
message? Usually these are avoided:
https://www.postgresql.org/docs/devel/error-style-guide.html

It seems to me that we may want something more like:
Primary: "could not use \"%s.%s\" as logical replication target".
Detail: "Relation %s.%s is a foreign table", "not a table", etc.

The existing error message in CheckSubscriptionRelkind() could also be
better regarding that...
--
Michael

On 2019/01/07 16:35, Michael Paquier wrote:

On Mon, Jan 07, 2019 at 01:49:49PM +0900, Amit Langote wrote:

{
/*
-	 * We currently only support writing to regular tables.
+	 * We currently only support writing to regular tables.  However, give
+	 * a more specific error for partitioned and foreign tables.
*/
+	if (relkind == RELKIND_PARTITIONED_TABLE)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s.%s\" is a partitioned table",
+						nspname, relname),
+				 errdetail("Partitioned tables are not
supported as logical replication targets.")));

Could it be possible to avoid a full sentence in the primary error
message? Usually these are avoided:
https://www.postgresql.org/docs/devel/error-style-guide.html

It seems to me that we may want something more like:
Primary: "could not use \"%s.%s\" as logical replication target".
Detail: "Relation %s.%s is a foreign table", "not a table", etc.

I've thought about that before and I tend to agree with you. Maybe:

ERROR: cannot use "%s.%s" as logical replication target
DETAIL: Using partitioned tables as logical replication target is not
supported.

Sounds a bit repetitive, but perhaps it's better to use the words "not
supported" in the DETAIL message.

Thanks,
Amit

On Mon, Jan 07, 2019 at 05:28:27PM +0900, Amit Langote wrote:

On 2019/01/07 16:35, Michael Paquier wrote:

It seems to me that we may want something more like:
Primary: "could not use \"%s.%s\" as logical replication target".
Detail: "Relation %s.%s is a foreign table", "not a table", etc.

I've thought about that before and I tend to agree with you. Maybe:

ERROR: cannot use "%s.%s" as logical replication target
DETAIL: Using partitioned tables as logical replication target is not
supported.

Sounds a bit repetitive, but perhaps it's better to use the words "not
supported" in the DETAIL message.

Or the detailed message could just say "\"%s.%s\" is a foreign table"
and such flavor for other relkinds? It is redundant to repeat
"logical replication target" for both message parts. The primary
message to use "cannot" instead of "could" is much better, so that
part sounds fine to me.
--
Michael

On 2019/01/08 11:10, Michael Paquier wrote:

On Mon, Jan 07, 2019 at 05:28:27PM +0900, Amit Langote wrote:

On 2019/01/07 16:35, Michael Paquier wrote:

It seems to me that we may want something more like:
Primary: "could not use \"%s.%s\" as logical replication target".
Detail: "Relation %s.%s is a foreign table", "not a table", etc.

I've thought about that before and I tend to agree with you. Maybe:

ERROR: cannot use "%s.%s" as logical replication target
DETAIL: Using partitioned tables as logical replication target is not
supported.

Sounds a bit repetitive, but perhaps it's better to use the words "not
supported" in the DETAIL message.

Or the detailed message could just say "\"%s.%s\" is a foreign table"
and such flavor for other relkinds? It is redundant to repeat
"logical replication target" for both message parts.

Yeah, I think so too. I also noticed that the patch uses
ERRCODE_WRONG_OBJECT_TYPE as the error code, whereas we may want to use
ERRCODE_FEATURE_NOT_SUPPORTED. Thoughts on that?

Attached updated patch, which changes the detail message text as you
suggest and updates the error code.

Thanks,
Amit

On Tue, Jan 08, 2019 at 01:13:11PM +0900, Amit Langote wrote:

Yeah, I think so too. I also noticed that the patch uses
ERRCODE_WRONG_OBJECT_TYPE as the error code, whereas we may want to use
ERRCODE_FEATURE_NOT_SUPPORTED. Thoughts on that?

ERRCODE_WRONG_OBJECT_TYPE is the right call I think as the feature is
actually supported, just not for all the object types.

Attached updated patch, which changes the detail message text as you
suggest and updates the error code.

Another suggestion I would have is also to change the third message of
CheckSubscriptionRelkind() so as its style maps the two others you are
adding, as what's on HEAD is not a model of its kind with its
formulation using a full sentence.
--
Michael