Re: Clarifying/rationalizing Vars' varno/varattno/varnoold/varoattno
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Date: 2019-12-31T03:50:21Z
Lists: pgsql-hackers
Attachments
- refactor-parser-RTE-handling-some-more-1.patch (text/x-diff) patch
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
Commits
-
Reconsider the representation of join alias Vars.
- 9ce77d75c5ab 13.0 landed
-
Make parser rely more heavily on the ParseNamespaceItem data structure.
- 5815696bc66b 13.0 landed
-
Refactor parser's generation of Var nodes.
- b541e9accb28 13.0 landed