Thread: Reduce "Var IS [NOT] NULL" quals during constant folding

Currently, we have an optimization that reduces an IS [NOT] NULL qual
on a NOT NULL column to constant true or constant false, provided we
can prove that the input expression of the NullTest is not nullable by
any outer joins.  This deduction happens pretty late in planner,
during the distribution of quals to relations in query_planner().  As
mentioned in [1], doing it at such a late stage has some drawbacks.

Ideally, this deduction should happen during constant folding.
However, we don't have the per-relation information about which
columns are defined as NOT NULL available at that point.  That
information is collected from catalogs when building RelOptInfos for
base or other relations.

I'm wondering whether we can collect that information while building
the RangeTblEntry for a base or other relation, so that it's available
before constant folding.  This could also enable other optimizations,
such as checking if a NOT IN subquery's output columns and its
left-hand expressions are all certainly not NULL, in which case we can
convert it to an anti-join.

Attached is a draft patch to reduce NullTest on a NOT NULL column in
eval_const_expressions.

Initially, I planned to get rid of restriction_is_always_true and
restriction_is_always_false altogether, since we now perform the
reduction of "Var IS [NOT] NULL" quals in eval_const_expressions.
However, removing them would prevent us from reducing some IS [NOT]
NULL quals that we were previously able to reduce, because (a) the
self-join elimination may introduce new IS NOT NULL quals after the
constant folding, and (b) if some outer joins are converted to inner
joins, previously irreducible NullTest quals may become reducible.

So I think maybe we'd better keep restriction_is_always_true and
restriction_is_always_false as-is.

Any thoughts?

[1] https://postgr.es/m/2323997.1740623184@sss.pgh.pa.us

Thanks
Richard

On Fri, Mar 21, 2025 at 6:14 PM Richard Guo <guofenglinux@gmail.com> wrote:
> I'm wondering whether we can collect that information while building
> the RangeTblEntry for a base or other relation, so that it's available
> before constant folding.  This could also enable other optimizations,
> such as checking if a NOT IN subquery's output columns and its
> left-hand expressions are all certainly not NULL, in which case we can
> convert it to an anti-join.
>
> Attached is a draft patch to reduce NullTest on a NOT NULL column in
> eval_const_expressions.

FWIW, reducing "Var IS [NOT] NULL" quals during constant folding can
somewhat influence the decision on join ordering later.  For instance,

create table t (a int not null, b int);

select * from t t1 left join
  (t t2 left join t t3 on t2.a is not null)
  on t1.b = t2.b;

For this query, "t2.a is not null" is reduced to true during constant
folding and then ignored, which leads to us being unable to commute
t1/t2 join with t2/t3 join.

OTOH, constant-folding NullTest for Vars may enable join orders that
were previously impossible.  For instance,

select * from t t1 left join
  (t t2 left join t t3 on t2.a is null or t2.b = t3.b)
  on t1.b = t2.b;

Previously the t2/t3 join's clause is not strict for t2 due to the IS
NULL qual, which prevents t2/t3 join from commuting with t1/t2 join.
Now, the IS NULL qual is removed during constant folding, allowing us
to generate a plan with the join order (t1/t2)/t3.

Not quite sure if this is something we need to worry about.

Thanks
Richard

On Fri, Mar 21, 2025 at 10:21 AM Richard Guo <guofenglinux@gmail.com> wrote:
> Not quite sure if this is something we need to worry about.

I haven't really dug into this but I bet it's not that serious, in the
sense that we could probably work around it with more logic if we
really need to.

However, I'm a bit concerned about the overall premise of the patch
set. It feels like it is moving something that really ought to happen
at optimization time back to parse time. I have a feeling that's going
to break something, although I am not sure right now exactly what.
Wouldn't it be better to have this still happen in the planner, but
sooner than it does now?

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> However, I'm a bit concerned about the overall premise of the patch
> set. It feels like it is moving something that really ought to happen
> at optimization time back to parse time. I have a feeling that's going
> to break something, although I am not sure right now exactly what.

Ugh, no, that is *completely* unworkable.  Suppose that the user
does CREATE VIEW, and the parse tree recorded for that claims that
column X is not-nullable.  Then the user drops the not-null
constraint, and then asks to execute the view.  We'll optimize on
the basis of stale information.

The way to make this work is what I said before: move the planner's
collection of relation information to somewhere a bit earlier in
the planner.  But not to outside the planner.

            regards, tom lane

On Sat, Mar 22, 2025 at 1:12 AM Robert Haas <robertmhaas@gmail.com> wrote:
> However, I'm a bit concerned about the overall premise of the patch
> set. It feels like it is moving something that really ought to happen
> at optimization time back to parse time. I have a feeling that's going
> to break something, although I am not sure right now exactly what.
> Wouldn't it be better to have this still happen in the planner, but
> sooner than it does now?

You're right.  It's just flat wrong to collect catalog information in
the parser and use it in the planner.  As Tom pointed out, the catalog
information could change in between, which would cause us to use stale
data.

Yeah, this should still happen in the planner, perhaps before
pull_up_sublinks, if we plan to leverage that info to convert NOT IN
to anti-join.

Thanks
Richard

On Sat, Mar 22, 2025 at 2:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Ugh, no, that is *completely* unworkable.  Suppose that the user
> does CREATE VIEW, and the parse tree recorded for that claims that
> column X is not-nullable.  Then the user drops the not-null
> constraint, and then asks to execute the view.  We'll optimize on
> the basis of stale information.

Thanks for pointing this out.

> The way to make this work is what I said before: move the planner's
> collection of relation information to somewhere a bit earlier in
> the planner.  But not to outside the planner.

I'm considering moving the collection of attnotnull information before
pull_up_sublinks, in hopes of leveraging this info to pull up NOT IN
in the future, something like attached.

Maybe we could also collect the attgenerated information in the same
routine, making life easier for expand_virtual_generated_columns.

Another issue I found is that in convert_EXISTS_to_ANY, we pass the
parent's root to eval_const_expressions, which can cause problems when
reducing "Var IS [NOT] NULL" quals.  To fix, v2 patch constructs up a
dummy PlannerInfo with "glob" link set to the parent's and "parser"
link set to the subquery.  I believe these are the only fields used.

Thanks
Richard

On Sun, Mar 23, 2025 at 6:25 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Sat, Mar 22, 2025 at 2:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > The way to make this work is what I said before: move the planner's
> > collection of relation information to somewhere a bit earlier in
> > the planner.  But not to outside the planner.

> I'm considering moving the collection of attnotnull information before
> pull_up_sublinks, in hopes of leveraging this info to pull up NOT IN
> in the future, something like attached.

Here is an updated version of the patch with some cosmetic changes and
a more readable commit message.  I'm wondering if it's good enough to
be pushed.  Any comments?

Thanks
Richard

On Wed, Mar 26, 2025 at 3:06 PM Tender Wang <tndrwang@gmail.com> wrote:
> The comment about  notnullattnums in struct RangeTblEntry says that:
> * notnullattnums is zero-based set containing attnums of NOT NULL
> * columns.
>
> But in get_relation_notnullatts():
> rte->notnullattnums = bms_add_member(rte->notnullattnums,
>                                                                     i + 1);
>
> The notnullattnums seem to be 1-based.

This corresponds to the attribute numbers in Var nodes; you can
consider zero as representing a whole-row Var.

Thanks
Richard

On Wed, 26 Mar 2025 at 19:31, Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Wed, Mar 26, 2025 at 3:06 PM Tender Wang <tndrwang@gmail.com> wrote:
> > The comment about  notnullattnums in struct RangeTblEntry says that:
> > * notnullattnums is zero-based set containing attnums of NOT NULL
> > * columns.
> >
> > But in get_relation_notnullatts():
> > rte->notnullattnums = bms_add_member(rte->notnullattnums,
> >                                                                     i + 1);
> >
> > The notnullattnums seem to be 1-based.
>
> This corresponds to the attribute numbers in Var nodes; you can
> consider zero as representing a whole-row Var.

Yeah, and a negative number is a system attribute, which the Bitmapset
can't represent... The zero-based comment is meant to inform the
reader that they don't need to offset by
FirstLowInvalidHeapAttributeNumber when indexing the Bitmapset. If
there's some confusion about that then maybe the wording could be
improved. I used "zero-based" because I wanted to state what it was
and that was the most brief terminology that I could think of to do
that. The only other way I thought about was to say that "it's not
offset by FirstLowInvalidHeapAttributeNumber", but I thought it was
better to say what it is rather than what it isn't.

I'm open to suggestions if people are confused about this.

David

On Wed, Mar 26, 2025 at 6:45 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Wed, 26 Mar 2025 at 19:31, Richard Guo <guofenglinux@gmail.com> wrote:
> > On Wed, Mar 26, 2025 at 3:06 PM Tender Wang <tndrwang@gmail.com> wrote:
> > > The comment about  notnullattnums in struct RangeTblEntry says that:
> > > * notnullattnums is zero-based set containing attnums of NOT NULL
> > > * columns.
> > >
> > > But in get_relation_notnullatts():
> > > rte->notnullattnums = bms_add_member(rte->notnullattnums,
> > >                                                                     i + 1);
> > >
> > > The notnullattnums seem to be 1-based.

> > This corresponds to the attribute numbers in Var nodes; you can
> > consider zero as representing a whole-row Var.

> Yeah, and a negative number is a system attribute, which the Bitmapset
> can't represent... The zero-based comment is meant to inform the
> reader that they don't need to offset by
> FirstLowInvalidHeapAttributeNumber when indexing the Bitmapset. If
> there's some confusion about that then maybe the wording could be
> improved. I used "zero-based" because I wanted to state what it was
> and that was the most brief terminology that I could think of to do
> that. The only other way I thought about was to say that "it's not
> offset by FirstLowInvalidHeapAttributeNumber", but I thought it was
> better to say what it is rather than what it isn't.
>
> I'm open to suggestions if people are confused about this.

I searched the current terminology used in code and can find "offset
by FirstLowInvalidHeapAttributeNumber", but not "not offset by
FirstLowInvalidHeapAttributeNumber".  I think "zero-based" should be
sufficient to indicate that this bitmapset is offset by zero, not by
FirstLowInvalidHeapAttributeNumber.  So I'm fine to go with
"zero-based".

I'm planning to push this patch soon, barring any objections.

Thanks
Richard

Richard Guo <guofenglinux@gmail.com> writes:
> I'm planning to push this patch soon, barring any objections.

FWIW, I have not reviewed it at all.

            regards, tom lane

On Thu, Mar 27, 2025 at 10:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Richard Guo <guofenglinux@gmail.com> writes:
> > I'm planning to push this patch soon, barring any objections.

> FWIW, I have not reviewed it at all.

Oh, sorry.  I'll hold off on pushing it.

Thanks
Richard

On Thu, Mar 27, 2025 at 10:08 AM Richard Guo <guofenglinux@gmail.com> wrote:
> On Thu, Mar 27, 2025 at 10:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Richard Guo <guofenglinux@gmail.com> writes:
> > > I'm planning to push this patch soon, barring any objections.
>
> > FWIW, I have not reviewed it at all.
>
> Oh, sorry.  I'll hold off on pushing it.

As a general point, non-trivial patches should really get some
substantive review before they are pushed. Please don't be in a rush
to commit. It is very common for the time from when a patch is first
posted to commit to be 3-6 months even for committers. Posting a
brand-new feature patch on March 21st and then pressing to commit on
March 27th is really not something you should be doing. I think it's
particularly inappropriate here where you actually got a review that
pointed out a serious design problem and then had to change the
design. If you didn't get it right on the first try, you shouldn't be
too confident that you did it perfectly the second time, either.

I took a look at this today and I'm not entirely comfortable with this:

+ rel = table_open(rte->relid, NoLock);
+
+ /* Record NOT NULL columns for this relation. */
+ get_relation_notnullatts(rel, rte);
+
+ table_close(rel, NoLock);

As a general principle, I have found that it's usually a sign that
something has been designed poorly when you find yourself wanting to
open a relation, get exactly one piece of information, and close the
relation again. That is why, today, all the information that the
planner needs about a particular relation is retrieved by
get_relation_info(). We do not just wander around doing random catalog
lookups wherever we need some critical detail. This patch increases
the number of places where we fetch relation data from 1 to 2, but
it's still the case that almost everything happens in
get_relation_info(), and there's now just exactly this 1 thing that is
done in a different place. That doesn't seem especially nice. I
thought the idea was going to be to move get_relation_info() to an
earlier stage, not split one thing out of it while leaving everything
else the same.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Apr 1, 2025 at 1:55 AM Robert Haas <robertmhaas@gmail.com> wrote:
> As a general principle, I have found that it's usually a sign that
> something has been designed poorly when you find yourself wanting to
> open a relation, get exactly one piece of information, and close the
> relation again. That is why, today, all the information that the
> planner needs about a particular relation is retrieved by
> get_relation_info(). We do not just wander around doing random catalog
> lookups wherever we need some critical detail. This patch increases
> the number of places where we fetch relation data from 1 to 2, but
> it's still the case that almost everything happens in
> get_relation_info(), and there's now just exactly this 1 thing that is
> done in a different place. That doesn't seem especially nice. I
> thought the idea was going to be to move get_relation_info() to an
> earlier stage, not split one thing out of it while leaving everything
> else the same.

I initially considered moving get_relation_info() to an earlier stage,
where we would collect all the per-relation data by relation OID.
Later, when building the RelOptInfos, we could link them to the
per-relation-OID data.

However, I gave up this idea because I realized it would require
retrieving a whole bundle of catalog information that isn't needed
until after the RelOptInfos are built, such as max_attr, pages,
tuples, reltablespace, parallel_workers, extended statistics, etc.
And we may also need to create the IndexOptInfos for the relation's
indexes.  It seems to me that it's not a trivial task to move
get_relation_info() before building the RelOptInfos, and more
importantly, it's unnecessary most of the time.

In other words, of the many pieces of catalog information we need to
retrieve for a relation, only a small portion is needed at an early
stage.  As far as I can see, this small portion includes:

* relhassubclass, which we retrieve immediately after we have finished
adding rangetable entries.

* attgenerated, which we retrieve in expand_virtual_generated_columns.

* attnotnull, as discussed here, which should ideally be retrieved
before pull_up_sublinks.

My idea is to retrieve only this small portion at an early stage, and
still defer collecting the majority of the catalog information until
building the RelOptInfos.  It might be possible to optimize by
retrieving this small portion in one place instead of three, but
moving the entire get_relation_info() to an earlier stage doesn't seem
like a good idea to me.

It could be argued that the separation of catalog information
collection isn't very great, but it seems this isn't something new in
this patch.  So I respectfully disagree with your statement that "all
the information that the planner needs about a particular relation is
retrieved by get_relation_info()", and that "there's now just exactly
this 1 thing that is done in a different place".  For instance,
relhassubclass and attgenerated are retrieved before expression
preprocessing, a relation's constraint expressions are retrieved when
setting the relation's size estimates, and more.

Thanks
Richard

On Tue, Apr 1, 2025 at 2:34 AM Richard Guo <guofenglinux@gmail.com> wrote:
> However, I gave up this idea because I realized it would require
> retrieving a whole bundle of catalog information that isn't needed
> until after the RelOptInfos are built, such as max_attr, pages,
> tuples, reltablespace, parallel_workers, extended statistics, etc.

Why is that bad? I mean, if we're going to need that information
anyway, then gathering it at earlier stage doesn't hurt. Of course, if
we move it too early, say before partition pruning, then we might
gather information we don't really need and hurt performance. But
otherwise it doesn't seem to hurt anything.

> And we may also need to create the IndexOptInfos for the relation's
> indexes.  It seems to me that it's not a trivial task to move
> get_relation_info() before building the RelOptInfos, and more
> importantly, it's unnecessary most of the time.

But again, if the work is going to have to be done anyway, who cares?

> It could be argued that the separation of catalog information
> collection isn't very great, but it seems this isn't something new in
> this patch.  So I respectfully disagree with your statement that "all
> the information that the planner needs about a particular relation is
> retrieved by get_relation_info()", and that "there's now just exactly
> this 1 thing that is done in a different place".  For instance,
> relhassubclass and attgenerated are retrieved before expression
> preprocessing, a relation's constraint expressions are retrieved when
> setting the relation's size estimates, and more.

Nonetheless I think we ought to be trying to consolidate more things
into get_relation_info(), not disperse some of the things that are
there to other places.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Apr 2, 2025 at 4:34 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Apr 1, 2025 at 2:34 AM Richard Guo <guofenglinux@gmail.com> wrote:
> > However, I gave up this idea because I realized it would require
> > retrieving a whole bundle of catalog information that isn't needed
> > until after the RelOptInfos are built, such as max_attr, pages,
> > tuples, reltablespace, parallel_workers, extended statistics, etc.

> Why is that bad? I mean, if we're going to need that information
> anyway, then gathering it at earlier stage doesn't hurt. Of course, if
> we move it too early, say before partition pruning, then we might
> gather information we don't really need and hurt performance. But
> otherwise it doesn't seem to hurt anything.

The attnotnull catalog information being discussed here is intended
for use during constant folding (and possibly sublink pull-up), which
occurs long before partition pruning.  Am I missing something?

Additionally, I'm doubtful that the collection of relhassubclass can
be moved after partition pruning.  How can we determine whether a
relation is inheritable without retrieving its relhassubclass
information?

As for attgenerated, I also doubt that it can be retrieved after
partition pruning.  It is used to expand virtual generated columns,
which can result in new PlaceHolderVars.  This means it must be done
before deconstruct_jointree, as make_outerjoininfo requires all active
PlaceHolderVars to be present in root->placeholder_list.

If these pieces of information cannot be retrieved after partition
pruning, and for performance reasons we don't want to move the
gathering of the majority of the catalog information before partition
pruning, then it seems to me that moving get_relation_info() to an
earlier stage might not be very meaningful.  What do you think?

> > It could be argued that the separation of catalog information
> > collection isn't very great, but it seems this isn't something new in
> > this patch.  So I respectfully disagree with your statement that "all
> > the information that the planner needs about a particular relation is
> > retrieved by get_relation_info()", and that "there's now just exactly
> > this 1 thing that is done in a different place".  For instance,
> > relhassubclass and attgenerated are retrieved before expression
> > preprocessing, a relation's constraint expressions are retrieved when
> > setting the relation's size estimates, and more.

> Nonetheless I think we ought to be trying to consolidate more things
> into get_relation_info(), not disperse some of the things that are
> there to other places.

I agree with the general idea of more consolidation and less
dispersion, but squashing all information collection into
get_relation_info() seems quite challenging.  However, I think we can
make at least one optimization, as I mentioned upthread — retrieving
the small portion of catalog information needed at an early stage in
one place instead of three.  Perhaps we could start with moving the
retrieval of relhassubclass into collect_relation_attrs() and rename
this function to better reflect this change.

Thanks
Richard

On Tue, Apr 1, 2025 at 10:14 PM Richard Guo <guofenglinux@gmail.com> wrote:
> The attnotnull catalog information being discussed here is intended
> for use during constant folding (and possibly sublink pull-up), which
> occurs long before partition pruning.  Am I missing something?

Hmm, OK, so you think that we need to gather this information early,
so that we can do constant folding correctly, but you don't want to
gather everything that get_relation_info() does at this stage, because
then we're doing extra work on partitions that might later be pruned.
Is that correct?

> Additionally, I'm doubtful that the collection of relhassubclass can
> be moved after partition pruning.  How can we determine whether a
> relation is inheritable without retrieving its relhassubclass
> information?

We can't -- but notice that we open the relation before fetching
relhassubclass, and then pass down the Relation object to where
get_relation_info() is ultimately called, so that we do not repeatedly
open and close the Relation. I don't know if I can say exactly what's
going to go wrong if we add an extra table_open()/table_close() as you
do in the patch, but I've seen enough performance and correctness
problems with such code over the years to make me skeptical.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Sat, Apr 5, 2025 at 4:14 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Apr 1, 2025 at 10:14 PM Richard Guo <guofenglinux@gmail.com> wrote:
> > The attnotnull catalog information being discussed here is intended
> > for use during constant folding (and possibly sublink pull-up), which
> > occurs long before partition pruning.  Am I missing something?

> Hmm, OK, so you think that we need to gather this information early,
> so that we can do constant folding correctly, but you don't want to
> gather everything that get_relation_info() does at this stage, because
> then we're doing extra work on partitions that might later be pruned.
> Is that correct?

That's correct.  As I mentioned earlier, I believe attnotnull isn't
the only piece of information we need to gather early on.  My general
idea is to separate the collection of catalog information into two
phases:

* Phase 1 occurs at an early stage and collects the small portion of
catalog information that is needed for constant folding, setting the
inh flag for a relation, or expanding virtual generated columns.  All
these happen very early in the planner, before partition pruning.

* Phase 2 collects the majority of the catalog information and occurs
when building the RelOptInfos, like what get_relation_info does.

FWIW, aside from partition pruning, I suspect there may be other cases
where a relation doesn't end up having a RelOptInfo created for it.
And the comment for add_base_rels_to_query further strengthens my
suspicion.

 * Note: the reason we find the baserels by searching the jointree, rather
 * than scanning the rangetable, is that the rangetable may contain RTEs
 * for rels not actively part of the query, for example views.  We don't
 * want to make RelOptInfos for them.

If my suspicion is correct, then partition pruning isn't the only
reason we might not want to move get_relation_info to an earlier
stage.

> > Additionally, I'm doubtful that the collection of relhassubclass can
> > be moved after partition pruning.  How can we determine whether a
> > relation is inheritable without retrieving its relhassubclass
> > information?
>
> We can't -- but notice that we open the relation before fetching
> relhassubclass, and then pass down the Relation object to where
> get_relation_info() is ultimately called, so that we do not repeatedly
> open and close the Relation. I don't know if I can say exactly what's
> going to go wrong if we add an extra table_open()/table_close() as you
> do in the patch, but I've seen enough performance and correctness
> problems with such code over the years to make me skeptical.

I'm confused here.  AFAICS, we don't open the relation before fetching
relhassubclass, according to the code that sets the inh flag in
subquery_planner.  Additionally, I do not see we pass down the
Relation object to get_relation_info.  In get_relation_info, we call
table_open to obtain the Relation object, use it to retrieve the
catalog information, and then call table_close to close the Relation.

Am I missing something, or do you mean that the relcache entry is
actually built earlier, and that table_open/table_close call in
get_relation_info merely increments/decrements the reference count?

IIUC, you're concerned about calling table_open/table_close to
retrieve catalog information.  Do you know of a better way to retrieve
catalog information without calling table_open/table_close?  I find
the table_open/table_close pattern is quite common in the current
code.  In addition to get_relation_info(), I've also seen it in
get_relation_constraints(), get_relation_data_width(), and others.

Thanks
Richard

On Sun, Apr 6, 2025 at 10:59 PM Richard Guo <guofenglinux@gmail.com> wrote:
> That's correct.  As I mentioned earlier, I believe attnotnull isn't
> the only piece of information we need to gather early on.  My general
> idea is to separate the collection of catalog information into two
> phases:
>
> * Phase 1 occurs at an early stage and collects the small portion of
> catalog information that is needed for constant folding, setting the
> inh flag for a relation, or expanding virtual generated columns.  All
> these happen very early in the planner, before partition pruning.
>
> * Phase 2 collects the majority of the catalog information and occurs
> when building the RelOptInfos, like what get_relation_info does.

OK. Maybe I shouldn't be worrying about the table_open() /
table_close() here, because I see that you are right that
has_subclass() is nearby, which admittedly does not involve opening
the relation, but it does involve fetching from the syscache a tuple
that we wouldn't need to fetch if we had a Relation available at that
point. And I see now that expand_virtual_generated_columns() is also
in that area and works similar to your proposed function in that it
just opens and closes all the relations. Perhaps that's just the way
we do things at this very early stage of the planner? But I feel like
it might make sense to do some reorganization of this code, though, so
that it more resembles the phase 1 and phase 2 as you describe them.
Both expand_virtual_generated_columns() and collect_relation_attrs()
care about exactly those relations with rte->rtekind == RTE_RELATION,
and even if we have to open and close all of those relations once to
do this processing, perhaps we can avoid doing it twice, and maybe
avoid the has_subclass() call along the way? Maybe we can hoist a loop
over parse->rtable up into subquery_planner and then have it call a
virtual-column expansion function and a non-null collection function
once per RTE_RELATION entry?

A related point that I'm noticing is that you record the not-NULL
information in the RangeTblEntry. I wonder whether that's going to be
a problem, because I think of the RangeTblEntry as a parse-time
structure and the RelOptInfo as a plan-time structure, meaning that we
shouldn't scribble on the former and that we should record any
plan-time details we need in the latter. I understand that the problem
is precisely that the RelOptInfo isn't yet available, but I'm not sure
that makes it OK to use the RangeTblEntry instead.

> I'm confused here.  AFAICS, we don't open the relation before fetching
> relhassubclass, according to the code that sets the inh flag in
> subquery_planner.  Additionally, I do not see we pass down the
> Relation object to get_relation_info.  In get_relation_info, we call
> table_open to obtain the Relation object, use it to retrieve the
> catalog information, and then call table_close to close the Relation.

You're right. I don't know what I was thinking.

> IIUC, you're concerned about calling table_open/table_close to
> retrieve catalog information.  Do you know of a better way to retrieve
> catalog information without calling table_open/table_close?  I find
> the table_open/table_close pattern is quite common in the current
> code.  In addition to get_relation_info(), I've also seen it in
> get_relation_constraints(), get_relation_data_width(), and others.

You're also right about this.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> OK. Maybe I shouldn't be worrying about the table_open() /
> table_close() here, because I see that you are right that
> has_subclass() is nearby, which admittedly does not involve opening
> the relation, but it does involve fetching from the syscache a tuple
> that we wouldn't need to fetch if we had a Relation available at that
> point. And I see now that expand_virtual_generated_columns() is also
> in that area and works similar to your proposed function in that it
> just opens and closes all the relations. Perhaps that's just the way
> we do things at this very early stage of the planner? But I feel like
> it might make sense to do some reorganization of this code, though, so
> that it more resembles the phase 1 and phase 2 as you describe them.

Indeed, I think those are hacks that we should get rid of, not
emulate.  Note in particular that expand_virtual_generated_columns
is new in v18 and has exactly zero credibility as precedent.  In fact,
I'm probably going to harass Peter about fixing it before v18 ships.
Randomly adding table_opens in the planner is not a route to high
planning performance.

> A related point that I'm noticing is that you record the not-NULL
> information in the RangeTblEntry.

Did we not just complain about that w.r.t. the v1 version of this
patch?  RangeTblEntry is not where to store this info.  We need
a new data structure, and IMO the data structure should be a hashtable
based on table OID, not relid.  That way we can amortize across
multiple references to the same table within a query.

            regards, tom lane

On Thu, Apr 10, 2025 at 3:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > A related point that I'm noticing is that you record the not-NULL
> > information in the RangeTblEntry.
>
> Did we not just complain about that w.r.t. the v1 version of this
> patch?  RangeTblEntry is not where to store this info.  We need
> a new data structure, and IMO the data structure should be a hashtable
> based on table OID, not relid.  That way we can amortize across
> multiple references to the same table within a query.

It's not quite the same complaint, because the earlier complaint was
that it was actually being done at parse time, and this complaint is
that it is scribbling on a parse-time data structure.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> It's not quite the same complaint, because the earlier complaint was
> that it was actually being done at parse time, and this complaint is
> that it is scribbling on a parse-time data structure.

Ah, right.  But that's still not the direction we want to be
going in [1].

            regards, tom lane

[1] https://www.postgresql.org/message-id/2531459.1743871597%40sss.pgh.pa.us

On Fri, Apr 11, 2025 at 4:45 AM Robert Haas <robertmhaas@gmail.com> wrote:
> OK. Maybe I shouldn't be worrying about the table_open() /
> table_close() here, because I see that you are right that
> has_subclass() is nearby, which admittedly does not involve opening
> the relation, but it does involve fetching from the syscache a tuple
> that we wouldn't need to fetch if we had a Relation available at that
> point. And I see now that expand_virtual_generated_columns() is also
> in that area and works similar to your proposed function in that it
> just opens and closes all the relations. Perhaps that's just the way
> we do things at this very early stage of the planner? But I feel like
> it might make sense to do some reorganization of this code, though, so
> that it more resembles the phase 1 and phase 2 as you describe them.
> Both expand_virtual_generated_columns() and collect_relation_attrs()
> care about exactly those relations with rte->rtekind == RTE_RELATION,
> and even if we have to open and close all of those relations once to
> do this processing, perhaps we can avoid doing it twice, and maybe
> avoid the has_subclass() call along the way?

Yeah, I agree on this.  This is the optimization I mentioned upthread
in the last paragraph of [1] - retrieving the small portion of catalog
information needed at an early stage in one place instead of three.
Hopefully, this way we only need one table_open/table_close at the
early stage of the planner.

> Maybe we can hoist a loop
> over parse->rtable up into subquery_planner and then have it call a
> virtual-column expansion function and a non-null collection function
> once per RTE_RELATION entry?

Hmm, I'm afraid there might be some difficulty with this approach.
The virtual-column expansion needs to be done after sublink pull-up to
ensure that the virtual-column references within the SubLinks that
should be transformed into joins can get expanded, while non-null
collection needs to be done before sublink pull-up since we might want
to leverage the non-null information to convert NOT IN sublinks to
anti joins.

What I had in mind is that we hoist a loop over parse->rtable before
pull_up_sublinks to gather information about which columns of each
relation are defined as NOT NULL and which are virtually generated.
These information will be used in sublink pull-up and virtual-column
expansion.  We also call has_subclass for each relation that is maked
'inh' within that loop and clear the inh flag if needed.

This seems to require a fair amount of code changes, so I'd like to
get some feedback on this approach before proceeding with the
implementation.

> A related point that I'm noticing is that you record the not-NULL
> information in the RangeTblEntry. I wonder whether that's going to be
> a problem, because I think of the RangeTblEntry as a parse-time
> structure and the RelOptInfo as a plan-time structure, meaning that we
> shouldn't scribble on the former and that we should record any
> plan-time details we need in the latter. I understand that the problem
> is precisely that the RelOptInfo isn't yet available, but I'm not sure
> that makes it OK to use the RangeTblEntry instead.

Fair point.  We should do our best to refrain from scribbling on the
parsetree in the planner.  I initially went with the hashtable
approach as Tom suggested, but later found it quite handy to store the
not-null information in the RangeTblEntry, especially since we do
something similar with rte->inh.  However, I've come to realize that
inh may not be a good example to follow after all, so I'll go back to
the hashtable method.

Thank you for pointing that out before I went too far down the wrong
path.

[1] https://postgr.es/m/CAMbWs4-DryEm_U-juPn3HwUiwZRXW3jhfX18b_AFgrgihgq4kg@mail.gmail.com

Thanks
Richard

On Fri, Apr 11, 2025 at 3:51 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Fri, Apr 11, 2025 at 4:45 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > OK. Maybe I shouldn't be worrying about the table_open() /
> > table_close() here, because I see that you are right that
> > has_subclass() is nearby, which admittedly does not involve opening
> > the relation, but it does involve fetching from the syscache a tuple
> > that we wouldn't need to fetch if we had a Relation available at that
> > point. And I see now that expand_virtual_generated_columns() is also
> > in that area and works similar to your proposed function in that it
> > just opens and closes all the relations. Perhaps that's just the way
> > we do things at this very early stage of the planner? But I feel like
> > it might make sense to do some reorganization of this code, though, so
> > that it more resembles the phase 1 and phase 2 as you describe them.
> > Both expand_virtual_generated_columns() and collect_relation_attrs()
> > care about exactly those relations with rte->rtekind == RTE_RELATION,
> > and even if we have to open and close all of those relations once to
> > do this processing, perhaps we can avoid doing it twice, and maybe
> > avoid the has_subclass() call along the way?
>
> Yeah, I agree on this.  This is the optimization I mentioned upthread
> in the last paragraph of [1] - retrieving the small portion of catalog
> information needed at an early stage in one place instead of three.
> Hopefully, this way we only need one table_open/table_close at the
> early stage of the planner.

Here is the patchset that implements this optimization.  0001 moves
the expansion of virtual generated columns to occur before sublink
pull-up.  0002 introduces a new function, preprocess_relation_rtes,
which scans the rangetable for relation RTEs and performs inh flag
updates and virtual generated column expansion in a single loop, so
that only one table_open/table_close call is required for each
relation.  0003 collects NOT NULL attribute information for each
relation within the same loop, stores it in a relation OID based hash
table, and uses this information to reduce NullTest quals during
constant folding.

I think the code now more closely resembles the phase 1 and phase 2
described earlier: it collects all required early-stage catalog
information within a single loop over the rangetable, allowing each
relation to be opened and closed only once.  It also avoids the
has_subclass() call along the way.

Thanks
Richard

> On May 1, 2025, at 16:33, Richard Guo <guofenglinux@gmail.com> wrote:
>
> Here is the patchset that implements this optimization.  0001 moves
> the expansion of virtual generated columns to occur before sublink
> pull-up.  0002 introduces a new function, preprocess_relation_rtes,
> which scans the rangetable for relation RTEs and performs inh flag
> updates and virtual generated column expansion in a single loop, so
> that only one table_open/table_close call is required for each
> relation.  0003 collects NOT NULL attribute information for each
> relation within the same loop, stores it in a relation OID based hash
> table, and uses this information to reduce NullTest quals during
> constant folding.
>
> I think the code now more closely resembles the phase 1 and phase 2
> described earlier: it collects all required early-stage catalog
> information within a single loop over the rangetable, allowing each
> relation to be opened and closed only once.  It also avoids the
> has_subclass() call along the way.
>
> Thanks
> Richard
>
<v4-0001-Expand-virtual-generated-columns-before-sublink-p.patch><v4-0002-Centralize-collection-of-catalog-info-needed-earl.patch><v4-0003-Reduce-Var-IS-NOT-NULL-quals-during-constant-fold.patch>

Hi,

I've been following the V4 patches (focusing on 1 and 2 for now): Patch 2's preprocess_relation_rtes is a nice
improvementfor efficiently gathering early catalog info like inh and attgenerated definitions in one pass. 

However, Patch 1 needs to add expansion calls inside specific pull-up functions (like convert_EXISTS_sublink_to_join)
becausethe main expansion work was moved before pull_up_sublinks. 

Could we perhaps simplify this? What if preprocess_relation_rtes only collected the attgenerated definitions (storing
them,maybe in a hashtable like planned for attnotnull in Patch 3), but didn't perform the actual expansion (Var
replacement)?

Then, we could perform the actual expansion (Var replacement) in a separate, single, global step later on. Perhaps
afterpull_up_sublinks (closer to the original timing), or maybe even later still, for instance after
flatten_simple_union_all,once the main query structure including pulled-up subqueries/links has stabilized? A unified
expansionafter the major structural changes seems cleaner. I'm not sure where is the better position now. 

This might avoid the need for the extra expansion calls within convert_EXISTS_sublink_to_join, etc., keeping the
informationgathering separate from the expression transformation and potentially making the overall flow a bit cleaner. 

Any thoughts?

Thanks,

Chengpeng Yan

On Sat, May 3, 2025 at 7:48 PM Chengpeng Yan <chengpeng_yan@outlook.com> wrote:
> I've been following the V4 patches (focusing on 1 and 2 for now): Patch 2's preprocess_relation_rtes is a nice
improvementfor efficiently gathering early catalog info like inh and attgenerated definitions in one pass. 
>
> However, Patch 1 needs to add expansion calls inside specific pull-up functions (like convert_EXISTS_sublink_to_join)
becausethe main expansion work was moved before pull_up_sublinks. 
>
> Could we perhaps simplify this? What if preprocess_relation_rtes only collected the attgenerated definitions (storing
them,maybe in a hashtable like planned for attnotnull in Patch 3), but didn't perform the actual expansion (Var
replacement)?
>
> Then, we could perform the actual expansion (Var replacement) in a separate, single, global step later on. Perhaps
afterpull_up_sublinks (closer to the original timing), or maybe even later still, for instance after
flatten_simple_union_all,once the main query structure including pulled-up subqueries/links has stabilized? A unified
expansionafter the major structural changes seems cleaner. I'm not sure where is the better position now. 
>
> This might avoid the need for the extra expansion calls within convert_EXISTS_sublink_to_join, etc., keeping the
informationgathering separate from the expression transformation and potentially making the overall flow a bit cleaner. 
>
> Any thoughts?

This approach is possible, but I chose not to go that route because 1)
it would require an additional loop over the rangetable; 2) it would
involve collecting and storing in hash table a lot more information
that is only used during the expansion of virtual generated columns.
This includes not only the attgenerated attributes of columns you
mentioned, but also the default values of columns and the total number
of attributes in the tuple.

Therefore, it seems to me that expanding the virtual generated columns
within the same loop is cleaner and more efficient.

Please note that even if we move the expansion of virtual generated
columns into a separate loop, it still needs to occur before subquery
pull-up.  This is because we must ensure that RTE_RELATION RTEs do not
have lateral markers.  In other words, the expansion still needs to
take place within the subquery pull-up function.

Thanks
Richard

On Thu, May 1, 2025 at 4:33 AM Richard Guo <guofenglinux@gmail.com> wrote:
> Here is the patchset that implements this optimization.  0001 moves
> the expansion of virtual generated columns to occur before sublink
> pull-up.  0002 introduces a new function, preprocess_relation_rtes,
> which scans the rangetable for relation RTEs and performs inh flag
> updates and virtual generated column expansion in a single loop, so
> that only one table_open/table_close call is required for each
> relation.  0003 collects NOT NULL attribute information for each
> relation within the same loop, stores it in a relation OID based hash
> table, and uses this information to reduce NullTest quals during
> constant folding.
>
> I think the code now more closely resembles the phase 1 and phase 2
> described earlier: it collects all required early-stage catalog
> information within a single loop over the rangetable, allowing each
> relation to be opened and closed only once.  It also avoids the
> has_subclass() call along the way.

Before we commit to something along these lines, I think we need to
understand whether Tom intends to press Peter for some bigger change
around expand_virtual_generated_columns.

If Tom doesn't respond right away, I suggest that we need to add an
open item for http://postgr.es/m/602561.1744314879@sss.pgh.pa.us

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> Before we commit to something along these lines, I think we need to
> understand whether Tom intends to press Peter for some bigger change
> around expand_virtual_generated_columns.
> If Tom doesn't respond right away, I suggest that we need to add an
> open item for http://postgr.es/m/602561.1744314879@sss.pgh.pa.us

I think that we do need to do something about that, but it may be
in the too-late-for-v18 category by now.  Not sure.  I definitely
don't love the idea of table_open'ing every table in every query
an extra time just to find out (about 99.44% of the time) that it
does not have any virtual generated columns.

I wonder if a better answer would be to make the rewriter responsible
for this.  If you hold your head at the correct angle, a table with
virtual generated columns looks a good deal like a view, and we don't
ask the planner to handle those.

BTW, in my mind the current thread is certainly v19 material,
so I have not looked at Richard's patch yet.

            regards, tom lane