Re: bug: virtual generated column can be partition key

Thanks Jian for your review.

On Tue, Apr 22, 2025 at 12:32 PM jian he <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> 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