Postgresql: sql snippet causes hang in parser

[d2a-raudenaerde]

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

[wumpz]

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

[wumpz]

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

[wumpz]

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

[wumpz]

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

[manticore-projects]

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.

[manticore-projects]

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

[wumpz]

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.

[manticore-projects]

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

1. the OP's statement actually fails because of extract('hours' from t0.vc_begintijd_operatie) is not supported
2. the Parser does not quit, when the timeout is reached
3. 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.

[manticore-projects]

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

[manticore-projects]

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.