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: named statements were used when fetchSize was non-zero and prepareThreshold=0 #870

Merged
merged 1 commit into from Jul 24, 2017

Conversation

Projects
None yet
5 participants
@vlsi
Member

vlsi commented Jul 21, 2017

Non-zero fetchSize triggers use of named portals (for subsequent fetch requests),
however named portals does not require to use named statements.

fixes #869

@vlsi vlsi added this to the 42.1.4 milestone Jul 21, 2017

@soon

This comment has been minimized.

soon commented Jul 21, 2017

@vlsi So, the comment above (3b1af9d#diff-0ac94407005dd81cdbe1efa6066de013R1750)

 // nb: if we decide to use a portal (usePortal == true) we must also use a named statement
 // (oneShot == false) as otherwise the portal will be closed under us unexpectedly when
 // the unnamed statement is next reused.

Is useless now? Shouldn't it be also updated?

@codecov-io

This comment has been minimized.

codecov-io commented Jul 21, 2017

Codecov Report

Merging #870 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #870      +/-   ##
============================================
+ Coverage     65.65%   65.67%   +0.02%     
- Complexity     3544     3545       +1     
============================================
  Files           166      166              
  Lines         15253    15253              
  Branches       2474     2474              
============================================
+ Hits          10014    10018       +4     
+ Misses         4060     4058       -2     
+ Partials       1179     1177       -2

@vlsi vlsi force-pushed the vlsi:fetchsize_noserverprepared branch 2 times, most recently from 4131ac4 to f9d03d9 Jul 21, 2017

@vlsi vlsi requested a review from davecramer Jul 21, 2017

@vlsi vlsi force-pushed the vlsi:fetchsize_noserverprepared branch from f9d03d9 to e876837 Jul 21, 2017

fix: named statements were used when fetchSize was non-zero and prepa…
…reThreshold=0

Non-zero fetchSize triggers use of named portals (for subsequent fetch requests),
however named portals does not require to use named statements.

fixes #869

@vlsi vlsi force-pushed the vlsi:fetchsize_noserverprepared branch from e876837 to 01540af Jul 23, 2017

@vlsi

This comment has been minimized.

Member

vlsi commented Jul 24, 2017

@davecramer , @sehrope , what do you think of the change?

Should I add some other test scenario?
I'm not very familiar with portals, thus I wonder what could be the edge cases there.

As far as I can see, interleaved fetch from two ResultSets works.

@davecramer

Unfortunately we will only find out how this works when people start using it extensively. That being said according to postgres documentation it should work fine

@vlsi vlsi merged commit f0deabf into pgjdbc:master Jul 24, 2017

2 checks passed

codecov/project 65.67% (+0.02%) compared to 246b759
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vlsi

This comment has been minimized.

Member

vlsi commented Jul 24, 2017

Apparently, 8.2 does not like "interleaved fetch" from two named portals.
There are two options:

  1. Ignore fetchSize for PG < 8.4, and effectively fetch all the rows in a single fetch
  2. Ignore the test for PG < 8.4, and just hope nobody would ever use that combo (old database + fetchSize + interleaved resultsets)
@davecramer

This comment has been minimized.

Member

davecramer commented Jul 24, 2017

I'm OK with 2 as long as we document it.

@vlsi

This comment has been minimized.

Member

vlsi commented Jul 24, 2017

Unfortunately, 2 does not fit for 42.1.4 as it is not just a bugfix, but it is a breaking change.
Just in case, the documentation for 8.2 has the same wording saying

If successfully created, a named portal object lasts till the end of the current transaction, unless explicitly destroyed

@jorsol

This comment has been minimized.

Contributor

jorsol commented Jul 24, 2017

Option 3:

  • Drop support for PG < 8.4 and avoid lots of headaches 😁
@davecramer

This comment has been minimized.

Member

davecramer commented Jul 24, 2017

I'd prefer not.

vlsi added a commit to vlsi/pgjdbc that referenced this pull request Jul 24, 2017

fix: check how named statements, named portals, and interleaved fetch…
… plays with 8.2

This is a follow-up for pgjdbc#870 and pgjdbc#869 for PostgreSQL < 8.4

vlsi added a commit to vlsi/pgjdbc that referenced this pull request Jul 24, 2017

test: skip ConcurrentStatementFetch for PostgreSQL < 8.4
PostgreSQL 8.2 is known to close named portals unexpectedly, so we just ignore the test.
The following scenario might be affected: old backend version + setFetchSize + interleaved ResultSet processing

This is a follow-up for pgjdbc#870 and pgjdbc#869 for PostgreSQL < 8.4

vlsi added a commit that referenced this pull request Jul 24, 2017

test: skip ConcurrentStatementFetch for PostgreSQL < 8.4 (#884)
PostgreSQL 8.2 is known to close named portals unexpectedly, so we just ignore the test.
The following scenario might be affected: old backend version + setFetchSize + interleaved ResultSet processing

This is a follow-up for #870 and #869 for PostgreSQL < 8.4

davecramer added a commit to davecramer/pgjdbc that referenced this pull request Sep 19, 2017

fix: named statements were used when fetchSize was non-zero and prepa…
…reThreshold=0 (pgjdbc#870)

Non-zero fetchSize triggers use of named portals (for subsequent fetch requests),
however named portals does not require to use named statements.

As per PostgreSQL documentation, named portals are automatically closed as transaction completes, so named portals should play well with transaction-based poolers.

fixes pgjdbc#869

davecramer added a commit to davecramer/pgjdbc that referenced this pull request Sep 19, 2017

test: skip ConcurrentStatementFetch for PostgreSQL < 8.4 (pgjdbc#884)
PostgreSQL 8.2 is known to close named portals unexpectedly, so we just ignore the test.
The following scenario might be affected: old backend version + setFetchSize + interleaved ResultSet processing

This is a follow-up for pgjdbc#870 and pgjdbc#869 for PostgreSQL < 8.4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment