Re: Virtual generated columns

On Fri, 21 Feb 2025 at 06:16, Richard Guo <guofenglinux@gmail.com> wrote:
>
> 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?

OK. Makes sense.

> * 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.

Ah, nice catch. Yes, that makes sense.

> * 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.

Yes, good point.

> * 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).

Ah yes. That matches my initial instinct, which was to expand virtual
generated columns early in the planning process, but I didn't properly
understand why that was necessary.

> * 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.

Agreed. I had planned to do that, but ran out of steam.

> 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.

LGTM aside from a comment in fireRIRrules() that needed updating and a
minor issue in the callback function: when deciding whether to wrap
newnode in a ReturningExpr, if newnode is a Var, it should now compare
its varlevelsup with 0, not var->varlevelsup, since newnode hasn't had
its varlevelsup adjusted at that point. This is only a minor point,
because I don't think we ever currently need to wrap a newnode Var due
to differing varlevelsup, so all that was happening was that it was
wrapping when it didn't need to, which is actually harmless aside from
a small runtime performance hit.

Given that we're moving this part of expanding virtual generated
columns to the planner, I wonder if we should also move the other bits
(build_generation_expression and expand_generated_columns_in_expr)
too, so that they're all together. That could be a follow-on patch.

Regards,
Dean