Обсуждение: Improve new hash partition bound check error messages

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.

On Tue, Feb 2, 2021 at 7:36 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> 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

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

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.

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.