Opened 4 months ago

Closed 4 months ago

#77 closed task (fixed)

Bad performance of query_formatted()

Reported by: justin Owned by: cito
Priority: major Milestone: 5.1
Component: DB API 2 Version: 5.0
Keywords: pg, performance Cc:

Description

This was posted by Justin to the mailing list 2019-04-01:

I was going to do a performance test for prepared queries, specifically for INSERTs: simple queries, query_formatted, inline=True+False, SQL EXECUTE, pgdb, dicts and PQexecPrepared.

I didn't get far before needing to step back, as I'm shocked by how slow parameter formatting is, just to turn %s into $1. It's nearly 10x slower than query() with $1 params. That's true for pg (which can send params out of line) and pgdb (which always inlines params).

If I'd noticed 2 years ago, I might have avoided using DB.query_formatted() just to let developers use %s formats. Perhaps the only reason why this isn't causing issues for our data loaders is that each CPU heavy formatted INSERT is partially balanced with I/O on the DB server. But I can easily imagine that being a deal-breaker. Maybe I'm wrong, but I'd have expected the additional processing cost to be within 10% or 20% of query, and certainly within a factor of two.

BTW, prepared query completely avoids this by avoiding all the isinstance which appears to be most of the expense. The high performance cost and mitigation (prepares) should be documented. Also, I realized that SQL EXECUTE is a bad way to do it, and maybe shouldn't be documented, since it requires quoting each parameter (which is slow and thereby defeats itself).

I tried to use cProfile to find hotspots and guessed at the rest until it worked. My tweaks make it 3x faster - however, it's still 3x slower than query()!

My changes are not very "disciplined" so please consider this a proof of concept written by a non pytho-phone. I didn't touch pgdb and I haven't tested with dict formats. I imagine you may have a better idea how/what to cache.

3 runs on a semi-idle server with checkpoint_interval=60sec.
100000rows and 100cols (I believe this scales roughly as rows*cols)

methodunpatchedpatched time (sec)
pg simple insert with txn:(105 98 91)
pg query() $1 params:(169 172 178)174
pg query_formatted %s params:(745)314
pg insert using inline=True:(386)187
pg prepare_query + execute_prepared(n/a)(90, 88, 94)
note, that's not included in this patch
pgdb insert with/out paramsTODO
SQL EXECUTE + $1 + inline=True:TODO
dictsTODO
psycopgTODO

Find attached my patch and test, which was meant to approximate our loader processes, which I'd like to convert to use prepared query.

Attachments (1)

v1-0001-Optimize-query-formatting.patch (8.7 KB) - added by cito 4 months ago.
Optimized query_formatted patch

Download all attachments as: .zip

Change History (4)

Changed 4 months ago by cito

Optimized query_formatted patch

comment:1 Changed 4 months ago by cito

Also posted by Justin 2019-01-19:

1) in 5.0, document that relative to query, query_formatted has an overhead "which can be significant for queries repeated many times", and document that the mitigation is to use inline=True; or, use prepared statements "available since 5.1". Note that for simple queries like INSERT, the significant overhead is in pygres, but for complex queries like JOINs/large inheritence trees/etc, the more overhead is in planning.

2) For 5.1.1 (and maybe 5.0), something to mitigate the cost of isinstance() in pg and pgdb.

3) In 5.1 (but probably not 5.0?), consider changing query_formatted default to inline=True. In my test, this inserted 30% faster (!) even with no 2nd patch.

$ python2.7 ./testinsert.py
diff 192.718273878
vs
$ python2.7 ./testinsert.py
diff 309.562824965

That might be good to consider for other reasons: there's 1) pqExec vs 2) pqExecParams. 1) supports multiple commands; but 2) allows binary protocol (which pygres doesn't currently support). Binary protocol (or anything using pqExecParams) will never support multiple commands.

If there aren't params, query_formatted currently calls query and pqExec, to allow the possibility of including multiple commands. I wonder whether (starting in v5.1) perhaps pygres shouldn't call pqExecParams() in the case that there are no params? Otherwise it's odd that query_formatted would call pqExec sometimes only, and an odd conditional which complicates any future support for things like binary format. I realize that binary format isn't going to happentime anytime soon, if ever, but 5.1 is maybe an opportunity to make that change.

Maybe multiple commands should be documented, and it's odd to write that query_formatted supports multiple command "if there are no params, or if inline=True". It'd be better to be able to say "multiple commands are not supported except when inline=True"; or, if that was the default, "multiple commands are not supported if inline=False".

If pg always (or defaulted) used inline=True, we could always use multiple commands (even with pqExecParams), and it would be similar to pgdb. But maybe that's moving in the wrong direction, too.. Or maybe that should wait until binary protocol is on the table

I realize this message is addressing multiple things and maybe not very focused, but a few of these are kind of connected.

comment:2 Changed 4 months ago by cito

Thank you, Justin, and sorry for looking into this only now.

With a similar query as you're using (inserting 250 columns of zeroes), I measured:

Test simple inline query():
  Average time = 4.93 secs
Test query() with params:
  Average time = 6.55 secs
Test query_formatted() with inline=False:
  Average time = 12.47 secs
Test query_formatted() with types and inline=False:
  Average time = 10.54 secs
Test query_formatted() with inline=True:
  Average time = 7.34 secs

So I can basically confirm what you are reporting. As far as I see, the difference between inline=False vs. True comes from creating two separate lists (one for formatting the string, the other one for the separately passed params) vs. only one, and a general performance penalty when using PQExecParams vs PQEexec.

But I think the huge performance difference in your example is only an artifact of the special kind of query you were using, with very simple values (just integer zeroes) and many columns.

When I use a slightly different query, inserting into 250 columns a string of 26 chars instead of the integer zero, I measure these numbers:

Test simple inline query():
  Average time = 4.82 secs
Test query() with params:
  Average time = 3.84 secs
Test query_formatted() with inline=False:
  Average time = 4.15 secs
Test query_formatted() with types and inline=False:
  Average time = 4.13 secs
Test query_formatted() with inline=True:
  Average time = 5.26 secs

This is because strings need to be escaped when inserted inline, which turns the tide against inline=True.

So the performance and which kind of parameter passing is better depends very much on the kind of values. For more complex values and less columns, I think inline=False is better.

And as you say, it also depends on the type of query and speed of the database whether the overhead for conversion and adaptation of the values matters or not.

The problem of inline=True is also that you can't pass types along, so I think it is not so good as default value. I think you can also handle more complicated types with inline=False. So I'd rather keep the old default.

I also had a look at the code, checked with a profiler and made some optimizations in r970, r971 and r972, but did not really find much more we can do here, at least no low-hanging fruits. Some of the optimizations in your patch will not work in the general case, when you have mixed parameters containing Literal values. But I used a similar optimization to fetch the simple types for the most frequent Python types. We could also use a cache, like the _SimpleTypes cache for Postgres types, but I'm not sure if that's worth the effort and it might grow out of bounds when types are dynamically generated.

So I think the best we can do is document the issue as you suggested, and refer to query and query_prepared for performance-critial queries and batch runs which I have done in r973.

Last edited 4 months ago by cito (previous) (diff)

comment:3 Changed 4 months ago by cito

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.