Most of this is in conversion, and I assume it became unused with some
of the v5 changes and refactors to a codec-based approach.
There are likely a few more cleanups to be made, but these ones seemed
easy and safe to start with.
As recommended by go-staticcheck, but also might be a bit more efficient
for the compiler to implement, since we don't care about which slice of
bytes is greater than the other one.
This ensures the output of Encode can pass through Scan and produce
the same input. This found two two minor problems with the text
codec. These are not bugs: These situations do not happen when using
pgx with Postgres. However, I think it is worth fixing to ensure the
code is internally consistent.
The problems with the text codec are:
* It did not correctly distinguish between nil and empty. This is not
a problem with Postgres, since NULL values are marked separately,
but the binary codec distinguishes between them, so it seems like
the text codec should as well.
* It did not output spaces between keys. Postgres produces output in
this format, and the parser now only strictly parses the Postgres
format. This is not a bug, but seems like a good idea.
Arrays with values that start or end with vtab ("\v") must be quoted.
Postgres's array parser skips leading and trailing whitespace with
the array_isspace() function, which is slightly different from the
scanner_isspace() function that was previously linked. Add a test
that reproduces this failure, and fix the definition of isSpace.
This also includes a change to use strings.EqualFold which should
really not matter, but does not require copying the string.
* use []string for value string pointers: one allocation instead of
one per value.
* use one string for all key/value pairs, instead of one for each.
After this change, one Hstore will share two allocations: one string
and one []string. The disadvantage is that it cannot be deallocated
until all key/value pairs are unused. This means if an application
takes a single key or value from the Hstore and holds on to it, its
memory footprint will increase. I would guess this is an unlikely
problem, but it is possible.
The benchstat results from my M1 Max are below.
goos: darwin
goarch: arm64
pkg: github.com/jackc/pgx/v5/pgtype
│ orig.txt │ new.txt │
│ sec/op │ sec/op vs base │
HstoreScan/databasesql.Scan-10 82.11µ ± 1% 82.66µ ± 2% ~ (p=0.436 n=10)
HstoreScan/text-10 83.30µ ± 1% 84.24µ ± 3% ~ (p=0.165 n=10)
HstoreScan/binary-10 15.987µ ± 2% 7.459µ ± 6% -53.35% (p=0.000 n=10)
geomean 47.82µ 37.31µ -21.98%
│ orig.txt │ new.txt │
│ B/op │ B/op vs base │
HstoreScan/databasesql.Scan-10 56.23Ki ± 0% 56.23Ki ± 0% ~ (p=0.324 n=10)
HstoreScan/text-10 65.12Ki ± 0% 65.12Ki ± 0% ~ (p=0.675 n=10)
HstoreScan/binary-10 21.09Ki ± 0% 20.73Ki ± 0% -1.70% (p=0.000 n=10)
geomean 42.58Ki 42.34Ki -0.57%
│ orig.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
HstoreScan/databasesql.Scan-10 744.0 ± 0% 744.0 ± 0% ~ (p=1.000 n=10) ¹
HstoreScan/text-10 743.0 ± 0% 743.0 ± 0% ~ (p=1.000 n=10) ¹
HstoreScan/binary-10 464.00 ± 0% 41.00 ± 0% -91.16% (p=0.000 n=10)
geomean 635.4 283.0 -55.46%
¹ all samples are equal
I am working on an application that uses hstore types, and we found
that returning the values is slow, particularly when using the text
protocol, such as when using database/sql. This improves parsing to
be about 6X faster (currently faster than binary). The changes are:
* referencing the original string instead of copying into new strings
(very large win)
* using string.IndexByte to scan double quoted strings: it has
architecture-specific assembly implementations, and most of the
time is spent in key/value strings.
* estimating the number of key/value pairs to allocate the correct
size of the slice and map up front. This reduces the number of
allocations and bytes allocated by a factor of 2, and was a small
CPU win.
* parsing directly into the Hstore, rather than copying into it.
This parser is stricter than the old one. It only accepts hstore
strings serialized by Postgres. The old one was already stricter
than Postgres's own parser, but previously accepted any whitespace
character after a comma. This one only accepts space. Example:
"k1"=>"v1",\t"k2"=>"v2"
Postgres only ever uses ", " as the separator. See hstore_out:
https://github.com/postgres/postgres/blob/master/contrib/hstore/hstore_io.c
The result of using benchstat to compare the benchmark on my M1 Pro
with the following command line in below. The new text parser is now
faster than the binary parser. I will improve the binary parser in a
separate change.
for i in $(seq 10); do go test ./pgtype -run=none -bench=BenchmarkHstoreScan -benchtime=1s >> new.txt; done
goos: darwin
goarch: arm64
pkg: github.com/jackc/pgx/v5/pgtype
│ orig.txt │ new.txt │
│ sec/op │ sec/op vs base │
HstoreScan/databasesql.Scan-10 82.11µ ± 1% 10.51µ ± 0% -87.20% (p=0.000 n=10)
HstoreScan/text-10 83.30µ ± 1% 11.49µ ± 1% -86.20% (p=0.000 n=10)
HstoreScan/binary-10 15.99µ ± 2% 15.77µ ± 1% -1.35% (p=0.007 n=10)
geomean 47.82µ 12.40µ -74.08%
│ orig.txt │ new.txt │
│ B/op │ B/op vs base │
HstoreScan/databasesql.Scan-10 56.23Ki ± 0% 11.68Ki ± 0% -79.23% (p=0.000 n=10)
HstoreScan/text-10 65.12Ki ± 0% 20.58Ki ± 0% -68.40% (p=0.000 n=10)
HstoreScan/binary-10 21.09Ki ± 0% 21.09Ki ± 0% ~ (p=0.378 n=10)
geomean 42.58Ki 17.18Ki -59.66%
│ orig.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
HstoreScan/databasesql.Scan-10 744.00 ± 0% 44.00 ± 0% -94.09% (p=0.000 n=10)
HstoreScan/text-10 743.00 ± 0% 44.00 ± 0% -94.08% (p=0.000 n=10)
HstoreScan/binary-10 464.0 ± 0% 464.0 ± 0% ~ (p=1.000 n=10) ¹
geomean 635.4 96.49 -84.81%
¹ all samples are equal
on Windows connecting on a closed port takes about 2 seconds.
You can test with something like this
start := time.Now()
_, err := d.DialContext(context.Background(), "tcp", "127.0.0.1:1")
fmt.Printf("finished, time %s, err: %v\n", time.Since(start), err)
This seems by design
https://groups.google.com/g/comp.os.ms-windows.programmer.win32/c/jV6kRVY3BqM
Generally TestConnectWithFallback takes about 8-9 seconds on Windows.
Increase timeout to avoid random failures under load
This commit adds a background reader that can optionally buffer reads.
It is used whenever a potentially blocking write is made to the server.
The background reader is started on a slight delay so there should be no
meaningful performance impact as it doesn't run for quick queries and
its overhead is minimal relative to slower queries.
The non-blocking IO system was designed to solve three problems:
1. Deadlock that can occur when both sides of a connection are blocked
writing because all buffers between are full.
2. The inability to use a write deadline with a TLS.Conn without killing
the connection.
3. Efficiently check if a connection has been closed before writing.
This reduces the cases where the application doesn't know if a query
that does a INSERT/UPDATE/DELETE was actually sent to the server or
not.
However, the nbconn package is extraordinarily complex, has been a
source of very tricky bugs, and has OS specific code paths. It also does
not work at all with underlying net.Conn implementations that do not
have platform specific non-blocking IO syscall support and do not
properly implement deadlines. In particular, this is the case with
golang.org/x/crypto/ssh.
I believe the deadlock problem can be solved with a combination of a
goroutine for CopyFrom like v4 used and a watchdog for regular queries
that uses time.AfterFunc.
The write deadline problem actually should be ignorable. We check for
context cancellation before sending a query and the actual Write should
be almost instant as long as the underlying connection is not blocked.
(We should only have to wait until it is accepted by the OS, not until
it is fully sent.)
Efficiently checking if a connection has been closed is probably the
hardest to solve without non-blocking reads. However, the existing code
only solves part of the problem. It can detect a closed or broken
connection the OS knows about, but it won't actually detect other types
of broken connections such as a network interruption. This is currently
implemented in CheckConn and called automatically when checking a
connection out of the pool that has been idle for over one second. I
think that changing CheckConn to a very short deadline read and changing
the pool to do an actual Ping would be an acceptable solution.
Remove nbconn and non-blocking code. This does not leave the system in
an entirely working state. In particular, CopyFrom is broken, deadlocks
can occur for extremely large queries or batches, and PgConn.CheckConn
is now a `select 1` ping. These will be resolved in subsequent commits.
I am working on an application that reads a lot of hstore values, and
have discovered that scanning it is fairly slow. I'm working on some
improvements, but first I wanted a better benchmark. This adds more
realistic data, and extends it to cover the three APIs: database/sql,
and pgconn.Rows.Scan with both text and binary protocols.
Remove unneeded string to []byte to string conversion, which saves 2
allocs and should make Hstore text scanning slightly faster.
The Hstore.Scan() function takes a string as input, converts it to
[]byte, and calls scanPlanTextAnyToHstoreScanner.Scan(). That
function converts []byte back to string and calls parseHstore. This
refactors scanPlanTextAnyToHstoreScanner.Scan into
scanPlanTextAnyToHstoreScanner.scanString so the database/sql Scan
function can call it directly, bypassing this conversion.
The added Benchmark shows this saves 2 allocs for longer strings, and
saves about 5% CPU overall on my M1 Pro. benchstat output:
goos: darwin
goarch: arm64
pkg: github.com/jackc/pgx/v5/pgtype
│ orig.txt │ new.txt │
│ sec/op │ sec/op vs base │
HstoreScan-10 1.334µ ± 2% 1.257µ ± 2% -5.77% (p=0.000 n=10)
│ orig.txt │ new.txt │
│ B/op │ B/op vs base │
HstoreScan-10 2.094Ki ± 0% 1.969Ki ± 0% -5.97% (p=0.000 n=10)
│ orig.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
HstoreScan-10 36.00 ± 0% 34.00 ± 0% -5.56% (p=0.000 n=10)
Postgres on Mac OS X has a bug in how it parses hstore text values
that causes it to misinterpret some Unicode values as spaces. This
causes values sent by pgx to be misinterpreted. To avoid this, always
quote hstore values, which is how Postgres serializes them itself.
The test change fails on Mac OS X without this fix.
While I suspect this should not be performance critical for any
application, I added a quick benchmark to test the performance of the
encoding. This change actually makes encoding slightly faster on my
M1 Pro. The output from the benchstat program on this banchmark is:
goos: darwin
goarch: arm64
pkg: github.com/jackc/pgx/v5/pgtype
│ orig.txt │ new-quotes.txt │
│ sec/op │ sec/op vs base │
HstoreSerialize/text-10 207.1n ± 0% 142.3n ± 1% -31.31% (p=0.000 n=10)
HstoreSerialize/binary-10 100.10n ± 0% 99.64n ± 1% -0.45% (p=0.013 n=10)
geomean 144.0n 119.1n -17.31%
I have also attempted to fix the Postgres bug, but it will take a
long time for this fix to get upstream:
https://www.postgresql.org/message-id/CA%2BHWA9awUW0%2BRV_gO9r1ABZwGoZxPztcJxPy8vMFSTbTfi4jig%40mail.gmail.com
Dan McGee
Jack Christensen
Adrian-Stefan Mares
Evan Jones
Gerasimos (Makis) Maropoulos
Brandon Kauffman
Nicola Murino
Lev Zakharov
Klaus
Massimo Costa