Re: Virtual generated columns

On Wed, Aug 21, 2024 at 6:52 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On Wed, 21 Aug 2024 at 08:00, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> >
> > On 08.08.24 20:22, Dean Rasheed wrote:
> > > Looking at the rewriter changes, it occurred to me that it could
> > > perhaps be done more simply using ReplaceVarsFromTargetList() for each
> > > RTE with virtual generated columns. That function already has the
> > > required wholerow handling code, so there'd be less code duplication.
> >
> > Hmm, I don't quite see how ReplaceVarsFromTargetList() could be used
> > here. It does have the wholerow logic that we need somehow, but other
> > than that it seems to target something different?
> >
>

> Well what I was thinking was that (in fireRIRrules()'s final loop over
> relations in the rtable), if the relation had any virtual generated
> columns, you'd build a targetlist containing a TLE for each one,
> containing the generated expression. Then you could just call
> ReplaceVarsFromTargetList() to replace any Vars in the query with the
> corresponding generated expressions. That takes care of descending
> into subqueries, adjusting varlevelsup, and expanding wholerow Vars
> that might refer to the generated expression.
>
> I also have half an eye on how this patch will interact with my patch
> to support RETURNING OLD/NEW values. If you use
> ReplaceVarsFromTargetList(), it should just do the right thing for
> RETURNING OLD/NEW generated expressions.
>
> > > I think it might be better to do this from within fireRIRrules(), just
> > > after RLS policies are applied, so it wouldn't need to worry about
> > > CTEs and sublink subqueries. That would also make the
> > > hasGeneratedVirtual flags unnecessary, since we'd already only be
> > > doing the extra work for tables with virtual generated columns. That
> > > would eliminate possible bugs caused by failing to set those flags.
> >
> > Yes, ideally, we'd piggy-back this into fireRIRrules(). One thing I'm
> > missing is that if you're descending into subqueries, there is no link
> > to the upper levels' range tables, which we need to lookup the
> > pg_attribute entries of column referencing Vars. That's why there is
> > this whole custom walk with its own context data. Maybe there is a way
> > to do this already that I missed?
> >
>
> That link to the upper levels' range tables wouldn't be needed because
> essentially using ReplaceVarsFromTargetList() flips the whole thing
> round: instead of traversing the tree looking for Var nodes that need
> to be replaced (possibly from upper query levels), you build a list of
> replacement expressions to be applied and apply them from the top,
> descending into subqueries as needed.
>

CREATE TABLE gtest1 (a int, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
INSERT INTO gtest1 VALUES (1,default), (2, DEFAULT);

select b from (SELECT b FROM gtest1) sub;
here we only need to translate the second "b" to (a *2), not the first one.
but these two "b" query tree representation almost the same (varno,
varattno, varlevelsup)

I am not sure how ReplaceVarsFromTargetList can disambiguate this?
Currently v4-0001-Virtual-generated-columns.patch
works. because v4 properly tags the main query hasGeneratedVirtual to false,
and tag subquery's hasGeneratedVirtual to true.