Postgresql: sql snippet causes hang in parser

This postgres snippet causes the parser to hang:

            select
              t0.operatienr
            , case
                when
                    case when (t0.vc_begintijd_operatie is null or lpad((extract('hours' from t0.vc_begintijd_operatie::timestamp))::text,2,'0') ||':'|| lpad(extract('minutes' from t0.vc_begintijd_operatie::timestamp)::text,2,'0') = '00:00') then null
                         else (greatest(((extract('hours' from (t0.vc_eindtijd_operatie::timestamp-t0.vc_begintijd_operatie::timestamp))*60 + extract('minutes' from (t0.vc_eindtijd_operatie::timestamp-t0.vc_begintijd_operatie::timestamp)))/60)::numeric(12,2),0))*60
                end = 0 then null
                    else '25. Meer dan 4 uur'
                end                                                                                                                                                  
              as snijtijd_interval

@manticore-projects

@manticore-projects Did I see this right that CCJSqlParserUtil.parseStatement is now using an ExecutorService and a fixed timeout to do its job? Sorry, that I obviously overread that. I should never have accepted this. So you have no chance of parser longer lasting requests to be parsed right, or is there a replacement for the original parse stuff? I will rewind all these changes and we could introduce a CCJSqlParserUtil class variant that holds this stuff.

@d2a-raudenaerde

@d2a-raudenaerde For now you could use something like this, to circumvent this timeout stuff:

Statement statement = CCJSqlParserUtil.newParser(sql).Statement();

However, it produces after a couple of seconds a parsing error since there are some unknown constructs in it. I will look into it, because I am not sure why it is so slow.

@d2a-raudenaerde

@d2a-raudenaerde The performance problem occurs due to activated complex parsing of case conditions, that could be any expression, so if you switch that off, you get a result in a reasonable time:

Statement statement = CCJSqlParserUtil.newParser(sql)
                .withAllowComplexParsing(false)
                .Statement();

@manticore-projects

@manticore-projects Did I see this right that CCJSqlParserUtil.parseStatement is now using an ExecutorService and a fixed timeout to do its job? Sorry, that I obviously overread that. I should never have accepted this. So you have no chance of parser longer lasting requests to be parsed right, or is there a replacement for the original parse stuff? I will rewind all these changes and we could introduce a CCJSqlParserUtil class variant that holds this stuff.

I agree, that we should provide an option to modify/increase the timeout.
Although I am unsure, how you want to move the Hanging Detection to any other Class and why. There are statement, which "Hang" the parser for ever. We have tests. DBeaver has complains. So why would you not want to fix that problem at the source.

Please note: the reason, that it does not work for the example above is not the Timeout itself or the Class, where we wrap into an Executor Service. The reason is, that we have not "Break" condition inside the Grammar for this particular statement.

@manticore-projects

@manticore-projects IMHO JSqlParser should not have to deal with this kind of stuff. Only the user of this library can determine the time till blocking. However, at least it should be configurable and optional. So I will remove it directly from the utils class. We could implement a second class to explicitly make this timing stuff.

For me this "hanging" detection would be equal to Apache Commons integrate a "hanging" detection for StringUtils. It could take a while as well to complete but this should be in the hand of the user of this library and not in the responsibility of this library itself.

BTW the parser will not hang forever. There, at least if JavaCC did its job right, are no endless loops. Sure JSqlParser has to deal with two limited resources: CPU power and Memory size, but as I said, the timeout or memory consumption check should be done outside JSqlParser.

I have plans to rewrite / reimplement this CCJSqlParserUtil class to provide some kind of fluent interface. Within this effort one could implement this timeout detection in a visible but easy way. I don't like (at the moment :)) this lambda variant to change some parser internal variables.

Greetings.

On Thu, 2022-07-14 at 23:34 -0700, Tobias wrote: @manticore-projects IMHO JSqlParser should not have to deal with this kind of stuff. Only the user of this library can determine the time till blocking. However, at least it should be configurable and optional. So I will remove it directly from the utils class.

There is one big problem though: the Parser will hang and can't be killed from Java directly. You may detect the hanging from outside -- and then what? Next thing: for reasonable performance, we have to try "simple parsing" first before we try "complex parsing". If "simple parsing" hangs, then what? How to get to "complex parsing" next? To me, this was a fork criteria. It is bad enough that we can't guarantee to parse valid statements in reasonable time -- but at least we must provide methods to deal with it.

BTW the parser will not hang forever. There, at least if JavaCC did its job right, are no endless loops. Sure JSqlParser has to deal with two limited resources: CPU power and Memory size, but as I said, the timeout or memory consumption check should be done outside JSqlParser.

True in theory, but in practise we have statements which freeze for more than 10 minutes.

The only real issue is to find out, why the Parser did not exit after timeout in this particular usecase.

There is one big problem though: the Parser will hang and can't be
killed from Java directly.
You may detect the hanging from outside -- and then what?

What does your code? It throws an Exception. I do not mean the implementation that uses this timeout to switch from complex to simple parsing, but e.g. parseStatement.

Next thing: for reasonable performance, we have to try "simple parsing"
first before we try "complex parsing". If "simple parsing" hangs, then
what? How to get to "complex parsing" next?

That's right, but it should not be the normal behavior for parseStatement. It is simply bad design to implement this timer in a method that does not enforce this decision.

To me, this was a fork criteria. It is bad enough that we can't
guarantee to parse valid statements in reasonable time -- but at least
we must provide methods to deal with it.

I am with you on that, but the user should know, what he is dealing with. The main problems here are

the fixed timeout, that prevents processing of a whole class of more complex expressions
the implementation of this timeout check in a public method
the implementation without the user giving a hint, what this preparser (timer and complex level checking) process does
the inconsistency of this implementation because some methods do this timeout decision and some don't
last but worst, the parser is not able to deal with those complex expressions on its own

Maybe I was a bit harsh about that, so sorry, but this decision handling should go into one util class, but the user should be able to use the parser without it and he should know how to do both variants.

The only real issue is to find out, why the Parser did not exit after timeout in this particular use case.

This is no miracle. He used some method that does not enforce this timeout. Look at my example.

So to make all users of this timed approach happy I would go for complete implementation in CCJSqlParserUtil and do the other direct approach in a different class.

However, improving the parsing performance for complex expressions should not be abandoned. But this seems to me heading to a massive refactoring of the grammar. And there still is the possibility of failure here and after all this work we could be forced to go back to this timed decision.

@d2a-raudenaerde

As long as PR's are pending for several months, I will rather focus on things that are actually broken:

the OP's statement actually fails because of extract('hours' from t0.vc_begintijd_operatie) is not supported
the Parser does not quit, when the timeout is reached
the timeout should be configurable of course

@d2a-raudenaerde: the special function Extract( ... FROM ...) is a challenge and not support yet. When I rewrite this statement with a normal fucntion Extract( param1, param2) everything works and fast:

SELECT  t0.operatienr
        , CASE
                WHEN CASE
                                WHEN ( t0.vc_begintijd_operatie IS NULL
                                            OR Lpad( (extract('hours', t0.vc_begintijd_operatie::timestamp))::text, 2, '0' ) || ':' || Lpad( extract('minutes', t0.vc_begintijd_operatie::timestamp)::text, 2, '0' ) = '00:00' )
                                    THEN NULL
                                ELSE ( Greatest( ((extract('hours', (t0.vc_eindtijd_operatie::timestamp - t0.vc_begintijd_operatie::timestamp)) * 60 + extract('minutes', (t0.vc_eindtijd_operatie::timestamp - t0.vc_begintijd_operatie::timestamp))) / 60)::numeric (12, 2), 0 ) ) * 60
                            END  = 0
                    THEN NULL
                ELSE '25. Meer dan 4 uur'
            END AS snijtijd_interval
;

You can try that online here and interactively

I will work on all three tasks mention above.

@d2a-raudenaerde

@d2a-raudenaerde: JSQLParser supports already the Postgresql's Extract function.
However, your used syntax is not in line with my understanding of Postgresql's Grammar:

-- works according https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT
SELECT EXTRACT(HOUR FROM TIMESTAMP '2001-02-16 20:38:40');

-- won't work
SELECT EXTRACT('HOUR' FROM TIMESTAMP '2001-02-16 20:38:40');

SELECT EXTRACT('HOURS' FROM TIMESTAMP '2001-02-16 20:38:40');

When I rewrite your statement in a compliant way by replacing 'hours' with HOUR and 'minutes' with MINUTE, everything works fine:

SELECT  t0.operatienr
        , CASE
                WHEN CASE
                                WHEN ( t0.vc_begintijd_operatie IS NULL
                                            OR Lpad( (EXTRACT(HOUR FROM t0.vc_begintijd_operatie::timestamp))::text, 2, '0' ) || ':' || Lpad( EXTRACT(MINUTE FROM t0.vc_begintijd_operatie::timestamp)::text, 2, '0' ) = '00:00' )
                                    THEN NULL
                                ELSE ( Greatest( ((EXTRACT(HOUR FROM (t0.vc_eindtijd_operatie::timestamp - t0.vc_begintijd_operatie::timestamp)) * 60 + EXTRACT(MINUTE FROM (t0.vc_eindtijd_operatie::timestamp - t0.vc_begintijd_operatie::timestamp))) / 60)::numeric (12, 2), 0 ) ) * 60
                            END  = 0
                    THEN NULL
                ELSE '25. Meer dan 4 uur'
            END AS snijtijd_interval
;

You can test it online here and interactively

So please clarify on the use of 'hours' and 'minutes' -- I am not sure if it is valid at all. Although I will need to improve the parser and get a proper error message out.

Well, we learn something new every day:

[postgres@ryzen ~]$ psql -d test
psql (14.3)
Type "help" for help.

test=# SELECT EXTRACT(HOUR FROM TIMESTAMP '2001-02-16 20:38:40');
 extract 
---------
      20
(1 row)

test=# SELECT EXTRACT('HOUR' FROM TIMESTAMP '2001-02-16 20:38:40');
 extract 
---------
      20
(1 row)

test=# SELECT EXTRACT('HOURS' FROM TIMESTAMP '2001-02-16 20:38:40');
 extract 
---------
      20
(1 row)

test=#

All three variants are parsed by Postgres 14 -- so I am going enable the support for the "inofficial" variants in JSQLParser now.

extended support Postgres' `Extract( field FROM source)` where `field…

@d2a-raudenaerde

Some more findings on the Time Out Issue itself:

With Latest Snapshot, the Parser does NOT HANG -- but runs into the timeout as expected (@d2a-raudenaerde: please confirm, what Version you have used and maybe try out latest snapshot)
Without the Timeout-Detection, that statement will fail only AFTER 40 seconds.

So everything works as expected, the Timeout Detection is effective and the only open issue is to make the Timeout configurable.

Configurable Parser Timeout via Feature

The 2 PRs should solve this issue:

a) the EXTRACT function is parsed, when String Arguments are provided (instead of Keywords0
b) the failing Statements times out as expected and the parser does not hang. Added a test to prove that.
c) the Timeout is configurable now. I am open minded to discuss a sensible Default TimeOut (currently 6 seconds).

@manticore-projects

Wow @manticore-projects thanks for the detailed reply, explanation and PRs! I will try to trigger a timeout, I have a pretty decent laptop and let it run for maybe a minute before; I would expect parsing such a small script should either fails fast or get stuck in an endless loop, so I did not let it run much longer.
And yes, Postgresql is rather forgiving in it's syntax, it seems even the British spelling of ANALYZE -> ANALYSE is supported, althought that is also not in the docs :)

Your are most welcome!
Important: call net.sf.jsqlparser.parser.CCJSqlParserUtil#parse(java.lang.String, java.util.function.Consumer<CCJSqlParser>) or net.sf.jsqlparser.parser.CCJSqlParserUtil#parseStatement instead of the parser itself.

I would expect parsing such a small script should either fails fast or get stuck in an endless loop,

Unfortunately that is not necessarily true. The Parser is "generated" by JavaCC and can get completely lost easily, even with a quite simple statement, when there are a lot of nested expressions (like in your statement).

@manticore-projects

Wow @manticore-projects thanks for the detailed reply, explanation and PRs! I will try to trigger a timeout, I have a pretty decent laptop and let it run for maybe a minute before; I would expect parsing such a small script should either fails fast or get stuck in an endless loop, so I did not let it run much longer. And yes, Postgresql is rather forgiving in it's syntax, it seems even the British spelling of ANALYZE -> ANALYSE is supported, althought that is also not in the docs :)

And yes, Postgresql is rather forgiving in it's syntax, it seems even the British spelling of ANALYZE -> ANALYSE is supported, althought that is also not in the docs :)

Thank you for the hint, I have not been using PostgreSql since a decade or so.
Of course we can support only the "official Grammar" as its documented, but sometimes we need to add only small syntax sugar to make additional things work.

I can verify I get a timeout after about 6 seconds using the CCJSqlParserUtil.parse(expr, null); where the expr is the sql that I used in the opening post.

Thank you. So all as expected and explained.
Official syntax would work already, for the unofficial syntax you will need to wait for the PR to get accepted. I will release a Preview within the next days as an alternative.

Cool, thanks!

extended support Postgres' `Extract( field FROM source)` where `field…

Configurable Parser Timeout via Feature (#1592)

@wumpz

@wumpz : thank you for merging so fast.

Navigation Menu

Repository navigation

Postgresql: sql snippet causes hang in parser #1582

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Postgresql: sql snippet causes hang in parser #1582

Description

Activity

wumpz commented on Jul 7, 2022

wumpz commented on Jul 7, 2022

wumpz commented on Jul 7, 2022

manticore-projects commented on Jul 7, 2022

wumpz commented on Jul 15, 2022

manticore-projects commented on Jul 15, 2022

manticore-projects commented on Jul 15, 2022

wumpz commented on Jul 17, 2022

manticore-projects commented on Jul 18, 2022

manticore-projects commented on Jul 18, 2022

manticore-projects commented on Jul 18, 2022

manticore-projects commented on Jul 18, 2022

manticore-projects commented on Jul 18, 2022

d2a-raudenaerde commented on Jul 19, 2022

manticore-projects commented on Jul 19, 2022

manticore-projects commented on Jul 19, 2022

d2a-raudenaerde commented on Jul 19, 2022

manticore-projects commented on Jul 19, 2022

d2a-raudenaerde commented on Jul 19, 2022

manticore-projects commented on Jul 19, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions