Re: CommitFest status summary 2010-01-27

Hi,

Greg Smith írta:
>> Provide rowcount for utility SELECTs
>
> I think I can write a review for this one right now based on the
> discussion that's already happened:
>
> -Multiple people think the feature is valuable and it seems worth
> exploring
> -The right way to handle the protocol change here is still not clear
> -There are any number of subtle ways clients might be impacted by this
> change, and nailing that down and determining who might break is an
> extremely wide ranging bit of QA work that's barely been exploring yet
>
> That last part screams "returned with feedback" for something
> submitted to the last CF before beta to me. As a candidate for
> 9.1-alpha1 where there's plenty of time to figure out what it breaks
> and revert if that turns out to be bad? That sounds like a much
> better time to be fiddling with something in this area.

I understand your position, this is a subtle change that might
turn out to break clients, indeed.

> I would like to see the following in order to make this patch have a
> shot at being comittable:
>
> 1) Provide some examples showing how the feature would work in
> practice and of use-cases for it.

I'll try to explore all the things affected by this change
and reflect them in a regression test.

> 2) To start talking about what's going to break, some notes about what
> this does to the regression tests would be nice.

Is there a way to run a regression test under src/test/regress so the
command status string is also written into the *.out file? It doesn't
seem to me so because all the current *.out files only contain
results for tuple-returning statements and
src/test/regress/pg_regress_main.c
runs psql in quiet mode.

So, first I suggest dropping the "-q" option from the psql command line
in psql_start_test() in pg_regress_main.c to be able to see the command
strings.

> 3) Demonstrate with example sessions the claims that there are no
> backward compatibility issues here. I read "when you mix old server
> with new clients or new server with old client, it will just work as
> before", but until I see a session proving that I have to assume
> that's based on code inspections rather than actual tests, and
> therefore not necessarily true. (Nothing personal, Zoltan--just done
> way too much QA in the last year to believe anything I'm told about
> code without a matching demo).
> 4) Investigate and be explicit about the potential breakage here both
> for libpq clients and at least one additional driver too. If I saw a
> demonstration that this didn't break the JDBC driver, for example, I'd
> feel a lot better about the patch.

I thought the libpq change was obvious.
strncmp(status, "SELECT ", 7) /* one space at the end */
doesn't match "SELECT" (no spaces). So:
1. old server sends "SELECT", the code in the new libpq client
gets a "doesn't match", PQcmdTuples() returns "".
2. new server sends "SELECT N", old libpq client doesn't look
for strings starting with "SELECT", PQcmdTuples() returns ""

I looked at the latest JDBC source (currently it's postgresql-jdbc-8.4-701)
these are the places I found where the command status interpreted
in core/v3/QueryExecutorImpl.java:

private String receiveCommandStatus() throws IOException {
//TODO: better handle the msg len
int l_len = pgStream.ReceiveInteger4();
//read l_len -5 bytes (-4 for l_len and -1 for trailing \0)
String status = pgStream.ReceiveString(l_len - 5);
//now read and discard the trailing \0
pgStream.Receive(1);

if (logger.logDebug())
logger.debug(" <=BE CommandStatus(" + status + ")");

return status;
}

and

private void interpretCommandStatus(String status, ResultHandler
handler) {
int update_count = 0;
long insert_oid = 0;

if (status.startsWith("INSERT") || status.startsWith("UPDATE")
|| status.startsWith("DELETE") || status.startsWith("MOVE"))
{
try
{
update_count = Integer.parseInt(status.substring(1 +
status.lastIndexOf(' ')));
if (status.startsWith("INSERT"))
insert_oid = Long.parseLong(status.substring(1 +
status.indexOf(' '),
status.lastIndexOf(' ')));
}
catch (NumberFormatException nfe)
{
handler.handleError(new PSQLException(GT.tr("Unable to
interpret the update count in command completion tag: {0}.", status),
PSQLState.CONNECTION_FAILURE));
return ;
}
}

handler.handleCommandStatus(status, update_count, insert_oid);
}

receiveCommandStatus() reads everything up to and including
the zero termination byte. interpretCommandStatus() handles
only the old strings expected to carry the rowcount.
This wouldn't break for the new "SELECT N" string as it
falls into the latest (here nonexisting) "else" branch, effectively
ignoring "SELECT" or "SELECT N".

The version of the same in ./core/v2/QueryExecutorImpl.java is:

protected void processResults(Query originalQuery, ResultHandler
handler, int maxRows, int flags) throws IOException {
...
case 'C': // Command Status
String status = pgStream.ReceiveString();

if (logger.logDebug())
logger.debug(" <=BE CommandStatus(" + status + ")");

if (fields != null)
{
handler.handleResultRows(originalQuery, fields,
tuples, null);
fields = null;

if (bothRowsAndStatus)
interpretCommandStatus(status, handler);
}
else
{
interpretCommandStatus(status, handler);
}

break;
...

private void interpretCommandStatus(String status, ResultHandler
handler) throws IOException {
int update_count = 0;
long insert_oid = 0;

if (status.equals("BEGIN"))

protoConnection.setTransactionState(ProtocolConnection.TRANSACTION_OPEN);
else if (status.equals("COMMIT") || status.equals("ROLLBACK"))

protoConnection.setTransactionState(ProtocolConnection.TRANSACTION_IDLE);
else if (status.startsWith("INSERT") ||
status.startsWith("UPDATE") || status.startsWith("DELETE") ||
status.startsWith("MOVE"))
{
try
{
update_count = Integer.parseInt(status.substring(1 +
status.lastIndexOf(' ')));
if (status.startsWith("INSERT"))
insert_oid = Long.parseLong(status.substring(1 +
status.indexOf(' '),
status.lastIndexOf(' ')));
}
catch (NumberFormatException nfe)
{
handler.handleError(new PSQLException(GT.tr("Unable to
interpret the update count in command completion tag: {0}.", status),
PSQLState.CONNECTION_FAILURE));
return ;
}
}

handler.handleCommandStatus(status, update_count, insert_oid);
}

This also handles only the old strings expected to carry a rowcount.
Reading the code, both v2 and v3 would return 0 for this patch in
the server. Handling the new case needs a new condition, something like:
|| (status.startsWith("SELECT") && !status.equals("SELECT"))

The question is whether new versions of psqlODBC and the old
ones shipped in unixODBC handle the change well.

> Expecting a community reviewer to do all that just to confirm this
> code change is worthwhile seems a pretty big stretch to me,

True.

> and I wouldn't consider it very likely that set of work could get
> finished in time for this CF.
>

Also true.

Best regards,
Zoltán Böszörményi

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/