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

pass client when getting PortalStore #139

Closed
wants to merge 2 commits into from

Conversation

poudels14
Copy link
Contributor

@poudels14 poudels14 commented Dec 14, 2023

Postgres doc says prepared statements and portals exist only within a session. So, when getting a portalstore, pass in a client so that store corresponding to that client can be returned. This makes it possible to have different portalstore for different clients. Since default names are used when creating a portal, clients sharing a portalstore would lead to leaks

@poudels14 poudels14 marked this pull request as draft December 14, 2023 23:56
  - this makes it possible to have different portalstore for
    different clients. Since default names are used when creating
    a portal, clients sharing a portalstore would lead to leaks
  - portal can have associated state that gets dropped when portal
    is closed like transaction, query plan, etc. so, add a field to
    hold the state
@sunng87
Copy link
Owner

sunng87 commented Dec 15, 2023

@poudels14 good catch! this issue was introduced by my previous refactoring. By the way, do you think it's a good idea to make portalstore part of Client? So it's automatically recycled when client is destroyed.

@sunng87
Copy link
Owner

sunng87 commented Dec 18, 2023

I provided an alternative approach in #140

@sunng87
Copy link
Owner

sunng87 commented Dec 23, 2023

hi @poudels14, I'm going to merge #140 as a solution for this issue. It should require minimal code change for existed users. And we can further enhance it if there are new issues found with that design.

@poudels14
Copy link
Contributor Author

Sounds good! thanks for adding that

@poudels14 poudels14 closed this Dec 23, 2023
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.

None yet

2 participants