error message when subscription target is a partitioned table

classic Classic list List threaded Threaded
31 messages Options
12
Reply | Threaded
Open this post in threaded view
|

error message when subscription target is a partitioned table

Amit Langote-2
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?

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Tatsuo Ishii-3
> 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

Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Magnus Hagander-2

On Mon, Dec 3, 2018 at 7:39 AM Tatsuo Ishii <[hidden email]> 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... 


--
Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Amit Langote-2
On 2018/12/03 17:51, Magnus Hagander wrote:

> On Mon, Dec 3, 2018 at 7:39 AM Tatsuo Ishii <[hidden email]> 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

partitioned-table-not-supported-error-logrep.patch (1006 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Amit Langote-2
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


Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Magnus Hagander-2

On Tue, Dec 4, 2018 at 3:38 AM Amit Langote <[hidden email]> 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. 

--
Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Amit Langote-2
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


Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Amit Langote-2
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

partitioned-and-foreign-table-not-supported-error-logrep.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Amit Langote-2
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



Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Amit Langote-2
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

partitioned-table-not-supported-error-logrep-v3.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: error message when subscription target is a partitioned table

Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

Re: [Sender Address Forgery]Re: error message when subscription target is a partitioned table

Amit Langote-2
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

partitioned-table-not-supported-error-logrep-v4.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Sender Address Forgery]Re: error message when subscription target is a partitioned table

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Sender Address Forgery]Re: error message when subscription target is a partitioned table

Amit Langote-2
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


Reply | Threaded
Open this post in threaded view
|

Re: [Sender Address Forgery]Re: error message when subscription target is a partitioned table

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Sender Address Forgery]Re: error message when subscription target is a partitioned table

Amit Langote-2
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

partitioned-table-not-supported-error-logrep-v5.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Sender Address Forgery]Re: error message when subscription target is a partitioned table

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
12