Обсуждение: 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?

Thanks,
Amit

> 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 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 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

On 2019/01/08 13:44, Michael Paquier wrote:
> 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.

I meant to say that the feature to add relations to a subscription is not
supported for certain relation types, even though it's practically
*possible* to do so.  But maybe, I'm misunderstanding the error code
selection policy.

>> 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.

I'm not totally opposed to do that while we're here, but note that there
might be many such instances in the existing code.

Thanks,
Amit

On Tue, Jan 08, 2019 at 03:05:42PM +0900, Amit Langote wrote:
> I meant to say that the feature to add relations to a subscription is not
> supported for certain relation types, even though it's practically
> *possible* to do so.  But maybe, I'm misunderstanding the error code
> selection policy.

I can see your point, though I would stick with
ERRCODE_WRONG_OBJECT_TYPE for consistency with the existing code and
because the code is intended to not work on anything else than plain
tables, at least now.

> I'm not totally opposed to do that while we're here, but note that there
> might be many such instances in the existing code.

Sure.  I am not noticing anything in the surrounding area but it is
easy enough to miss something.
--
Michael

On Tue, Jan 08, 2019 at 04:42:35PM +0900, Michael Paquier wrote:
> I can see your point, though I would stick with
> ERRCODE_WRONG_OBJECT_TYPE for consistency with the existing code and
> because the code is intended to not work on anything else than plain
> tables, at least now.

Attached are my suggestions shaped as a patch.  Thoughts?
--
Michael

On 2019/01/10 14:25, Michael Paquier wrote:
> On Tue, Jan 08, 2019 at 04:42:35PM +0900, Michael Paquier wrote:
>> I can see your point, though I would stick with
>> ERRCODE_WRONG_OBJECT_TYPE for consistency with the existing code and
>> because the code is intended to not work on anything else than plain
>> tables, at least now.
> 
> Attached are my suggestions shaped as a patch.  Thoughts?

Thanks for updating the patch and sorry couldn't reply sooner.

Your rewritten version is perhaps fine, although I remain a bit concerned
that some users might be puzzled when they see this error, that is, if
they interpret the message as "it's impossible to use a partitioned table
as logical replication target".

Thanks,
Amit

Hi,

On 2019/01/10 17:46, Arkhena wrote:
>> Your rewritten version is perhaps fine, although I remain a bit concerned
>> that some users might be puzzled when they see this error, that is, if
>> they interpret the message as "it's impossible to use a partitioned table
>> as logical replication target".
>>
>>
>>From [documentation](
> https://www.postgresql.org/docs/current/logical-replication-restrictions.html
> ) :
>> Attempts to replicate tables other than base tables will result in an
> error.
> 
> That's basicaly what I had understood about logicial replication...

Ah, if the documentation contains such description then maybe it's fine.

The reason I started this thread is due to this Stack Overflow question:

https://stackoverflow.com/questions/53554727/logical-replication-and-declarative-partitioning-in-postgresql-11

So, it appears that there may be an element of surprise involved in
encountering such an error (despite the documentation).

Thanks,
Amit

On Thu, Jan 10, 2019 at 05:58:10PM +0900, Amit Langote wrote:
> The reason I started this thread is due to this Stack Overflow question:
>
> https://stackoverflow.com/questions/53554727/logical-replication-and-declarative-partitioning-in-postgresql-11
>
> So, it appears that there may be an element of surprise involved in
> encountering such an error (despite the documentation).

Improving the user experience is definitely a good thing in my
opinion because the current error message can be confusing, so you
were right to start this thread.  Still I don't agree that classifying
those relkinds as not supported is right either for consistency with
the code existing for two years and for the way the code is designed
to work as rows are replicated on a per-physically-defined relation
basis.
--
Michael

On 2019/01/10 19:27, Michael Paquier wrote:
> On Thu, Jan 10, 2019 at 05:58:10PM +0900, Amit Langote wrote:
>> The reason I started this thread is due to this Stack Overflow question:
>>
>> https://stackoverflow.com/questions/53554727/logical-replication-and-declarative-partitioning-in-postgresql-11
>>
>> So, it appears that there may be an element of surprise involved in
>> encountering such an error (despite the documentation).
> 
> Improving the user experience is definitely a good thing in my
> opinion because the current error message can be confusing, so you
> were right to start this thread.  Still I don't agree that classifying
> those relkinds as not supported is right either for consistency with
> the code existing for two years and for the way the code is designed
> to work as rows are replicated on a per-physically-defined relation
> basis.

Okay, I withdraw my objection to the wording proposed by you.

Thanks,
Amit

On Fri, Jan 11, 2019 at 10:50:32AM +0900, Amit Langote wrote:
> Okay, I withdraw my objection to the wording proposed by you.

Thanks.  I can commit this version if there are no objections then.
--
Michael

On Fri, Jan 11, 2019 at 04:04:41PM +0900, Michael Paquier wrote:
> Thanks.  I can commit this version if there are no objections then.

And done.
--
Michael

On 2019/01/13 16:56, Michael Paquier wrote:
> On Fri, Jan 11, 2019 at 04:04:41PM +0900, Michael Paquier wrote:
>> Thanks.  I can commit this version if there are no objections then.
> 
> And done.

Thank you!

Regards,
Amit