Обсуждение: Binary protocol patch v7

Hi,

Just to remind you that the I updated the patches for sending receiving
columns in v3 binary format are available at:
http://mokki.dyndns.org/~mtiihone/postgresql/binarytransfer/

Changes since last update:
- fixed handling of empty ResultSet.
- fixed ArrayIndexOutOfBounds in CompositeParameterList

It passes the current unit test suite and all other applications I have
tried it with.

I would be interested in any benchmarks. What you would hopefully see is
reduced CPU load in both the backend and in the Java application. Also the
network traffic should be somewhat reduced.

Also, Kris, could you please try to find time to review the patch again
and see what still needs to be fixed/clarified/tested before the patch
can be accepted.

---------------------------

I downloaded BenchmarkSQL 2.3.2 and run it against freshly installed
postgres 8.2 backend. Results (best of 10 runs):
Jan 7, 2007 CVS: 5891 tpmC - 100%
+ binaryPatches: 6531 tpmC - 110%

The client and server ran on the same host. The Java client used
around 25% of available cpu, backend the remaining time, IO was not
a bottleneck.

Benchmark setup:
----------------
PostgreSQL 8.2.0 with following modified config options:
fsync=off, autovacuum=on, stats_row_level=on, redirect_stderr=on,
checkpoint_segments=12, max_connections=10, max_prepared_transactions=50,
shared_buffers=128MB, standard_conforming_strings=on

The BenchmarkSQL was set up with 1 warehouse and 4 terminals. The
following URL was used to connect to the backend:
jdbc:postgresql://localhost:5432/test?binaryEncoding=true&prepareThreshold=1

Java HotSpot(TM) 64-Bit Server VM (build 1.6.0-b105, mixed mode)
Athlon64 2x2GHz, Linux 2.6.19.1

I ran "vacuumdb --all --full" between each benchmark run to get consistent
results.

-Mikko

On Sun, 7 Jan 2007, Mikko Tiihonen wrote:

> Just to remind you that the I updated the patches for sending receiving
> columns in v3 binary format are available at:
> http://mokki.dyndns.org/~mtiihone/postgresql/binarytransfer/
>
> Also, Kris, could you please try to find time to review the patch again
> and see what still needs to be fixed/clarified/tested before the patch
> can be accepted.
>

I was actually looking at the v7 patch earlier this week, specifically
looking at the conversion code.  To try and throw some of the toughest
test cases at it I modified the timestamp and time zone test code like
k1-timestamp-test.patch.  When running the tests with a
build.local.properties including binaryTransfer=true and
preparethreshold=1 this test fails.

So I thought about ways to get more testing without having to rewrite
every single regression test.  I'm not suggesting committing any of this
code, but it's just something I hacked together to try more cases.

k2-describe-then-execute.patch issues a describe then an execute for every
prepared statement so it won't need to be executed more than once to start
using the binary transfer mode if we've set prepareThreshold to 1.  This
caused only one failure in an updatable resultset test because the results
of ResultSet.refreshRow() were in binary while the ResultSet was in text.
This failure doesn't concern me because without the hack the
PreparedStatement is only used once and won't trigger a binary receive.

So then I tried to k3-stmt-as-prepstmt.patch to send all
Statement.executeQuery calls using a PreparedStatement to trigger binary
receiver.  This resulted in a deadlock in the regression tests that I did
not have time to investigate.

Even though this didn't work, I think it's the way to go to convince
people that the new conversion code works exactly as the existing code
does.  I don't care how ugly it is or how slow it is, if there's a way to
get the majority of our existing tests to run through the binary codepath
I'd be happy to apply the patch.

Kris Jurka

On Sun, 7 Jan 2007, Kris Jurka wrote:

> On Sun, 7 Jan 2007, Mikko Tiihonen wrote:
>
>> Just to remind you that the I updated the patches for sending receiving
>> columns in v3 binary format are available at:
>> http://mokki.dyndns.org/~mtiihone/postgresql/binarytransfer/
>>
>
> I was actually looking at the v7 patch earlier this week, specifically
> looking at the conversion code.  To try and throw some of the toughest test
> cases at it I modified the timestamp and time zone test code like
> k1-timestamp-test.patch.  When running the tests with a
> build.local.properties including binaryTransfer=true and preparethreshold=1
> this test fails.
>
> So I thought about ways to get more testing without having to rewrite every
> single regression test.  I'm not suggesting committing any of this code, but
> it's just something I hacked together to try more cases.
>

With the attached patch and binaryTransfer=true and preparethreshold=1 in
build.local.properties I was able to run the regression tests completely
and got a number of failures.

1) DatabaseMetaData creates ResultSets by sometimes pulling raw bytes out
of another ResultSet, so it needs to also pass the binary flag to the new
ResultSet.

2) A number of failures in date/time tests.  It seems to have
some problems with time zones.  Also, conversion to strings
should result in something identical to the text format.

3) There's also a couple of fake failures due to the methodology used.
testUpdateStreams and testHoldableResultSet fail only because of the
changes I made.

Kris Jurka