On Sat, Feb 15, 2025 at 9:37 PM Dean Rasheed <dean.a.rash...@gmail.com> wrote: > On Fri, 14 Feb 2025 at 10:59, Peter Eisentraut <pe...@eisentraut.org> wrote: > > Maybe a short-term fix would be to error out if we find ourselves about > > to expand a Var with varnullingrels != NULL. That would mean you > > couldn't use a virtual generated column on the nullable output side of > > an outer join, which is annoying but not fatal, and we could fix it > > incrementally later. > > I think that would be rather a sad limitation to have. It would be > nice to have this fully working for the next release.
Besides being a limitation, this approach doesn't address all the issues with incorrect results. In some cases, PHVs are needed to isolate subexpressions, even when varnullingrels != NULL. As an example, please consider create table t (a int primary key, b int generated always as (10 + 10)); insert into t values (1); insert into t values (2); # select a, b from t group by grouping sets (a, b) having b = 20; a | b ---+---- 2 | 1 | | 20 (3 rows) This result set is incorrect. The first two rows, where b is NULL, should not be included in the result set. > Attached is a rough patch that moves the expansion of virtual > generated columns to the planner. It needs a lot more testing (and > some regression tests), but it does seem to fix all the issues > mentioned in this thread. Yeah, I believe this is the right way to go: virtual generated columns should be expanded in the planner, rather than in the rewriter. It seems to me that, for a relation in the rangetable that has virtual generated columns, we can consider it a subquery to some extent. For instance, suppose we have a query: select ... from ... join t on ...; and suppose t.b is a virtual generated column. We can consider this query as: select ... from ... join (select a, expr() as b from t) as t on ...; In this sense, I'm wondering if we can leverage the pullup_replace_vars architecture to expand the virtual generated columns. I believe this would help avoid a lot of duplicate code with pullup_replace_vars_callback. Thanks Richard
