Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support parsing complex queries with deeply nested ASTs #17

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

seanlinsley
Copy link
Member

This matches the same behavior in the Ruby gem: pganalyze/pg_query#238

Upstream changes were needed in the prost crate that haven't been merged yet: tokio-rs/prost#785

Without further changes, the test suite would run into a stack overflow. There are some ways around this:

  • Create a thread with a larger stack size. This is what the PR currently does b/c it didn't require adding a dependency. But its performance is limited by it creating a new thread for every call. There is also safety concerns with passing unsafe data structures to a different thread.
  • Use the heap to dynamically grow the stack size https://crates.io/crates/stacker. This is a fairly popular crate, but I'm wary of the maybe_grow performance. If someone hasn't put together benchmarks we may want to use grow instead.
  • https://crates.io/crates/decurse
  • https://crates.io/crates/tailcall

@seanlinsley seanlinsley requested review from lfittl and msepga January 11, 2023 15:50
@seanlinsley
Copy link
Member Author

Since any solution to the stack overflow issue would involve pulling in dependencies that most users won't need, I decided to document this issue in the README so downstream applications can increase the stack size as needed for their application.

Copy link
Member

@lfittl lfittl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 FWIW, this looks good to me - depending on when the upstream change to prost lands it might be nice to hold off until that's actually merged, but apart from that it looks good to ship.

@seanlinsley
Copy link
Member Author

@lfittl thoughts on best way to install the protobuf compiler on Windows, for CI?

@lfittl
Copy link
Member

lfittl commented Dec 2, 2024

@lfittl thoughts on best way to install the protobuf compiler on Windows, for CI?

Probably install the binary provided on the releases page: https://github.com/protocolbuffers/protobuf/releases/tag/v29.0 (not sure if there is an alias for a current version, but I guess we could pin it)

To help me understand the change better, is there a reason we need to add this here, vs didn't need it on main?

@seanlinsley
Copy link
Member Author

This PR is built on top of a more recent version of the prost crate, which now relies that your system have the protobuf compiler installed separately.

@lfittl
Copy link
Member

lfittl commented Dec 2, 2024

This PR is built on top of a more recent version of the prost crate, which now relies that your system have the protobuf compiler installed separately.

Makes sense!

I think maybe you can use this GH action to install the compiler in a OS-agnostic way: https://github.com/marketplace/actions/setup-protoc (we already have this forked to https://github.com/pganalyze/setup-protoc for security reasons, so you can probably just go ahead and use that fork?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants