Improve new hash partition bound check error messages

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Improve new hash partition bound check error messages

Peter Eisentraut-7
I had a bit of trouble parsing the error message "every hash partition
modulus must be a factor of the next larger modulus", so I went into the
code, added some comments and added errdetail messages for each case.  I
think it's a bit clearer now.

0001-Improve-new-hash-partition-bound-check-error-message.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Improve new hash partition bound check error messages

Amit Langote
On Tue, Feb 2, 2021 at 7:36 PM Peter Eisentraut
<[hidden email]> wrote:
> I had a bit of trouble parsing the error message "every hash partition
> modulus must be a factor of the next larger modulus", so I went into the
> code, added some comments and added errdetail messages for each case.  I
> think it's a bit clearer now.

That is definitely an improvement, thanks.

--
Amit Langote
EDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Improve new hash partition bound check error messages

Heikki Linnakangas
In reply to this post by Peter Eisentraut-7
On 02/02/2021 12:35, Peter Eisentraut wrote:
> I had a bit of trouble parsing the error message "every hash partition
> modulus must be a factor of the next larger modulus", so I went into the
> code, added some comments and added errdetail messages for each case.  I
> think it's a bit clearer now.

Yeah, that error message is hard to understand. This is an improvement,
but it still took me a while to understand it.

Let's look at the example in the regression test:

-- check partition bound syntax for the hash partition
CREATE TABLE hash_parted (
     a int
) PARTITION BY HASH (a);
CREATE TABLE hpart_1 PARTITION OF hash_parted FOR VALUES WITH (MODULUS
10, REMAINDER 0);
CREATE TABLE hpart_2 PARTITION OF hash_parted FOR VALUES WITH (MODULUS
50, REMAINDER 1);
CREATE TABLE hpart_3 PARTITION OF hash_parted FOR VALUES WITH (MODULUS
200, REMAINDER 2);

With this patch, you get this:

CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS
25, REMAINDER 3);
ERROR:  every hash partition modulus must be a factor of the next larger
modulus
DETAIL:  The existing modulus 10 is not a factor of the new modulus 25.

CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS
150, REMAINDER 3);
ERROR:  every hash partition modulus must be a factor of the next larger
modulus
DETAIL:  The new modulus 150 is not factor of the existing modulus 200.


How about this?

CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS
25, REMAINDER 3);
ERROR:  every hash partition modulus must be a factor of the next larger
modulus
DETAIL:  25 is not divisible by 10, the modulus of existing partition
"hpart_1"

CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS
150, REMAINDER 3);
ERROR:  every hash partition modulus must be a factor of the next larger
modulus
DETAIL:  150 is not a factor of 200, the modulus of existing partition
"hpart_3"

Calling the existing partition by name seems good. And this phrasing
puts the focus on the new modulus in both variants; presumably the
existing partition is OK and the problem is in the new definition.

- Heikki


Reply | Threaded
Open this post in threaded view
|

Re: Improve new hash partition bound check error messages

Peter Eisentraut-7
On 2021-02-02 13:26, Heikki Linnakangas wrote:
> How about this?
>
> CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS
> 25, REMAINDER 3);
> ERROR:  every hash partition modulus must be a factor of the next larger
> modulus
> DETAIL:  25 is not divisible by 10, the modulus of existing partition
> "hpart_1"

I don't know if we can easily get the name of the existing partition.
I'll have to check that.

I'm worried that this phrasing requires the user to understand that
"divisible by" is related to "factor of", which is of course correct but
introduces yet more complexity into this.

I'll play around with this a bit more.



Reply | Threaded
Open this post in threaded view
|

Re: Improve new hash partition bound check error messages

Peter Eisentraut-7
On 2021-02-03 15:52, Peter Eisentraut wrote:

> On 2021-02-02 13:26, Heikki Linnakangas wrote:
>> How about this?
>>
>> CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS
>> 25, REMAINDER 3);
>> ERROR:  every hash partition modulus must be a factor of the next larger
>> modulus
>> DETAIL:  25 is not divisible by 10, the modulus of existing partition
>> "hpart_1"
>
> I don't know if we can easily get the name of the existing partition.
> I'll have to check that.
>
> I'm worried that this phrasing requires the user to understand that
> "divisible by" is related to "factor of", which is of course correct but
> introduces yet more complexity into this.
>
> I'll play around with this a bit more.
Here is a new patch that implements the suggestions.

v2-0001-Improve-new-hash-partition-bound-check-error-mess.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Improve new hash partition bound check error messages

Peter Eisentraut-7
On 15.02.21 17:45, Peter Eisentraut wrote:

> On 2021-02-03 15:52, Peter Eisentraut wrote:
>> On 2021-02-02 13:26, Heikki Linnakangas wrote:
>>> How about this?
>>>
>>> CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS
>>> 25, REMAINDER 3);
>>> ERROR:  every hash partition modulus must be a factor of the next larger
>>> modulus
>>> DETAIL:  25 is not divisible by 10, the modulus of existing partition
>>> "hpart_1"
>>
>> I don't know if we can easily get the name of the existing partition.
>> I'll have to check that.
>>
>> I'm worried that this phrasing requires the user to understand that
>> "divisible by" is related to "factor of", which is of course correct but
>> introduces yet more complexity into this.
>>
>> I'll play around with this a bit more.
>
> Here is a new patch that implements the suggestions.

committed