Re: pg_stat_statements replaces SQL/JSON ON EMPTY / ON ERROR keywords with parameters

Hi Sami,

Thanks for the review.

Sorry about the threading: my mail client didn't add In-Reply-To to
that message, and our mail server added the [MASSMAIL] tag, which I've
removed here. The start of the thread is [1].

> That does not mean the text can always be passed directly to EXPLAIN
> (GENERIC_PLAN), or executed as a prepared statement.

Agreed, I shouldn't have used that as an argument. The problem 0001
fixes is that the keyword is lost: TRUE ON ERROR and FALSE ON ERROR get
separate entries with the same text. I've reworded the commit message
accordingly, and changed the tests to use '{}'::jsonb, except for one
with a comment, as you suggested. That one uses UNKNOWN ON ERROR, which
wasn't covered before.

> With 0002 they get different query IDs, AFAICS because
> JsonExpr.column_name is included through TableFunc.colvalexprs. That
> does not look like a different query to me.

Agreed. The column names in RangeTblEntry.eref are not jumbled either,
so my comparison with table aliases wasn't a good one. In v3, neither
the column names nor the path names (AS p1) are part of the query ID;
the path names are only used to refer to paths in the PLAN clause and
when deparsing.

There is one exception: a column without PATH gets the path '$."name"'
derived from its name. That path doesn't appear in the query text, so
it has no location, and since Const values are not jumbled, the name
is the only thing that tells such columns apart. If it were ignored,
these two would be merged, although they read different keys, and the
entry would show "a int" for both:

SELECT * FROM JSON_TABLE('[{"a": 1, "b": 2}]'::jsonb, '$[*]'
COLUMNS (a int)) jt;
SELECT * FROM JSON_TABLE('[{"a": 1, "b": 2}]'::jsonb, '$[*]'
COLUMNS (b int)) jt;

So v3 gives JsonExpr.column_name a custom jumble function, which
includes the name only when the path has no location, that is, for a
column without PATH. As a result, a column without PATH and the same
column with the equivalent explicit PATH get different query IDs, which
seems harmless. Also, the other SQL/JSON query functions no longer
jumble their NULL column_name, so their query IDs change too; the
commit message mentions that.

> I also find the TableFunc.plan part a bit hard to get comfortable
> with.

TableFunc.plan is built by parse analysis, in transformJsonTable(), not
by the planner. It's the only place in the Query that records the
NESTED PATH structure and the PLAN clause: which columns belong to
which path, OUTER or INNER, UNION or CROSS. With the path names left
out, everything that remains in these nodes affects the result:

JsonTablePath: value (the path Const, which is normalized)
JsonTablePathScan: path, errorOnError, child, outerJoin,
colMin, colMax
JsonTableSiblingJoin: lplan, rplan, cross

(outerJoin and cross exist only on master.) And since it is the
resolved plan, equivalent spellings still get the same query ID. On
master, a query without a PLAN clause and the same query with PLAN (p0
OUTER p1), which is the default, are merged, also with different path
names, while PLAN (p0 INNER p1) gets a separate entry. So I think
jumbling the plan without the path names is about as targeted as it
gets, but if you'd leave out some of these fields, I can change that.

A correction to my previous message: it's not the number of columns
that was part of the query ID, but the references to the columns from
the rest of the query, such as the expansion of SELECT *. The commit
message now says that.

The test in 0002 now also checks that queries which differ only in
column names or path names are merged, and that columns without PATH
and with different names are not. The PLAN clause doesn't exist in
REL_19_STABLE, so the test doesn't use it; I checked those cases on
master by hand. The series applies to master and REL_19_STABLE, and
I've tested it on both.

[1] https://www.postgresql.org/message-id/flat/086A90EE-9771-4387-8B46-BF84A610F04F%40localhost.localdomain

--
Ilya Staroverov