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. Another argument for doing it that way round is to not add too many extra cycles to the processing of existing queries that don't reference generated expressions. ISTM that this patch is potentially adding quite a lot of additional overhead -- it looks like, for every Var in the tree, it's calling get_attgenerated(), which involves a syscache lookup to see if that column is a generated expression (which most won't be). Ideally, we should be trying to do the minimum amount of extra work in the common case where there are no generated expressions. Looking ahead, I can also imagine that one day we might want to support subqueries in generated expressions. That would require recursive processing of generated expressions in the generated expression's subquery, as well as applying RLS policies to the new relations pulled in, and checks to guard against infinite recursion. fireRIRrules() already has the infrastructure to support all of that, so that feels like a much more natural place to do this. Regards, Dean
