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

perf: avoid string allocation for oid/rows parsing in command tag #1232

Merged
merged 14 commits into from Jul 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
108 changes: 108 additions & 0 deletions pgjdbc/src/main/java/org/postgresql/core/CommandCompleteParser.java
@@ -0,0 +1,108 @@
/*
* Copyright (c) 2018, PostgreSQL Global Development Group
* See the LICENSE file in the project root for more information.
*/

package org.postgresql.core;

import org.postgresql.util.GT;
import org.postgresql.util.PSQLException;
import org.postgresql.util.PSQLState;

/**
* Parses {@code oid} and {@code rows} from a {@code CommandComplete (B)} message (end of Execute).
*/
public final class CommandCompleteParser {
private long oid;
private long rows;

public CommandCompleteParser() {
}

public long getOid() {
return oid;
}

public long getRows() {
return rows;
}

void set(long oid, long rows) {
this.oid = oid;
this.rows = rows;
}

/**
* Parses {@code CommandComplete (B)} message.
* Status is in the format of "COMMAND OID ROWS" where both 'OID' and 'ROWS' are optional
* and COMMAND can have spaces within it, like CREATE TABLE.
*
* @param status COMMAND OID ROWS message
* @throws PSQLException in case the status cannot be parsed
*/
public void parse(String status) throws PSQLException {
// Assumption: command neither starts nor ends with a digit
if (!Parser.isDigitAt(status, status.length() - 1)) {
set(0, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Should return here, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

right

Copy link
Member

Choose a reason for hiding this comment

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

apparently there isn't a test case for this :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

It is not really valid, but something like CREATE 2ABLE would have failed previously.

return;
}

// Scan backwards, while searching for a maximum of two number groups
// COMMAND OID ROWS
// COMMAND ROWS
long oid = 0;
long rows = 0;
try {
int lastSpace = status.lastIndexOf(' ');
// Status ends with a digit => it is ROWS
if (Parser.isDigitAt(status, lastSpace + 1)) {
rows = Parser.parseLong(status, lastSpace + 1, status.length());

if (Parser.isDigitAt(status, lastSpace - 1)) {
int penultimateSpace = status.lastIndexOf(' ', lastSpace - 1);
if (Parser.isDigitAt(status, penultimateSpace + 1)) {
oid = Parser.parseLong(status, penultimateSpace + 1, lastSpace);
}
}
}
} catch (NumberFormatException e) {
// This should only occur if the oid or rows are out of 0..Long.MAX_VALUE range
throw new PSQLException(
GT.tr("Unable to parse the count in command completion tag: {0}.", status),
PSQLState.CONNECTION_FAILURE, e);
}
set(oid, rows);
}

@Override
public String toString() {
return "CommandStatus{"
+ "oid=" + oid
+ ", rows=" + rows
+ '}';
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}

CommandCompleteParser that = (CommandCompleteParser) o;

if (oid != that.oid) {
return false;
}
return rows == that.rows;
}

@Override
public int hashCode() {
int result = (int) (oid ^ (oid >>> 32));
result = 31 * result + (int) (rows ^ (rows >>> 32));
return result;
}
}
45 changes: 45 additions & 0 deletions pgjdbc/src/main/java/org/postgresql/core/Parser.java
Expand Up @@ -691,6 +691,26 @@ public static boolean parseValuesKeyword(final char[] query, int offset) {
&& (query[offset + 5] | 32) == 's';
}

/**
* Faster version of {@link Long#parseLong(String)} when parsing a substring is required
*
* @param s string to parse
* @param beginIndex begin index
* @param endIndex end index
* @return long value
*/
public static long parseLong(String s, int beginIndex, int endIndex) {
// Fallback to default implementation in case the string is long
if (endIndex - beginIndex > 16) {
return Long.parseLong(s.substring(beginIndex, endIndex));
}
long res = digitAt(s, beginIndex);
for (beginIndex++; beginIndex < endIndex; beginIndex++) {
res = res * 10 + digitAt(s, beginIndex);
}
return res;
}

/**
* Parse string to check presence of WITH keyword regardless of case.
*
Expand Down Expand Up @@ -725,6 +745,31 @@ public static boolean parseAsKeyword(final char[] query, int offset) {
&& (query[offset + 1] | 32) == 's';
}

/**
* Returns true if a given string {@code s} has digit at position {@code pos}.
* @param s input string
* @param pos position (0-based)
* @return true if input string s has digit at position pos
*/
public static boolean isDigitAt(String s, int pos) {
return pos > 0 && pos < s.length() && Character.isDigit(s.charAt(pos));
}

/**
* Converts digit at position {@code pos} in string {@code s} to integer or throws.
* @param s input string
* @param pos position (0-based)
* @return integer value of a digit at position pos
* @throws NumberFormatException if character at position pos is not an integer
*/
public static int digitAt(String s, int pos) {
int c = s.charAt(pos) - '0';
if (c < 0 || c > 9) {
throw new NumberFormatException("Input string: \"" + s + "\", position: " + pos);
}
return c;
}

/**
* @param c character
* @return true if the character is a whitespace character as defined in the backend's parser
Expand Down
42 changes: 14 additions & 28 deletions pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
Expand Up @@ -10,6 +10,7 @@
import org.postgresql.copy.CopyIn;
import org.postgresql.copy.CopyOperation;
import org.postgresql.copy.CopyOut;
import org.postgresql.core.CommandCompleteParser;
import org.postgresql.core.Encoding;
import org.postgresql.core.EncodingPredictor;
import org.postgresql.core.Field;
Expand Down Expand Up @@ -63,17 +64,13 @@
import java.util.TimeZone;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Matcher;
import java.util.regex.Pattern;


/**
* QueryExecutor implementation for the V3 protocol.
*/
public class QueryExecutorImpl extends QueryExecutorBase {

private static final Logger LOGGER = Logger.getLogger(QueryExecutorImpl.class.getName());
private static final Pattern COMMAND_COMPLETE_PATTERN = Pattern.compile("^([A-Za-z]++)(?: (\\d++))?+(?: (\\d++))?+$");

/**
* TimeZone of the current connection (TimeZone backend parameter).
Expand Down Expand Up @@ -122,6 +119,11 @@ public class QueryExecutorImpl extends QueryExecutorBase {

private final ReplicationProtocol replicationProtocol;

/**
* {@code CommandComplete(B)} messages are quite common, so we reuse instance to parse those
*/
private final CommandCompleteParser commandCompleteParser = new CommandCompleteParser();

public QueryExecutorImpl(PGStream pgStream, String user, String database,
int cancelSignalTimeout, Properties info) throws SQLException, IOException {
super(pgStream, user, database, cancelSignalTimeout, info);
Expand Down Expand Up @@ -2469,31 +2471,15 @@ private String receiveCommandStatus() throws IOException {
}

private void interpretCommandStatus(String status, ResultHandler handler) {
long oid = 0;
long count = 0;
Matcher matcher = COMMAND_COMPLETE_PATTERN.matcher(status);
if (matcher.matches()) {
// String command = matcher.group(1);
String group2 = matcher.group(2);
String group3 = matcher.group(3);
try {
if (group3 != null) {
// COMMAND OID ROWS
oid = Long.parseLong(group2);
count = Long.parseLong(group3);
} else if (group2 != null) {
// COMMAND ROWS
count = Long.parseLong(group2);
}
} catch (NumberFormatException e) {
// As we're performing a regex validation prior to parsing, this should only
// occurr if the oid or count are out of range.
handler.handleError(new PSQLException(
GT.tr("Unable to parse the count in command completion tag: {0}.", status),
PSQLState.CONNECTION_FAILURE));
return;
}
try {
commandCompleteParser.parse(status);
} catch (SQLException e) {
handler.handleError(e);
return;
}
long oid = commandCompleteParser.getOid();
long count = commandCompleteParser.getRows();

int countAsInt = 0;
if (count > Integer.MAX_VALUE) {
// If command status indicates that we've modified more than Integer.MAX_VALUE rows
Expand Down
@@ -0,0 +1,49 @@
/*
* Copyright (c) 2018, PostgreSQL Global Development Group
* See the LICENSE file in the project root for more information.
*/

package org.postgresql.core;

import org.postgresql.util.PSQLException;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

import java.util.Arrays;

@RunWith(Parameterized.class)
public class CommandCompleteParserNegativeTest {

@Parameterized.Parameter(0)
public String input;

@Parameterized.Parameters(name = "input={0}")
public static Iterable<Object[]> data() {
return Arrays.asList(new Object[][]{
{"SELECT 0_0 42"},
{"SELECT 42 0_0"},
{"SELECT 0_0 0_0"},
});
}

@Test
public void run() throws PSQLException {
CommandCompleteParser parser = new CommandCompleteParser();
try {
parser.parse(input);
Assert.fail("CommandCompleteParser should throw NumberFormatException for " + input);
} catch (PSQLException e) {
Throwable cause = e.getCause();
if (cause == null) {
throw e;
}
if (!(cause instanceof NumberFormatException)) {
throw e;
}
// NumerFormatException is expected
}
}
}
@@ -0,0 +1,58 @@
/*
* Copyright (c) 2018, PostgreSQL Global Development Group
* See the LICENSE file in the project root for more information.
*/

package org.postgresql.core;

import org.postgresql.util.PSQLException;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

import java.util.Arrays;

@RunWith(Parameterized.class)
public class CommandCompleteParserTest {

@Parameterized.Parameter(0)
public String input;
@Parameterized.Parameter(1)
public long oid;
@Parameterized.Parameter(2)
public long rows;

@Parameterized.Parameters(name = "input={0}, oid={1}, rows={2}")
public static Iterable<Object[]> data() {
return Arrays.asList(new Object[][]{
{"SELECT 0", 0, 0},
{"SELECT -42", 0, 0},
{"SELECT", 0, 0},
{"", 0, 0},
{"A", 0, 0},
{"SELECT 42", 0, 42},
{"UPDATE 43 42", 43, 42},
{"UPDATE 43 " + Long.MAX_VALUE, 43, Long.MAX_VALUE},
{"UPDATE " + Long.MAX_VALUE + " " + Long.MAX_VALUE, Long.MAX_VALUE, Long.MAX_VALUE},
{"UPDATE " + (Long.MAX_VALUE / 10) + " " + (Long.MAX_VALUE / 10), (Long.MAX_VALUE / 10),
(Long.MAX_VALUE / 10)},
{"UPDATE " + (Long.MAX_VALUE / 100) + " " + (Long.MAX_VALUE / 100), (Long.MAX_VALUE / 100),
(Long.MAX_VALUE / 100)},
{"CREATE TABLE " + (Long.MAX_VALUE / 100) + " " + (Long.MAX_VALUE / 100),
(Long.MAX_VALUE / 100), (Long.MAX_VALUE / 100)},
{"CREATE TABLE", 0, 0},
{"CREATE OR DROP OR DELETE TABLE 42", 0, 42},
});
}

@Test
public void run() throws PSQLException {
CommandCompleteParser expected = new CommandCompleteParser();
CommandCompleteParser actual = new CommandCompleteParser();
expected.set(oid, rows);
actual.parse(input);
Assert.assertEquals(input, expected, actual);
}
}
Expand Up @@ -5,6 +5,8 @@

package org.postgresql.test.jdbc2;

import org.postgresql.core.CommandCompleteParserNegativeTest;
import org.postgresql.core.CommandCompleteParserTest;
import org.postgresql.core.OidToStringTest;
import org.postgresql.core.OidValueOfTest;
import org.postgresql.core.ParserTest;
Expand Down Expand Up @@ -78,6 +80,8 @@
TimezoneCachingTest.class,
ParserTest.class,
ReturningParserTest.class,
CommandCompleteParserTest.class,
CommandCompleteParserNegativeTest.class,

OidToStringTest.class,
OidValueOfTest.class,
Expand Down