On Wed, Aug 21, 2024 at 6:52 PM Dean Rasheed <dean.a.rash...@gmail.com> wrote: > > On Wed, 21 Aug 2024 at 08:00, Peter Eisentraut <pe...@eisentraut.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.
