PgQuery.parse: Support complex queries with deeply nested ASTs #238
Conversation
lfittl
force-pushed
the
specify-protobuf-max-recursion-depth-setting
branch
from
74d75f4
to
c60464a
Compare
lfittl
force-pushed
the
specify-protobuf-max-recursion-depth-setting
branch
from
c60464a
to
f98eab4
Compare
lib/pg_query/parse.rb
Outdated
| result = if PgQuery::ParseResult.method(:decode).arity == 1 | ||
| PgQuery::ParseResult.decode(result) | ||
| elsif PgQuery::ParseResult.method(:decode).arity == -1 | ||
| PgQuery::ParseResult.decode(result, max_recursion_depth: 100) |
There was a problem hiding this comment.
Back in v1 the nesting limit was 1000, would you be willing to restore it to 1000 again? That would be comfortably enough for my use case.
There was a problem hiding this comment.
Good question - I don't see a problem with that, but it'd be good to test if there are any other constraints (e.g. stack size).
Do you have an example query that is deeper than 100 levels? (most that I could find easily were < 100)
There was a problem hiding this comment.
Good idea. Here is a query that is deeper than 100 levels. Join conditions get nested within each other in the tree, so a query with enough joins will do the trick. I haven't measured its depth but I think this one is at least 200 levels deep.
'SELECT * FROM foo ' + (1..100).to_a.map { |i| "JOIN foo_#{i} ON foo.id = foo_#{i}.foo_id" }.join(' ')There was a problem hiding this comment.
The good news is, the fix for this has been merged in Protobuf v3.20.0, so we can now rely on the recursion depth setting when the newer gem is available (based on the arity check, i.e. its a user choice whether you use the newer protobufs or not).
This PR is now updated, and I'll merge that shortly. I've decided to use a limit of 1000, per your test case (and because I don't see a good reason right now why we can't do that).
lfittl
force-pushed
the
specify-protobuf-max-recursion-depth-setting
branch
from
2775893
to
2ecdc24
Compare
Requires at least google-protobuf v3.20.0, if using older releases this change has no effect. To avoid complex dependency updates, we do not require google-protobuf to be current when you use this library, so you may want to pin it to the newer version if you depend on this behaviour. Fixes #209
lfittl
force-pushed
the
specify-protobuf-max-recursion-depth-setting
branch
from
2ecdc24
to
9b5f121
Compare
|
Thank you @lfittl ! Could we get this out in a new release when you get a chance? |
Depends on protocolbuffers/protobuf#9218, until
that is merged this can only be used with a custom built protobuf gem.
Fixes #209