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

Setup SQL server based on pgx frontend/backend protocol #4881

Closed
wants to merge 36 commits into from

Conversation

k-anshul
Copy link
Member

@k-anshul k-anshul commented May 11, 2024

closes #4817

@k-anshul k-anshul self-assigned this May 17, 2024
@k-anshul k-anshul requested a review from begelundmuller May 27, 2024 10:35
Copy link
Contributor

@begelundmuller begelundmuller left a comment

Choose a reason for hiding this comment

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

Two overall comments:

  1. It's worth going over the PR and refactoring or clarifying use of the word "Postgres". There are many places where it's not clear that the code pertains to Postgres wire compatibility, as opposed to connecting to an external Postgres database. In some cases, a docstring would help, in other cases, it's worth considering using a term like psql or pgwire instead of postgres.
  2. The code in runtime/resolvers/postgres_sql.go is pretty hard to navigate, despite the logic being fundamentally pretty simple. I suggest taking a fresh look at the code with a focus on simplifying and documenting the code. There's also a question (raised in one of the below comments) of whether this should even be a resolver, or if it's better implemented in a dedicated package for Postgres wire compatibility.

go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
cli/cmd/admin/start.go Outdated Show resolved Hide resolved
cli/cmd/runtime/start.go Outdated Show resolved Hide resolved
cli/cmd/start/start.go Outdated Show resolved Hide resolved
runtime/server/postgres.go Outdated Show resolved Hide resolved
runtime/resolvers/postgres_sql.go Outdated Show resolved Hide resolved
runtime/resolvers/postgres_sql.go Outdated Show resolved Hide resolved
runtime/server/postgres.go Outdated Show resolved Hide resolved
runtime/resolvers/postgres_sql.go Outdated Show resolved Hide resolved
@k-anshul k-anshul requested a review from begelundmuller June 5, 2024 08:36
admin/server/psql.go Outdated Show resolved Hide resolved
admin/server/psql.go Outdated Show resolved Hide resolved
cli/cmd/admin/start.go Show resolved Hide resolved
runtime/resolver.go Outdated Show resolved Hide resolved
runtime/resolver.go Outdated Show resolved Hide resolved
runtime/resolver.go Outdated Show resolved Hide resolved
runtime/resolvers/sql.go Show resolved Hide resolved
runtime/server/auth/interceptors.go Outdated Show resolved Hide resolved
runtime/server/psql.go Outdated Show resolved Hide resolved
runtime/server/psql/sql.go Outdated Show resolved Hide resolved
Comment on lines +70 to +78
// NOTE :: the duplicate call to runtime.Resolve is made so that we do not keep the results and eventually leak memory if query is never executed.
// This should resolve fast in most cases since results will be cached.
res, err := s.runtime.Resolve(ctx, &runtime.ResolveOptions{
InstanceID: instanceID,
Resolver: "metrics_sql",
ResolverProperties: map[string]any{"sql": query},
Args: map[string]any{"priority": 1},
UserAttributes: auth.GetClaims(ctx).Attributes(),
})
Copy link
Contributor

Choose a reason for hiding this comment

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

We only cache for DuckDB, not Druid or ClickHouse. I think running each psql query twice, even if it may lead into DB internal caches is too excessive. If using s.runtime.Resolve, which returns JSON, why can't that just be passed in here? There is risk of a leaked connection then as far as I can tell?

runtime/server/psql.go Show resolved Hide resolved
runtime/server/psql.go Show resolved Hide resolved
runtime/server/psql.go Show resolved Hide resolved
@k-anshul
Copy link
Member Author

During continuous analysis of psql library we have encountered some critical issues in the library:

  1. The statement cache and portal cache is at server level instead of connection level which is a deviation from standard protocol and also a probable security issue.
  2. The prepared statements never go out of memory even when connection is terminated which can lead to weird memory leaks.
  3. Connections are not closed properly on server close.
  4. The design is very server centric instead of connection centric.

Also we can simplify our code using pgx pkg and using a connection centric design. I am closing this PR and will raise a follow up that builds on some of the changes done here.

@k-anshul k-anshul closed this Jun 24, 2024
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.

Derisk postgres wire protocol
2 participants