Re: bug: virtual generated column can be partition key

On Mon, Apr 21, 2025 at 2:42 PM jian he <jian.universality@gmail.com> wrote:

> On Mon, Apr 21, 2025 at 4:02 PM Fujii Masao <masao.fujii@oss.nttdata.com>
> wrote:
> >
> >
> >
> > On 2025/04/21 11:30, jian he wrote:
> > > hi.
> > > While trying to make the virtual generated column be part of the
> partition key,
> > > I found this bug.
>
>
I noticed that the check for a generated column in the partition key
expression already exists a few lines below. Had that check placed before
the if .. else ... block, we wouldn't have had this problem.
if (IsA(expr, Var) && ((Var *) expr)->varattno > 0)
{
 ...
}
else
{

I admit that pull_varattnos() + loop through bms_next_member() is a bit
wasteful when the expression is just a Var. But probably it's worth taking
that hit for the sake of avoiding code duplication. Further, I believe that
the two loops: one for system attribute check and the other for generated
attribute check can be combined, something like attached. Then it's not as
wasteful as it looks and we probably save a loop.

While looking at this I realised that a generated column may end up being
part of the partition key if the partition key expression contains a whole
row reference. Attached patch also has a fix and a testcase for the
same. PARTITION BY RANGE ((gtest_part_key is not null)) expression in the
test is kinda silly, but it tests the whole-row reference as part of an
expression. I haven't looked for more sensible expressions.

I have included your original tests, but ended up rewriting code. Please
let me know what do you think.

-- 
Best Wishes,
Ashutosh Bapat