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(dot)universality(at)gmail(dot)com
> <mailto:jian(dot)universality(at)gmail(dot)com>> wrote:
>
> On Tue, Apr 22, 2025 at 11:45 AM Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com <mailto:ashutosh(dot)bapat(dot)oss(at)gmail(dot)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