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

Open
wants to merge 23 commits into
base: main
Choose a base branch
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
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
@@ -72,6 +74,7 @@ require (
github.com/rs/cors v1.9.0
github.com/santhosh-tekuri/jsonschema/v5 v5.2.0
github.com/sashabaranov/go-openai v1.19.3
github.com/shopspring/decimal v1.4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use the math/big package directly and avoid this dependency?

Copy link
Member Author

@k-anshul k-anshul May 28, 2024

Choose a reason for hiding this comment

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

My understanding is that we need shopspring/decimal to easily encode Numeric : [https://github.com/jackc/pgx/issues/56

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't go in depth here, but shopstring/decimal is not a dependency of the pgx project (based on its go.mod file), so they must internally have found a different way around that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I posted the wrong link.pgx recommends using shopspring/decimal : https://github.com/jackc/pgx/wiki/Numeric-and-decimal-support
The default solution can only be used for float64 or string.

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