Re: Virtual generated columns

On Thu, Feb 20, 2025 at 12:25 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On Wed, 19 Feb 2025 at 01:42, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > One thing I don't like about this is that it's introducing more code
> > duplication between pullup_replace_vars() and
> > ReplaceVarsFromTargetList(). Those already had a lot of code in common
> > before RETURNING OLD/NEW was added, and this is duplicating even more
> > code. I think it'd be better to refactor so that they share common
> > code, since it has become quite complex, and it would be better to
> > have just one place to maintain.

Yeah, it's annoying that the two replace_rte_variables callbacks have
so much code duplication.  I think it's a win to make them share
common code.  What do you think about making this refactor a separate
patch, as it doesn't seem directly related to the bug fix here?

> I've been doing some more testing of this, and attached is another
> update, improving a few comments and adding regression tests based on
> the cases discussed so far here.

Hmm, there are some issues with v4 as far as I can see.

* In pullup_replace_vars_callback, the varlevelsup of the newnode is
adjusted before its nullingrels is updated.  This can cause problems.
If the newnode is not a Var/PHV, we adjust its nullingrels with
add_nulling_relids, and this function only works for level-zero vars.
As a result, we may fail to put the varnullingrels into the
expression.

I think we should insist that ReplaceVarFromTargetList generates the
replacement expression with varlevelsup = 0, and that the caller is
responsible for adjusting the varlevelsup if needed.  This may need
some changes to ReplaceVarsFromTargetList_callback too.

* When expanding whole-tuple references, it is possible that some
fields are expanded as Consts rather than Vars, considering dropped
columns.  I think we need to check for this when generating the fields
for a RowExpr.

* The expansion of virtual generated columns occurs after subquery
pullup, which can lead to issues.  This was an oversight on my part.
Initially, I believed it wasn't possible for an RTE_RELATION RTE to
have 'lateral' set to true, so I assumed it would be safe to expand
virtual generated columns after subquery pullup.  However, upon closer
look, this doesn't seem to be the case: if a subquery had a LATERAL
marker, that would be propagated to any of its child RTEs, even for
RTE_RELATION child RTE if this child rel has sampling info (see
pull_up_simple_subquery).

* Not an issue but I think that maybe we can share some common code
between expand_virtual_generated_columns and
expand_generated_columns_internal on how we build the generation
expressions for a virtual generated column.

I've worked on these issues and attached are the updated patches.
0001 expands virtual generated columns in the planner.  0002 refactors
the code to eliminate code duplication in the replace_rte_variables
callback functions.

> One of the new regression tests fails, which actually appears to be a
> pre-existing grouping sets bug, due to the fact that only non-Vars are
> wrapped in PHVs. This bug can be triggered without virtual generated
> columns:

Interesting. I'll take a look at this issue.

Thanks
Richard