Fix parser performance

[LennartSpitzner]

Hey, thanks for maintaining this package! And for considering this non-trivial PR :)

This PR fixes some bad parsing behaviour that leads to exponential time usage.

This PR looks big, but note that I applied an auto-formatter on Parsing.hs before I started to work on it. If this is a blocker just tell me and I can rebase the changes on a non-auto-formatted branch. To see the changes beyond auto-formatting see proda-ai#2, which is everything but the first commit here.

Testcase

((((((    ( COALESCE (mytable.oiuqweu_puiwrip_eoprdc, 0)
          + COALESCE (mytable.insdad_eoprdc, 0)
          + COALESCE (mytable.basdnasd_rates_eoprdc, 0)
          + COALESCE (mytable.poadpod_tpio_eoprdc, 0)
          + COALESCE (mytable.mkadldod_puiwrip_eoprdc, 0)
          + COALESCE (mytable.woidp_eoprdc, 0)
          + COALESCE (mytable.poiqwehda_eoprdc, 0)
          + COALESCE (mytable.htaing_eoprdc, 0)
          + COALESCE (mytable.clingd_eoprdc, 0)
          + COALESCE (mytable.lkjaldjj_eoprdc, 0)
          + COALESCE (mytable.dopasdop_eoprdc, 0)
          + COALESCE (mytable.thotd_idiaspdoid_eoprdc, 0)
          ) - 
          ( COALESCE (mytable.poadpod_tpio, 0)
          + COALESCE (mytable.mkadldod_puiwrip, 0)
          + COALESCE (mytable.insdad_poaisd, 0)
          + COALESCE (mytable.basdnasd_rates_poaisd, 0)
          + COALESCE (mytable.oiuqweu_puiwrip_poaisd, 0)
          + COALESCE (mytable.lkjfjf_dopasdop_cost_iuasudd, 0)
          + COALESCE (mytable.lkjfjf_htaing_iuasudd, 0)
          + COALESCE (mytable.prepaud_dopasdop_cost_poaisd, 0)
          + COALESCE (mytable.prepaud_mkadldod_puiwrip_poaisd, 0)
          + COALESCE (mytable.prepaud_woidp_poaisd, 0)
          + COALESCE (mytable.prepaud_poiqwehda_poaisd, 0)
          + COALESCE (mytable.thotd_idiaspdoid_poaisd, 0)
          + COALESCE (mytable.lkjfjf_woidp_iuasudd, 0)
          + COALESCE (mytable.lkjfjf_poiqwehda_iuasudd, 0)
          + COALESCE (mytable.lkjfjf_clingd_iuasudd, 0)
          + COALESCE (mytable.lkjfjf_lkjaldjj_iuasudd, 0)
          + COALESCE (mytable.prepaud_clingd_poaisd, 0)
          + COALESCE (mytable.prepaud_lkjaldjj_poaisd, 0)
          + COALESCE (mytable.prepaud_htaing_poaisd, 0)
          + COALESCE (mytable.prepaud_thotd_idiaspdoid_poaisd, 0)
          + COALESCE (mytable.lkjfjf_thotd_idiaspdoid_iuasudd, 0)
          + COALESCE (mytable.lkjfjf_mkadldod_puiwrip_iuasudd, 0)
          + COALESCE (mytable.dopasdop_eoprdc_poaisd, 0)
          + COALESCE (mytable.htaing_poaisd, 0)
          + COALESCE (mytable.clingd_poaisd, 0)
          + COALESCE (mytable.woidp_poaisd, 0)
          + COALESCE (mytable.poiqwehda_poaisd, 0)
          + COALESCE (mytable.lkjaldjj_poaisd, 0)
          )
))))))

Parsing this does terminate, but you definitely start noticing the cpu time, and if you profile you can see some suspiciously large values. We had some larger queries that did not terminate, but with the same underlying problem.

Analysis

There are a couple of parsers around aExpr, customizedAExpr, cExpr, customizedCExpr that have alternatives with common prefixes and with ambiguous parses.

1. Examples of common prefixes are:

• spaces in alternatives (customizedAExpr/suffix)
• open parenthesis + aExpr is a prefix of both InParensCExpr or ImplicitRowCExpr
• colId is a prefix of both FuncCExpr and ColumnrefCExpr

On their own these might be harmless, but once such cases nest they can create a problem. I think the last of the above is just a constant-factor problem (so rather harmless); still, I cleaned these up in the PR because a) it was relatively simple to do b) It is hard to diagnose which common prefixes lead to problems, so it makes debugging the whole thing easier if they get cleaned up.

2. As example of a ambiguous parse consider the following two trees:

InParensCExpr (SelectWithParensCExpr (NoParensSelectWithParens a) Nothing)
SelectWithParensCExpr (WithParensSelectWithParens a) Nothing

both should correspond to something like ((SELECT …)).

The ambiguity might be known because the hedgehog tests explicitly generate only one of those two options (see

postgresql-syntax/hedgehog-test/Main/Gen.hs

Line 475 in 6caff75

nonSelectAExpr = choice [

).

Approach

I follow a simple process: Take any alternative (asum [..]) where alternatives have common prefixes, remove those alternatives and replace with a single new one that first matches the common prefix, then branches again. As an example, I replaced

… = asum
  [ …
  , inParens foo
  , inParens bar
  , …
  ]

with

… = asum
  [ …
  , char '(' *> space *> asum [ fooTail, barTail ] *< space *< char ')'
  , …
  ]

It is not always this simple, for example if the inParens appears deeper in the call stack then this gets messier, with multiple new XTail bindings.

Notes

• I am not sure if I applied endHead and wrapHead correctly everywhere. The tests pass, but I am not sure that the tests cover the readability of the error-messages (iiuc the "HeadParser" also has the purpose of helping with those).

• There are some cases still where the order of the parsers (in asums) matters. I don't think that is ideal, nor should it be necessary. As I understand it this might be due to committing to some branch (via endHead) too soon. But it is also harmless given that the tests pass.

• I added https://github.com/proda-ai/postgresql-syntax/blob/e0450870e35dcbfd4a11d6757bcb54c4bd65cc10/library/PostgresqlSyntax/Parsing.hs#L1288-L1301 after resolving the ambiguity-problem: The tests expect nested WithParensSelectWithParens, so the parser simply converts InParensCExpr (CExprAExpr …) to that where necessary. Could instead fix the tests, but maybe the slightly flatter trees for nested selects are worth this cost. I have no idea why exactly the AST types are defined as they are, to be honest.

• I feel like the code now combines several different approaches to ensure good performance:

  • the customizedCExpr approach to parameterise by some child-parser
  • just defining multiple parsers
  • defining "tail" parsers (what I did here)

  I am not proud of my addition as it makes the whole logic harder to follow. It might be cleaner make use of continuations, but still that does not improve things much. But I don't see an easy-to-read/maintain approach with good performance ..

  At least I only had to touch the "innermost loop" when parsing expressions, so maybe it is worth it.

• the fact that this

  postgresql-syntax/library/PostgresqlSyntax/Parsing.hs

  Line 1025 in 6caff75

  symbolicBinOpExpr a aExpr SymbolicBinOpAExpr

  uses aExpr instead of base means that a) you get a more "left-leaning" output tree but also b) that if parsing fails, you might end up failing again and again for every level of a nested aExpr. Same for bExpr. I left a note about this in the code. Could be fixed by the same "parse different tree (using base), then post-process to restructure the tree" approach that I used for nested selects.

• might be nice to have performance tests included. There should be some simple trivial expression like ((((((((((a+b)))))))))) that where parsing does not terminate on master but is instant with this PR. Might need a bit more nesting. But yeah, perf tests are a bit annoying at times because they can depend on the performance of the system running the tests.

[LennartSpitzner]

Replaced by #8