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

Invalidate cache on Rows.NextResultSet call #858

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

Conversation

kamilkoduo
Copy link

When reading multiple result sets from sqlx.Rows using StructScan, some information obtained usCCCing costly reflect operations is cached for performance optimisation.
However, different result sets in a single sqlx.Rows object might have different structures, so cache should be invalidated and rebuild after each NextResultSet call. Now this method is not overwritten, so old cache interferes with new structures after first result set scan.

Relates #857

When reading multiple result sets from sqlx.Rows using StructScan,
some information obtained usCCCing costly reflect operations is
cached for performance optimisation.
However, different result sets in a single sqlx.Rows object might
have different structures, so cache should be invalidated and rebuild
after each NextResultSet call. Now this method is not overwritten,
so old cache interferes with new structures after first result set scan.

Relates jmoiron#857
@kamilkoduo kamilkoduo force-pushed the feature-857-drop-row-cache-on-next-result-set branch from 83d76fb to a817d1d Compare April 12, 2023 14:32
@cornelk
Copy link

cornelk commented Jun 30, 2023

Thanks for the fix! There is another PR open for this issue: #747 which includes a unit test.

@jmoiron any chance to get this or the other fix merged?

@batazor
Copy link

batazor commented Aug 28, 2023

@jmoiron Yeah, that's a real problem, it would be cool if you could merge one of these solutions

@tazimmerman
Copy link

@jmoiron Are you open to additional maintainers? There are a handful of really useful PRs that look straightforward and I'd be happy to help get them merged. This is a fantastic project and given its widespread use, keeping future changes in the original repo would be ideal.

@dlsniper
Copy link
Collaborator

dlsniper commented Feb 1, 2024

Hello, @ardan-bkennedy, and I recently stepped in to help maintain this project.
We are currently evaluating all the opened PRs.
We'll take a look at this shortly and see if this, #747 or another, will be a good solution for this problem.

@dlsniper dlsniper added the needs testing The PR needs more testing before being accepted label Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs testing The PR needs more testing before being accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants