Hello David,
Attached is an attempt at improving things. I have added a explicit note
and hijacked an existing example to better illustrate the purpose of the
function.
This patch does not build on Linux due to some unused functions and
variables:
Bonjour Michaël,
Except for the addition of a test case to skip empty results when
\aset is used, I think that we are pretty good here.
While hacking on the patch more by myself, I found that mixing tests
for \gset and \aset was rather messy. A test for an empty result
leads also to a
Hello,
FWIW, I don't think so. Generally a trailing backspace is an escape
character for the following newline. And '\ ' is a escaped space,
which is usualy menas a space itself.
In this case escape character doesn't work generally and I think it is
natural that a backslash in the middle
Hello,
As I wrote about an earlier version of the patch, ISTM that instead of
reinventing, extending, adapting various ls variants (with/without
metadata, which show only files, which shows target of links, which shows
directory, etc.) we would just need *one* postgres "ls" implementation
Hello Justin,
Well, the following comment says "ignore anything but regular files",
so I'm supposing that that is the behavior that we actually want here
and failed to implement correctly. There might be scope for
additional directory-reading functions, but I'd think you'd want
more
Hello Andres,
That being the case, I'd think a better design principle is "make your
new code look like the code around it", which would tend to weigh against
introducing StringInfo uses into pgbench when there's none there now and
a bunch of PQExpBuffer instead. So I can't help thinking the
Hello Tom,
I cannot say that I "want" to fix something which already works the same
way, because it is against my coding principles. [...]
I counted nearly 3500 calls under src/bin.
Yeah, that's the problem. If someone does come forward with a patch to do
that, I think it'd be summarily
Hello Tom,
Thanks for your feedback,
I'd be rather unclear about what the actual feedback is, though. I'd
interpret it as "pg does not care much about code coverage". Most clients
are in the red on coverage.postgresql.org. I'd like pgbench at least to be
in the green, but it does not look
Ok. I find it strange to mix PQExpBuffer & StringInfo in the same file.
Agreed, but we'd rather use StringInfo going forward. However, I don't think
that puts you on the hook for updating all the PQExpBuffer references.
Unless you want to...
I cannot say that I "want" to fix something
Assuming there's no one willing to fix the behavior (and that seems
unlikely given we're in the middle of a 2020-01 commitfest) it makes
sense to at least document the behavior.
That being said, I think the proposed patch may be a tad too brief. I
don't think we need to describe the exact
This patch is registered in 2020-01, but the last message in the thread
seems to be from 2019/05/23. The patch itself seems to be OK (it applies
fine etc.) What do we need to get it over the line, instead of just
moving it to the next one CF over and over?
It does not look like the remainder
Patch v5 is a rebase with some adjustements.
This patch is failing on the Windows build:
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.85698
I'm not sure if this had been fixed in v3 and this is a new issue or if it
has been failing all along. Either way, it should
Hello David,
I'd prefer not to expand the use of pqexpbuffer in more places, and
instead rather see this use StringInfo, now that's also available to
frontend programs.
Franckly, one or the other does not matter much to me.
FWIW, I agree with Andres with regard to using StringInfo.
Ok.
Bonjour Michaël,
[...] Still sounds strange to me to invent a new variable to this
structure if it is possible to track the exact same thing with an
existing part of a Command, or it would make sense to split Command into
two different structures with an extra structure used after the
Hello Justin,
thanks for the feedback.
- Records cannot be continued across lines.
+ Records can be backslash-continued across lines.
Maybe say: "lines ending with a backslash are logically continued on the next
line", or similar.
I tried to change it along that.
Since it puts a
Hello,
After writing an unreadable and stupidly long line for ldap
authentification in a "pg_hba.conf" file, I figured out that allowing
continuations looked simple enough and should just be done.
Patch attached.
--
Fabien.diff --git a/doc/src/sgml/client-auth.sgml
About v13, seens as one patch:
Function "pg_ls_dir_metadata" documentation suggests a variable number of
arguments with brackets, but parameters are really mandatory.
postgres=# SELECT pg_ls_dir_metadata('.');
ERROR: function pg_ls_dir_metadata(unknown) does not exist
LINE 1: SELECT
Hello Justin,
psql> SELECT * FROM pg_ls_dir_recurse('.');
ERROR: could not stat file
About v11, ISTM that the recursive function should check for symbolic
links and possibly avoid them:
sh> cd data/base
sh> ln -s .. foo
psql> SELECT * FROM pg_ls_dir_recurse('.');
ERROR: could not stat file
Hello Justin,
Some feedback on v10:
All patches apply cleanly, one on top of the previous. I really wish there
would be less than 9 patches…
* v10.1 doc change: ok
* v10.2 doc change: ok, not sure why it is not merged with previous
* v10.3 test add: could be merge with both previous
Hello Thomas,
Thank you very much! I'm going to send a new patch set until the end of
this week (I'm sorry I was very busy in the release of Postgres Pro
11...).
Is anyone interested in rebasing this, and summarising what needs to
be done to get it in? It's arguably a bug or at least quite
Hello Justin,
Patch series applies cleanly. The last status compiles and passes "make
check". A few more comments:
* v8.[123] ok.
* v8.4
Avoid using the type name as a field name? "enum dir_action dir_action;"
-> "enum dir_action action", or maybe rename "dir_action" enum
It seems that lists are used as FIFO structures by appending, fetching &
deleting last, all of which are O(n). ISTM it would be better to use the
head of the list by inserting, getting and deleting first, which are O(1).
I think you're referring to linked lists, but pglists are now arrays,
Hello Justin,
Some feedback about the v7 patch set.
About v7.1, seems ok.
About v7.2 & v7.3 seems ok, altought the two could be merged.
About v7.4:
The documentation sentences could probably be improved "for for", "used
... used". Maybe:
For the temporary directory for tablespace, ...
Hallo Andres,
Slight aside: Have you ever looked at moving pgbench to non-blocking
connection establishment? It seems weird to use non-blocking everywhere
but connection establishment.
Attached an attempt at doing that, mostly done for fun. It seems to be a
little slower on a local socket.
Hello Andres,
I've changed the performance calculations depending on -C or not. Ramp-up
effects are smoothed.
I've merged all time-related stuff (time_t, instr_time, int64) to use a
unique type (pg_time_usec_t) and set of functions/macros, which simplifies
the code somehow.
Hm. I'm not
Hello Andres,
Slight aside: Have you ever looked at moving pgbench to non-blocking
connection establishment? It seems weird to use non-blocking everywhere
but connection establishment.
Nope. If there is some interest, why not. The reason for not doing it is
that the typical use case is just
Bonjour Michaël,
All the tools mentioned in $subject have been switched recently to use
the central logging infrastructure, which means that they have gained
coloring output. However we (mostly I) forgot to update the docs.
Attached is a patch to fix this issue. Please let me know if there
Hello Andres,
FWIW, leaving windows, error handling, and other annoyances aside, this
can be implemented fairly simply. See below.
Attached an attempt at improving things.
I've put 2 barriers: one so that all threads are up, one when all
connections are setup and the bench is ready to go.
Hello Kyotaro-san,
I think this also shows that "including/excluding connections
establishing" as well as some of the other stats reported pretty
bogus. In the 'before' case a substantial numer of the connections had
not yet been established until the end of the test run!
I see it useful.
Hello Andres,
Therefore I'd like to make pgbench wait till it has established all
connections, before they run queries.
Does anybody else see this as being useful?
Yes, I think that having this behavior available would make sense.
If so, should this be done unconditionally?
Dunno. I
+1 one for this change, it's something I also add to every .psqlrc I setup.
So.. We have:
+1: Vik, Ian, Daniel, Alvaro, Christoph
+-0: Tom (?), Fabien (?)
I did not know I had a vote. I'm "+1" on this change, if that matters.
Just this morning I had a case where I wished I had the
Hello Tomas,
This patch was marked as ready for committer, but clearly there's an
ongoin discussion about what should be the default behavoir, if this
breaks existing apps etc. So I've marked it as "needs review" and moved
it to the next CF.
The issue is that root (aka Tom) seems to be
Hello Masahiko-san,
I've started a new separate thread from the previous long thread[1]
for internal key management system to PostgreSQL. As I mentioned in
the previous mail[2], we've decided to step back and focus on only
internal key management system for PG13. The internal key management
Hello Alvaro,
I read the whole thread, I still don't know what this patch is supposed to
do. I know what the words in the subject line mean, but I don't know how
this helps a pgbench user run better benchmarks. I feel this is also the
sentiment expressed by others earlier in the thread. You
Hello Peter,
This patch was marked as RFC on 2019-03-30, but since then there have
been a couple more issues pointed out in a review by Thomas Munro, and
it went through 2019-09 and 2019-11 without any attention. Is the RFC
status still appropriate?
Thomas review was about
Hello Vik,
Isn't there examples in the documentation which use the default prompts?
If so, should they be updated accordingly?
Good catch!
I thought about the documentation but not the examples therein.
Updated patch attached.
Ok.
Only one transaction prompt example in the whole
Hello Vik,
I cannot ever think of a time when I don't want to know if I'm in a
transaction or not (and what its state is). Every new setup I do, I add
%x to the psql prompt.
I think it should be part of the default prompt. Path attached.
Isn't there examples in the documentation which
Hello,
+1 on the idea.
By quickly looking at the patch, I notice that there are no tests.
Is it possible to emulate somthing without the actual hardware, at least
for testing purposes?
--
Fabien.
My own vote would be to use the initial patch (after applying any
unrelated changes per later review), ie. add the feature with its
disable button.
I can do that, but not if there is a veto from Tom on the feature.
I wish definite negative opinions by senior committers would be expressed
Hello Tom,
This is one of the patches already marked as RFC (since September by
Alvaro). Anyone interested in actually pushing it, so that it does not
fall through to yet another commitfest?
TBH, I think we'd be better off to reject it. This makes a nontrivial
change in a very
Hello Karl,
New patch attached: doc_base64_v13.patch
This required surprisingly little re-wording.
Added word "binary" into the descriptions of convert(),
substring(), convert_from(), and convert_to().
I also added data types to the call syntax of set_bit()
and set_byte().
And this patch
Hello Justin,
I'm trying to think about how to get rid of the strange structure and hacks,
and the arbitrary looking size 2 array.
Also the recursion is one step, but I'm not sure why, ISTM it could/should
go on always?
Because tmpfiles only go one level deep.
I'm not sure it is a
Hello Justin,
About this v4.
It applies cleanly.
I'm trying to think about how to get rid of the strange structure and
hacks, and the arbitrary looking size 2 array.
Also the recursion is one step, but I'm not sure why, ISTM it could/should
go on always?
Maybe the recursive
Patch applies and compiles.
Given that the paragraph begins with "Plain VACUUM (without FULL)", it is
better to have the VACUUM FULL explanations on a separate paragraph, and the
The original patch does that (Fabien agreed when I asked off list)
Indeed. I may have looked at it in reverse,
Michaël,
So I think that the current situation is a good thing at least for debug.
If you look at some of my messages on other threads, you would likely
notice that my mood of the day is to not design things which try to
outsmart a user's expectations :)
I'm not following you.
ISTM that
Bonjour Michaël,
TBH, my recommendation would be to drop *all* of these likely()
and unlikely() calls. What evidence have you got that those are
meaningfully improving the quality of the generated code? And if
they're buried inside macros, they certainly aren't doing anything
useful in terms
Patch v4 is a just a rebase.
Patch v5 is a rebase with some adjustements.
--
Fabien.diff --git a/src/bin/pgbench/Makefile b/src/bin/pgbench/Makefile
index f402fe7b91..e1d3ef9bb3 100644
--- a/src/bin/pgbench/Makefile
+++ b/src/bin/pgbench/Makefile
@@ -10,6 +10,7 @@ include
Bonjour Michaël,
I don't follow what you mean by that.
The first versions of the patch did not change syntax_error(), and the
version committed has switched to use PQExpBufferData there. I think
that we should just do the same for the debug logs executing the meta
commands. This way, we
Hello Karl,
Another option would be to use "bytes bytea".
(The current patch uses "string bytea".)
This would probably also require some re-wording throughout.
Please let me know your preference.
I like it, but this is only my own limited opinion, and I'm not a native
English
Patch v6 makes syntax error location code more compact and tests the case.
Committed.
Thanks!
--
Fabien.
Patch v5 attached tries to follow your above suggestions.
Patch v6 makes syntax error location code more compact and tests the case.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index a1e0663c8b..cfb5110608 100644
--- a/src/bin/pgbench/pgbench.c
+++
Bonjour Michaël,
I think that I would just remove the "debug" variable defined in
pgbench.c all together, and switch the messages for the duration and the
one in executeMetaCommand to use info-level logging..
Ok, done.
Thanks. The output then gets kind of inconsistent when using --debug:
Hello Merlin,
For low-level arithmetic code like this one, with tests and loops
containing very few hardware instructions, I think that helping compiler
optimizations is a good idea.
Do you have any performance data to back that up?
Yep.
A generic data is the woes about speculative
Hello Robert,
if (arg1 == PG_INT32_MIN)
if (arg2 == 0 || arg2 == PG_INT32_MIN)
And possibly a "likely" on the while.
I don't think decoration the code with likely() and unlikely() all
over the place is a very good idea.
Odds are good that we'll end up with a bunch that are actually
Bonjour Michaël,
Without looking at the context I thought that argv[0] was the program name,
which is not the case here. I put it back everywhere, including the DEBUG
message.
The variable names in Command are confusing IMO...
Somehow, yes. Note that there is a logic, it will indeed be the
Hello Karl,
Attached is doc_base64_v11.patch
Patch applies cleanly and compiles.
I'm in favor of moving and reorganizing these function descriptions, as
they are somehow scattered with a unclear logic when you are looking for
them.
+ bytea ||
+bytea
bytea
This patch was marked as RFC on 2019-03-30, but since then there have
been a couple more issues pointed out in a review by Thomas Munro, and
it went through 2019-09 and 2019-11 without any attention. Is the RFC
status still appropriate?
Thomas review was about comments/documentation
Hello Vik,
Add unlikely() where appropriate.
Any particular place in mind where I didn't already put it?
In GCD implementations, for instance:
if (arg1 == PG_INT32_MIN)
if (arg2 == 0 || arg2 == PG_INT32_MIN)
And possibly a "likely" on the while.
In LCM implementations, for instance:
Bonjour Vik,
Here is an updated patch fixing that.
As I said, welcome to arithmetic:-)
Patch v5 applies cleanly.
Doc: I'd consider using an example the result of which is 42 instead of
21, for obvious geek motivations. Possibly gcd(2142, 462) should be ok.
I got it wrong with my
Hello Tom,
Which architecture has single cycle division? I think it's way above
that, based on profiles I've seen. And Agner seems to back me up:
https://www.agner.org/optimize/instruction_tables.pdf
That lists a 32/64 idiv with a latency of ~26/~42-95 cycles, even on a
moder uarch like
.
--
Fabien Coelho - CRI, MINES ParisTech
Bonsoir Vik,
+int4gcd_internal(int32 arg1, int32 arg2)
+{
+ int32 swap;
+
+ /*
+* Put the greater value in arg1.
+* This would happen automatically in the loop below, but avoids an
+* expensive modulo simulation on some architectures.
+*/
Hello Peter,
I took another crack at this. Attached is a new patch that addresses
the semantic comments from this and the other thread. It's all a bit
tricky, comments welcome.
It seems that this patch does not apply anymore after Tom's 5815696.
--
Fabien.
Hello Peter,
The documentation and pgbench --help output that accompanied this patch
claims that the argument to pgbench --partition-method is optional and
defaults to "range", but that is not actually the case, as the
implementation requires an argument. Could you please sort this out?
Hello Peter,
Compared to v1 I have also made a few changes to be more consistent when
using fatal/error/info.
The renaming of debug to debug_level seems unnecessary and unrelated.
Indeed. It was when I used "debug" as a shorthand for "pg_log_debug".
In runShellCommand(), you removed some
Normally gcd() returns a positive integer, and gcd(a,0) = gcd(a,a) =
abs(a). But since abs(INT_MIN) cannot be represented as a 32-bit
integer, both those cases should throw an integer-out-of-range error.
I'm also in favor of that option, rather than sending a negative result as
a result.
Hello Peter,
The patch seems pretty straightforward, but this
+/*
+ * Convenient shorcuts
+ */
+#define fatal pg_log_fatal
+#define error pg_log_error
+#define warning pg_log_warning
+#define info pg_log_info
+#define debug pg_log_debug
seems counterproductive. Let's just use the normal
Bonjour Michaël, et excellente année 2020 !
Hmm. Wouldn't it make sense to output the log generated as
information from the test using pg_log_info() instead of using
fprintf(stderr) (the logs of the initial data load, progress report)?
For the progress report, the reason I decided against
Hello Tom,
I do not think it is a good idea, because help output is quite large,
there are many of them, and we should certainly not want it stored
repeatedly in output files for diffs.
Hm, I don't follow --- we are most certainly not going to exercise
\help for every possible SQL keyword,
That is what my patch does: it tests prompts, tab completion, help,
command options… and I added tests till I covered most psql source.
Well, I think that where possible we ought to test using the existing
test infrastructure -- help, for example, seems like it could perfectly
well be
I'm not fan of relying on the configure stuff ("with_readline"), in my
Expect version I tested if history capabilities are available from psql
itself.
No, I disagree with that. If configure thinks it built with readline,
and then the actual binary acts like it doesn't have readline, that's
a
Hello Tom,
If you have to install IO::Pty anyway, ISTM you can also install Expect.
My point is precisely that buildfarm owners *won't* have to install
IO::Pty; it comes in a default Perl install almost everywhere.
I'm afraid that's not true of Expect.
Hmmm. That is a good argument.
Now
Hello,
Because I do not trust C modulo as I had a lot of problems with it?:-)
If I recall correctly (and I'm traveling and away from those notes),
the exact semantics of C's % with negative operands was left
implementation-defined until, was it, C99 ?
Indeed, my woes with C % started
Bonjour Vik,
Should there be a NUMERIC version as well? I'd say maybe yes.
I thought about that, too, but also decided against it for this patch.
Hmmm. ISTM that int functions are available for numeric?
I'm wondering what it should do on N, 0 and 0, 0. Raise an error?
Return 0? Return 1?
Hello Tom,
We've often talked about the problem that we have no regression test
coverage for psql's tab completion code. I got interested in this
issue while messing with the filename completion logic therein [1],
so here is a draft patch that adds some testing for that code.
After you
Hello Tom,
We've often talked about the problem that we have no regression test
coverage for psql's tab completion code. I got interested in this
issue while messing with the filename completion logic therein [1],
so here is a draft patch that adds some testing for that code.
This is just
Bonsoir Vik,
I recently came across the need for a gcd function (actually I needed
lcm) and was surprised that we didn't have one.
Why not.
So here one is, using the basic Euclidean algorithm. I made one for
smallint, integer, and bigint.
Should pg provide the LCM as well? Hmmm,
Hello Justin,
Ok, so this suggests recursing into subdirs, which requires to make a
separate function of the inner loop.
Yea, it suggests that; but, SRF_RETURN_NEXT doesn't make that very easy.
It'd need to accept the fcinfo argument, and pg_ls_dir_files would call it once
for every tuple to
Hello Justin,
Why not simply showing the files underneath their directories?
/path/to/tmp/file1
/path/to/tmp/subdir1/file2
In which case probably showing the directory itself is not useful,
and the is_dir column could be dropped?
The names are expected to look like this:
$ sudo find
Re-added -hackers.
Indeed, I left it out by accident. I tried to bounce the original mail but
it did not work.
Thanks for reviewing.
On Fri, Dec 27, 2019 at 05:22:47PM +0100, Fabien COELHO wrote:
The implementation simply extends an existing functions with a boolean to
allow for sub
Hello Justin,
I started writing this patch to avoid the possibly-misleading phrase: "with no
extra space" (since it's expected to typically take ~2x space, or 1x "extra"
space).
But the original phrase "with no extra space" seems to be wrong anyway, since
it actually follows fillfactor, so
On Tue, 17 Sep 2019, Alvaro Herrera wrote:
Indeed, that seems like a problem, and it's a good question. You can
see this on unpatched master with SELECT x.filler FROM
(pgbench_tellers AS t JOIN b USING (bid)) AS x.
I'm not sure I understand why that problem is a blocker for this patch.
Bonjour Michaël,
the syscache mainly, so instead the attached patch does the following
changes:
- amapi.h -> indexam.h
- amapi.c -> indexamapi.c. Here we have an equivalent in access/table/
as tableamapi.c.
- amvalidate.c -> indexamvalidate.c
- amvalidate.h -> indexamvalidate.h
- genam.c ->
Bonjour Michaël, hello devs,
As suggested in "cce64a51", this patch make pgbench use postgres logging
capabilities.
I tried to use fatal/error/warning/info/debug where appropriate.
Some printing to stderr remain for some pgbench specific output.
The patch fixes a inconsistent test case
Hello Tom,
My 0.02 €:
Given the rather small number of existing uses of CancelRequested,
I wonder if it wouldn't be a better idea to rename it to cancel_pressed?
I prefer the former because it is more functional (a cancellation has been
requested, however the mean to do so) while "pressed"
I've not dug into code itself, I just bisected it.
Thanks for the report. I'll look into it.
Looked at it already.
Ah, the magic of timezones!
And yes, I can see the difference. This comes from the switch from
cancel_pressed to CancelRequested in psql, especially PSQLexecWatch() in
explain (analyze) select * from pgbench_accounts \watch 1
It behaves as expected. But once I break out of the loop with ctrl-C, then
if I execute the same thing again it executes the command once, but shows
no output and doesn't loop. It seems like some flag is getting set with
ctrl-C, but
Hello Craig,
New users frequently attempt to run PostgreSQL's command line utilities
from the psql prompt.
Alas, that is true.
I also have the reverse, i.e. SQL commands fed to bash, which does not
like it much.
They tend to be confused when this appears to do absolutely nothing:
Attached v4.
Patch applies cleanly, compiles, works for me. Put it back to ready.
--
Fabien.
Bonjour Michaël,
Committed the patch after splitting things into two commits and after
testing things from Linux and from a Windows console: the actual
refactoring and the pgbench changes.
I have found that we have a useless declaration of CancelRequested in
common.h, which is already part
Another question I have is why doing only that for the data
initialization phase? Wouldn't it make sense to be consistent with the
other tools having --progress and do the same dance in pgbench's
printProgressReport()?
I thought of it but did not suggest it.
When running a bench I like
The below addition can be removed, it seems to be duplicate:
Indeed. I'm unsure how this got into the patch, probably some rebase mix-up.
Attached v7 removes the duplicates.
Attached patch v8 is a rebase.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out
I wrote the v1 patch on CentOS Linux, and now on MacOS. It would be
great if someone can volunteer to test on Windows terminal.
I do not have that.
Attached v3.
Patch applies, compiles, works for me. No further comments.
I switched the patch as ready.
--
Fabien.
Hello Amit,
I have updated the patch based on these observations. Attached v2.
Patch v2 applies & compiles cleanly, works for me.
I'm not partial to Hungarian notation conventions, which is not widely
used elsewhere in pg. I'd suggest eolchar -> eol or line_end or whatever,
but others
I wonder why we don't use the same style for $subject as pg_basebackup
--progress, that is, use a carriage return instead of a newline after
each line reporting the number of tuples copied?
Patch applies cleanly, compiles, and works for me.
My 0.02€:
fprintf -> fputs or fputc to avoid a
Michaël,
+ boolaset;
It seems to me that there is no point to have the variable aset in
Command because this structure includes already MetaCommand, so the
information is duplicated. [...] Perhaps I am missing something?
Yep. ISTM that you are missing that aset is not an
Bonjour Michaël,
The query cancellation added to pgbench is different than the actual
refactoring, and it is a result of the refactoring, so I would rather
split that into two different commits for clarity. The split is easy
enough, so that's fine not to send two different patches.
Yep,
Hello Amit,
I wonder why we don't use the same style for $subject as pg_basebackup
--progress, that is, use a carriage return instead of a newline after
each line reporting the number of tuples copied?
Why not.
Attached patch for that.
I'll look into it. Could you add it to the CF app?
Michaël,
Not this round.
You have registered yourself as a reviewer of this patch since the end
of June. Could you please avoid that? Sometimes people skips patches
when they see someone already registered to review it.
Yep. ISTM that I did a few reviews on early versions of the patch,
401 - 500 of 1293 matches
Mail list logo
