Re: bug: virtual generated column can be partition key

On Tue, Apr 22, 2025 at 4:55 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Sorry I missed this email while sending the patches - our emails crossed in the air.
>
> On Tue, Apr 22, 2025 at 2:15 PM jian he <jian.universality@gmail.com> wrote:
>>
>> On Tue, Apr 22, 2025 at 3:02 PM jian he <jian.universality@gmail.com> wrote:
>> > Other than that, it looks good to me for fixing this bug.
>>
>> The error message seems not that intuitive.
>>
>> +CREATE TABLE gtest_part_key (f1 date NOT NULL, f2 bigint, f3 bigint
>> GENERATED ALWAYS AS (f2 * 2) VIRTUAL) PARTITION BY RANGE
>> ((gtest_part_key));
>> +ERROR:  cannot use generated column in partition key
>> +LINE 1: ...D ALWAYS AS (f2 * 2) VIRTUAL) PARTITION BY RANGE ((gtest_par...
>> +                                                             ^
>> +DETAIL:  Column "f3" is a generated column.
>
>
> Yes. It's not apparent where does f3 appear in the partition key, to a lay users. Users who explicitly use whole row
expressionin a partition key, would know that a whole row expression contains all columns. And the error location
pointsto the whole-row reference. So the current state isn't that bad. 
>
>>
>>
>> with the attached patch. now,
>> +CREATE TABLE gtest_part_key (f1 date NOT NULL, f2 bigint, f3 bigint
>> GENERATED ALWAYS AS (f2 * 2) VIRTUAL) PARTITION BY RANGE
>> ((gtest_part_key));
>> ERROR:  cannot use generated column in partition key
>> LINE 1: ...D ALWAYS AS (f2 * 2) VIRTUAL) PARTITION BY RANGE ((gtest_par...
>>                                                              ^
>> DETAIL:  Generated column "f3" is part of the partition key of
>> relation "gtest_part_key"
>>
>
> The DETAIL here is just mentioning what's already known from the command, so the change in the DETAIL may not be
useful.
>
> If I understand your intention correctly, we have to mention something to the effect "partition key expression
containsa whole-row reference which in turn contains a generated column." But that might be too verbose. 
>

you understand my intention correctly,
I aggrees current error message in that location is not that bad.

overall your latest patch looks good to me.