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

PrepareThreshold=0 but prepared statement is stored #869

Closed
f0def opened this Issue Jul 20, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@f0def

f0def commented Jul 20, 2017

Similar to #742

I use PgBouncer with pool_mode=transaction. To work in that mode I had to install prepareThreshold=0 in the connection string as described in the FAQ.

I'm using Spring Framework + Hibernate with his @Transactional annotation. This annotation cause to switch off the autoCommit and make the commit at the end of the transaction.

I execute the request and I know that the query will return a lot of data (thousand or millions of records). I want it to work quickly, so I get the prepared statement and modify the fetchSize, e.g. fetchSize=10000.

If you use these three parameters simultaneously:

  • prepareThreshold=0
  • autoCommit=false
  • fetchSize>0

This leads to error from PgBouncer, he answers with ERROR: prepared statement "S_1" does not exist.

I think, the problem is here org.postgresql.core.v3.QueryExecutorImpl:

boolean usePortal = (flags & QueryExecutor.QUERY_FORWARD_CURSOR) != 0 && !noResults && !noMeta
    && fetchSize > 0 && !describeOnly;
boolean oneShot = (flags & QueryExecutor.QUERY_ONESHOT) != 0 && !usePortal;

usePortal is true because I use fetchSize
oneShot is false because usePortal is true
but I set prepareThreshold to 0 and I don't want to store statement

Here is the test that describe the problem

  @Test
  public void testBatchWithPrepareThreshold0AutoCommitFalseFetchSizeNonZero() throws SQLException {
    assumeBinaryModeRegular();
    Assume.assumeTrue("simple protocol only does not support prepared statement requests",
        preferQueryMode != PreferQueryMode.SIMPLE);

    con.setAutoCommit(false);
    PreparedStatement pstmt = con.prepareStatement("SELECT 1");
    ((PgStatement) pstmt).setPrepareThreshold(0);
    pstmt.setFetchSize(1);
    pstmt.executeQuery();
    pstmt.close();

    pstmt = con.prepareStatement("select count(*) from pg_prepared_statements where statement = 'SELECT 1'");
    ResultSet rs = pstmt.executeQuery();
    assertTrue(rs.next());
    // Failed here:
    assertEquals(0, rs.getInt(1));
    rs.close();
    pstmt.close();
  }

Tested on pgjdbc 42.1.3

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Jul 20, 2017

Member
Member

davecramer commented Jul 20, 2017

@AntonIvanov87

This comment has been minimized.

Show comment
Hide comment
@AntonIvanov87

AntonIvanov87 Jul 21, 2017

@f0def, try add one more parameter preparedStatementCacheQueries=0, does it help?

@f0def, try add one more parameter preparedStatementCacheQueries=0, does it help?

vlsi added a commit to vlsi/pgjdbc that referenced this issue 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 added this to the 42.1.4 milestone Jul 21, 2017

@f0def

This comment has been minimized.

Show comment
Hide comment
@f0def

f0def Jul 21, 2017

@AntonIvanov87 with preparedStatementCacheQueries=0 I can't reproduce issue with PgBouncer. I think it helped, thank you!
Although @vlsi have already made a correction?

f0def commented Jul 21, 2017

@AntonIvanov87 with preparedStatementCacheQueries=0 I can't reproduce issue with PgBouncer. I think it helped, thank you!
Although @vlsi have already made a correction?

vlsi added a commit to vlsi/pgjdbc that referenced this issue 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

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Jul 21, 2017

Member

Re pool_mode=transaction, it could make sense to add server_prepare_mode=transaction kind of setting to pgjdbc, so it would invalidate statement handles on commit.

@davecramer , what do you think?

Member

vlsi commented Jul 21, 2017

Re pool_mode=transaction, it could make sense to add server_prepare_mode=transaction kind of setting to pgjdbc, so it would invalidate statement handles on commit.

@davecramer , what do you think?

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Jul 21, 2017

Member

off the top of my head isn't this exactly the same as the un-named statement? I guess the biggest difference is that we currently transfer in binary once we used a named statement, but of course if we are going to invalidate then is there any point in doing the describe as well ? I think we need to deal with connection pools, but it seems to me the easiest way is simply not to use named statements?

Member

davecramer commented Jul 21, 2017

off the top of my head isn't this exactly the same as the un-named statement? I guess the biggest difference is that we currently transfer in binary once we used a named statement, but of course if we are going to invalidate then is there any point in doing the describe as well ? I think we need to deal with connection pools, but it seems to me the easiest way is simply not to use named statements?

vlsi added a commit to vlsi/pgjdbc that referenced this issue 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 added a commit to vlsi/pgjdbc that referenced this issue 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

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Jul 21, 2017

Member

but it seems to me the easiest way is simply not to use named statements?

Of course the statements have to be invalidated at commit, however it could theoretically gain some performance out of named statements.

Member

vlsi commented Jul 21, 2017

but it seems to me the easiest way is simply not to use named statements?

Of course the statements have to be invalidated at commit, however it could theoretically gain some performance out of named statements.

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Jul 21, 2017

Member

my guess is this is highly dependent on the query in question. At the risk of questionable sanity it might be useful to have a "connection pool" set of config parameters which would allow the user to configure this as they see fit ? I imagine one could construct a benchmark which would show both gains and losses from using named statements, so this one falls in the category of letting the user decide what they want to do.

Member

davecramer commented Jul 21, 2017

my guess is this is highly dependent on the query in question. At the risk of questionable sanity it might be useful to have a "connection pool" set of config parameters which would allow the user to configure this as they see fit ? I imagine one could construct a benchmark which would show both gains and losses from using named statements, so this one falls in the category of letting the user decide what they want to do.

@vlsi

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Jul 21, 2017

Member

I imagine one could construct a benchmark which would show both gains and losses from using named statements

The thing is no current pooler supports named statements. So the users who sit behind poolers just have to use unnamed statements always.
Then we could implement a transaction-wide named statements, so those who use transaction-scoped poolers could leverage named statements.

Of course one can always disable named statements, however it looks like the option to "enable" is missing for pgbouncer-like environment.

Member

vlsi commented Jul 21, 2017

I imagine one could construct a benchmark which would show both gains and losses from using named statements

The thing is no current pooler supports named statements. So the users who sit behind poolers just have to use unnamed statements always.
Then we could implement a transaction-wide named statements, so those who use transaction-scoped poolers could leverage named statements.

Of course one can always disable named statements, however it looks like the option to "enable" is missing for pgbouncer-like environment.

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Jul 21, 2017

Member

Since nobody is really asking for this, I suggest we just allow for disabling named statements.

Member

davecramer commented Jul 21, 2017

Since nobody is really asking for this, I suggest we just allow for disabling named statements.

vlsi added a commit to vlsi/pgjdbc that referenced this issue Jul 23, 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 closed this in #870 Jul 24, 2017

vlsi added a commit that referenced this issue Jul 24, 2017

fix: named statements were used when fetchSize was non-zero and prepa…
…reThreshold=0 (#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 #869

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

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

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

vlsi added a commit to vlsi/pgjdbc that referenced this issue 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 #870 and #869 for PostgreSQL < 8.4

vlsi added a commit that referenced this issue 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 issue Sep 19, 2017

fix: named statements were used when fetchSize was non-zero and prepa…
…reThreshold=0 (#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 #869

davecramer added a commit to davecramer/pgjdbc that referenced this issue Sep 19, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment