Hello Nathan,
I'm unclear about what variety of scripts that could be provided given the
tables made available with pgbench. ISTM that other scenari would involve
both an initialization and associated scripts, and any proposal would be
bared because it would open the door to anything.
Why's
About pgbench exit on abort v4:
Patch applies cleanly, compiles, local make check ok, doc looks ok.
This looks ok to me.
--
Fabien.
[...] Further changes are already needed for their "main" branch (LLVM
17-to-be), so this won't quite be enough to shut seawasp up.
For information, the physical server which was hosting my 2 bf animals
(seawasp and moonjelly) has given up rebooting after a power cut a few
weeks/months
Hello Nathan,
1. so I don't have to create the script and function manually each
time I want to test mainly the database (instead of the
client-database system)
2. so that new users of PostgreSQL can easily see how much better OLTP
workloads perform when packaged up as a server-side function
4. libevent development seems slugish, last bugfix was published 3 years ago,
version
2.2 has been baking for years, but the development seems lively (+100
contributors).
Ugh, I would stay away from something like that. Would we become
hostage to an undelivering group? No thanks.
Interesting. In my understanding this also needs to make Latch
frontend-friendly?
It could be refactored to support a different subset of event types --
maybe just sockets, no latches and obviously no 'postmaster death'.
But figuring out how to make latches work between threads might also
be
Hello Thomas,
Pgbench is managing clients I/Os manually with select or poll. Much of this
could be managed by libevent.
Or maybe libuv (used by nodejs?).
From preliminary testing libevent seems not too good at fine grain time
management which are used for throttling, whereas libuv
Bonjour Michaël,
On Sun, Aug 13, 2023 at 11:22:33AM +0200, Fabien COELHO wrote:
Test run is ok on my Ubuntu laptop.
I have a few comments about this patch.
Argh, sorry!
I looked at what was removed (a lot) from the previous version, not what
was remaining and should also have been
Pgbench is managing clients I/Os manually with select or poll. Much of this
could be managed by libevent.
Or maybe libuv (used by nodejs?).
From preliminary testing libevent seems not too good at fine grain time
management which are used for throttling, whereas libuv advertised that it
is
Hello devs,
Pgbench is managing clients I/Os manually with select or poll. Much of
this could be managed by libevent.
Pros:
1. libevent is portable, stable, and widely used (eg Chromium, Memcached,
PgBouncer).
2. libevent implements more I/O wait methods, which may be more efficient on
Hello Yugo-san,
I attached the updated patch v3 including changes above, a test,
and fix of the typo you pointed out.
I'm sorry but the test in the previous patch was incorrect.
I attached the correct one.
About pgbench exit on abort v3:
Patch does not "git apply", but is ok with "patch"
Hello Yugo-san,
Currently, the psql's test of query cancelling (src/bin/psql/t/020_cancel.pl)
gets the PPID of a running psql by using "\!" meta command, and sends SIGINT to
the process by using "kill". However, IPC::Run provides signal() routine that
sends a signal to a running process, so I
Hello Yugo-san,
Some feedback about v2.
There is some dead code (&& false) which should be removed.
Maybe it should check that cancel is not NULL before calling PQcancel?
I think this is already checked as below, but am I missing something?
+ if (all_state[i].cancel != NULL)
+
Hello Yugo-san,
There are cases where "goto done" is used where some error like
"invalid socket: ..." happens. I would like to make pgbench exit in
such cases, too, so I chose to handle errors below the "done:" label.
However, we can change it to call "exit" instead of "goo done" at each
Hello Yugo-san,
I attached v2 patch including the documentation and some comments
in the code.
I've looked at this patch.
I'm unclear whether it does what it says: "exit immediately on abort", I
would expect a cold call to "exit" (with a clear message obviously) when
the abort occurs.
Yugo-san,
Some feedback about v1 of this patch.
Patch applies cleanly, compiles.
There are no tests, could there be one? ISTM that one could be done with a
"SELECT pg_sleep(...)" script??
The global name "all_state" is quite ambiguous, what about "client_states"
instead? Or maybe it
Hello Yugo-san,
In thread #0, setup_cancel_handler is called before the loop, the
CancelRequested flag is set when Ctrl+C is issued. In the loop, cancel
requests are sent when the flag is set only in thread #0. SIGINT is
blocked in other threads, but the threads will exit after their query
Hello Dave,
I am running pgbench with the following
pgbench -h localhost -c 100 -j 100 -t 2 -S -s 1000 pgbench -U pgbench
--protocol=simple
Without pgbouncer I get around 5k TPS
with pgbouncer I get around 15k TPS
Looking at the code connection initiation time should not be part of the
Hello Jelte,
This patch seems to have quite some use case overlap with my patch which
adds load balancing to libpq itself:
https://www.postgresql.org/message-id/flat/pr3pr83mb04768e2ff04818eeb2179949f7...@pr3pr83mb0476.eurprd83.prod.outlook.com
Thanks for the pointer.
The end purpose of
of as "inverted
lambda symbol" :)
:-)
If there are no strong objections, I'll add it to the commitfest as well
Please do that.
--
Fabien Coelho.
Bonjour Michaël,
Overall, I think that there should be a clearer discussion and plan about
which random functionS postgres should provide to complement the standard
instead of going there… randomly:-)
So, what does the specification tells about seeds, normal and random
functions? A bunch of
Hello Paul,
My 0.02€ about the patch:
Patch did not apply with "git apply", I had to "patch -p1 <" and there was
a bunch of warnings.
Patches compile and make check is okay.
The first patch adds empty lines at the end of "sql/random.sql", I think
that it should not.
# About
Hello Peter,
I had noticed that most getopt() or getopt_long() calls had their letter
lists in pretty crazy orders. There might have been occasional attempts
at grouping, but those then haven't been maintained as new options were
added. To restore some sanity to this, I went through and
Now some of the output generated by test_extdepend gets a bit
confusing:
+-- QUERY:
+
+
+-- QUERY:
That's not entirely this patch's fault. Still that's not really
intuitive to see the output of a query that's just a blank spot..
Hmmm.
What about adding an explicit \echo before these empty
Hmm. The refactoring is worth it as much as the differentiation
between QUERY and INTERNAL QUERY as the same pattern is repeated 5
times.
Now some of the output generated by test_extdepend gets a bit
confusing:
+-- QUERY:
+
+
+-- QUERY:
That's not entirely this patch's fault. Still that's
Hello David,
Question: is there any way to really abort a psql script from an
included file?
Under what circumstances would it be appropriate for a script to take
it on itself to decide that? It has no way of knowing what the next -f
option is or what the user intended.
Can we add an
Hello David,
vagrant@vagrant:~$ /usr/local/pgsql/bin/psql -v ON_ERROR_STOP=1 -f two.psql
-f three.psql postgres
?column?
--
2
(1 row)
?column?
--
3
(1 row)
(there is a \quit at the end of two.psql)
Yep, that summarizes my issues!
ON_ERROR_STOP is only of SQL
Hello Tom,
- when the current script is included from something,
you quit the current script and proceed after the \i of next -f, BAD
Question: is there any way to really abort a psql script from an included
file?
Under what circumstances would it be appropriate for a script to take
Hello devs,
I want to abort a psql script. How can I do that? The answer seems to be
\quit, but it is not so simple:
- when the current script is from a terminal, you exit psql, OK
- when the current script is from a file (-f, <), you exit psql, OK
- when the current script is included
Hello Ian,
cfbot reports the patch no longer applies. As CommitFest 2022-11 is
currently underway, this would be an excellent time to update the patch.
Attached a v5 which is just a rebase.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index
Bonjour Daniel,
Good catch! Thanks for the quick fix!
As usual, what is not tested does not:-(
Attached a tap test to check for the expected behavior with multiple
command \g.
--
Fabien.diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl
index f447845717..c81feadd4e
With our current PRNG infrastructure it doesn't cost much to have
a separate PRNG for every purpose. I don't object to having
array_shuffle() and array_sample() share one PRNG, but I don't
think it should go much further than that.
Thanks for your thoughts, Tom. I have a couple of
Hello,
Thank you for your feedback. I attached a patch, that addresses most of your
points.
I'll look into it. It would help if the patch could include a version
number at the end.
Should the exchange be skipped when i == k?
The additional branch is actually slower (on my machine,
i came to the same conclusions and went with Option 1 (see patch). Mainly
because most code in utils/adt is organized by type and this way it is
clear, that this is a thin wrapper around pg_prng.
Small patch update. I realized the new functions should live
array_userfuncs.c (rather than
Hello Thomas,
llvmjit.c:1233:81: error: too few arguments to function call, expected 3,
have 2
ref_gen =
LLVMOrcCreateCustomCAPIDefinitionGenerator(llvm_resolve_symbols, NULL);
Ah yes, I hadn't seen that one yet. That function grew a "Dispose"
argument, which we can just pass
Just a note/reminder that "seawasp" has been unhappy for some days now
because of yet another change in the unstable API provided by LLVM:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp=2022-06-23%2023%3A18%3A17
llvmjit.c:1115:50: error: use of undeclared identifier
Hello Robert,
I think for this purpose we should limit ourselves to algorithms
whose output size is, at minimum, 64 bits, and ideally, a multiple of
64 bits. I'm sure there are plenty of options other than the ones that
btrfs uses; I mentioned them only as a way of jump-starting the
Hello Tomas,
At on of the pgcon unconference sessions a couple days ago, I presented
a bunch of benchmark results comparing performance with different
data/WAL block size. Most of the OLTP results showed significant gains
(up to 50%) with smaller (4k) data pages.
You wrote something about
Probably it would be appropriate to add a test case. I'll propose something
later.
committed with a test
Thanks!
--
Fabien.
Hello,
Thanks for the catch and the proposed fix! Indeed, on errors the timing is
not updated appropriately.
ISTM that the best course is to update the elapsed time whenever a result
is obtained, so that a sensible value is always available.
See attached patch which is a variant of
It find it a little annoying that there is no change in tests, it
means that the format is not checked at all:-(
Yeah. Perhaps it's a little bit hard to perform this kind of tests in
the TAP test?
Not really. I'll look into it.
--
Fabien.
Ok. Fine with me. Possibly at some point there was the idea that there
could be other failures counted, but there are none. Also, there has
been questions about the failures detailed option, or whether the
reports should always be detailed, and the result may be some kind of
not convincing
How about we just say "uses a linear-feedback shift register algorithm"?
I think it'd be sufficient to just say that it's a deterministic
pseudorandom number generator. I don't see much value in documenting
the internal algorithm used.
Hmmm… I'm not so sure. ISTM that people interested in
Hello Tom,
The buildfarm is still complaining about the synopsis being too
wide for PDF format. I think what we ought to do is give up on
using a for log lines at all, and instead convert the
documentation into a tabular list of fields. Proposal attached,
which also fixes a couple of
serfail dlfail
because I intend to add connection failures handling at some point, and it
would make more sense to add the corresponding count at the end with other
fails.
--
Fabien Coelho - CRI, MINES ParisTech
Or those three columns always, sum_lag sum_lag_2, min_lag max_lag,
skipped, retried retries?
What about this? (a log line is not actually folded)
interval_start num_transactions sum_latency sum_latency_2 min_latency
max_latency
failures serialization_failures deadlock_failures retried
According to the cfbot this patch needs a rebase
Indeed. v4 attached.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index ebdb4b3f46..d96d2d291d 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -29,7 +29,7 @@ PostgreSQL
Again, after the SendQuery refactoring extraction.
I'm doing this locally, so don't feel obliged to send more of these. ;-)
Good for me :-)
--
Fabien.
Attached a rebase.
Again, after the SendQuery refactoring extraction.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
---
Or those three columns always, sum_lag sum_lag_2, min_lag max_lag,
skipped, retried retries?
Anyway now that current CF is closing, it will not be possible to
change those logging design soon. Or can we change the logging design
even after CF is closed?
My 0.02€: I'm not sure how the
Attached a rebase.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++
Hello Peter,
As Tom said earlier, wasn't this fixed by 618c16707? If not, is there any
other discussion on the specifics of this issue? I'm not aware of one.
This answer is that I had kept psql's calls to PQerrorMessage which
reports errors from the connection, whereas it needed to change
As Tom said earlier, wasn't this fixed by 618c16707? If not, is there any
other discussion on the specifics of this issue? I'm not aware of one.
Hmmm… I'll try to understand why the doubled message seems to be still
there.
--
Fabien.
Pgbench is a simple benchmark tool by design, and I wonder if adding
a multiconnect feature will cause pgbench to be used incorrectly.
Maybe, but I do not see how it would be worse that what pgbench already
allows.
I agree that pgbench is simple; perhaps really too simple when it comes to
Hello Peter,
Attached v17 is another try. The point is to record the current status,
whatever it is, buggy or not, and to update the test when libpq fixes
things, whenever this is done.
[...]
The expected output (which passes) contains this line twice:
psql::2: FATAL: terminating
Hi Sami,
Pgbench is a simple benchmark tool by design, and I wonder if adding
a multiconnect feature will cause pgbench to be used incorrectly.
Maybe, but I do not see how it would be worse that what pgbench already
allows.
A real world use-case will be helpful for this thread.
Hello Peter,
See attached v16 which removes the libpq workaround.
I suppose this depends on
https://www.postgresql.org/message-id/flat/ab4288f8-be5c-57fb-2400-e3e857f53e46%40enterprisedb.com
getting committed, because right now this makes the psql TAP tests fail
because of the duplicate
Hello Greg,
It looks like David sent a patch and Fabien sent a followup patch. But
there hasn't been a whole lot of discussion or further patches.
It sounds like there are some basic questions about what the right
interface should be. Are there specific questions that would be
helpful for
Are you planning to send a rebased patch for this commit fest?
Argh, I did it in a reply in another thread:-( Attached v15.
So as to help moves things forward, I'd suggest that we should not to care
too much about corner case repetition of some error messages which are due to
libpq
Hello Yugo-san,
About Pgbench error handling v16:
This patch set needs a minor rebase because of 506035b0. Otherwise, patch
compiles, global and local "make check" are ok. Doc generation is ok.
This patch is in good shape, the code and comments are clear.
Some minor remarks below,
Hello Justin,
Review about v34, up from v32 which I reviewed in January. v34 is an
updated version of v32, without the part about lstat at the end of the
series.
All 7 patches apply cleanly.
# part 01
One liner doc improvement about the directory flag.
OK.
# part 02
Add tests for
Hello Justin,
I hope to look at it over the week-end.
--
Fabien Coelho - CRI, MINES ParisTech
It seems we should just remove the use of register?
I have a vague idea that it was once important to say "register" if
you are going to use the variable in an asm snippet that requires it
to be in a register. That might be wrong, or it might be obsolete
even if once true. We could try
Just FYI. Better to follow up to the thread for the patch that's
already in the CF. Otherwise your patch will missed by someone who
looks at the CF entry to see the latest patch.
Indeed. Done.
--
Fabien.
Are you planning to send a rebased patch for this commit fest?
Argh, I did it in a reply in another thread:-( Attached v15.
So as to help moves things forward, I'd suggest that we should not to care
too much about corner case repetition of some error messages which are due
to libpq
Hello Greg,
Peter posted an updated version of Fabiens patch about a month ago (which at
this point no longer applies)
Attached a v15 which is a rebase, after some minor changes in the source
and some new test cases added (good!).
fixing a few issues, but also point at old review comments
Hello Tatsuo-san,
It seems the patch is ready for committer except below. Do you guys want
to do more on below?
I'm planning a new review of this significant patch, possibly over the
next week-end, or the next.
--
Fabien.
So fine with me wrt having a more homogeneous report.
So are you fine with Kawamoto-san's patch?
Yes.
Patch applies cleanly (hmmm, it would have been better to have it as an
attachement). Make & make check ok. Fine with me.
--
Fabien.
Hello Daniel,
I think that the break of typographical rules is intentional to allow
such simplistic low-level stream handling through pipes or scripts. I'd
prefer that the format is not changed. Maybe a comment could be added
to explain the reason behind it.
That doesn't sound like an
Bonjour Michaël,
I think it's better to back-patch this to stable branches if there's
no objection. Thought?
That's only cosmetic, so I would just bother about HEAD if I were to
change something like that (I would not bother at all, personally).
One argument against a backpatch is that this
Hello Tom,
I concur with Fabien's analysis: we report the FATAL message from
the server during the first PQgetResult, and then the second call
discovers that the connection is gone and reports "server closed
the connection unexpectedly". Those are two independent events
(in libpq's view
Hello Andres,
I'm doubtful that tracking development branches of LLVM is a good
investment. Their development model is to do changes in-tree much more than we
do. Adjusting to API changes the moment they're made will often end up with
further changes to the same / related lines. Once they
Speaking of buildfarm breakage, seawasp has been failing for the
past several days. It looks like bleeding-edge LLVM has again
changed some APIs we depend on. First failure is here:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp=2022-01-28%2000%3A17%3A48
Indeed.
I'm
Hello Peter,
It would be better to do without. Also, it makes one wonder how others
are supposed to use this multiple-results API properly, if even psql can't
do it without extending libpq. Needs more thought.
Fine with me! Obviously I'm okay if libpq is repaired instead of writing
command = SELECT pg_terminate_backend(pg_backend_pid());
result 1 status = PGRES_FATAL_ERROR
error message = "FATAL: terminating connection due to administrator command
"
result 2 status = PGRES_FATAL_ERROR
error message = "FATAL: terminating connection due to administrator command
server
Hallo Peter,
Attached v14 moves the status extraction before the possible clear. I've
added a couple of results = NULL after such calls in the code.
In the psql.sql test file, the test I previously added concluded with \set
ECHO none, which was a mistake that I have now fixed. As a
Hello Andres,
The reason this test constantly fails on cfbot windows is a use-after-free
bug.
Indeed! Thanks a lot for the catch and the debug!
The ClearOrSaveResult function is quite annoying because it may or may not
clear the result as a side effect.
Attached v14 moves the status
Hello Peter,
quite weird and confusing. Maybe this type of test should be done in
the TAP framework instead.
Attached v13 where the crash test is moved to tap.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out
Hello Peter,
With this "voluntary crash", the regression test output is now
psql ... ok (test process exited with exit
code 2) 281 ms
Normally, I'd expect this during development if there was a crash somewhere,
but showing this during a normal run
Here is my review about v32:
I forgot to tell that doc generation for the cumulated changes also works.
--
Fabien.
Hello Justin,
Happy new year!
I think the 2nd half of the patches are still waiting for fixes to lstat() on
windows.
Not your problem?
Here is my review about v32:
All patches apply cleanly.
# part 01
One liner doc improvement to tell that creation time is only available on
windows.
It
[...]
I agree that these two behaviors in libpq are dubious, especially the
second one. I want to spend some time analyzing this more and see if
fixes in libpq might be appropriate.
Ok.
My analysis is that fixing libpq behavior is not in the scope of a psql
patch, and that if I was to
Hello Justin,
It seems that the v31 patch does not apply anymore:
postgresql> git apply
~/v31-0001-Document-historic-behavior-of-links-to-directori.patch
error: patch failed: doc/src/sgml/func.sgml:27410
error: doc/src/sgml/func.sgml: patch does not apply
--
Fabien.
Hello Peter,
I finally took some time to look at this.
Attached v11 is a rebase.
This patch still has a few of the problems reported earlier this year.
In [0], it was reported that certain replication commands result in infinite
loops because of faulty error handling. This still happens.
Pushed with that change and some others, notably:
Thanks for the improvements and the push!
--
Fabien.
Hello,
They are not more nor less relevant than any other "random" constant. The
state needs a default initialization. The point of using DK's is that it
is somehow cannot be some specially crafted value which would have some
special property only know to the purveyor of the constant and could
Hello Tom,
Thanks for the feedback.
+/* use Donald Knuth's LCG constants for default state */
How did Knuth get into this? This algorithm is certainly not his,
so why are those constants at all relevant?
They are not more nor less relevant than any other "random" constant. The
state
Hello Daniel,
This patch still has a few of the problems reported earlier this year.
The patch fails to apply and the thread seems to have taken a nap.
I'm not napping:-) I just do not have enough time available this month. I
intend to work on the patch in the next CF (January). AFAICR
Hello Tom,
As I threatened in another thread, I've looked through all of the
oldest commitfest entries to see which ones should maybe be tossed,
on the grounds that they're unlikely to ever get committed so we
should stop pushing them forward to the next CF.
psql - add SHOW_ALL_RESULTS
Hello Peter,
Attached v9 integrates your tests and makes them work.
Attached v11 is a rebase.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index b52d187722..0cf4a37a5f 100644
---
I guess the declaration needs PGDLLIMPORT.
Indeed, thanks!
Attached v16 adds that.
--
Fabien.diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index d19f73127c..b250ae912b 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@
Attached v15 also does call srandom if it is there, and fixes yet another
remaining random call.
I think that I have now removed all references to "random" from pg source.
However, the test still fails on windows, because the linker does not find
a global variable when compiling
Just FTR, I strongly object to your removal of process-startup srandom()
calls.
Ok. The point of the patch is to replace and unify the postgres underlying
PRNG, so there was some logic behind this removal.
FTR, this was triggered by your comment on Jul 1:
[...] I see that you probably
Hello Tom,
Just FTR, I strongly object to your removal of process-startup srandom()
calls.
Ok. The point of the patch is to replace and unify the postgres underlying
PRNG, so there was some logic behind this removal.
Those are not only setting the seed for our own use, but also ensuring
[1]: http://cfbot.cputube.org/
Indeed. I wish that these results would be available from the cf
interface.
Attached a v11 which might improve things.
Not enough. Here is a v12 which might improve things further.
Not enough. Here is a v13 which might improve things further more.
Not
[1]: http://cfbot.cputube.org/
Indeed. I wish that these results would be available from the cf interface.
Attached a v11 which might improve things.
Not enough. Here is a v12 which might improve things further.
Not enough. Here is a v13 which might improve things further more.
--
Hello again,
Just wanted to let you know that cfbot [1] doesn't seem to be happy with
the patch. Apparently, some tests are falling. To be honest, I didn't
invest too much time into investigating this. Hopefully, it's not a big
deal.
[1]: http://cfbot.cputube.org/
Indeed. I wish that these
Hallo Peter,
It turns out that your v8 patch still has the issue complained about in [0].
The issue is that after COMMIT AND CHAIN, the internal savepoint is gone, but
the patched psql still thinks it should be there and tries to release it,
which leads to errors.
Indeed. Thanks for the
Hello Aleksander,
Attached a v10 which is some kind of compromise where the interface uses
inclusive min and max bounds, so that all values can be reached.
Just wanted to let you know that cfbot [1] doesn't seem to be happy with
the patch. Apparently, some tests are falling. To be honest, I
Hello Fujii-san,
Stop counting skipped transactions under -T as soon as the timer is
exceeded. Because otherwise it can take a very long time to count all of
them especially when quite a lot of them happen with unrealistically
high rate setting in -R, which would prevent pgbench from ending
1 - 100 of 1293 matches
Mail list logo
