Horton

Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size

Jonathan S. Katz <jkatz@postgresql.org>

View thread View on postgr.es
From: "Jonathan S. Katz" <jkatz@postgresql.org>
To: Andres Freund <andres@anarazel.de>, Amit Langote <amitlangote09@gmail.com>
Cc: David Rowley <dgrowleyml@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Andrew Dunstan <andrew@dunslane.net>, Michael Paquier <michael@paquier.xyz>, John Naylor <john.naylor@enterprisedb.com>, Tom Lane <tgl@sss.pgh.pa.us>, Peter Geoghegan <pg@bowt.ie>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Nikita Glukhov <n.gluhov@postgrespro.ru>
Date: 2022-08-09T13:59:51Z
Lists: pgsql-hackers 
On 8/5/22 4:36 PM, Andres Freund wrote:
> Hi,
> 
> I tried to look into some of the questions from Amit, but I have e.g. no idea
> what exactly the use of subtransactions tries to achieve - afaics 1a36bc9dba8
> is the first patch to introduce needing to evaluate parts expressions in a
> subtransaction - but there's not a single comment explaining why.
> 
> It's really hard to understand the new json code. It's a substantial amount of
> new infrastructure, without any design documentation that I can find. And it's
> not like it's just code that's equivalent to nearby stuff. To me this falls
> way below our standards and I think it's *months* of work to fix.
> 
> Even just the expresion evaluation code: EvalJsonPathVar(),
> ExecEvalJsonConstructor(), ExecEvalJsonExpr(), ExecEvalJson(). There's one
> layer of subtransactions in one of the paths in ExecEvalJsonExpr(), another in
> ExecEvalJson(). Some paths of ExecEvalJsonExpr() go through subtransactions,
> others don't.
> 
> It's one thing for a small set of changes to be of this quality. But this is
> pretty large - a quick summing of diffstat ends up with about 17k lines added,
> of which ~2.5k are docs, ~4.8k are tests.

The RMT met today to discuss the state of this open item surrounding the 
SQL/JSON feature set. We discussed the specific concerns raised about 
the code and debated four different options:

   1. Do nothing and continue with the community process of stabilizing 
the code without significant redesign

   2. Recommend holding up the v15 release to allow for the code to be 
redesigned and fixed (as based on Andres' estimates, this would push the 
release out several months).

   3. Revert the problematic parts of the code but try to include some 
of the features in the v15 release (e.g. JSON_TABLE)

   4. Revert the feature set and redesign and try to include for v16

Based on the concerns raised, the RMT is recommending option #4, to 
revert the SQL/JSON changes for v15, and come back with a redesign for v16.

If folks think there are some bits we can include in v15, we can 
consider option #3. (Personally, I would like to see if we can keep 
JSON_TABLE, but if there is too much overlap with the problematic 
portions of the code I am fine with waiting for v16).

At this stage in the release process coupled with the concerns, we're a 
bit worried about introducing changes that are unpredictable in terms of 
stability and maintenance. We also do not want to hold up the release 
while this feature set is goes through a redesign without agreement on 
what such a design would look like as well as a timeline.

Note that the above are the RMT's recommendations; while the RMT can 
explicitly call for a revert, we want to first guide the discussion on 
the best path forward knowing the challenges for including these 
features in v15.

Thanks,

Jonathan

Commits

  1. JSON_TABLE: Add support for NESTED paths and columns

  2. Add basic JSON_TABLE() functionality

  3. Add SQL/JSON query functions

  4. Add soft error handling to some expression nodes

  5. Adjust populate_record_field() to handle errors softly

  6. Refactor code used by jsonpath executor to fetch variables

  7. Add more SQL/JSON constructor functions

  8. SQL/JSON: support the IS JSON predicate

  9. SQL/JSON: add standard JSON constructor functions

  10. Add static assertion ensuring sizeof(ExprEvalStep) <= 64 bytes

  11. Remove size increase in ExprEvalStep caused by hashed saops

  12. pgstat: reduce timer overhead by leaving timer running.

  13. expression eval: Fix EEOP_JSON_CONSTRUCTOR and EEOP_JSONEXPR size.

  14. SQL/JSON query functions

  15. Speedup ScalarArrayOpExpr evaluation