On Thu, May 11, 2023 at 01:28:40PM +0900, Michael Paquier wrote:
> On Tue, May 09, 2023 at 09:48:24AM -0700, Andres Freund wrote:
>> On 2023-05-08 12:11:17 -0700, Andres Freund wrote:
>>> I can reproduce a significant regression due to f193883fc of a workload just
>>> running
>>> SELECT CURRENT_TIMESTAMP;
>>>
>>> A single session running it on my workstation via pgbench -Mprepared gets
>>> before:
>>> tps = 89359.128359 (without initial connection time)
>>> after:
>>> tps = 83843.585152 (without initial connection time)
>>>
>>> Obviously this is an extreme workload, but that nevertheless seems too large
>>> to just accept...
Extreme is adapted for a worst-case scenario. Looking at my notes
from a few months back, that's kind of what I did on my laptop, which
was the only machine I had at hand back then:
- Compilation of code with -O2.
- Prepared statement of a simple SELECT combined with a DO block
running executed the query in a loop a few million times on a single
backend:
PREPARE test AS SELECT CURRENT_TIMESTAMP;
DO $$
BEGIN
FOR i IN 1..10000000
LOOP
EXECUTE 'EXECUTE test';
END LOOP;
END $$;
- The second test is mentioned at [1], with a generate_series() on a
keyword.
- And actually I recall some pgbench runs similar to that.. But I
don't have any traces of that in my notes.
This was not showing much difference, and it does not now, either.
Funny thing is that the pre-patch period was showing signs of being a
bit slower in this environment. Anyway, I have just tested the DO
case in a second "bigbox" environment that I have set up for
benchmarking a few days ago, and the DO test is showing me a 1%~1.5%
regression in runtime. That's repeatable so that's not noise.
I have re-run a bit more pgbench (1 client, prepared query with a
single SELECT on a SQL keyword, etc.). And, TBH, I am not seeing as
much difference as you do (nothing with default pgbench setup, FWIW),
still that's able to show a bit more difference than the other two
cases. HEAD shows me an average output close to 43900 TPS (3 run of
60s each, for instance), while relying on SQLValueFunction shows an
average of 45000TPS. That counts for ~2.4% output regression here
on bigbox based on these numbers. Not a regression as high as
mentioned above, still that's visible.
>> Added an open item for this.
>
> Thanks for the report, I'll come back to it and look at it at the
> beginning of next week. In the worst case, that would mean a revert
> of this refactoring, I assume.
So, this involves commits 7aa81c6, f193883 and fb32748. 7aa81c6 has
added some tests, so I would let the tests it added be on HEAD as the
precision was not tested for the SQL keywords this has added cover
for.
fb32748 has removed the dependency of SQLValueFunction on collations
by making the name functions use COERCE_SQL_SYNTAX. One case where
these could be heavily used is RLS, for example, so that could be
visible. f193883 has removed the rest and the timestamp keywords.
I am not going to let that hang in the air with beta1 getting released
next week, though, so attached are two patches to revert the change
(these would be combined, just posted this way for clarity). The only
conflict I could see is caused by the query jumbling where a
SQLValueFunction needs "typmod" and "op" in the computation, ignoring
the rest, so the node definition primnodes.h gains the required
node_attr. Making the execution path cheaper while avoiding the
collation tweaks for SQLValueFunction would be nice, but not at this
time of the year on this branch.
[1]: https://www.postgresql.org/message-id/Y0+dHDYA46UnnLs/@paquier.xyz
--
Michael