Skip to content

Commit

Permalink
Always Describe a query in a batch that returns generated keys
Browse files Browse the repository at this point in the history
This is a workaround for issue #267, where we unprepare a statement
in a batch when the parameter types change but we expect to still
know the field types in the statement we just unprepared.

It's much more complicated than that, see
#267 for the gory details.
  • Loading branch information
ringerc committed Mar 16, 2015
1 parent 4d2b046 commit a6bd36f
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 11 deletions.
7 changes: 7 additions & 0 deletions org/postgresql/core/QueryExecutor.java
Expand Up @@ -88,6 +88,13 @@ public interface QueryExecutor {
* both the ResultSet and associated update count from the command status.
*/
static int QUERY_BOTH_ROWS_AND_STATUS = 64;

/**
* Force this query to be described at each execution. This is done in
* pipelined batches where we might need to detect mismatched result
* types.
*/
static int QUERY_FORCE_DESCRIBE_PORTAL = 128;

This comment has been minimized.

Copy link
@vlsi

vlsi Oct 2, 2015

Member

This QUERY_FORCE_DESCRIBE_PORTAL = 128; happens to collide with static int QUERY_DISALLOW_BATCHING = 128;
Was that intentional?
I'm facing degradation of executeBatch, and it looks like this 128 forces batch to be executed one-by-one

This comment has been minimized.

Copy link
@ringerc

ringerc Oct 7, 2015

Author Member

That was a very definite error on my part. Good find.


/**
* Flag to disable batch execution when we expect results (generated keys)
Expand Down
29 changes: 18 additions & 11 deletions org/postgresql/core/v3/QueryExecutorImpl.java
Expand Up @@ -1624,6 +1624,7 @@ private void sendOneQuery(SimpleQuery query, SimpleParameterList params, int max
boolean usePortal = (flags & QueryExecutor.QUERY_FORWARD_CURSOR) != 0 && !noResults && !noMeta && fetchSize > 0 && !describeOnly;
boolean oneShot = (flags & QueryExecutor.QUERY_ONESHOT) != 0 && !usePortal;
boolean noBinaryTransfer = (flags & QUERY_NO_BINARY_TRANSFER) != 0;
boolean forceDescribePortal = (flags & QUERY_FORCE_DESCRIBE_PORTAL) != 0;

// Work out how many rows to fetch in this pass.

Expand Down Expand Up @@ -1692,18 +1693,24 @@ else if (maxRows != 0 && fetchSize > maxRows)
/*
* don't send describe if we already have cached the row description
* from previous executions
*
* XXX Clearing the fields / unpreparing the query is incorrect, see
* bug #267. We might clear the cached fields in a later execution
* of this query if the bind parameter types change, but we're
* assuming here that they'll still be valid when we come to process
* the results of this query, so we don't send a new describe here.
* We re-describe after the fields are cleared, but the result of
* that gets processed after processing the results from earlier
* executions that we didn't describe because we didn't think we had
* to.
*
* XXX Clearing the fields / unpreparing the query (in sendParse) is
* incorrect, see bug #267. We might clear the cached fields in a
* later execution of this query if the bind parameter types change,
* but we're assuming here that they'll still be valid when we come
* to process the results of this query, so we don't send a new
* describe here. We re-describe after the fields are cleared, but
* the result of that gets processed after processing the results
* from earlier executions that we didn't describe because we didn't
* think we had to.
*
* To work around this, force a Describe at each execution in
* batches where this can be a problem. It won't cause more round
* trips so the performance impact is low, and it'll ensure that the
* field information available when we decoded the results. This
* is undeniably a hack, but there aren't many good alternatives.
*/
if (query.getFields() == null) {
if (query.getFields() == null || forceDescribePortal) {
sendDescribePortal(query, portal);
}
}
Expand Down
6 changes: 6 additions & 0 deletions org/postgresql/jdbc2/AbstractJdbc2Statement.java
Expand Up @@ -2905,6 +2905,12 @@ public int[] executeBatch() throws SQLException
// we'll be able to queue up before we risk a deadlock.
// (see v3.QueryExecutorImpl's MAX_BUFFERED_RECV_BYTES)
preDescribe = wantsGeneratedKeysAlways && !queries[0].isStatementDescribed();
/*
* It's also necessary to force a Describe on the first execution of the
* new statement, even though we already described it, to work around
* bug #267.
*/
flags |= QueryExecutor.QUERY_FORCE_DESCRIBE_PORTAL;
}

if (connection.getAutoCommit())
Expand Down

7 comments on commit a6bd36f

@davecramer
Copy link
Member

Choose a reason for hiding this comment

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

I don't know the history on this, although I seem to recall that it was intentional, but we are going to have to investigate

@vlsi
Copy link
Member

@vlsi vlsi commented on a6bd36f Oct 5, 2015

Choose a reason for hiding this comment

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

For "intentional" usages I would expect int QUERY_FORCE_DESCRIBE_PORTAL = QUERY_DISALLOW_BATCHING kind of declaration.
However, "disallow_batching" is a rather unexpected strategy of "batch API" support.

@davecramer
Copy link
Member

Choose a reason for hiding this comment

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

While I agree the assumption is logical, We can't rely on that

@vlsi
Copy link
Member

@vlsi vlsi commented on a6bd36f Oct 5, 2015

Choose a reason for hiding this comment

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

The testcase for #267 is in the test set (https://gist.github.com/ringerc/25d36e9f0044077c664b), and it passes with QUERY_FORCE_DESCRIBE_PORTAL having its own value (see #380)

@davecramer
Copy link
Member

Choose a reason for hiding this comment

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

OK, lets see if @ringerc responds to this? He may remember better than I

@ringerc
Copy link
Member Author

@ringerc ringerc commented on a6bd36f Oct 7, 2015

Choose a reason for hiding this comment

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

I'm confident this was an error on my part in the original patch. Thanks @vlsi for identifying it.

The only thing that surprises me is that, given this oversight, it's been working happily for so long. I validated the original batching in wireshark and with debug message tracing, and I know at least one site that's been using it.

@ringerc
Copy link
Member Author

@ringerc ringerc commented on a6bd36f Oct 7, 2015

Choose a reason for hiding this comment

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

Fixed in #380 by @vlsi, no further action required here

Please sign in to comment.