Skip to content

chore: port browser-proxy to pg-gateway Web API #100

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

Merged
merged 5 commits into from
Sep 17, 2024

Conversation

jgoux
Copy link
Contributor

@jgoux jgoux commented Sep 13, 2024

In this PR I'm porting browser-proxy to the upcoming version of pg-gateway rewritten with Web API support.

I'll hold until this PR is merged and published so I can update the dependency of pg-gateway here.

Copy link

vercel bot commented Sep 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
postgres-new ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 17, 2024 0:10am

SNICallback: (servername, callback) => {
debug('SNICallback', servername)
if (isValidServername(servername)) {
debug('SNICallback', 'valid')
callback(null, tls.createSecureContext(tlsOptions))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the end we don't need that, the default secureContext is used.

const tcpSocket = tcpConnections.get(databaseId)
tcpSocket?.write(data)
const tcpConnection = tcpConnections.get(databaseId)
tcpConnection?.streamWriter?.write(data)
Copy link
Contributor Author

@jgoux jgoux Sep 13, 2024

Choose a reason for hiding this comment

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

here is why I needed to add the streamWriter property in pg-gateway, because getting a new writer from duplex here wouldn't work as the stream.writable is locked by processData.

Comment on lines -93 to +85
tls: tlsOptions,
const connection = await fromNodeSocket(socket, {
tls: getTls,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now the TLS certificates should be correctly refreshed after some time. 😅

@jgoux jgoux marked this pull request as ready for review September 13, 2024 10:30
await db.sql`rollback;`.catch()
// we clean the session state, see: https://www.pgbouncer.org/faq.html#how-to-use-prepared-statements-with-session-pooling
// we do this to avoid having old prepared statements in the session
await db.sql`discard all;`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added after working on making pg_dump works. Running pg_dump twice showed that we kept old prepared statements in the session for the next connected client, which ended up erroring as the next pg_dump creates the same prepare statement.

Copy link
Collaborator

@gregnr gregnr Sep 13, 2024

Choose a reason for hiding this comment

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

I suppose we should mention rollback + discard all in the pg-gateway pglite examples too? I imagine this would apply to any pg-gateway + PGlite stack.

if (parameters.client_ip === '') {
setConnectedClientIp(null)
// we ensure we're not in a transaction block first
await db.sql`rollback;`.catch()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

discard all can't be run in a transaction block and it seems like after a pg_dump we still have a transaction opened somehow. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

How bizarre!

Comment on lines +38 to +39
// cache the TLS certificate for 1 week
const cache = new ExpiryMap(1000 * 60 * 60 * 24 * 7)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if this server restarted 1 day before renewal? I suppose the old cert is still good for another week, so this should be safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to check but I think we renew the certificate monthly while the validity is 3 months. So in any cases with 2 week refresh we should be safe.

if (parameters.client_ip === '') {
setConnectedClientIp(null)
// we ensure we're not in a transaction block first
await db.sql`rollback;`.catch()
Copy link
Collaborator

Choose a reason for hiding this comment

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

How bizarre!

await db.sql`rollback;`.catch()
// we clean the session state, see: https://www.pgbouncer.org/faq.html#how-to-use-prepared-statements-with-session-pooling
// we do this to avoid having old prepared statements in the session
await db.sql`discard all;`
Copy link
Collaborator

@gregnr gregnr Sep 13, 2024

Choose a reason for hiding this comment

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

I suppose we should mention rollback + discard all in the pg-gateway pglite examples too? I imagine this would apply to any pg-gateway + PGlite stack.

@jgoux jgoux merged commit edb1958 into feat/db-sharing Sep 17, 2024
2 checks passed
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