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 PGCopyOutputStream out of order writes #1854

Merged

Conversation

sehrope
Copy link
Member

@sehrope sehrope commented Aug 12, 2020

This PR fixes #1777

It's just a fix for the out of order writes and an additional test for the corrected functionality. It does not deprecate the existing copy classes or add new replacements. We can follow up with that in a subsequent release.

@codecov-commenter
Copy link

Codecov Report

Merging #1854 into master will increase coverage by 0.02%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master    #1854      +/-   ##
============================================
+ Coverage     69.26%   69.29%   +0.02%     
- Complexity     4206     4208       +2     
============================================
  Files           197      197              
  Lines         17981    17983       +2     
  Branches       2914     2914              
============================================
+ Hits          12455    12461       +6     
+ Misses         4181     4175       -6     
- Partials       1345     1347       +2     

@sehrope
Copy link
Member Author

sehrope commented Aug 13, 2020

@davecramer This is ready too. Let's get this in for the upcoming release as it's a minor standalone fix.

@vlsi vlsi force-pushed the master branch 2 times, most recently from 4c39f96 to 866c6a9 Compare August 21, 2020 18:17
@vlsi vlsi changed the base branch from master to release/42.2 August 28, 2020 10:19
@codecov-io
Copy link

Codecov Report

Merging #1854 into release/42.2 will increase coverage by 0.07%.
The diff coverage is 60.00%.

@@                Coverage Diff                 @@
##             release/42.2    #1854      +/-   ##
==================================================
+ Coverage           69.21%   69.29%   +0.07%     
- Complexity           4193     4208      +15     
==================================================
  Files                 197      197              
  Lines               17996    17983      -13     
  Branches             2916     2914       -2     
==================================================
+ Hits                12456    12461       +5     
+ Misses               4188     4175      -13     
+ Partials             1352     1347       -5     

@davecramer
Copy link
Member

anyone know why this just showed up again? @sehrope we should get this in. can you rebase and I will take care of it

Adds a helper method to execute a query and reads a single text column as a string.
Adds a failing test that writes to a PGCopyOutputStream using both the write(byte[])
and write(ByteStreamWriter) APIs to catch internal buffering breaking the order.
…StreamWriter

Fixes out of order writes when interleaving writes using buffered write(byte[]) and
writeToCopy(ByteStreamWriter) APIs.
@sehrope
Copy link
Member Author

sehrope commented Nov 9, 2020

@davecramer No idea. I tried rebasing it locally to bring it up to date but am getting a bunch of unrelated conflicts. Was part of history on master rewritten?

I cherry-picked the changes on to a v2 branch. Once it get's through travis (https://travis-ci.org/github/sehrope/pgjdbc/builds/742441641) I'll force push this original PR so it get's picked up. The individual commits are all localized so shouldn't be any true conflicts.

@davecramer
Copy link
Member

History was not rewritten, however we did move some tags. Not sure why there are conflicts

@sehrope
Copy link
Member Author

sehrope commented Nov 9, 2020

No I think something in master was definitely rewritten. I ran git log upstream/master..HEAD on this branch to see which commits are missing from master. Besides the actual changes for this PR there are three commits.

Not sure what happened but the same commits were published to the mailing list twice as arriving on master (linked below).


Older commits in this PR but not in master

Fix header on existing Parameter Status doc page, and include entry in the index html page. (#1845)

commit 0fe766b
Author: draderaws 41007433+draderaws@users.noreply.github.com
Date: Sun Aug 9 06:23:09 2020 -0400
https://www.postgresql.org/message-id/pgjdbc/pgjdbc/push/refs/heads/master/3802b4-0fe766@github.com

fix: preserve unquoted unicode whitespace in array literals (#1266)

commit 8ad48a6
Author: Brett Okken brett.okken.os@gmail.com
Date: Mon Aug 10 14:59:49 2020 -0500
https://www.postgresql.org/message-id/pgjdbc/pgjdbc/push/refs/heads/master/4fef49-8ad48a@github.com

remove postgresql-jre6 and postgresql-jre7 projects (#1848)

commit c80add9 (fix-copy-out-stream, deprecate-legacy-pg-streams)
Author: Dave Cramer davecramer@gmail.com
Date: Tue Aug 11 14:09:29 2020 -0400
https://www.postgresql.org/message-id/pgjdbc/pgjdbc/push/refs/heads/master/8ad48a-c80add@github.com


Same commits (by substance) in master:

Fix header on existing Parameter Status doc page, and include entry in the index html page. (#1845)

commit 9962307
Author: draderaws 41007433+draderaws@users.noreply.github.com
Date: Sun Aug 9 06:23:09 2020 -0400
https://www.postgresql.org/message-id/pgjdbc/pgjdbc/push/refs/heads/master/fe09f8-d722a9@github.com

fix: preserve unquoted unicode whitespace in array literals (#1266)

commit e8923c7
Author: Brett Okken brett.okken.os@gmail.com
Date: Mon Aug 10 14:59:49 2020 -0500
https://www.postgresql.org/message-id/pgjdbc/pgjdbc/push/refs/heads/master/fe09f8-d722a9@github.com (same batch as previous)

remove postgresql-jre6 and postgresql-jre7 projects (#1848)

commit 866c6a9
Author: Vladimir Sitnikov sitnikov.vladimir@gmail.com
Date: Fri Aug 21 20:55:20 2020 +0300
https://www.postgresql.org/message-id/pgjdbc/pgjdbc/push/refs/heads/master/4c39f9-866c6a@github.com

@sehrope
Copy link
Member Author

sehrope commented Nov 9, 2020

@davecramer Looks like the cherry-picked branch passed Travis. Let me know if you want me to force it atop this or submit it as a separate PR to keep these commits in place (to track down the history rewrite).

@davecramer
Copy link
Member

just force on top of it. Lets move forward

@sehrope sehrope force-pushed the fix-pgcopyoutput-stream-out-of-order branch from a17a24b to 8395a50 Compare November 9, 2020 20:01
@sehrope sehrope changed the base branch from release/42.2 to master November 9, 2020 20:03
@sehrope sehrope closed this Nov 9, 2020
@sehrope sehrope reopened this Nov 9, 2020
@davecramer
Copy link
Member

so now that I think about it we did reorder some commits when we released a while ago. So I presume this causes a rewrite of history?

@sehrope
Copy link
Member Author

sehrope commented Nov 10, 2020

Yes that must have been it. That's a big no-no and the master branch should be locked to prevent force-pushes. Can you do that from the GitHub console to prevent this in the future?

It's fine to force-push PR and even pre-release branches if it's before they're published. But rewriting master or released branches causes all kinds of issues.

@davecramer
Copy link
Member

There were extenuating circumstances..

@davecramer davecramer merged commit dbb30af into pgjdbc:master Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants