On Sat, Feb 15, 2025 at 9:37 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On Fri, 14 Feb 2025 at 10:59, Peter Eisentraut <peter@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
