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

feat: wrapper connection implementation #400

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ShubhranshuSanjeev
Copy link
Collaborator

Problem

Describe the problem you are trying to solve here

Solution

Provide a brief summary of your solution so that reviewers can understand your code

Environment variable changes

What ENVs need to be added or changed

Pre-deployment activity

Things needed to be done before deploying this change (if any)

Post-deployment activity

Things needed to be done after deploying this change (if any)

API changes

Endpoint Method Request body Response Body
API GET/POST, etc request response

Possible Issues in the future

Describe any possible issues that could occur because of this change

@ShubhranshuSanjeev ShubhranshuSanjeev requested a review from a team as a code owner January 31, 2025 14:12
@ShreyBana
Copy link
Collaborator

ShreyBana commented Jan 31, 2025

A couple of things.

  1. What is happening to the prepared statement objects in postgres? Are we creating them everytime? And if so are they being GC'd?
  2. How are we planning to handle schema changes during the execution of flow? For instance, during workspace setup we want to first create the workspace schema, ran from inside the public schema & then switch to the superposition schema for inserting into the workspaces table, all in one transaction.
  3. Can we please add the debug logs for each query based on an env as well? That would be a very useful thing to have.
  4. Can you add tests that verify that things are working as intended?

use diesel::PgConnection;
use diesel::RunQueryDsl;

pub struct SuperTransactionManager;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about TransactionManagerImpl?

}
}

pub struct SuperConnection {
Copy link
Collaborator

Choose a reason for hiding this comment

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

ConnectionImpl?

}
};

let schema_name = req.extensions().get::<SchemaName>().cloned().unwrap().0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might have to update tenant.rs to insert superposition schema in case we want to add APIs for just querying things from those tables. I don't think we have that today.

@ShreyBana
Copy link
Collaborator

I am seeing that we don't need the .schema(..) DSL w/ this, we might just be able to move back to standard diesel!

@ShreyBana
Copy link
Collaborator

...
4. Can you add tests that verify that things are working as intended?

I can do the tests part if something is taking time.

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