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

On 2022-08-09 Tu 09:59, Jonathan S. Katz wrote:
> 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.
>
>

I very much doubt option 3 is feasible. The parts that are controversial
go back at least in part to the first patches of the series. Trying to
salvage anything would almost certainly be more disruptive than trying
to fix it.

I'm not sure what the danger is to stability, performance or correctness
in applying the changes Amit has proposed for release 15. But if that
danger is judged to be too great then I agree we should revert.

I should add that I'm very grateful to Amit for his work, and I'm sure
it isn't wasted effort, whatever the decision.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com