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

Fix effectful copyIn (Fixes #1512) #1567

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

oyvindberg
Copy link
Contributor

The challenge faced here was to capture the number of affected rows when ending a COPY.

To safely end a copy, it needs to be run in the finalizer of a bracket. We cannot retrieve values from a finalizer, so there was some logic in place to call PFCI.endCopy in two places and use isActive to determine if it should be called the second time. isActive seems to not be an !isCopyEnded, but rather "is connection reserved for this Copy operation?".

The fix is to replace the double closing with a reference

The challenge faced here was to capture the number of affected rows when ending a `COPY`.
To safely end a copy, it needs to be run in the finalizer of a `bracket`. We cannot retrieve values from a finalizer, so there was some logic in place to call `PFCI.endCopy` in two places and use `isActive` to determine if it should be called the second time. `isActive` seems to not be an `!isClosed`, but rather "is connection reserved for this Copy operation?".

The fix is to replace the double closing with a reference
@oyvindberg
Copy link
Contributor Author

@jatcwang When you have time :)

@jatcwang jatcwang self-requested a review October 12, 2021 10:13
@jatcwang
Copy link
Collaborator

@oyvindberg Please bear with me. Busy with real world:tm: atm :sweat_smile:

@oyvindberg
Copy link
Contributor Author

no worries @jatcwang , I know what that feels like :)

Copy link
Collaborator

@jatcwang jatcwang left a comment

Choose a reason for hiding this comment

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

LGTM Thanks for the fix @oyvindberg!

@jatcwang jatcwang merged commit 650c8b9 into typelevel:main Dec 10, 2021
@oyvindberg oyvindberg deleted the fix-effectful-copy-in branch December 10, 2021 16:22
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