Thread: pgsql: Make parser rely more heavily on the ParseNamespaceItem data str

pgsql: Make parser rely more heavily on the ParseNamespaceItem data str

From
Tom Lane
Date:
Make parser rely more heavily on the ParseNamespaceItem data structure.

When I added the ParseNamespaceItem data structure (in commit 5ebaaa494),
it wasn't very tightly integrated into the parser's APIs.  In the wake of
adding p_rtindex to that struct (commit b541e9acc), there is a good reason
to make more use of it: by passing around ParseNamespaceItem pointers
instead of bare RTE pointers, we can get rid of various messy methods for
passing back or deducing the rangetable index of an RTE during parsing.
Hence, refactor the addRangeTableEntryXXX functions to build and return
a ParseNamespaceItem struct, not just the RTE proper; and replace
addRTEtoQuery with addNSItemToQuery, which is passed a ParseNamespaceItem
rather than building one internally.

Also, add per-column data (a ParseNamespaceColumn array) to each
ParseNamespaceItem.  These arrays are built during addRangeTableEntryXXX,
where we have column type data at hand so that it's nearly free to fill
the data structure.  Later, when we need to build Vars referencing RTEs,
we can use the ParseNamespaceColumn info to avoid the rather expensive
operations done in get_rte_attribute_type() or expandRTE().
get_rte_attribute_type() is indeed dead code now, so I've removed it.
This makes for a useful improvement in parse analysis speed, around 20%
in one moderately-complex test query.

The ParseNamespaceColumn structs also include Var identity information
(varno/varattno).  That info isn't actually being used in this patch,
except that p_varno == 0 is a handy test for a dropped column.
A follow-on patch will make more use of it.

Discussion: https://postgr.es/m/2461.1577764221@sss.pgh.pa.us

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/5815696bc66b3092f6361f53e0394909647042c8

Modified Files
--------------
src/backend/catalog/heap.c                       |  16 +-
src/backend/commands/copy.c                      |  10 +-
src/backend/commands/policy.c                    |  63 +--
src/backend/commands/tablecmds.c                 |  23 +-
src/backend/commands/trigger.c                   |  24 +-
src/backend/commands/view.c                      |  20 +-
src/backend/optimizer/plan/subselect.c           |  12 +-
src/backend/parser/analyze.c                     | 139 +++--
src/backend/parser/parse_clause.c                | 436 +++++++--------
src/backend/parser/parse_coerce.c                |   7 +-
src/backend/parser/parse_expr.c                  |   4 +-
src/backend/parser/parse_relation.c              | 648 +++++++++++++----------
src/backend/parser/parse_target.c                |  11 +-
src/backend/parser/parse_utilcmd.c               | 100 ++--
src/backend/replication/logical/tablesync.c      |   4 +-
src/backend/rewrite/rewriteHandler.c             |  12 +-
src/include/parser/parse_node.h                  |  57 +-
src/include/parser/parse_relation.h              | 106 ++--
src/include/parser/parsetree.h                   |  10 +-
src/test/modules/test_rls_hooks/test_rls_hooks.c |  19 +-
20 files changed, 927 insertions(+), 794 deletions(-)


Tom Lane <tgl@sss.pgh.pa.us> writes:
> Make parser rely more heavily on the ParseNamespaceItem data structure.

Hmm, the buildfarm seems to think this screwed up something affecting
collations.  Looking ...

            regards, tom lane



Re: pgsql: Make parser rely more heavily on the ParseNamespaceItemdata str

From
Julien Rouhaud
Date:
On Thu, Jan 2, 2020 at 6:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Tom Lane <tgl@sss.pgh.pa.us> writes:
> > Make parser rely more heavily on the ParseNamespaceItem data structure.
>
> Hmm, the buildfarm seems to think this screwed up something affecting
> collations.  Looking ...

My animal is using fr_FR collation by default, and
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2020-01-02%2016%3A40%3A21
is reporting:

diff -U3 /home/postgres/buildroot/HEAD/pgsql.build/src/test/regress/expected/collate.out
/home/postgres/buildroot/HEAD/pgsql.build/src/test/regress/results/collate.out
--- /home/postgres/buildroot/HEAD/pgsql.build/src/test/regress/expected/collate.out
2019-06-04 18:32:50.356908034 +0000
+++ /home/postgres/buildroot/HEAD/pgsql.build/src/test/regress/results/collate.out
2020-01-02 17:21:27.696908000 +0000
@@ -540,18 +540,18 @@
 SELECT * FROM unnest((SELECT array_agg(b ORDER BY b) FROM
collate_test1)) ORDER BY 1;
  unnest
 --------
- ABD
- Abc
  abc
+ Abc
+ ABD
  bbc
 (4 rows)

 SELECT * FROM unnest((SELECT array_agg(b ORDER BY b) FROM
collate_test2)) ORDER BY 1;
  unnest
 --------
- ABD
- Abc
  abc
+ Abc
+ ABD
  bbc
 (4 rows)

Is ORDER BY 1 COLLATE "C" an option here?



Julien Rouhaud <rjuju123@gmail.com> writes:
> On Thu, Jan 2, 2020 at 6:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hmm, the buildfarm seems to think this screwed up something affecting
>> collations.  Looking ...

> Is ORDER BY 1 COLLATE "C" an option here?

Nah, that would just mask the bug not fix it.  See 4d02eb017.

            regards, tom lane



Re: pgsql: Make parser rely more heavily on the ParseNamespaceItemdata str

From
Julien Rouhaud
Date:
On Thu, Jan 2, 2020 at 7:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Julien Rouhaud <rjuju123@gmail.com> writes:
> > On Thu, Jan 2, 2020 at 6:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Hmm, the buildfarm seems to think this screwed up something affecting
> >> collations.  Looking ...
>
> > Is ORDER BY 1 COLLATE "C" an option here?
>
> Nah, that would just mask the bug not fix it.  See 4d02eb017.

Ah indeed I see, thanks.