On Thu, May 29, 2025 at 11:06 AM Richard Guo <guofengli...@gmail.com> wrote: > > On Fri, May 16, 2025 at 5:35 PM jian he <jian.universal...@gmail.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.
