Re: Virtual generated columns

On Thu, May 29, 2025 at 11:06 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> On Fri, May 16, 2025 at 5:35 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> > we have used the USING expression in ATPrepAlterColumnType,
> > ATColumnChangeRequiresRewrite.
> > expanding it on ATPrepAlterColumnType seems to make more sense?
> >
> > @@ -14467,7 +14467,7 @@ ATPrepAlterColumnType(List **wqueue,
> > */
> > newval = (NewColumnValue *) palloc0(sizeof(NewColumnValue));
> > newval->attnum = attnum;
> > - newval->expr = (Expr *) transform;
> > + newval->expr = (Expr *)
> > expand_generated_columns_in_expr(transform, rel, 1);
> > newval->is_generated = false;
>
> Yeah, ATPrepAlterColumnType does seem like a better place. But we
> need to ensure that ATColumnChangeRequiresRewrite sees the expanded
> version of the expression — your proposed change fails to do that.
>
> Additionally, I think we also need to ensure that the virtual
> generated columns are expanded before the expression is fed through
> expression_planner, to ensure it can be successfully transformed into
> an executable form.
>
> Hence, the attached patch.

looks good to me.