Re: Clarifying/rationalizing Vars' varno/varattno/varnoold/varoattno

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Clarifying/rationalizing Vars' varno/varattno/varnoold/varoattno
Date: 2019-12-31 03:50:21
Message-ID: 2461.1577764221@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> Anyway, I had started to work on getting parse analysis to label
> outer-join-nullable Vars properly, and soon decided that no matter how
> we do it, there's not enough information available at the point where
> parse analysis makes a Var. The referenced RTE is not, in itself,
> enough info, and I don't think we want to decorate RTEs with more info
> that's only needed during parse analysis. What would be saner is to add
> any extra info to the ParseNamespaceItem structs.

Here is a further step on this journey. It's still just parser
refactoring, and doesn't (AFAICT) result in any change in generated
parse trees, but it seems worth posting and committing separately.

The two key ideas here are:

1. Integrate ParseNamespaceItems a bit further into the parser's
relevant APIs. In particular, the addRangeTableEntryXXX functions
no longer return just a bare RTE, but a ParseNamespaceItem wrapper
for it. This gets rid of a number of kluges we had for finding out
the RT index of the new RTE, since that's now carried along in the
nsitem --- we no longer need fragile assumptions about how the new
RTE is still the last one in the rangetable, at some point rather
distant from where it was actually appended to the list.

Most of the callers of addRangeTableEntryXXX functions just turn
around and pass the result to addRTEtoQuery, which I've renamed
to addNSItemtoQuery; it doesn't gin up a new nsitem anymore but
just installs the one it's passed. It's perhaps a bit inconsistent
that I renamed that function but not addRangeTableEntryXXX.
I considered making those addNamespaceItemXXX, but desisted on the
perhaps-thin grounds that they don't link the new nsitem into the
parse state, only the RTE. This could be argued of course.

2. Add per-column information to the ParseNamespaceItems. As of
this patch, the useful part of that is column type/typmod/collation
info which can be used to generate Vars referencing this RTE.
I envision that the next step will involve generating the Vars'
identity (varno/varattno columns) from that as well, and this
patch includes logic to set up some associated per-column fields.
But those are not actually getting fed into the Vars quite yet.
(The step after that will be to add outer-join-nullability info.)

But independently of those future improvements, this patch is
a win because it allows carrying forward column-type info that's
known at the time we do addRangeTableEntryXXX, and using that
when we make a Var, instead of having to do the rather expensive
computations involved in expandRTE() or get_rte_attribute_type().
get_rte_attribute_type() is indeed gone altogether, and while
expandRTE() is still needed, it's not used in any performance-critical
parse analysis code paths.

On a complex-query test case that I've used before [1], microbenchmarking
just raw parsing plus parse analysis shows a full 20% speedup over HEAD,
which I think can mostly be attributed to getting rid of the syscache
lookups that get_rte_attribute_type() did for Vars referencing base
relations. The total impact over a complete query execution cycle
is a lot less of course. Still, it's pretty clearly a performance win,
and to my mind the code is also cleaner --- this is paying down some
technical debt from when we bolted JOIN syntax onto pre-existing
parsing code.

Barring objections, I plan to commit this fairly soon and get onto the
next step, which will start to have ramifications outside the parser.

regards, tom lane

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

Attachment Content-Type Size
refactor-parser-RTE-handling-some-more-1.patch text/x-diff 105.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2019-12-31 04:05:31 Re: Cache relation sizes?
Previous Message Amit Kapila 2019-12-31 03:38:56 Re: [HACKERS] Block level parallel vacuum