Re: bug: virtual generated column can be partition key

committed

On 22.04.25 10:47, Ashutosh Bapat wrote:
> Thanks Jian for your review.
> 
> On Tue, Apr 22, 2025 at 12:32 PM jian he <jian.universality@gmail.com 
> <mailto:jian.universality@gmail.com>> wrote:
> 
>     On Tue, Apr 22, 2025 at 11:45 AM Ashutosh Bapat
>     <ashutosh.bapat.oss@gmail.com <mailto:ashutosh.bapat.oss@gmail.com>>
>     wrote:
>      >
>      > I have included your original tests, but ended up rewriting code.
>     Please let me know what do you think.
>      >
> 
>     + if (attno < 0)
>     + ereport(ERROR,
>     + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>     + errmsg("partition key expressions cannot contain system column
>     references")));
>     +
>     + /*
>     + * We do not check dropped columns explicitly since they will
>     + * be eliminated when expanding the the whole row expression
>     + * anyway.
>     + */
>     typo: "the the".
>     I am confused by the above comments.
>     ComputePartitionAttrs only called in DefineRelation.
>     DefineRelation will only CREATE a table, there will be no dropped
>     column via DefineRelation.
> 
> 
> You are right! Fixed.
> 
> 
> 
>     + /*
>     + * transformPartitionSpec() should have already rejected
>     + * subqueries, aggregates, window functions, and SRFs, based
>     + * on the EXPR_KIND_ for partition expressions.
>     + */
>     "EXPR_KIND_" can change to EXPR_KIND_PARTITION_EXPRESSION
>     ?
> 
> 
> That's an existing comment which appears to be moved in diff but it's 
> actually untouched by the patch. Maybe you are right, IDK, but since 
> it's not related to the fix, let's leave it untouched. I did wonder 
> whether that comment has any relation to the pull_varattnos call which 
> has been moved earlier since pull_varattnos doesn't expect any Query 
> node in the tree. But pondering more, I think the comment is related to 
> the number of rows participating in the partition key computation - 
> there should be exactly one key for one tuple. Having SRFs, subqueries 
> in part expression means a possibility of producing more than one set of 
> partition keys, aggregates and window functions OTOH will produce one 
> partition key for more than one row - all of them breaking 1:1 mapping 
> between a tuple and a partition key. Hence I think the comment is at the 
> right place.
> 
> PFA revised patch.
> 
> -- 
> Best Wishes,
> Ashutosh Bapat