On Tue, Apr 22, 2025 at 11:45 AM Ashutosh Bapat <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.
