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
|

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

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


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

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

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

logirep-relkind-errmsg.patch (1K) Download Attachment
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/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


Reply | Threaded
Open this post in threaded view
|

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

Arkhena

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


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

Cheers,

Lætitia
--
Adoptez l'éco-attitude
N'imprimez ce mail que si c'est vraiment nécessaire
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
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


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

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


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

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

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

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


12