Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size - Mailing list pgsql-hackers

Hi,

On 2022-08-09 14:04:48 -0400, Jonathan S. Katz wrote:
> On 8/9/22 11:03 AM, Andrew Dunstan wrote:
> > 
> > On 2022-08-09 Tu 09:59, Jonathan S. Katz wrote:
> 
> > > 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).

Obviously that's a question of the resources brought to bear.

From my angle: I've obviously some of my own work to take care of as well, but
it's also just hard to improve something that includes a lot of undocumented
design decisions.


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

> > 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.

Agreed.


> > >    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.

Unless we decide on 4 immediately, I think it might be worth starting a
separate thread to get more attention. The subject doesn't necessarily have
everyone follow along.



> > 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.

My primary problem is that as-is the code is nearly unmaintainable. It's hard
for Amit to fix that, given that he's not one of the original authors.


> Rereading the thread for the umpteenth time, I have seen Amit working
> through Andres' concerns. From my read, the ones that seem pressing are:
> 
> * Lack of design documentation, which may be leading to some of the general
> design concerns

> * The use of substransactions within the executor, though docs explaining
> the decisions on that could alleviate it (I realize this is a big topic and
> any summary I give won't capture the full nuance)

I don't think subtransactions per-se are a fundamental problem. I think the
error handling implementation is ridiculously complicated, and while I started
to hack on improving it, I stopped when I really couldn't understand what
errors it actually needs to handle when and why.


> * Debate on how to handle the type coercions

That's partially related to the error handling issue above.

One way this code could be drastically simplified is to force all
type-coercions to go through the "io coercion" path, which could be
implemented as a single execution step (which thus could trivially
start/finish a subtransaction) and would remove a lot of the complicated code
around coercions.


> > 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.
> 
> +1. While I've been quiet on this thread to date, I have definitely seen
> Amit working hard on addressing the concerns.

+1

Greetings,

Andres Freund