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

Hi,

Note that this thread should be a continuation of [1], but in the archives,
it shows as a separate thread [2].

> CREATE TABLE jt (js jsonb);
> SELECT JSON_QUERY(js, '$.b' EMPTY ARRAY ON EMPTY) FROM jt;
> SELECT JSON_EXISTS(js, 'strict $.b' TRUE ON ERROR) FROM jt;
> SELECT JSON_EXISTS(js, 'strict $.b' FALSE ON ERROR) FROM jt;
> SELECT query FROM pg_stat_statements WHERE query LIKE 'SELECT JSON%';
> query
> -----------------------------------------------------
> SELECT JSON_EXISTS(js, $1 $2 ON ERROR) FROM jt
> SELECT JSON_EXISTS(js, $1 $2 ON ERROR) FROM jt
> SELECT JSON_QUERY(js, $1 $2 ARRAY ON EMPTY) FROM jt

I agree that the behavior keywords should not be turned into constants here.
0001 fixes that part correctly, AFAICS. These Consts are created to represent
behavior keywords, not constants that appeared in the query text, so leaving
their location unset seems right.

> The same happens with EMPTY OBJECT and NULL. DEFAULT <expr> is
> normalized correctly. The normalized text is not valid SQL, so it can't
> be used with EXPLAIN (GENERIC_PLAN):
>
> EXPLAIN (GENERIC_PLAN) SELECT JSON_QUERY(js, $1 $2 ARRAY ON EMPTY) FROM jt;
> ERROR: syntax error at or near "$2"

That fixes the behavior-keyword problem, but pg_stat_statements.query is
representative text as documented [3]:

```
When a constant's value has been ignored for purposes of matching the query
to other queries, the constant is replaced by a parameter symbol, such as $1,
in the pg_stat_statements display.
```

That does not mean the text can always be passed directly to EXPLAIN
(GENERIC_PLAN), or executed as a prepared statement. A lot of the time it can,
but I don't think that is guaranteed.

The tests in 0001 show this too if they use typed string literal syntax.
For example, `jsonb '{}'` can still be displayed as `jsonb $1`, which is not
valid SQL. That is a separate, pre-existing pg_stat_statements normalization
issue.

Maybe most of the 0001 tests should use `'{}'::jsonb`, so they stay focused on
the behavior keyword fix. One typed string literal case could remain with a
comment saying that a form such as `jsonb '{}'` cannot be replayed as-is.

As far as 0002 goes; this one gives me more pause. I see why the examples in
the email are bad. If two JSON_TABLE queries differ in column type, or in NULL
ON EMPTY vs ERROR ON EMPTY, merging them under one query ID does not look
right. But I don't think that means the whole column definition should become
part of the query ID.

For example, with an explicit PATH these two only differ in the output column
name:

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

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

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. It looks more like changing an output alias.

As a comparison, changing only the output column name for generate_series()
still merges into one entry:

```
SELECT pg_stat_statements_reset();

SELECT * FROM generate_series(1, 1) AS g(a);
SELECT * FROM generate_series(1, 1) AS g(b);

SELECT queryid, calls, query
FROM pg_stat_statements
WHERE query LIKE 'SELECT * FROM generate_series%'
ORDER BY query COLLATE "C";
```

which gives one query ID with calls = 2.

I also find the TableFunc.plan part a bit hard to get comfortable with. I
realize this is not a normal planner Plan node, but it is still generated
state for how JSON_TABLE is processed. AFAICS planner/path plan nodes, and even
SubPlan, are not query jumbled. So making pg_stat_statements walk all of that
feels broader than what this bug needs.

If we do want to do something here, maybe the fix needs to be more targeted.
For example, include the parts that change the result, such as the column type,
behavior clauses, and PLAN clause details, but not the output column name and
not every field in the generated JSON_TABLE state. Does that seem like the
right direction?

[1] https://www.postgresql.org/message-id/flat/086A90EE-9771-4387-8B46-BF84A610F04F%40localhost.localdomain
[2] https://www.postgresql.org/message-id/flat/DFE8E2CE-7A51-447C-B600-7D3979E38F1B%40localhost.localdomain
[3] https://www.postgresql.org/docs/current/pgstatstatements.html

--
Sami Imseih
Amazon Web Services (AWS)