Re: [HACKERS] Variable substitution in psql backtick expansion
Hello Tom & Michaël, I think this patch should be rejected. +1 for rejection [...] The noes have it! Note that the motivation was really symmetric completion: fabien=# \echo :VERSION_NAME 11devel fabien=# \echo :VERSION_NUM 11 fabien=# \echo :VERSION PostgreSQL 11devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit fabien=# \echo :SERVER_VERSION_NAME 10.1 fabien=# \echo :SERVER_VERSION_NUM 11 But fabien=# \echo :SERVER_VERSION :SERVER_VERSION To get it into a variable the work around is really: fabien=# SELECT version() AS "SERVER_VERSION" \gset fabien=# \echo :SERVER_VERSION PostgreSQL 10.1 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit -- Fabien -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add some const decorations to prototypes
Hello Tom, ISTM That there is still at least one strange cast: +static const char **LWLockTrancheArray = NULL; + LWLockTrancheArray = (const char **) // twice These are not cases of "cheating". This is just the return value of a memory allocation function being cast from void * to the appropriate result type. That is an orthogonal style decision that I have maintained in these cases. Would it make sense that the function returns "const void *", i.e. the cast is not on the const part but on the pointer type part? Certainly not -- if malloc-like functions returned "const void *" then you could never write on allocated space without having casted away const. Ok. Sure. LWLockTrancheArray = (char **) MemoryContextAllocZero(TopMemoryContext, LWLockTranchesAllocated * sizeof(char *)); and the reader can see by inspection that the calculation of how much to allocate (so many char-* items) is consistent with the char-** result. It is not necessary to go find the declaration of LWLockTrancheArray and verify that it's char **, because we trust the compiler to whine if char ** isn't assignment-compatible with that. But if we left off the cast and just assigned the function result directly to the variable, then there would be nothing directly tying the variable's type to this allocation computation. Thanks for the reflesher course about the intricacies of "const". After your explanation, and on third thoughts, ISTM that the assignment should not include "const" in the explicit cast, i.e., use extern void * msg_func(void); const char * msg = (char *) msg_func(); The variable or field is constant, not what the function returns, so const char * msg = (const char *) msg_func(); does not really make full sense to me, and moreover the compiler does not complain without the const. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: psql command \graw
Hello, Maybe I'm missing something, but it looks that it could be made to work without adding another boolean. The tuples only cannot be disabled, because then other parts print number of rows postgres=# \pset format unaligned Output format is unaligned. postgres=# select 10 as a, 20 as b; a|b 10|20 (1 row) < Argh. Too bad. I'm not at ease with having two bools which nearly mean the opposite one of the other but not exactly... however I'm not sure that there is a simpler way out of this, some exception handling is needed one way or the other, either within the header or within the footer... Maybe the whole topt logic should be reviewed, but that is not the point of this patch. So I switched the patch to "ready for committer". -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add some const decorations to prototypes
Would it make sense that the function returns "const void *", i.e. the cast
is not on the const part but on the pointer type part?
Or maybe you do not really need a cast, the following code does not
generate any warning when compiled with clang & gcc.
#include
// const void * would be ok as well
void * msg_fun(void)
{
return "hello world";
}
int main(void)
{
const char * msg = msg_fun();
printf("message: %s\n", msg);
return 0;
}
Or basically all is fine, I'm just nitpicking for nothing, shame on me.
As I said, I rather like more precise declarations.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add some const decorations to prototypes
ISTM That there is still at least one strange cast: +static const char **LWLockTrancheArray = NULL; + LWLockTrancheArray = (const char **) // twice These are not cases of "cheating". This is just the return value of a memory allocation function being cast from void * to the appropriate result type. That is an orthogonal style decision that I have maintained in these cases. Ok. I'm at the limit of my C abilities. Your answer is about void * vs char *, I'm okay with that. My question was about no const / const in the same operation. Would it make sense that the function returns "const void *", i.e. the cast is not on the const part but on the pointer type part? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: psql command \graw
ISTM that you can remove "force_column_header" and just set "tuple_only" to what you need, that is you do not need to change anything in function "print_unaligned_text". Last point is not possible - I would not to break original tuple only mode. Hmmm... I do not understand. I can see only one use of force_column_header in the function: - if (!opt_tuples_only) + if (!opt_tuples_only || opt_force_column_header) So I would basically suggest to do: my_popt.topt.tuples_only = !pset.g_raw_header; in the driver. Looking at the detailed code in that function, probably you need to set start_table to on when headers are needed and stop_table to off for the raw mode anyway? Maybe I'm missing something, but it looks that it could be made to work without adding another boolean. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: psql command \graw
Hello Pavel, I hope so I fixed all mentioned issues. Patch applies with a warning: > git apply ~/psql-graw-2.patch /home/fabien/psql-graw-2.patch:192: new blank line at EOF. + warning: 1 line adds whitespace errors. Otherwise it compiles. "make check" ok. doc gen ok. Two spurious empty lines are added before StoreQueryTuple. Doc: "If + is appended to the command name, a column names are displayed." I suggest instead: "When + is appended, column names are also displayed." ISTM that you can remove "force_column_header" and just set "tuple_only" to what you need, that is you do not need to change anything in function "print_unaligned_text". -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pow support for pgbench
I don't want to go too deep into it, but you get stuff like this: Select pow(2.0, -3)::text = pow(2, -3)::text; Sure. It does so with any overloaded operator or function: fabien=# SELECT (2.0 + 3)::TEXT = (2 + 3)::TEXT; # f Patch applies, make check ok in pgbench, doc gen ok. ipow code is nice and simple. I switched the patch to "Ready for Committer" Let's now hope that a committer gets around to consider these patch some day. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pow support for pgbench
Hello,
Sorry for the confusion, I wasn't aware that SQL pow changed types
depending on the input value.
Indeed, this is quite strange...
fabien=# SELECT i, POW(2, i) FROM generate_series(-2, 2) AS i;
-2 | 0.25
-1 | 0.5
0 | 1
1 | 2
2 | 4
I've modified the function to match more closely the behaviour of SQL,
except that 0^(negative) returns 'double inf'. Do you think there is any
value in raising an error instead?
fabien=# SELECT POW(0,-1);
ERROR: zero raised to a negative power is undefined
H... I'm fine with double inf, because exception in pgbench means the
end of the script, which is not desirable for benchmarking purposes.
I think that:
- you can simplify the ipow function by removing handling of y<0 case,
maybe add an assert to be sure to avoid it.
- you should add more symmetry and simplify the evaluation:
if (int & int)
{
i1, i2 = ...;
if (i2 >= 0)
setIntValue(retval, ipow(i1, i2));
else
// conversion is done by C, no need to coerce again
setDoubleValue(retval, pow(i1, i2));
}
else
{
d1, d2 = ...;
setDoubleValue(retval, pow(d1, d2));
}
Add a test case to show what happens on NULL arguments, hopefully the
result is NULL.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pow support for pgbench
Hello Raúl, I've fixed the documentation and added an ipow function that handles both positive and negative ints, having 0^0 == 1 and 0^(negative) == PG_INT64_MAX since that's what my glibc math.h pow() is returning. From the comment: * For exp < 0 return 0 except when the base is 1 or -1 I think that it should do what POW does in psql, i.e.: fabien=# SELECT POW(2, -2); # 0.25 that is if exp < 0 the double version should be used, it should not return 0. Basically the idea is that the pgbench client-side version should behave the same as the SQL version. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add some const decorations to prototypes
Just leave it as char*. If you change the endptr argument you're going to force every call site to change their return variable, and some of them would end up having to cast away the const on their end. OK, here is an updated patch with the controversial bits removed. I'm in general favor in helping compilers, but if you have to cheat. ISTM That there is still at least one strange cast: +static const char **LWLockTrancheArray = NULL; + LWLockTrancheArray = (const char **) // twice Maybe some function should return a "const char **", or the const is not really justified? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pow support for pgbench
Hello Raúl,
Sorry about the patch. Attaching it now so it can be considered as
submitted.
There is a typo in the XML doc:
1024.0/
Please check that the documentation compiles.
I'm at odds with having the integer version rely on a double pow(), even
if it works. I think that there should be a specific integer version which
does use integer operations. From stack overflow, the following is
suggested:
int ipow(int base, int exp)
{
int result = 1;
while (exp)
{
if (exp & 1)
result *= base;
exp >>= 1;
base *= base;
}
return result;
}
The integer version should be when x & y are integers *AND* y >= 0.
if y is a negative integer, the double version should be used.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench - allow to store select results into variables
Hello Pavel,
Here is a v13, which is just a rebase after the documentation xml-ization.
Here is a v14, after yet another rebase, and some comments added to answer
your new comments.
I am looking to this patch.
Not sure if "cset" is best name - maybe "eset" .. like embeded set?
I used c for "compound", because they compound SQL commands.
Now I do not have a very strong opinion, only that it should be some
letter which some logic followed by "set".
The variables and fields in the source currently use "compound"
everywhere, if this is changed they should be updated accordingly.
ISTM that the ";" is embedded, but the commands are compound, so
"compound" seems better word to me. However it is debatable.
If there some standard naming for the concept, it should be used.
The code of append_sql_command is not too readable and is not enough
commented.
Ok. I have added comments in the code.
I don't understand why you pass a param compounds to append_sql_command,
when this value is stored in my_command->compound from create_sql_command?
This is the number of compound commands in the "more" string. It must be
updated as well as the command text, so that the my_command text and
number of compounds is consistant.
Think of one initialization followed by two appends:
SELECT 1 AS x \cset
SELECT 2 \; SELECT 3 AS y \cset
SELECT 4 \; SELECT 5 \; SELECT 6 AS z \gset
In the end, we must have the full 6 queries
"SELECT 1 AS x \; SELECT 2 \; SELECT 3 AS y \; SELECT 4 \; SELECT 5 \; SELECT 6 AS
z"
and know that we want to set variables from queries 1, 3 and 6 and ignore
the 3 others.
Or maybe some unhappy field or variable names was chosen.
It seems ok to me. What would your suggest?
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index e509e6c..44e8896 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -818,6 +818,51 @@ pgbench options d
+
+
+ \cset [prefix] or
+ \gset [prefix]
+
+
+
+
+ These commands may be used to end SQL queries, replacing a semicolon.
+ \cset replaces an embedded semicolon (\;) within
+ a compound SQL command, and \gset replaces a final
+ (;) semicolon which ends the SQL command.
+
+
+
+ When these commands are used, the preceding SQL query is expected to
+ return one row, the columns of which are stored into variables named after
+ column names, and prefixed with prefix if provided.
+
+
+
+ The following example puts the final account balance from the first query
+ into variable abalance, and fills variables
+ one, two and
+ p_three with integers from a compound query.
+
+UPDATE pgbench_accounts
+ SET abalance = abalance + :delta
+ WHERE aid = :aid
+ RETURNING abalance \gset
+-- compound of two queries
+SELECT 1 AS one, 2 AS two \cset
+SELECT 3 AS three \gset p_
+
+
+
+
+
+\cset and \gset commands do not work when
+empty SQL queries appear within a compound SQL command.
+
+
+
+
+
\set varname expression
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d4a6035..32262df 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -384,12 +384,15 @@ static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
typedef struct
{
- char *line; /* text of command line */
+ char *line; /* first line for short display */
+ char *lines; /* full multi-line text of command */
int command_num; /* unique index of this Command struct */
int type; /* command type (SQL_COMMAND or META_COMMAND) */
MetaCommand meta; /* meta command identifier, or META_NONE */
int argc; /* number of command words */
char *argv[MAX_ARGS]; /* command word list */
+ int compound; /* last compound command (number of \;) */
+ char **gset; /* per-compound command prefix */
PgBenchExpr *expr; /* parsed expression, if needed */
SimpleStats stats; /* time spent in this command */
} Command;
@@ -1264,6 +1267,104 @@ getQueryParams(CState *st, const Command *command, const char **params)
params[i] = getVariable(st, command->argv[i + 1]);
}
+/* read all responses from backend */
+static bool
+read_response(CState *st, char **gset)
+{
+ PGresult *res;
+ int compound = 0;
+
+ while ((res = PQgetResult(st->con)) != NULL)
+ {
+ switch (PQresultStatus(res))
+ {
+ case PGRES_COMMAND_OK: /* non-SELECT commands */
+ case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */
+if (gset[compound] != NULL)
+{
+ fprintf(stderr,
+ "client %d file %d command %d compound %d: "
+ "\\gset expects a row\n",
+ st->id, st->use_file, st->command, compound);
+ st->ecnt++;
+ return false;
+}
+break; /* OK */
+
+ case PGRES_TUPLES_OK:
+if (gset[compound] != NULL)
+{
+ /* store
Re: [HACKERS] pgbench - use enum for meta commands
[ pgbench-enum-meta-2.patch ] Pushed with some trivial cosmetic adjustments (pgindent changed it more than I did). Ok. Thanks. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: pgbench - option to build using ppoll() for larger connection counts
Hello, Could you rebase the v11 patch? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench - use enum for meta commands
Updated version attached.
. Here is the patch. Sorry for the noise.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 5d8a01c..6bd3e52 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -373,11 +373,21 @@ typedef enum QueryMode
static QueryMode querymode = QUERY_SIMPLE;
static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
+typedef enum MetaCommand
+{
+ META_NONE,
+ META_SET,
+ META_SETSHELL,
+ META_SHELL,
+ META_SLEEP
+} MetaCommand;
+
typedef struct
{
char *line; /* text of command line */
int command_num; /* unique index of this Command struct */
int type; /* command type (SQL_COMMAND or META_COMMAND) */
+ MetaCommand meta; /* meta command identifier, if appropriate */
int argc; /* number of command words */
char *argv[MAX_ARGS]; /* command word list */
PgBenchExpr *expr; /* parsed expression, if needed */
@@ -1721,6 +1731,26 @@ evaluateExpr(TState *thread, CState *st, PgBenchExpr *expr, PgBenchValue *retval
}
}
+/* return meta-command enum identifier */
+static MetaCommand
+getMetaCommand(const char * cmd)
+{
+ MetaCommand mc;
+ if (cmd == NULL)
+ mc = META_NONE;
+ else if (pg_strcasecmp(cmd, "set") == 0)
+ mc = META_SET;
+ else if (pg_strcasecmp(cmd, "setshell") == 0)
+ mc = META_SETSHELL;
+ else if (pg_strcasecmp(cmd, "shell") == 0)
+ mc = META_SHELL;
+ else if (pg_strcasecmp(cmd, "sleep") == 0)
+ mc = META_SLEEP;
+ else
+ mc = META_NONE;
+ return mc;
+}
+
/*
* Run a shell command. The result is assigned to the variable if not NULL.
* Return true if succeeded, or false on error.
@@ -2214,7 +2244,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
fprintf(stderr, "\n");
}
- if (pg_strcasecmp(argv[0], "sleep") == 0)
+ if (command->meta == META_SLEEP)
{
/*
* A \sleep doesn't execute anything, we just get the
@@ -2240,7 +2270,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
}
else
{
- if (pg_strcasecmp(argv[0], "set") == 0)
+ if (command->meta == META_SET)
{
PgBenchExpr *expr = command->expr;
PgBenchValue result;
@@ -2259,7 +2289,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
break;
}
}
- else if (pg_strcasecmp(argv[0], "setshell") == 0)
+ else if (command->meta == META_SETSHELL)
{
bool ret = runShellCommand(st, argv[1], argv + 2, argc - 2);
@@ -2279,7 +2309,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
/* succeeded */
}
}
- else if (pg_strcasecmp(argv[0], "shell") == 0)
+ else if (command->meta == META_SHELL)
{
bool ret = runShellCommand(st, NULL, argv + 1, argc - 1);
@@ -3023,6 +3053,7 @@ process_sql_command(PQExpBuffer buf, const char *source)
my_command = (Command *) pg_malloc0(sizeof(Command));
my_command->command_num = num_commands++;
my_command->type = SQL_COMMAND;
+ my_command->meta = META_NONE;
initSimpleStats(_command->stats);
/*
@@ -3091,7 +3122,9 @@ process_backslash_command(PsqlScanState sstate, const char *source)
my_command->argv[j++] = pg_strdup(word_buf.data);
my_command->argc++;
- if (pg_strcasecmp(my_command->argv[0], "set") == 0)
+ my_command->meta = getMetaCommand(my_command->argv[0]);
+
+ if (my_command->meta == META_SET)
{
/* For \set, collect var name, then lex the expression. */
yyscan_t yyscanner;
@@ -3146,7 +3179,7 @@ process_backslash_command(PsqlScanState sstate, const char *source)
expr_scanner_offset(sstate),
true);
- if (pg_strcasecmp(my_command->argv[0], "sleep") == 0)
+ if (my_command->meta == META_SLEEP)
{
if (my_command->argc < 2)
syntax_error(source, lineno, my_command->line, my_command->argv[0],
@@ -3187,13 +3220,13 @@ process_backslash_command(PsqlScanState sstate, const char *source)
my_command->argv[2], offsets[2] - start_offset);
}
}
- else if (pg_strcasecmp(my_command->argv[0], "setshell") == 0)
+ else if (my_command->meta == META_SETSHELL)
{
if (my_command->argc < 3)
syntax_error(source, lineno, my_command->line, my_command->argv[0],
"missing argument", NULL, -1);
}
- else if (pg_strcasecmp(my_command->argv[0], "shell") == 0)
+ else if (my_command->meta == META_SHELL)
{
if (my_command->argc < 2)
syntax_error(source, lineno, my_command->line, my_command->argv[0],
@@ -3201,6 +3234,7 @@ process_backslash_command(PsqlScanState sstate, const char *source)
}
else
{
+ /* my_command->meta == META_NONE */
syntax_error(source, lineno, my_command->line, my_command->argv[0],
"invalid command", NULL, -1);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench - use enum for meta commands
The only thing I'm not quite sure about is a comment "which meta command ...". Maybe it's better to write it without question word, something like "meta command identifier..."? Ok. I agree. Updated version attached. I also added a const on a function parameter. Just a note about the motivation: I want to add the same "\if" syntax added to psql, but it requires to look at the meta command in a number of places to manage the automaton status, and the strcmp solution looked both ugly and inefficient. So this small refactoring is just a preliminary to the "\if" patch, some day, after this one get committed, if it gets committed. The new status of this patch is: Ready for Committer Thanks for the review. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pow support for pgbench
Hello Michaël, I'm fine with having pow in pgbench. I am adding as well Fabien in CC who worked in getting the internal function infrastructure in the shape it is now (waaay better) with commit 86c43f4. It might be even better if https://commitfest.postgresql.org/15/985/, which has been around for over one year (!), get through some day... -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench - allow to store select results into variables
Here is a v12.
Here is a v13, which is just a rebase after the documentation xml-ization.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index e509e6c..44e8896 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -818,6 +818,51 @@ pgbench options d
+
+
+ \cset [prefix] or
+ \gset [prefix]
+
+
+
+
+ These commands may be used to end SQL queries, replacing a semicolon.
+ \cset replaces an embedded semicolon (\;) within
+ a compound SQL command, and \gset replaces a final
+ (;) semicolon which ends the SQL command.
+
+
+
+ When these commands are used, the preceding SQL query is expected to
+ return one row, the columns of which are stored into variables named after
+ column names, and prefixed with prefix if provided.
+
+
+
+ The following example puts the final account balance from the first query
+ into variable abalance, and fills variables
+ one, two and
+ p_three with integers from a compound query.
+
+UPDATE pgbench_accounts
+ SET abalance = abalance + :delta
+ WHERE aid = :aid
+ RETURNING abalance \gset
+-- compound of two queries
+SELECT 1 AS one, 2 AS two \cset
+SELECT 3 AS three \gset p_
+
+
+
+
+
+\cset and \gset commands do not work when
+empty SQL queries appear within a compound SQL command.
+
+
+
+
+
\set varname expression
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 5d8a01c..37ed07b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -375,11 +375,14 @@ static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
typedef struct
{
- char *line; /* text of command line */
+ char *line; /* first line for short display */
+ char *lines; /* full multi-line text of command */
int command_num; /* unique index of this Command struct */
int type; /* command type (SQL_COMMAND or META_COMMAND) */
int argc; /* number of command words */
char *argv[MAX_ARGS]; /* command word list */
+ int compound; /* last compound command (number of \;) */
+ char **gset; /* per-compound command prefix */
PgBenchExpr *expr; /* parsed expression, if needed */
SimpleStats stats; /* time spent in this command */
} Command;
@@ -1254,6 +1257,104 @@ getQueryParams(CState *st, const Command *command, const char **params)
params[i] = getVariable(st, command->argv[i + 1]);
}
+/* read all responses from backend */
+static bool
+read_response(CState *st, char **gset)
+{
+ PGresult *res;
+ int compound = 0;
+
+ while ((res = PQgetResult(st->con)) != NULL)
+ {
+ switch (PQresultStatus(res))
+ {
+ case PGRES_COMMAND_OK: /* non-SELECT commands */
+ case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */
+if (gset[compound] != NULL)
+{
+ fprintf(stderr,
+ "client %d file %d command %d compound %d: "
+ "\\gset expects a row\n",
+ st->id, st->use_file, st->command, compound);
+ st->ecnt++;
+ return false;
+}
+break; /* OK */
+
+ case PGRES_TUPLES_OK:
+if (gset[compound] != NULL)
+{
+ /* store result into variables */
+ int ntuples = PQntuples(res),
+ nfields = PQnfields(res),
+ f;
+
+ if (ntuples != 1)
+ {
+ fprintf(stderr,
+"client %d file %d command %d compound %d: "
+"expecting one row, got %d\n",
+st->id, st->use_file, st->command, compound, ntuples);
+ st->ecnt++;
+ PQclear(res);
+ discard_response(st);
+ return false;
+ }
+
+ for (f = 0; f < nfields ; f++)
+ {
+ char *varname = PQfname(res, f);
+ if (*gset[compound] != '\0')
+ varname = psprintf("%s%s", gset[compound], varname);
+
+ /* store result as a string */
+ if (!putVariable(st, "gset", varname,
+ PQgetvalue(res, 0, f)))
+ {
+ /* internal error, should it rather abort? */
+ fprintf(stderr,
+ "client %d file %d command %d compound %d: "
+ "error storing into var %s\n",
+ st->id, st->use_file, st->command, compound,
+ varname);
+ st->ecnt++;
+ PQclear(res);
+ discard_response(st);
+ return false;
+ }
+
+ if (*gset[compound] != '\0')
+ free(varname);
+ }
+}
+break; /* OK */
+
+ default:
+/* everything else is unexpected, so probably an error */
+fprintf(stderr,
+ "client %d file %d aborted in command %d compound %d: %s",
+ st->id, st->use_file, st->command, compound,
+ PQerrorMessage(st->con));
+st->ecnt++;
+PQclear(res);
+discard_response(st);
+return false;
+ }
+
+ PQclear(res);
+ compound += 1;
+ }
+
+ if (compound == 0)
+ {
+ fprintf(stderr, "client %d command %d: no results\n", st->id, st->command);
+ st->ecnt++;
+ return false;
+ }
+
+ return
Re: [HACKERS] pgbench more operators & functions
Here is a v13. No code changes, but TAP tests added to maintain pgbench coverage to green. Here is a v14, which is just a rebase after the documentation xml-ization. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index e509e6c..1f55967 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -827,14 +827,32 @@ pgbench options d Sets variable varname to a value calculated from expression. - The expression may contain integer constants such as 5432, + The expression may contain the NULL constant, + boolean constants TRUE and FALSE, + integer constants such as 5432, double constants such as 3.14159, references to variables :variablename, - unary operators (+, -) and binary operators - (+, -, *, /, - %) with their usual precedence and associativity, - function calls, and - parentheses. + operators + with their usual SQL precedence and associativity, + function calls, + SQL CASE generic conditional + expressions and parentheses. + + + + Functions and most operators return NULL on + NULL input. + + + + For conditional purposes, non zero numerical values are + TRUE, zero numerical values and NULL + are FALSE. + + + + When no final ELSE clause is provided to a + CASE, the default value is NULL. @@ -843,6 +861,7 @@ pgbench options d \set ntellers 10 * :scale \set aid (1021 * random(1, 10 * :scale)) % \ (10 * :scale) + 1 +\set divx CASE WHEN :x 0 THEN :y/:x ELSE NULL END @@ -919,6 +938,177 @@ pgbench options d + + Built-In Operators + + + The arithmetic, bitwise, comparison and logical operators listed in +are built into pgbench + and may be used in expressions appearing in + \set. + + + + pgbench Operators by increasing precedence + + + + Operator + Description + Example + Result + + + + + OR + logical or + 5 or 0 + TRUE + + + AND + logical and + 3 and 0 + FALSE + + + NOT + logical not + not false + TRUE + + + IS [NOT] (NULL|TRUE|FALSE) + value tests + 1 is null + FALSE + + + ISNULL|NOTNULL + null tests + 1 notnull + TRUE + + + = + is equal + 5 = 4 + FALSE + + + + is not equal + 5 4 + TRUE + + + != + is not equal + 5 != 5 + FALSE + + + + lower than + 5 4 + FALSE + + + = + lower or equal + 5 = 4 + FALSE + + + + greater than + 5 4 + TRUE + + + = + greater or equal + 5 = 4 + TRUE + + + | + integer bitwise OR + 1 | 2 + 3 + + + # + integer bitwise XOR + 1 # 3 + 2 + + + + integer bitwise AND + 1 3 + 1 + + + ~ + integer bitwise NOT + ~ 1 + -2 + + + + integer bitwise shift left + 1 2 + 4 + + + + integer bitwise shift right + 8 2 + 2 + + + + + addition + 5 + 4 + 9 + + + - + substraction + 3 - 2.0 + 1.0 + + + * + multiplication + 5 * 4 + 20 + + + / + division (integer truncates the results) + 5 / 3 + 1 + + + % + modulo + 3 % 2 + 1 + + + - + opposite + - 2.0 + -2.0 + + + + + + Built-In Functions @@ -965,6 +1155,13 @@ pgbench options d 5432.0 + exp(x) + double + exponential + exp(1.0) + 2.718281828459045 + + greatest(a [, ... ] ) double if any a is double, else integer largest value among arguments @@ -986,6 +1183,20 @@ pgbench options d 2.1 + ln(x) + double + natural logarithm + ln(2.718281828459045) + 1.0 + + + mod(i, bj) + integer + modulo + mod(54, 32) + 22 + + pi() double value of the constant PI diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y index b3a2d9b..770be98 100644 --- a/src/bin/pgbench/exprparse.y +++ b/src/bin/pgbench/exprparse.y @@ -19,13 +19,17 @@ PgBenchExpr *expr_parse_result; static PgBenchExprList *make_elist(PgBenchExpr *exp, PgBenchExprList *list); +static PgBenchExpr *make_null_constant(void); +static PgBenchExpr *make_boolean_constant(bool bval); static PgBenchExpr *make_integer_constant(int64 ival); static PgBenchExpr *make_double_constant(double
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
The patch needs a rebase in the documentation because of the xml-ilization of the sgml doc. Thank you for the notification! Attached rebased patch. Ok. The new version works for me. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Hello Masahiko-san, Attached the updated version patch. Applies, compiles, make check & tap test ok, doc is fine. All is well for me: the feature is useful, code is simple and clean, it is easy to invoke, easy to extend as well, which I'm planning to do once it gets in. I switched the patch to "Ready for Committers". No doubt they will have their own opinions about it. Let's wait and see. The patch needs a rebase in the documentation because of the xml-ilization of the sgml doc. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] show precise repos version for dev builds?
The make dependencies ensure that the header file is regenerated on each build with a phony target, and the C file is thus recompiled and linked into the executables on each build. It means that all executables are linked on each rebuild, even if not necessary, though. That'd be quite painful, consider e.g. people using LTO. If done right however, that should be avoidable to some degree. When creating the version file, only replace its contents if the contents differ - that's just a few lines of scripting. Indeed. A potential issue is with dynamic linking, potentially someone could recompile/reinstall just one shared object or dll, and the executable using the lib would change its behavior, and run with libs from heterogeneous version. What is the actual version? Hard to say. In dev mode we often use static linking so that we can copy the executable for a previous version and it would not change depending on updated libs, and so that we always know (or should know) what actual version is running. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] show precise repos version for dev builds?
Hello Robert, Mmph. I understand the desire to identify the exact commit used for a build somehow, but something whose output depends on whether or not I left a branch lying around locally doesn't seem that great. Indeed, the branch/tag search might have a little strange behavior. Probably you would be more happy with just the commit (--always) & knowing that it was changed (--dirty). sh> git describe --always --dirty b81eba6 sh> vi README # edit sh> git describe --always --dirty b81eba6-dirty -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] show precise repos version for dev builds?
Hello Tom, I've seen issues with a number of tools. The one I can remember most clearly is check_postgres.pl . Nobody's going to argue that this is pretty code, but last time I tested (9.4-era, admittedly) it exploded messily with extra-version. FWIW, Salesforce tried to do something similar to Peter's example while I was there. It did not work as well as we'd hoped :-( because what got baked into the built executables was the latest git commit hash as of the time you'd last run configure, not what was current as of the latest "make". Not to mention that you might have built from an uncommitted state. We tried to find a fix for the former problem that didn't create lots of overhead, without much success. My 0.02€: For a research project we regenerate a header file with a string containing the working copy status information, // file version.h #define REV "" and there is a very small C file which is recompiled with a constant string based on the version: // version.c #include "version.h" const char * version = REV; The make dependencies ensure that the header file is regenerated on each build with a phony target, and the C file is thus recompiled and linked into the executables on each build. It means that all executables are linked on each rebuild, even if not necessary, though. No idea what to do about the latter. "svnversion" adds a "M" for modified on the status. There is an option with "git describe" to get something similar: git describe --long --always --all --dirty Also there is a need of a fall back if this fails, to get "version>" instead. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: pgbench - option to build using ppoll() for larger connection counts
This patch enables building pgbench to use ppoll() instead of select()
to allow for more than (FD_SETSIZE - 10) connections. As implemented,
when using ppoll(), the only connection limitation is system resources.
One based on 'master' which can also apply to REL_10_STABLE.
/home/fabien/pgbench-ppoll.patch:137: trailing whitespace.
#define PFD_THREAD_INIT(t,s,n) { do ...
error: patch failed: configure:13024
error: configure: patch does not apply
error: patch failed: configure.in:1430
error: configure.in: patch does not apply
error: patch failed: src/bin/pgbench/pgbench.c:4588
error: src/bin/pgbench/pgbench.c: patch does not apply
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] show precise repos version for dev builds?
Hello, Fabien COELHO <coe...@cri.ensmp.fr> writes: I wanted to know which version it was, and "11devel" is kind of imprecise. ... ISTM that extending the version name with the commit id and or date in some version output, eg "11devel [2632bcc 2017-09-30 ...]", would do it. configure --with-extra-version=whateveryouwant Thanks for the pointer! So now I have to convince the apt.postgresql.org people to build the devel version with this trick. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] show precise repos version for dev builds?
Hello, I use postgres "11devel" packages kindly distributed on "apt.postgresql.org" and recompiled every few hours. I wanted to know which version it was, and "11devel" is kind of imprecise. Ok, there is a hint in the deb package name: 11~~devel~20170930.2214-1~87.git2632bcc.pgdg16.04+1 But this information does not seem to be available from the executables themselves: sh> psql --version psql (PostgreSQL) 11devel sh> pgbench --version pgbench (PostgreSQL) 9.6.5 Some projects provide a repository or build date information: sh> clang --version clang version 6.0.0 (trunk 314585) sh> gcc --version gcc (GCC) 8.0.0 20170930 (experimental) With some svn project I use "svnversion" which shows a summary of the status, eg "5432M" which tells that the sources are locally modified from version 5432. I would find it useful to have something like that in pg as well, and I have not found it available. ISTM that extending the version name with the commit id and or date in some version output, eg "11devel [2632bcc 2017-09-30 ...]", would do it. Thoughts? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench - minor fix for meta command only scripts
Hello Robert, ISTM that this bug exists since rate was introduced, so shame on me and back-patching should be needed. I took a look at this and found that the proposed patch applies cleanly all the way back to 9.5, but the regression is reported to have begun with a commit that starts in v10. I haven't probed into this in any depth, but are we sure that 12788ae49e1933f463bc59a6efe46c4a01701b76 is in fact where this problem originated? Yes. I just rechecked that the problem occurs at 12788ae but not at the preceding da6c4f6ca8. Now the situation before the restructuring is that it worked but given the spaghetti code it was very hard to guess why, not to fix issues when not... My late at night fuzzy interpretation is as follows: The issue is in the code above the fix I submitted which checks what has to be selected. In the previous version ISTM that the condition was laxed, so it filled the input_mask even if the client was not waiting for anything, so it was calling select later which was really just implementing the timeout. With the updated version the input mask and maxsock is only set if there is really something to wait, and if not then it fall through and is active instead of doing a simple sleep/timeout. So I would say that the previous version worked because of a side effect which may or may not have been intentional at the time, and was revealed by checking the condition better. Basically I'd say that the restructuring patch fixed a defect which triggered the bug. Programming is fun:-) -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench stuck with 100% cpu usage
Committed and back-patched to v10. I have to say I'm kind of surprised that the comment removed by this patch got committed in the first place. It's got a ??? in it and isn't very grammatical either. ISTM that I reviewed the initial patch. AFAICR I agreed with the comment that whether it was appropriate to go on was unclear, but it did not strike me as obviously a bad idea at the time, so I let it pass. Now it does strike me as a bad idea (tm):-) My English is kind of fuzzy, so I tend not to comment too much on English unless I'm really sure that it is wrong. Note that there is another 100% cpu pgbench bug, see https://commitfest.postgresql.org/15/1292/, which seems more likely to occur in the wild that this one, but there has been no review of the fix I sent. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench - minor fix for meta command only scripts
reality. So I don't know if this needs backpatching or not. But it should be fixed for v10, as there it becomes a demonstrably live issue. Yes. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql_check future
Hello Pavel, 1. It is placed on my personal repository on GitHub. It is far to perfect from security, from substitutability perspective. Any ideas? Ask to have a copy on git.postgresql.org? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench stuck with 100% cpu usage
- Run pgbench -c 10 -T 100 - Stop postgres with -m immediate That is a strange test to run, but it would be better if the behavior was not that one. Well, I think it's a very legitimate test, not for testing performance, but testing crash recovery and I use it very often. Ok, interesting. Now I understand your purpose. You may consider something like "BEGIN; UPDATE ...; \sleep 100 ms; COMMIT;" so that a crash is most likely to occur with plenty transactions in progress but without much load. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench stuck with 100% cpu usage
The commit that introduced this code is 12788ae49e1933f463bc. So I amn
copying Heikki.
AFAICR the commit was mostly a heavy restructuring of previous unmaintainable
spaghetti code. I'm not sure the problem was not there before under one form
or another.
I agree that it should error out & stop the client in this case at least.
Here is a probable "fix", which does was the comment said should be done.
I could not trigger an infinite loop with various kill -9 and other quick
stops. Could you try it on your side?
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e37496c..f039413 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2194,12 +2194,8 @@ doCustom(TState *thread, CState *st, StatsData *agg)
{
if (!sendCommand(st, command))
{
- /*
- * Failed. Stay in CSTATE_START_COMMAND state, to
- * retry. ??? What the point or retrying? Should
- * rather abort?
- */
- return;
+ commandFailed(st, "SQL command send failed");
+ st->state = CSTATE_ABORTED;
}
else
st->state = CSTATE_WAIT_RESULT;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench stuck with 100% cpu usage
While running some tests, I encountered a situation where pgbench gets
stuck in an infinite loop, consuming 100% cpu. The setup was:
- Start postgres server from the master branch
- Initialise pgbench
- Run pgbench -c 10 -T 100
- Stop postgres with -m immediate
That is a strange test to run, but it would be better if the behavior was
not that one.
Now it seems that pgbench gets stuck and it's state machine does not
advance. Attaching it to debugger, I saw that one of the clients remain
stuck in this loop forever.
if (!sendCommand(st, command))
{
/*
* Failed. Stay in CSTATE_START_COMMAND state, to
* retry. ??? What the point or retrying? Should
* rather abort?
*/
As the comments indicate and your situation shows, probably stopping the
client would be a better much option when send fails, instead of
retrying... indefinitely.
The commit that introduced this code is 12788ae49e1933f463bc. So I amn
copying Heikki.
AFAICR the commit was mostly a heavy restructuring of previous
unmaintainable spaghetti code. I'm not sure the problem was not there
before under one form or another.
I agree that it should error out & stop the client in this case at least.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: pgbench - option to build using ppoll() for larger connection counts
Hello Again, Two patches attached. One based on REL9_6_STABLE. I'd be surprise that there would be a backport unless there is a bug, so this one might not be useful. One based on 'master' which can also apply to REL_10_STABLE. Could you add your patches to the next CF? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: pgbench - break out timing data for initialization phases
Hello Doug, total time: 316.03 s (insert 161.60 s, commit 0.64 s, vacuum 60.77 s, index 93.01 s) Definitely interesting. There is a "ready for committers" patch in the CF which extensively rework the initialization: it becomes customizable, and this approach may not work as is after that... Maybe you could investigate how it may be implemented on top of that? Either show times when the phases are performed computed, or maybe use some auxiliary data structure to keep the information (hmmm...). -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \d sequence display
Hello, This should be fixed for PG10, so if you have any feedback on the design, please let me know soon. Works for me on head against a 9.6 server, which is good. My 0.02 €: \d+ does not show more. Maybe Type, Min, Max, Inc & Cycles are enough for \d? The next/future or last/previous value is not shown. If one could be available it would be nice to have? Maybe some names are a little large, eg "Increment" could be "Inc.". The value is nearly always 1? Not sure why it is "Cycles" (plural) instead of "Cycle". The non regression test could also show a more esoteric sequence (cyclic, ...). There is no documentation. Maybe none is needed. I do not understand why some queries have "... \n" "... \n" and others have "\n ..." "\n ...". I would suggest to homogeneize around the former, because "\nFROM ..." is less readable. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)
Hello Tom,
Well, I think it's mostly about valgrind making everything really slow. Since
we have seen some passes from skink recently, perhaps there was also a
component of more-load-on-the-machine-than-usual. But in the end this is
just evidence for my point that regression tests have to be very much not
timing-sensitive. We run them under all kinds of out-of-the-ordinary stress.
Attached is an attempt at improving the situation:
(1) there are added comments to explain the whys, which is to provide
coverage for pgbench time-related features... while still not being
time-sensitive, which is a challenge.
(2) the test now only expects "progress: \d" from the output, so it is enough
that one progress is shown, whenever it is shown.
(3) if the test is detected to have gone AWOL, detailed log checks are
coldly skipped.
This would have passed on "skink" under the special conditions it encountered.
I cannot guaranty that it would pass under any circumstance, though.
If it still encounters a failure, ISTM that it should only be a missing
"progress:" in the output, which has not been encountered so far.
If it occurs, a few options would remain, none of them very convincing:
- give the test some more time, eg 3 seconds (hmmm... could still fail
after any time...)
- drop the "progress:" expectation (hmmm... but then it does not check
anything).
- make the "progress:" output check conditional to the running time
(hmmm... it would require changing the command_checks_all function,
and there is still a chance that the bench was stuck for 2 seconds
then given time to realize that it has to stop right now...)
- use an even simpler transaction, eg "SELECT;" which is less likely to
get stuck (but still could get stuck...).
For the random-ness related test (--sample-rate), we could add a feature to
pgbench which allows to control the random seed, so that the number of samples
could be known in advance for testing purposes.
--
Fabien.diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 11bc0fe..5043504 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -4,13 +4,14 @@ use warnings;
use PostgresNode;
use TestLib;
use Test::More;
+use Time::HiRes qw{time};
# start a pgbench specific server
my $node = get_new_node('main');
$node->init;
$node->start;
-# invoke pgbench
+# invoke pgbench, return elapsed time
sub pgbench
{
my ($opts, $stat, $out, $err, $name, $files) = @_;
@@ -32,10 +33,13 @@ sub pgbench
append_to_file($filename, $$files{$fn});
}
}
+
+ my $t0 = time();
$node->command_checks_all(\@cmd, $stat, $out, $err, $name);
# cleanup?
#unlink @filenames or die "cannot unlink files (@filenames): $!";
+ return time() - $t0;
}
# Test concurrent insertion into table with UNIQUE oid column. DDL expects
@@ -445,14 +449,53 @@ sub check_pgbench_logs
ok(unlink(@logs), "remove log files");
}
-# with sampling rate
+# note: --progress-timestamp is not tested
+
+# The point of this test is coverage of various time-related features
+# (-T, -P, --aggregate-interval, --rate, --latency-limit...), so it is
+# somehow time sensitive.
+# The checks performed are quite loose so as to still pass even under
+# degraded (high load, slow host, valgrind run) testing conditions.
+# Maybe it might fail if no every second progress report is
+# shown over 2 seconds...
+my $elapsed = pgbench(
+ '-T 2 -P 1 -l --log-prefix=001_pgbench_log_1 --aggregate-interval=1'
+ . ' -S -b se@2 --rate=20 --latency-limit=1000 -j ' . $nthreads
+ . ' -c 3 -r',
+ 0,
+ [ qr{type: multiple},
+ qr{clients: 3},
+ qr{threads: $nthreads},
+ # the shown duration is really -T argument value
+ qr{duration: 2 s},
+ qr{script 1: .* select only},
+ qr{script 2: .* select only},
+ qr{statement latencies in milliseconds},
+ qr{FROM pgbench_accounts} ],
+ [ qr{vacuum},
+ # hopefully at least expect some progress report?
+ qr{progress: \d\b} ],
+ 'pgbench progress');
+
+# if the test has gone AWOL, coldly skip these detailed checks.
+if (abs($elapsed - 2.0) < 0.5)
+{
+ # $nthreads threads, 2 seconds, but due to timing imprecision we might get
+ # only 1 or as many as 3 progress reports per thread.
+ check_pgbench_logs('001_pgbench_log_1', $nthreads, 1, 3,
+ qr{^\d+ \d{1,2} \d+ \d+ \d+ \d+ \d+ \d+ \d+ \d+ \d+$});
+}
+
+# with sampling rate, not time sensitive
pgbench(
'-n -S -t 50 -c 2 --log --log-prefix=001_pgbench_log_2 --sampling-rate=0.5',
0,
[ qr{select only}, qr{processed: 100/100} ],
- [qr{^$}],
+ [ qr{^$} ],
'pgbench logs');
+# random 50% of 2*50 transactions, accept between 8 and 92
+# probability of failure is about 2 * 2^-42 (?)
check_pgbench_logs('001_pgbench_log_2', 1, 8, 92,
qr{^0 \d{1,2} \d+ \d \d+ \d+$});
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
Re: [HACKERS] pgbench regression test failure
Hello Tom,
# progress: 2.6 s, 6.9 tps, lat 0.000 ms stddev 0.000, lag 0.000 ms, 18 skipped
# progress: 3.0 s, 0.0 tps, lat -nan ms stddev -nan, lag -nan ms, 0 skipped
# progress: 4.0 s, 1.0 tps, lat 2682.730 ms stddev 0.000, lag 985.509 ms, 0
skipped
(BTW, the "-nan" bits suggest an actual pgbench bug, independently of
anything else.)
From my point of view, NaN is expected when no test were executed in the
interval: if there was no transaction, it does not make sense to talk
about its latency, so NaN is the right answer.
However, the above "6.9 tps, lat 0.000, stddev 0.000, lag 0.000" is
inconsistent. As "6.9 = 18 / 2.6", it means that progress tps calculation
should remove skipped transactions...
Attached patch attempts to report more consistent figures in the progress
and in final report when transactions are skipped.
sh> cat sleep-100.sql
\sleep 100 ms
SELECT 1;
sh> ./pgbench -P 1 -t 100 -f sleep-100.sql -R 20 -L 1
[...]
progress: 1.0 s, 7.0 tps, lat 100.145 ms stddev 0.042, lag 0.000 ms, 16
skipped
progress: 2.0 s, 6.0 tps, lat 100.133 ms stddev 0.040, lag 0.021 ms, 7 skipped
progress: 3.0 s, 9.0 tps, lat 100.115 ms stddev 0.016, lag 0.000 ms, 11
skipped
[...]
number of transactions actually processed: 38/100
number of transactions skipped: 62 (62.000 %)
number of transactions above the 1.0 ms latency limit: 38 (38.000 %)
latency average = 100.142 ms
tps = 7.091010 (including connections establishing)
tps = 7.094144 (excluding connections establishing)
script statistics:
- number of transactions skipped: 62 (62.000%)
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e37496c..9ca9734 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2584,7 +2584,7 @@ processXactStats(TState *thread, CState *st, instr_time *now,
doLog(thread, st, agg, skipped, latency, lag);
/* XXX could use a mutex here, but we choose not to */
- if (per_script_stats)
+ if (per_script_stats || latency_limit)
accumStats(_script[st->use_file].stats, skipped, latency, lag);
}
@@ -3522,11 +3522,14 @@ printResults(TState *threads, StatsData *total, instr_time total_time,
double time_include,
tps_include,
tps_exclude;
+ int64 ntx = total->cnt - total->skipped;
time_include = INSTR_TIME_GET_DOUBLE(total_time);
- tps_include = total->cnt / time_include;
- tps_exclude = total->cnt / (time_include -
-(INSTR_TIME_GET_DOUBLE(conn_total_time) / nclients));
+
+ /* tps is about actually executed transactions */
+ tps_include = ntx / time_include;
+ tps_exclude = ntx /
+ (time_include - (INSTR_TIME_GET_DOUBLE(conn_total_time) / nclients));
/* Report test parameters. */
printf("transaction type: %s\n",
@@ -3539,7 +3542,7 @@ printResults(TState *threads, StatsData *total, instr_time total_time,
{
printf("number of transactions per client: %d\n", nxacts);
printf("number of transactions actually processed: " INT64_FORMAT "/%d\n",
- total->cnt - total->skipped, nxacts * nclients);
+ total->cnt, nxacts * nclients);
}
else
{
@@ -4660,7 +4663,8 @@ threadRun(void *arg)
{
/* generate and show report */
StatsData cur;
-int64 run = now - last_report;
+int64 run = now - last_report,
+ ntx;
double tps,
total_run,
latency,
@@ -4675,7 +4679,7 @@ threadRun(void *arg)
* XXX: No locking. There is no guarantee that we get an
* atomic snapshot of the transaction count and latencies, so
* these figures can well be off by a small amount. The
- * progress is report's purpose is to give a quick overview of
+ * progress report's purpose is to give a quick overview of
* how the test is going, so that shouldn't matter too much.
* (If a read from a 64-bit integer is not atomic, you might
* get a "torn" read and completely bogus latencies though!)
@@ -4689,15 +4693,14 @@ threadRun(void *arg)
cur.skipped += thread[i].stats.skipped;
}
+/* we count only actually executed transactions */
+ntx = (cur.cnt - cur.skipped) - (last.cnt - last.skipped);
total_run = (now - thread_start) / 100.0;
-tps = 100.0 * (cur.cnt - last.cnt) / run;
-latency = 0.001 * (cur.latency.sum - last.latency.sum) /
- (cur.cnt - last.cnt);
-sqlat = 1.0 * (cur.latency.sum2 - last.latency.sum2)
- / (cur.cnt - last.cnt);
+tps = 100.0 * ntx / run;
+latency = 0.001 * (cur.latency.sum - last.latency.sum) / ntx;
+sqlat = 1.0 * (cur.latency.sum2 - last.latency.sum2) / ntx;
stdev = 0.001 * sqrt(sqlat - 100.0 * latency * latency);
-lag = 0.001 * (cur.lag.sum - last.lag.sum) /
- (cur.cnt - last.cnt);
+lag = 0.001 * (cur.lag.sum - last.lag.sum) / ntx;
if (progress_timestamp)
{
@@ -4714,6 +4717,7 @@ threadRun(void *arg)
(long) tv.tv_sec, (long) (tv.tv_usec / 1000));
}
else
+ /* round seconds are expected, nut the thread may
Re: [HACKERS] Variable substitution in psql backtick expansion
Hello Gerdan, This is an internal address that should not be exposed: postgresql@coelho.net I'm unsure why it gets substituted by the "commitfest application"... When i try apply this patch he failed with a following messenger: It just worked for me on head with git checkout -b test git apply ~/psql-server-version-1.patch My guess is that your mailer or navigator mangles the file contents because its mime type is "text/x-diff" and that it considers that it is okay to change NL to CR or CRNL... which is a BAD IDEA (tm). I re-attached the file compressed so that it uses another mime-type and should not change its contents. -- Fabien. psql-server-version-1.patch.gz Description: application/gzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pgbench - use enum for meta commands
Minor code enhancement.
While having a look at adding if/elif/else/endif to pgbench, and given the
current gset/cset added meta commands in cf queue, it occured to me that
repeated string comparisons to check for the various meta commands is
neither efficient nor readable. Use an enum instead, which are extensively
used already for other similar purposes.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e37496c..3a88150 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -373,11 +373,21 @@ typedef enum QueryMode
static QueryMode querymode = QUERY_SIMPLE;
static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
+typedef enum MetaCommand
+{
+ META_NONE,
+ META_SET,
+ META_SETSHELL,
+ META_SHELL,
+ META_SLEEP
+} MetaCommand;
+
typedef struct
{
char *line; /* text of command line */
int command_num; /* unique index of this Command struct */
int type; /* command type (SQL_COMMAND or META_COMMAND) */
+ MetaCommand meta; /* which meta command, if appropriate */
int argc; /* number of command words */
char *argv[MAX_ARGS]; /* command word list */
PgBenchExpr *expr; /* parsed expression, if needed */
@@ -1721,6 +1731,26 @@ evaluateExpr(TState *thread, CState *st, PgBenchExpr *expr, PgBenchValue *retval
}
}
+/* return meta-command enum identifier */
+static MetaCommand
+getMetaCommand(char * cmd)
+{
+ MetaCommand mc;
+ if (cmd == NULL)
+ mc = META_NONE;
+ else if (pg_strcasecmp(cmd, "set") == 0)
+ mc = META_SET;
+ else if (pg_strcasecmp(cmd, "setshell") == 0)
+ mc = META_SETSHELL;
+ else if (pg_strcasecmp(cmd, "shell") == 0)
+ mc = META_SHELL;
+ else if (pg_strcasecmp(cmd, "sleep") == 0)
+ mc = META_SLEEP;
+ else
+ mc = META_NONE;
+ return mc;
+}
+
/*
* Run a shell command. The result is assigned to the variable if not NULL.
* Return true if succeeded, or false on error.
@@ -2218,7 +2248,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
fprintf(stderr, "\n");
}
- if (pg_strcasecmp(argv[0], "sleep") == 0)
+ if (command->meta == META_SLEEP)
{
/*
* A \sleep doesn't execute anything, we just get the
@@ -2244,7 +2274,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
}
else
{
- if (pg_strcasecmp(argv[0], "set") == 0)
+ if (command->meta == META_SET)
{
PgBenchExpr *expr = command->expr;
PgBenchValue result;
@@ -2263,7 +2293,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
break;
}
}
- else if (pg_strcasecmp(argv[0], "setshell") == 0)
+ else if (command->meta == META_SETSHELL)
{
bool ret = runShellCommand(st, argv[1], argv + 2, argc - 2);
@@ -2283,7 +2313,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
/* succeeded */
}
}
- else if (pg_strcasecmp(argv[0], "shell") == 0)
+ else if (command->meta == META_SHELL)
{
bool ret = runShellCommand(st, NULL, argv + 1, argc - 1);
@@ -3027,6 +3057,7 @@ process_sql_command(PQExpBuffer buf, const char *source)
my_command = (Command *) pg_malloc0(sizeof(Command));
my_command->command_num = num_commands++;
my_command->type = SQL_COMMAND;
+ my_command->meta = META_NONE;
initSimpleStats(_command->stats);
/*
@@ -3095,7 +3126,9 @@ process_backslash_command(PsqlScanState sstate, const char *source)
my_command->argv[j++] = pg_strdup(word_buf.data);
my_command->argc++;
- if (pg_strcasecmp(my_command->argv[0], "set") == 0)
+ my_command->meta = getMetaCommand(my_command->argv[0]);
+
+ if (my_command->meta == META_SET)
{
/* For \set, collect var name, then lex the expression. */
yyscan_t yyscanner;
@@ -3150,7 +3183,7 @@ process_backslash_command(PsqlScanState sstate, const char *source)
expr_scanner_offset(sstate),
true);
- if (pg_strcasecmp(my_command->argv[0], "sleep") == 0)
+ if (my_command->meta == META_SLEEP)
{
if (my_command->argc < 2)
syntax_error(source, lineno, my_command->line, my_command->argv[0],
@@ -3191,13 +3224,13 @@ process_backslash_command(PsqlScanState sstate, const char *source)
my_command->argv[2], offsets[2] - start_offset);
}
}
- else if (pg_strcasecmp(my_command->argv[0], "setshell") == 0)
+ else if (my_command->meta == META_SETSHELL)
{
if (my_command->argc < 3)
syntax_error(source, lineno, my_command->line, my_command->argv[0],
"missing argument", NULL, -1);
}
- else if (pg_strcasecmp(my_command->argv[0], "shell") == 0)
+ else if (my_command->meta == META_SHELL)
{
if (my_command->argc < 2)
syntax_error(source, lineno, my_command->line, my_command->argv[0],
@@ -3205,6 +3238,7 @@ process_backslash_command(PsqlScanState sstate, const char *source)
}
else
{
+ /* my_command->meta == META_NONE */
syntax_error(source, lineno, my_command->line, my_command->argv[0],
"invalid command", NULL, -1);
}
Re: [HACKERS] pgbench regression test failure
[...] After another week of buildfarm runs, we have a few more cases of 3 rows of output, and none of more than 3 or less than 1. So I went ahead and pushed your patch. I'm still suspicious of these results, but we might as well try to make the buildfarm green pending investigation of how this is happening. Yep. I keep the issue of pgbench tap test determinism in my todo list, among other things. I think that it should be at least clearer under which condition (load ? luck ? bug ?) the result may be 1 or 3 when 2 are expected, which needs some thinking. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Hello Masahiko-san, ISTM that you probably intended "\(.*\)" (actual parenthesis) instead of "(.*)" (memorization) in the data generation message check. Thank you, fixed it. Otherwise all is well for me. Attached the updated version patch. Applies, compiles, make check & tap test ok, doc is fine. All is well for me: the feature is useful, code is simple and clean, it is easy to invoke, easy to extend as well, which I'm planning to do once it gets in. I switched the patch to "Ready for Committers". No doubt they will have their own opinions about it. Let's wait and see. Thanks, -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add ability to test whether a variable exists
Correct Fabien. I have already removed myself as a reviewer. Thanks. As you wish! Thanks for the feedback, which I understood as "works for me". -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add ability to test whether a variable exists
Hello Robins, I was able to test the functionality (which seemed to work fine) and fed in my comment to assist anyone else reviewing this patch (and intentionally let it's state as 'Needs Review'). While trying to provide my feedback, on hindsight I should have been more detailed about what I didn't test. Being my first review, I didn't understand that not checking a box meant 'failure'. For e.g. I read the sgml changes, which felt okay but didn't click 'Passed' because my env wasn't setup properly. Hmmm, ISTM that it was enough. The feature is psql specific, so the fact that it works against a 9.6 server is both expected and fine. So ISTM that your test "passed". Just running "make check" would run the non regression test, which is basically what you tested online, against the compiled version. Probably you should have a little look at the source code and doc as well. I've set this back to 'Needs Review' because clearly needs it. Hmmm. If you do a review, which I think you have done, then you have done it:-) If you consider that your test was not a review and you do not intend to provide one, then thanks for the feedback anyway, and maybe you should consider removing yourself from the "Reviewer" column, otherwise nobody will provide a review. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Hello Masahiko-san, v14 applies, compiles and works. TAP tests provide good coverage. ISTM that you probably intended "\(.*\)" (actual parenthesis) instead of "(.*)" (memorization) in the data generation message check. Otherwise all is well for me. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add ability to test whether a variable exists
Hello Robins,
Thanks for the review.
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, failed
Where ?
Spec compliant: not tested
Documentation:tested, failed
Where ? I just regenerated the html doc on the patch without a glitch.
The patch applies cleanly and compiles + installs fine (although am
unable to do installcheck-world on my Cygwin setup). This is how the
patch works on my setup.
$ /opt/postgres/reviewpatch/bin/psql -U postgres -h localhost
psql (11devel, server 9.6.1)
Type "help" for help.
postgres=# \set i 1
postgres=# \if :{?i}
postgres=# \echo 'testing'
testing
postgres=# \endif
postgres=# \if :{?id}
postgres@# \echo 'testing'
\echo command ignored; use \endif or Ctrl-C to exit current \if block
postgres@# \endif
postgres=#
ISTM that this is the expected behavior.
In the first case, "i" is defined, so the test is true and the echo
echoes.
In the second case, "id" is undefined, the test is false and the echo is
skipped.
I do not understand why you conclude that the feature "failed".
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Hello Masahiko-san,
Attached the latest version patch incorporated the tap tests.
Please review it.
Patch applies, compilation & make check ok.
Tests are simple and provide good coverage of new functionalities.
I would suggest to add '--unlogged-tables' so speedup the tests a little.
Comment: "# Custom initialization option, including a space"... ISTM that
there is no space. Space is tested in the next test because of the v's and
the --no-vacuum which turned them into space, which is enough.
Regex are just check for the whole output, so putting twice "qr{vacuum}"
does not check that vacuum appears twice, it checks twice that vacuum
appears once. I do not think that it is worth trying to check for the v
repetition, so I suggest to remove one from the first test. Repetition of
' ' is checked with the second test.
Maybe you could check that the data generation message is there.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql: new help related to variables are not too readable
Hello, Personnally I'm fine with a pager, so vertical spacing is fine. I just do not like paging horizontally. -1 [...] If I was going to try and read it like a book I'd want the extra white-space to make doing so easier (white-space gives the eye a breather when done with a particular concept) - and the length wouldn't really matter since I'd just make a single pass and be done with it. But the planned usage is for quick lookup of options that you know (or at least suspect) exist and which you probably have an approximate idea of how they are spelled. The all-caps and left-justified block headers are distinct enough to scan down - though I'd consider indenting 4 spaces instead of 2 to make that even easier (less effort to ignore the indented lines since ignoring nothing is easier than ignoring something). Having more fit on one screen makes that vertical skimming considerably easier as well (no break and re-acquire when scrolling in a new page). Interesting and fine arguments! So I'll agree that in an absolute sense reading the whole of the content in its condensed form is more difficult than if there were blank lines in between each block, but usability for the intended purpose is better in the current form. As far as usability is concerned, I most often use the "/" pager search feature, or page down to scan everything. Both uses are not really hampered by skipping lines, but I can leave with that as well. Help formatting could be an option, but that would require more coding and I'm not sure of the i18n aspect. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql: new help related to variables are not too readable
Hello Tom, Probably it needs some rebase after Tom committed result status variables. As it is a style thing, ISTM that the patch is ready if most people agree that it is better this way and there is no strong veto against. FWIW, I think it's a bad idea. We already nearly-doubled the vertical space required for this variable list. That was a heavy cost --- and we already got at least one complaint about it --- but it seemed warranted to avoid having to deal with very constrained variable descriptions. This proposes to make the vertical space nearly triple what it was in v10. In a typical-size window that's going to have a pretty severe impact on how much of the list you can see at once. And the readability gain is (at least to my eyes) very marginal. Ok, you do not like it. As Pavel said, it is subjective. When it is a matter of taste, people tend to differ, someone will always complain, one way or another, and they are neither right nor wrong. So, is it a -1 or a veto? If it is the later, the patch can be marked as "Rejected" and everybody will get more time for other things:-) If it is a not a veto, people can continue to give their opinions. Personnally I'm fine with a pager, so vertical spacing is fine. I just do not like paging horizontally. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench regression test failure
I have a serious, serious dislike for tests that seem to work until they're run on a heavily loaded machine. I'm not that sure the error message was because of that. No, this particular failure (probably) wasn't. But now that I've realized that this test case is timing-sensitive, I'm worried about what will happen when it's run on a sufficiently slow or loaded machine. I would not necessarily object to doing something in the code that would guarantee that, though. Hmmm. Interesting point. It could be as simple as putting the check-for-done at the bottom of the loop not the top, perhaps. I agree that it is best if tests should work in all reasonable conditions, including a somehow overloaded host... I'm going to think about it, but I'm not sure of the best approach. In the mean time, ISTM that the issue has not been encountered (yet), so this is not a pressing issue. Maybe under -T > --aggregate-interval pgbench could go on over the limit if the log file has not been written at all, but that would be some kind of kludge for this specific test... Note that to get test coverage for -T and have to assume that maybe a loaded host would not be able to generate just a one line log every second during that time is kind of a hard assumption... Maybe some test could be "warnings", i.e. it could be acceptable to accept a failure once in a while in specific conditions, if this is rare enough and documented. ISTM that there is such a test for random output. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add special variable to reflect the last query status
One thing we could think about if this seems too high is to drop ROW_COUNT. I'm unconvinced that it has a real use-case, and it seems to be taking more than its share of the work in non-error cases, because it turns out that PQcmdTuples() is not an amazingly cheap function. I do think that a small overhead on a contrived case is worth removing the feature, as it is really insignificant on any realistic case. Please read: I do NOT think that... -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Tom, I put back SetResultVariables function which is called twice, for SQL queries and the new descriptions. It worked out of the box with DECLARE which is just another SQL statement, so maybe I did not understood the cursor issue you were signaling... No, I was concerned about ExecQueryUsingCursor(), which is used when FETCH_COUNT is enabled. It's sort of a pain because you have to accumulate the row count across multiple PGresults. If you don't, then FETCH_COUNT mode isn't transparent, which it's supposed to be. Please allow me to disagree a little with this semantics. ISTM that the semantics of the simple previous implementation was fine, "number of rows returned by previous statement". If you do "FETCH 3 ...", then you get between 0 and 3 rows... Good. If you do it again, same... I'm not sure having an accumulation semantics helps a lot, because it creates an exception, and moreover I can think of legitimate use case where counting only last statement rows would be useful, eg to check that we are done with a cursor and it can be closed. If someone really wants to accumulate, it can be done by hand quite simply, currently as: SELECT :ROW_COUNT + :accum AS accum \gset or client side: \set accum `expr :ROW_COUNT + :accum` and maybe some day something like: \let accum :ROW_COUNT + :accum I did some performance testing of my own, based on this possibly-silly test case: [...] The idea was to run a trivial query and minimize all other psql overhead, particularly results-printing. With this, "perf" told me that SetResultVariables and its called functions accounted for 1.5% of total CPU (including the server processes). That's kind of high, but it's probably tolerable considering that any real application would involve both far more server work per query and far more psql work (at least for SELECTs). This seems pretty reasonable to me, and is consistent with my 1% elapsed time measure on a silent "SELECT;". One thing we could think about if this seems too high is to drop ROW_COUNT. I'm unconvinced that it has a real use-case, and it seems to be taking more than its share of the work in non-error cases, because it turns out that PQcmdTuples() is not an amazingly cheap function. I do think that a small overhead on a contrived case is worth removing the feature, as it is really insignificant on any realistic case. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql: new help related to variables are not too readable
Finally, as vertical scrolling is mandatory, I would be fine with skipping lines with entries for readability, but it is just a matter of taste and I expect there should be half a dozen different opinions on the matter of formatting. FWIW, +1 to extra lines from me - I find it way more readable, as it clearly separates the items. +1 As already said above +1 for me for having more space. I'll assign this patch to next commitfest Probably it needs some rebase after Tom committed result status variables. As it is a style thing, ISTM that the patch is ready if most people agree that it is better this way and there is no strong veto against. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add special variable to reflect the last query status
Well, if we provided a different SQLSTATE for each qualitatively
different type of libpq error, that might well be useful enough to
justify some risk of application breakage. But replacing a constant
string that we've had for ~15 years with a different constraint string
isn't doing anything about the lack-of-information problem you're
complaining about.
True. Well, the original point here was whether psql ought to be doing
something to mask libpq's (mis) behavior. I'm inclined to think not:
if it doesn't get a SQLSTATE from the PGresult, it should just set the
sqlstate variables to empty strings.
See v9 attached.
--
Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index a74caf8..b994fcd 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3518,6 +3518,16 @@ bar
+ ERROR
+
+
+ Whether the last query failed, as a boolean.
+ See also SQLSTATE.
+
+
+
+
+
FETCH_COUNT
@@ -3654,6 +3664,18 @@ bar
+ LAST_ERROR_SQLSTATE
+ LAST_ERROR_MESSAGE
+
+
+ The error code and associated error message of the last
+ error, or "0" and empty strings if no error occured
+ since the beginning of the script.
+
+
+
+
+
ON_ERROR_ROLLBACK
@@ -3722,6 +3744,25 @@ bar
+ ROW_COUNT
+
+
+ How many rows were returned or affected by the last query.
+
+
+
+
+
+ SQLSTATE
+
+
+ The error code associated to the last query, or
+ 0 if no error occured.
+
+
+
+
+
QUIET
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index b997058..cc7e3aa 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -493,7 +493,6 @@ ResetCancelConn(void)
#endif
}
-
/*
* AcceptResult
*
@@ -971,6 +970,45 @@ loop_exit:
return success;
}
+/*
+ * Set special variables
+ * - ERROR: true/false, whether an error occurred
+ * - SQLSTATE: code of error, or "0", or ""
+ * - LAST_ERROR_SQLSTATE: same for last error
+ * - LAST_ERROR_MESSAGE: message of last error
+ * - ROW_COUNT: how many rows were returned or affected, or "0"
+ */
+static void
+SetResultVariables(PGresult *results, bool success)
+{
+ if (success)
+ {
+ char *ntuples = PQcmdTuples(results);
+ SetVariable(pset.vars, "ERROR", "false");
+ SetVariable(pset.vars, "SQLSTATE", "0");
+ SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0");
+ }
+ else
+ {
+ char *code = PQresultErrorField(results, PG_DIAG_SQLSTATE);
+ char *mesg = PQresultErrorField(results, PG_DIAG_MESSAGE_PRIMARY);
+
+ SetVariable(pset.vars, "ERROR", "true");
+
+ /*
+ * if there is no code, use an empty string?
+ * libpq may return such thing on internal errors
+ * (lost connection, EOM).
+ */
+ if (code == NULL)
+ code = "" ;
+
+ SetVariable(pset.vars, "SQLSTATE", code);
+ SetVariable(pset.vars, "LAST_ERROR_SQLSTATE", code);
+ SetVariable(pset.vars, "LAST_ERROR_MESSAGE", mesg ? mesg : "");
+ SetVariable(pset.vars, "ROW_COUNT", "0");
+ }
+}
/*
* ProcessResult: utility function for use by SendQuery() only
@@ -1107,6 +1145,8 @@ ProcessResult(PGresult **results)
first_cycle = false;
}
+ SetResultVariables(*results, success);
+
/* may need this to recover from conn loss during COPY */
if (!first_cycle && !CheckConnection())
return false;
@@ -1214,7 +1254,6 @@ PrintQueryResults(PGresult *results)
return success;
}
-
/*
* SendQuery: send the query string to the backend
* (and print out results)
@@ -1523,7 +1562,11 @@ DescribeQuery(const char *query, double *elapsed_msec)
* good thing because libpq provides no easy way to do that.)
*/
results = PQprepare(pset.db, "", query, 0, NULL);
- if (PQresultStatus(results) != PGRES_COMMAND_OK)
+ OK = PQresultStatus(results) == PGRES_COMMAND_OK;
+
+ SetResultVariables(results, OK);
+
+ if (!OK)
{
psql_error("%s", PQerrorMessage(pset.db));
ClearOrSaveResult(results);
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 4d1c0ec..ae951f5 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -337,7 +337,7 @@ helpVariables(unsigned short int pager)
* Windows builds currently print one more line than non-Windows builds.
* Using the larger number is fine.
*/
- output = PageOutput(147, pager ? &(pset.popt.topt) : NULL);
+ output = PageOutput(155, pager ? &(pset.popt.topt) : NULL);
fprintf(output, _("List of specially treated variables\n\n"));
@@ -360,6 +360,8 @@ helpVariables(unsigned short int pager)
"if set to \"noexec\", just show them without execution\n"));
fprintf(output, _(" ENCODING\n"
"current client character set encoding\n"));
+ fprintf(output, _(" ERROR\n"
+
Re: [HACKERS] pgbench regression test failure
I have a serious, serious dislike for tests that seem to work until
they're run on a heavily loaded machine.
I'm not that sure the error message was because of that. ISTM that it was
rather finding 3 seconds in two because it started just at the right time,
or maybe because of slowness induce by load and the order in which the
different checks are performed.
So unless there is some reason why pgbench is *guaranteed* to run at
least one transaction per thread, I'd rather the test not assume that.
Well, pgbench is for testing performance... so if the checks allow zero
performance that's quite annoying as well:-) The tests are designed to
require very low performance (eg there are a lot of -t 1 when only one
transaction is enough to check a point), but maybe some test assume a
minimal requirement, maybe 10 tps with 2 threads...
I would not necessarily object to doing something in the code that
would guarantee that, though.
Hmmm. Interesting point.
There could be a client-side synchronization barrier, eg something like
"\sync :nclients/nthreads" could be easy enough to implement with pthread,
and quite error prone to use, but probably that could be okay for
validation purposes. Or maybe we could expose something at the SQL level,
eg "SELECT synchro('synchroname', whomanyclientstowait);" which would be
harder to implement server-side but possibly doable as well.
A simpler option may be to introduce a synchronization barrier at thread
start, so that all threads start together and that would set the "zero"
time. Not sure that would solve the potential issue you raise, although
that would help.
Currently the statistics collection and outputs are performed by thread 0
in addition to the client it runs, so that pgbench would work even if
there are no threads, but it also means that under a heavy load some
things may not be done on the target time but a little bit later, if some
thread is stuck somewhere. Although the async protocol try to avoid that.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench regression test failure
Apparently, one of the threads ran 3 transactions where the test script expects it to run at most 2. Is this a pgbench bug, or is the test being overoptimistic about how exact the "-T 2" cutoff is? Probably both? It seems that cutting off on time is not a precise science, so I suggest to accept 1, 2 and 3 lines, see attached. Before I'd deciphered the test output fully, I was actually guessing that the problem was the opposite, namely too few lines. The test was waiting for betwen 1 and 2 lines, so I assumed that the 3 should the number of lines found. Isn't it possible that some thread is slow enough to start up that it doesn't get to run any transactions? IOW, do we need to allow 0 to 3 lines? By definition, parallelism induces non determinism. When I put 2 seconds, the intention was that I would get a non empty trace with a "every second" aggregation. I would rather take a longer test rather than allowing an empty file: the point is to check that something is generated, but avoiding a longer test is desirable. So I would suggest to stick to between 1 and 3, and if it fails then maybe add one second... -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Tom,
Yep, I thought I was optimistic:-) Can I add a special SQLSTATE for that
situation where libpq did not report an error?
Meh. If we're going to do that I think it might be better to hack
libpq itself to do so, ie, force PQresultErrorField(..., PG_DIAG_SQLSTATE)
to always return something. But it seems like a hack either way.
I would not have took the liberty to hack into libpq internals for such a
small front-end feature. However I agree that having libpq always return some
diagnostic, even if it means "something unclear happened, sorry not to be
very precise", would be better.
Here is an attempt at implementing your suggestions.
I added two error codes, which is debatable. One is used hardcoded by
libpq if no diagnostic is found, and the other by psql if libpq returned
something empty, which might happen if psql is linked with an older libpq,
maybe. I do not know how to trigger such errors anyway, so this is rather
academic.
I put back SetResultVariables function which is called twice, for SQL
queries and the new descriptions. It worked out of the box with DECLARE
which is just another SQL statement, so maybe I did not understood the
cursor issue you were signaling...
--
Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index a74caf8..b994fcd 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3518,6 +3518,16 @@ bar
+ ERROR
+
+
+ Whether the last query failed, as a boolean.
+ See also SQLSTATE.
+
+
+
+
+
FETCH_COUNT
@@ -3654,6 +3664,18 @@ bar
+ LAST_ERROR_SQLSTATE
+ LAST_ERROR_MESSAGE
+
+
+ The error code and associated error message of the last
+ error, or "0" and empty strings if no error occured
+ since the beginning of the script.
+
+
+
+
+
ON_ERROR_ROLLBACK
@@ -3722,6 +3744,25 @@ bar
+ ROW_COUNT
+
+
+ How many rows were returned or affected by the last query.
+
+
+
+
+
+ SQLSTATE
+
+
+ The error code associated to the last query, or
+ 0 if no error occured.
+
+
+
+
+
QUIET
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index b997058..bbffcac 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -493,7 +493,6 @@ ResetCancelConn(void)
#endif
}
-
/*
* AcceptResult
*
@@ -971,6 +970,44 @@ loop_exit:
return success;
}
+/*
+ * Set special variables
+ * - ERROR: true/false, whether an error occurred
+ * - SQLSTATE: code of error, or "0"
+ * - LAST_ERROR_SQLSTATE: same for last error
+ * - LAST_ERROR_MESSAGE: message of last error
+ * - ROW_COUNT: how many rows were returned or affected, or "0"
+ */
+static void
+SetResultVariables(PGresult *results, bool success)
+{
+ if (success)
+ {
+ char *ntuples = PQcmdTuples(results);
+ SetVariable(pset.vars, "ERROR", "false");
+ SetVariable(pset.vars, "SQLSTATE", "0");
+ SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0");
+ }
+ else
+ {
+ char *code = PQresultErrorField(results, PG_DIAG_SQLSTATE);
+ char *mesg = PQresultErrorField(results, PG_DIAG_MESSAGE_PRIMARY);
+
+ SetVariable(pset.vars, "ERROR", "true");
+
+ /*
+ * Ensure that something sensible is shown,
+ * without assumption about libpq implementation
+ */
+ if (code == NULL || *code == '\0')
+ code = "PQ001" /* ERROR_LIBPQ_EMPTY_SQLSTATE */ ;
+
+ SetVariable(pset.vars, "SQLSTATE", code);
+ SetVariable(pset.vars, "LAST_ERROR_SQLSTATE", code);
+ SetVariable(pset.vars, "LAST_ERROR_MESSAGE", mesg ? mesg : "");
+ SetVariable(pset.vars, "ROW_COUNT", "0");
+ }
+}
/*
* ProcessResult: utility function for use by SendQuery() only
@@ -1107,6 +1144,8 @@ ProcessResult(PGresult **results)
first_cycle = false;
}
+ SetResultVariables(*results, success);
+
/* may need this to recover from conn loss during COPY */
if (!first_cycle && !CheckConnection())
return false;
@@ -1214,7 +1253,6 @@ PrintQueryResults(PGresult *results)
return success;
}
-
/*
* SendQuery: send the query string to the backend
* (and print out results)
@@ -1523,7 +1561,11 @@ DescribeQuery(const char *query, double *elapsed_msec)
* good thing because libpq provides no easy way to do that.)
*/
results = PQprepare(pset.db, "", query, 0, NULL);
- if (PQresultStatus(results) != PGRES_COMMAND_OK)
+ OK = PQresultStatus(results) == PGRES_COMMAND_OK;
+
+ SetResultVariables(results, OK);
+
+ if (!OK)
{
psql_error("%s", PQerrorMessage(pset.db));
ClearOrSaveResult(results);
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 4d1c0ec..ae951f5 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -337,7 +337,7 @@
Re: [HACKERS] pgbench regression test failure
francolin just showed a non-reproducing failure in the new pgbench tests:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=francolin=2017-09-12%2014%3A00%3A02
not ok 211 - transaction count for 001_pgbench_log_1.31583 (3)
# Failed test 'transaction count for 001_pgbench_log_1.31583 (3)'
# at t/001_pgbench_with_server.pl line 438.
Apparently, one of the threads ran 3 transactions where the test script
expects it to run at most 2. Is this a pgbench bug, or is the test
being overoptimistic about how exact the "-T 2" cutoff is?
Probably both? It seems that cutting off on time is not a precise science,
so I suggest to accept 1, 2 and 3 lines, see attached.
--
Fabien.diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 3609b9b..1fe3433 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -463,7 +463,8 @@ pgbench(
'pgbench progress');
# $nthreads threads, 2 seconds, sometimes only one aggregated line is written
-check_pgbench_logs('001_pgbench_log_1', $nthreads, 1, 2,
+# and sometimes 3 lines...
+check_pgbench_logs('001_pgbench_log_1', $nthreads, 1, 3,
qr{^\d+ \d{1,2} \d+ \d+ \d+ \d+ \d+ \d+ \d+ \d+ \d+$});
# with sampling rate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench - minor fix for meta command only scripts
Hello Jeff,
Shouldn't we use pg_usleep to ensure portability? it is defined for
front-end code. But it returns void, so the error check will have to be
changed.
Attached v3 with pg_usleep called instead.
I didn't see the problem before the commit I originally indicated , so I
don't think it has to be back-patched to before v10.
Hmmm you've got a point, although I'm not sure how it could work
without sleeping explicitely. Maybe the path was calling select with an
empty wait list plus timeout, and select is kind enough to just sleep on
an empty list, or some other miracle. ISTM clearer to explicitely sleep in
that case.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e37496c..524fcc6 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4582,20 +4582,30 @@ threadRun(void *arg)
* or it's time to print a progress report. Update input_mask to show
* which client(s) received data.
*/
- if (min_usec > 0 && maxsock != -1)
+ if (min_usec > 0)
{
- int nsocks; /* return from select(2) */
+ int nsocks = 0; /* return from select(2) if called */
if (min_usec != PG_INT64_MAX)
{
-struct timeval timeout;
+if (maxsock != -1)
+{
+ struct timeval timeout;
-timeout.tv_sec = min_usec / 100;
-timeout.tv_usec = min_usec % 100;
-nsocks = select(maxsock + 1, _mask, NULL, NULL, );
+ timeout.tv_sec = min_usec / 100;
+ timeout.tv_usec = min_usec % 100;
+ nsocks = select(maxsock + 1, _mask, NULL, NULL, );
+}
+else /* nothing active, simple sleep */
+{
+ pg_usleep(min_usec);
+}
}
- else
+ else /* no explicit delay, select without timeout */
+ {
nsocks = select(maxsock + 1, _mask, NULL, NULL, NULL);
+ }
+
if (nsocks < 0)
{
if (errno == EINTR)
@@ -4608,7 +4618,7 @@ threadRun(void *arg)
goto done;
}
}
- else
+ else /* min_usec == 0, i.e. something needs to be executed */
{
/* If we didn't call select(), don't try to read any data */
FD_ZERO(_mask);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add special variable to reflect the last query status
I think you're overly optimistic to believe that every failure will have a SQLSTATE; I don't think that's true for libpq-reported errors, such as connection loss. Yep, I thought I was optimistic:-) Can I add a special SQLSTATE for that situation where libpq did not report an error? Meh. If we're going to do that I think it might be better to hack libpq itself to do so, ie, force PQresultErrorField(..., PG_DIAG_SQLSTATE) to always return something. But it seems like a hack either way. I would not have took the liberty to hack into libpq internals for such a small front-end feature. However I agree that having libpq always return some diagnostic, even if it means "something unclear happened, sorry not to be very precise", would be better. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Tom, Hm. Looking closer at this, I see that it doesn't work so well after all to put the variable-setting code in ProcessResult: that fails to cover the ExecQueryUsingCursor code path. Ok, I'll investigate this path. And it also fails to cover DescribeQuery, which arguably should set these variables as well And this one. -- certainly so if it gets a failure. Maybe you could create a small subroutine along the lines of SetResultVariables(PGresult *result, bool success) for all three places to call. (ProcessResult certainly has already decided whether it's got a success, and I think the other paths would know that as well, so no need to re-extract it from the PGresult.) Ok. I think you're overly optimistic to believe that every failure will have a SQLSTATE; I don't think that's true for libpq-reported errors, such as connection loss. Yep, I thought I was optimistic:-) Can I add a special SQLSTATE for that situation where libpq did not report an error? Using upper-case TRUE/FALSE for the values of ERROR seems a bit ugly to me; we generally use lower case for other variable values, so I'd go with true/false. Ok. The choice is not aesthetic but systematic: I use upper-case for all SQL keywords, and lower-case or capitalized for anything user land. I can put lower-case if you want. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench - minor fix for meta command only scripts
Hello Jeff,
Ok, the problem was a little bit more trivial than I thought.
The issue is that under a low rate there may be no transaction in
progress, however the wait procedure was relying on select's timeout. If
nothing is active there is nothing to wait for, thus it was an active loop
in this case...
I've introduced a usleep call in place of select for this particular
case. Hopefully this is portable.
ISTM that this bug exists since rate was introduced, so shame on me and
back-patching should be needed.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e37496c..068dbe6 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4582,20 +4582,30 @@ threadRun(void *arg)
* or it's time to print a progress report. Update input_mask to show
* which client(s) received data.
*/
- if (min_usec > 0 && maxsock != -1)
+ if (min_usec > 0)
{
- int nsocks; /* return from select(2) */
+ int nsocks = 0; /* return from select(2) if called, or usleep() */
if (min_usec != PG_INT64_MAX)
{
-struct timeval timeout;
+if (maxsock != -1)
+{
+ struct timeval timeout;
-timeout.tv_sec = min_usec / 100;
-timeout.tv_usec = min_usec % 100;
-nsocks = select(maxsock + 1, _mask, NULL, NULL, );
+ timeout.tv_sec = min_usec / 100;
+ timeout.tv_usec = min_usec % 100;
+ nsocks = select(maxsock + 1, _mask, NULL, NULL, );
+}
+else /* nothing active, simple sleep */
+{
+ nsocks = usleep(min_usec);
+}
}
- else
+ else /* no delay, select without timeout */
+ {
nsocks = select(maxsock + 1, _mask, NULL, NULL, NULL);
+ }
+
if (nsocks < 0)
{
if (errno == EINTR)
@@ -4604,11 +4614,11 @@ threadRun(void *arg)
continue;
}
/* must be something wrong */
-fprintf(stderr, "select() failed: %s\n", strerror(errno));
+fprintf(stderr, "select() or usleep() failed: %s\n", strerror(errno));
goto done;
}
}
- else
+ else /* min_usec == 0 */
{
/* If we didn't call select(), don't try to read any data */
FD_ZERO(_mask);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench more operators & functions
Here is a v13. No code changes, but TAP tests added to maintain pgbench coverage to green. Summary of patch contents: [...] 1. there are no any problem with compilation, patching 2. all tests passed 3. doc is ok I'll mark this patch as ready for commiter Ok. Thanks for the reviews. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench more operators & functions
Hello Pavel, Here is a v13. No code changes, but TAP tests added to maintain pgbench coverage to green. Summary of patch contents: This patch extends pgbench expressions syntax while keeping compatibility with SQL expressions. It adds support for NULL and BOOLEAN, as well as assorted logical, comparison and test operators (AND, <>, <=, IS NULL...). A CASE construct is provided which takes advantage of the added BOOLEAN. Integer and double functions and operators are also extended: bitwise operators (<< & ...), exp/ln, mod() as synonymous to % (matching pg). Added TAP tests maintain pgbench source coverage to green (if you ignore lexer & parser generated files...). Future plans include extending and synchronizing psql & pgbench variable and expression syntaxes: - move expression parsing and evaluation in fe_utils, which would allow to - extend psql with some \let i cliend-side syntax (ISTM that extending the \set syntax cannot be upward compatible) and probably handle \let as a synonymous to \set in pgbench. - allow \if in psql instead of just \if - add \if ... support to pgbench - maybe add TEXT type support to the expression engine, if useful - maybe add :'var" and :"var" support to pgbench, if useful There are already patches in the queue for: - testing whether a variable is defined in psql feature could eventually be added to pgbench as well - adding \gset (& \cset) to pgbench to get output of possibly combined queries into variables, which can be used for making decisions later in the script. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index f5db8d1..59ca7a1 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -827,14 +827,31 @@ pgbench options dbname Sets variable varname to a value calculated from expression. - The expression may contain integer constants such as 5432, + The expression may contain the NULL constant, + boolean constants TRUE and FALSE, + integer constants such as 5432, double constants such as 3.14159, references to variables :variablename, - unary operators (+, -) and binary operators - (+, -, *, /, - %) with their usual precedence and associativity, - function calls, and - parentheses. + operators + with their usual SQL precedence and associativity, + function calls, + SQL CASE generic conditional + expressions and parentheses. + + + + Functions and most operators return NULL on + NULL input. + + + + For conditional purposes, non zero numerical values are TRUE, + zero numerical values and NULL are FALSE. + + + + When no final ELSE clause is provided to a CASE, + the default value is NULL. @@ -843,6 +860,7 @@ pgbench options dbname \set ntellers 10 * :scale \set aid (1021 * random(1, 10 * :scale)) % \ (10 * :scale) + 1 +\set divx CASE WHEN :x <> 0 THEN :y/:x ELSE NULL END @@ -919,6 +937,177 @@ pgbench options dbname + + Built-In Operators + + + The arithmetic, bitwise, comparison and logical operators listed in +are built into pgbench + and may be used in expressions appearing in + \set. + + + + pgbench Operators by increasing precedence + + + + Operator + Description + Example + Result + + + + + OR + logical or + 5 or 0 + TRUE + + + AND + logical and + 3 and 0 + FALSE + + + NOT + logical not + not false + TRUE + + + IS [NOT] (NULL|TRUE|FALSE) + value tests + 1 is null + FALSE + + + ISNULL|NOTNULL + null tests + 1 notnull + TRUE + + + = + is equal + 5 = 4 + FALSE + + + + is not equal + 5 4 + TRUE + + + != + is not equal + 5 != 5 + FALSE + + + + lower than + 5 4 + FALSE + + + = + lower or equal + 5 = 4 + FALSE + + + + greater than + 5 4 + TRUE + + + = + greater or equal + 5 = 4 + TRUE + + + | + integer bitwise OR + 1 | 2 + 3 + + + # + integer bitwise XOR + 1 # 3 + 2 + + + + integer bitwise AND + 1 3 + 1 + + + ~ + integer bitwise NOT + ~ 1 + -2 + + + + integer bitwise shift left + 1 2 + 4 + + + + integer bitwise shift right + 8 2 + 2 + + + + + addition + 5 + 4 + 9 + + + - + substraction + 3 - 2.0 + 1.0 + + + * + multiplication + 5 * 4 + 20 +
[HACKERS] Re: [COMMITTERS] pgsql: Fix assorted portability issues in new pgbench TAP tests.
Hello,
Please find attached "blind" additional fixes for Windows & AIX.
- more nan/inf variants
- different message on non existing user
- illegal vs unrecognized options
I suspect that $windows_os is not true on "bowerbird", in order to fix it
the value of "$Config{osname}" is needed...
--
Fabien.diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 66df4bc..54a6039 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -73,7 +73,11 @@ pgbench(
1,
[qr{^$}],
[ qr{connection to database "template0" failed},
- qr{FATAL: role "no-such-user" does not exist} ],
+ # FATAL: role "no-such-user" does not exist
+ # FATAL: SSPI authentication failed for user "no-such-user"
+ qr{FATAL:.* (role|user) "no-such-user"},
+ qr{FATAL:.* (does not exist|authentication failed)}
+ ],
'no such user');
pgbench(
@@ -217,9 +221,9 @@ pgbench(
qr{command=18.: double 18\b},
qr{command=19.: double 19\b},
qr{command=20.: double 20\b},
- qr{command=21.: double -?nan}i,
- qr{command=22.: double inf}i,
- qr{command=23.: double -inf}i,
+ qr{command=21.: double (-?nan|-1\.#IND)}i,
+ qr{command=22.: double (inf|1\.#INF)}i,
+ qr{command=23.: double (-inf|-1\.#INF)}i,
qr{command=24.: int 9223372036854775807\b}, ],
'pgbench expressions',
{ '001_pgbench_expressions' => q{-- integer functions
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index 631aa73..d6b3d4f 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -26,7 +26,7 @@ my @options = (
# name, options, stderr checks
[ 'bad option',
'-h home -p 5432 -U calvin -d --bad-option',
- [ qr{unrecognized option}, qr{--help.*more information} ] ],
+ [ qr{(unrecognized|illegal) option}, qr{--help.*more information} ] ],
[ 'no file',
'-f no-such-file',
[qr{could not open file "no-such-file":}] ],
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench - allow to store select results into variables
Here is a v12.
There is no changes in the code or documentation, only TAP tests are
added.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index f5db8d1..9ad82d4 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -818,6 +818,51 @@ pgbench options dbname
+
+
+ \cset [prefix] or
+ \gset [prefix]
+
+
+
+
+ These commands may be used to end SQL queries, replacing a semicolon.
+ \cset replaces an embedded semicolon (\;) within
+ a compound SQL command, and \gset replaces a final
+ (;) semicolon which ends the SQL command.
+
+
+
+ When these commands are used, the preceding SQL query is expected to
+ return one row, the columns of which are stored into variables named after
+ column names, and prefixed with prefix if provided.
+
+
+
+ The following example puts the final account balance from the first query
+ into variable abalance, and fills variables
+ one, two and p_three with
+ integers from a compound query.
+
+UPDATE pgbench_accounts
+ SET abalance = abalance + :delta
+ WHERE aid = :aid
+ RETURNING abalance \gset
+-- compound of two queries
+SELECT 1 AS one, 2 AS two \cset
+SELECT 3 AS three \gset p_
+
+
+
+
+
+\cset and \gset commands do not work when
+empty SQL queries appear within a compound SQL command.
+
+
+
+
+
\set varname expression
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e37496c..454127c 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -375,11 +375,14 @@ static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
typedef struct
{
- char *line; /* text of command line */
+ char *line; /* first line for short display */
+ char *lines; /* full multi-line text of command */
int command_num; /* unique index of this Command struct */
int type; /* command type (SQL_COMMAND or META_COMMAND) */
int argc; /* number of command words */
char *argv[MAX_ARGS]; /* command word list */
+ int compound; /* last compound command (number of \;) */
+ char **gset; /* per-compound command prefix */
PgBenchExpr *expr; /* parsed expression, if needed */
SimpleStats stats; /* time spent in this command */
} Command;
@@ -1254,6 +1257,104 @@ getQueryParams(CState *st, const Command *command, const char **params)
params[i] = getVariable(st, command->argv[i + 1]);
}
+/* read all responses from backend */
+static bool
+read_response(CState *st, char **gset)
+{
+ PGresult *res;
+ int compound = 0;
+
+ while ((res = PQgetResult(st->con)) != NULL)
+ {
+ switch (PQresultStatus(res))
+ {
+ case PGRES_COMMAND_OK: /* non-SELECT commands */
+ case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */
+if (gset[compound] != NULL)
+{
+ fprintf(stderr,
+ "client %d file %d command %d compound %d: "
+ "\\gset expects a row\n",
+ st->id, st->use_file, st->command, compound);
+ st->ecnt++;
+ return false;
+}
+break; /* OK */
+
+ case PGRES_TUPLES_OK:
+if (gset[compound] != NULL)
+{
+ /* store result into variables */
+ int ntuples = PQntuples(res),
+ nfields = PQnfields(res),
+ f;
+
+ if (ntuples != 1)
+ {
+ fprintf(stderr,
+"client %d file %d command %d compound %d: "
+"expecting one row, got %d\n",
+st->id, st->use_file, st->command, compound, ntuples);
+ st->ecnt++;
+ PQclear(res);
+ discard_response(st);
+ return false;
+ }
+
+ for (f = 0; f < nfields ; f++)
+ {
+ char *varname = PQfname(res, f);
+ if (*gset[compound] != '\0')
+ varname = psprintf("%s%s", gset[compound], varname);
+
+ /* store result as a string */
+ if (!putVariable(st, "gset", varname,
+ PQgetvalue(res, 0, f)))
+ {
+ /* internal error, should it rather abort? */
+ fprintf(stderr,
+ "client %d file %d command %d compound %d: "
+ "error storing into var %s\n",
+ st->id, st->use_file, st->command, compound,
+ varname);
+ st->ecnt++;
+ PQclear(res);
+ discard_response(st);
+ return false;
+ }
+
+ if (*gset[compound] != '\0')
+ free(varname);
+ }
+}
+break; /* OK */
+
+ default:
+/* everything else is unexpected, so probably an error */
+fprintf(stderr,
+ "client %d file %d aborted in command %d compound %d: %s",
+ st->id, st->use_file, st->command, compound,
+ PQerrorMessage(st->con));
+st->ecnt++;
+PQclear(res);
+discard_response(st);
+return false;
+ }
+
+ PQclear(res);
+ compound += 1;
+ }
+
+ if (compound == 0)
+ {
+ fprintf(stderr, "client %d command %d: no results\n", st->id, st->command);
+ st->ecnt++;
+ return false;
+ }
+
+
Re: [HACKERS] pgbench tap tests & minor fixes.
Hello Tom,
Pushed, with some minor fooling with comments and after running it
through perltidy. (I have no opinions about Perl code formatting,
but perltidy does ...)
Why not. I do not like the result much, but it homogeneity is not a bad
thing.
The only substantive change I made was to drop the test that attempted
to connect to no-such-host.postgresql.org. That's (a) unnecessary,
as this is a test of pgbench not libpq; (b) likely to provoke a wide
range of platform-specific error messages, which we'd have to account
for given that the test is looking for specific output; and (c) likely
to draw complaints from buildfarm owners and packagers who do not like
test scripts that try to make random external network connections.
Ok. Note that it was only an unsuccessful DNS request, but I understand
the concern. The libpq tap test mostly focus on url parsing but seem to
include host names ("example.com"/host) and various IPs.
Like you, I'm a bit worried about the code for extracting an exit
status from IPC::Run::run. We'll have to keep an eye on the buildfarm
for a bit. If there's any trouble, I'd be inclined to drop it down
to just success/fail rather than checking the exact exit code.
Ok. If this occurs, there is a $windows_os variable that can be tested to
soften the test only if necessary, and keep the exact check for systems
that can.
Thanks for the review, the improvements and the push. Now the various
patches about pgbench in the queue should include tap-tests.
Hopefully one line will be greener on https://coverage.postgresql.org/.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql: new help related to variables are not too readable
Hello, PSQL_HISTORY alternative location for the command history file I would prefer to revert to that more compact 9.6-formatting. There was a problem with line width .. its hard to respect 80 chars Yes. Scrolling in two dimensions because it does not fit either way is not too practical, so the idea was the it should at least fit a reasonable terminal in the horizontal dimension, the vertical one having been unfittable anyway for a long time. Once you want to do strict 80 columns, a lot of/most descriptions do not fit and need to be split somehow on two lines, one way or another. It seemed that XXX xxx xx xxx xxx Is a good way to do that systematically and with giving more space and chance for a description to fit in its line. ISTM that it was already done like for environment variables, so it is also for homogeneity. It also simplify work for translators that can just follow the same formatting and know what to do if a one line English explanation does not fit once translated. Finally, as vertical scrolling is mandatory, I would be fine with skipping lines with entries for readability, but it is just a matter of taste and I expect there should be half a dozen different opinions on the matter of formatting. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Pavel, I checked performance - the most fast queries are execution of simple prepared statement prepare x as select 1; -- 100 x execute x; execute x; execute x; execute x; ## patched [pavel@nemesis postgresql]$ time psql -At -1 postgres -f ~/xxx.sql > /dev/null real 0m44,887s user 0m11,703s sys 0m6,942s This is probably the most worst case, what is possible and see some slowdown - in this case there is about 10% slowdown - but it is pretty untypical - the one query was less than 50 microsec. When there will be any IO activity or network usage, than this patch doesn't create any significant overhead. Interesting. Thanks for the test. I tried to replicate with a variant without any output: "SELECT;" SELECT NOW() AS start \gset BEGIN; SELECT; -- 2^19 times END; SELECT NOW() - :'start'; The run time is about 19 µs per SELECT on my laptop. Over 33 runs each alternating master with and without the patch, I got the following stats on the measured time in seconds (average, stddev [min Q1 median Q3 max]): - with : 9.898 ± 0.158 [9.564, 9.762, 9.936, 10.037, 10.108] - without: 9.419 ± 0.294 [8.670, 9.226, 9.533, 9.625, 9.845] This seems consistent and significant. It suggests a 0.40-0.50 s difference, that is about 5%, i.e. about (under) 1 µs overhead per statement in pretty defavorable circumstances. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Very very minor comments that I should have noticed before, sorry for this additional round trip. Thank you for the dedicated review! I'm someone at times pigheaded, I think in the good sense if it is possible, and I like to finish what I start:-) Patch applies, compiles, works, everything is fine from my point of view. I switched it to "Ready for Committer". Again, if the pgbench tap test patch get through, it should be tap tested. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] merge psql ef/ev sf/sv handling functions
you can't do this sort of thing:
-psql_error("The server (version %s) does not support editing function
source.\n",
+psql_error("The server (version %s) does not support editing
%s.\n",
formatPGVersionNumber(pset.sversion, false,
- sverbuf, sizeof(sverbuf)));
+ sverbuf, sizeof(sverbuf)),
+ is_func ? "function source" : "view definitions");
It's too much of a pain in the rear for translators.
Argh, indeed, I totally forgot about translations. Usually there is a _()
hint for gettext.
See
https://www.postgresql.org/docs/devel/static/nls-programmer.html#nls-guidelines
Usually we just use two independent messages, and that's what I did.
Yep, makes sense. Thanks for the fix.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add psql variables showing server version and psql version.
Thus short, simple but meaningful examples which show how to do something useful with all that in the documentation may help people take advantage of these new features. I don't have an objection to providing an example. I wasn't terribly impressed with Simon's version, but maybe we can do better. That was just a quick idea to show how they could be used, probably it can be improved. I think it should be concrete, not partly abstract; and likely it needs to be with \if not the variable proper. ISTM that currently there is no "not". Maybe I do not understand your sentence. Given my experience with "\d*", I'm not sure I would assume that a new psql feature would work with older servers. I don't recall anyone questioning whether PQserverVersion() would work with older servers. Being able to tell what version the server is is sort of the point, no? If there were a restriction on what it would work with, I would agree that that needs to be documented. But I don't think "yes, it really works" is a helpful use of documentation space. Sure. I can use psql without knowing that there is a libpq underneath, that it contains a PQserverVersion function implemented since pg7, and that the SERVER_VERSION_* variables introduced in pg10 or pg11 rely on that thus it should work with pg7/8/9 as well. It is not obvious, as a new feature could depend on any combination of server side functions, protocol extension, client library functions or client code... -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add psql variables showing server version and psql version.
Hello, * Clarification that this will work for current AND past server versions The short answer is it works. I do not think we need a longer answer. To have something operational you have to know quite a bit of psql details (:-substitutions, backslash command logic, gset, if, quit...). Thus short, simple but meaningful examples which show how to do something useful with all that in the documentation may help people take advantage of these new features. Given my experience with "\d*", I'm not sure I would assume that a new psql feature would work with older servers. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
Seeing it as is, it calls for having "SERVER_VERSION" as well, but I'm not
sure of the better way to get it. I tried with "SELECT VERSION() AS
SERVER_VERSION \gset" but varnames are lowerized.
The problem there is you can't get version() without an extra round trip
to the server --- and an extra logged query --- which people are going to
complain about.
Here is a PoC that does it through a guc, just like "server_version" (the
short version) is transmitted, with a fallback if it is not there.
Whether it is worth it is debatable, but I like the symmetry of having
the same informations accessible the same way for client and server sides.
--
Fabien.diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 5f59a38..8b69ed1 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7961,8 +7961,8 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
-Reports the version number of the server. It is determined by the
-value of PG_VERSION when building the server.
+Reports the version number of the server as a short string. It is determined
+by the value of PG_VERSION when building the server.
@@ -7981,6 +7981,20 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
+
+ server_version_raw (string)
+
+ server_version_raw configuration parameter
+
+
+
+
+Reports the version of the server as a long string. It is determined
+by the value of PG_VERSION_STR when building the server.
+
+
+
+
wal_block_size (integer)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 5bdbc1e..1be57d2 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3690,11 +3690,14 @@ bar
+SERVER_VERSION
SERVER_VERSION_NAME
SERVER_VERSION_NUM
-The server's version number as a string, for
+The server's version number as a long string, for
+example PostgreSQL 11devel ...,
+as a short string, for
example 9.6.2, 10.1 or 11beta1,
and in numeric form, for
example 90602 or 11.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 246fea8..fd843d4 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -500,6 +500,7 @@ static char *locale_collate;
static char *locale_ctype;
static char *server_encoding_string;
static char *server_version_string;
+static char *server_version_raw_string;
static int server_version_num;
static char *timezone_string;
static char *log_timezone_string;
@@ -3298,6 +3299,18 @@ static struct config_string ConfigureNamesString[] =
},
{
+ /* Can't be set in postgresql.conf */
+ {"server_version_raw", PGC_INTERNAL, PRESET_OPTIONS,
+ gettext_noop("Shows the server version string."),
+ NULL,
+ GUC_REPORT | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+ },
+ _version_raw_string,
+ PG_VERSION_STR,
+ NULL, NULL, NULL
+ },
+
+ {
/* Not for general use --- used by SET ROLE */
{"role", PGC_USERSET, UNGROUPED,
gettext_noop("Sets the current role."),
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index fe0b83e..e2ba8ee 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3358,7 +3358,8 @@ void
SyncVariables(void)
{
char vbuf[32];
- const char *server_version;
+ const char *server_version,
+ *server_version_raw;
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
@@ -3385,6 +3386,17 @@ SyncVariables(void)
snprintf(vbuf, sizeof(vbuf), "%d", pset.sversion);
SetVariable(pset.vars, "SERVER_VERSION_NUM", vbuf);
+ server_version_raw = PQparameterStatus(pset.db, "server_version_raw");
+ /* fall back again */
+ if (!server_version_raw)
+ {
+ snprintf(vbuf, sizeof(vbuf), "PostgreSQL ");
+ formatPGVersionNumber(pset.sversion, true, vbuf + strlen(vbuf),
+ sizeof(vbuf) - strlen(vbuf));
+ server_version_raw = vbuf;
+ }
+ SetVariable(pset.vars, "SERVER_VERSION", server_version_raw);
+
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
@@ -3403,6 +3415,7 @@ UnsyncVariables(void)
SetVariable(pset.vars, "HOST", NULL);
SetVariable(pset.vars, "PORT", NULL);
SetVariable(pset.vars, "ENCODING", NULL);
+ SetVariable(pset.vars, "SERVER_VERSION", NULL);
SetVariable(pset.vars, "SERVER_VERSION_NAME", NULL);
SetVariable(pset.vars, "SERVER_VERSION_NUM", NULL);
}
diff --git a/src/interfaces/libpq/fe-protocol2.c b/src/interfaces/libpq/fe-protocol2.c
index a58f701..4aa18fd 100644
--- a/src/interfaces/libpq/fe-protocol2.c
+++ b/src/interfaces/libpq/fe-protocol2.c
@@ -281,6 +281,10 @@ pqSetenvPoll(PGconn *conn)
{
char *ptr;
+ /* keep
Re: [HACKERS] psql - add special variable to reflect the last query status
Here is a version 6.
Small v7 update, sorry for the noise.
Add testing the initial state of all variables.
Fix typos in a comment in tests.
Fix the documentation wrt the current implementation behavior.
--
Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 5bdbc1e..97962d4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3475,6 +3475,16 @@ bar
+ ERROR
+
+
+ Whether the last query failed, as a boolean.
+ See also SQLSTATE.
+
+
+
+
+
FETCH_COUNT
@@ -3611,6 +3621,18 @@ bar
+ LAST_ERROR_SQLSTATE
+ LAST_ERROR_MESSAGE
+
+
+ The error code and associated error message of the last
+ error, or "0" and empty strings if no error occured
+ since the beginning of the script.
+
+
+
+
+
ON_ERROR_ROLLBACK
@@ -3679,6 +3701,25 @@ bar
+ ROW_COUNT
+
+
+ How many rows were returned or affected by the last query.
+
+
+
+
+
+ SQLSTATE
+
+
+ The error code associated to the last query, or
+ 0 if no error occured.
+
+
+
+
+
QUIET
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index b997058..93950fd 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -493,7 +493,6 @@ ResetCancelConn(void)
#endif
}
-
/*
* AcceptResult
*
@@ -1107,6 +1106,35 @@ ProcessResult(PGresult **results)
first_cycle = false;
}
+ /*
+ * Set special variables
+ * - ERROR: TRUE/FALSE, whether an error occurred
+ * - SQLSTATE: code of error, or "0"
+ * - LAST_ERROR_SQLSTATE: same for last error
+ * - LAST_ERROR_MESSAGE: message of last error
+ * - ROW_COUNT: how many rows were returned or affected, or "0"
+ */
+ if (success)
+ {
+ char *ntuples = PQcmdTuples(*results);
+ SetVariable(pset.vars, "ERROR", "FALSE");
+ SetVariable(pset.vars, "SQLSTATE", "0");
+ SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0");
+ }
+ else
+ {
+ char *code = PQresultErrorField(*results, PG_DIAG_SQLSTATE);
+ char *mesg = PQresultErrorField(*results, PG_DIAG_MESSAGE_PRIMARY);
+
+ SetVariable(pset.vars, "ERROR", "TRUE");
+ /* if an error was detected, it must have a code! */
+ Assert(code != NULL);
+ SetVariable(pset.vars, "SQLSTATE", code);
+ SetVariable(pset.vars, "LAST_ERROR_SQLSTATE", code);
+ SetVariable(pset.vars, "LAST_ERROR_MESSAGE", mesg ? mesg : "");
+ SetVariable(pset.vars, "ROW_COUNT", "0");
+ }
+
/* may need this to recover from conn loss during COPY */
if (!first_cycle && !CheckConnection())
return false;
@@ -1214,7 +1242,6 @@ PrintQueryResults(PGresult *results)
return success;
}
-
/*
* SendQuery: send the query string to the backend
* (and print out results)
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 4d1c0ec..ae951f5 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -337,7 +337,7 @@ helpVariables(unsigned short int pager)
* Windows builds currently print one more line than non-Windows builds.
* Using the larger number is fine.
*/
- output = PageOutput(147, pager ? &(pset.popt.topt) : NULL);
+ output = PageOutput(155, pager ? &(pset.popt.topt) : NULL);
fprintf(output, _("List of specially treated variables\n\n"));
@@ -360,6 +360,8 @@ helpVariables(unsigned short int pager)
"if set to \"noexec\", just show them without execution\n"));
fprintf(output, _(" ENCODING\n"
"current client character set encoding\n"));
+ fprintf(output, _(" ERROR\n"
+ "whether the last query failed\n"));
fprintf(output, _(" FETCH_COUNT\n"
"the number of result rows to fetch and display at a time (0 = unlimited)\n"));
fprintf(output, _(" HISTCONTROL\n"
@@ -374,6 +376,9 @@ helpVariables(unsigned short int pager)
"number of EOFs needed to terminate an interactive session\n"));
fprintf(output, _(" LASTOID\n"
"value of the last affected OID\n"));
+ fprintf(output, _(" LAST_ERROR_SQLSTATE\n"
+ " LAST_ERROR_MESSAGE\n"
+ "error code and message of last error, or \"0\" and empty if none\n"));
fprintf(output, _(" ON_ERROR_ROLLBACK\n"
"if set, an error doesn't stop a transaction (uses implicit savepoints)\n"));
fprintf(output, _(" ON_ERROR_STOP\n"
@@ -388,6 +393,8 @@ helpVariables(unsigned short int pager)
"specifies the prompt used during COPY ... FROM STDIN\n"));
fprintf(output, _(" QUIET\n"
"run quietly (same as -q option)\n"));
+ fprintf(output, _(" ROW_COUNT\n"
+ "number of rows of last query, or 0\n"));
fprintf(output, _(" SERVER_VERSION_NAME\n"
" SERVER_VERSION_NUM\n"
"server's version (in
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Tom,
Here is a version 6.
A few thoughts about this patch:
* I think the ERROR_CODE variable should instead be named SQLSTATE.
That is what the SQL standard calls this string, and it's also what
just about all our documentation calls it; see PG_DIAG_SQLSTATE
in libpq, or the SQLSTATE 'x' construct in pl/pgsql, or the
sqlstate attribute of an exception object in plpython, etc etc.
ERROR_CODE -> SQLSTATE.
* I'm not exactly convinced that there's a use-case for STATUS
Removed, but I think it was nice to have, it is easier to interpret than
error codes and their classes that I have not memorized yet:-)
* It might be better if SQLSTATE and ERROR_MESSAGE were left
unchanged by a non-error query. That would reduce the need to
copy them into other variables just because you needed to do
something else before printing them. It'd save a few cycles too.
Added LAST_ERROR_SQLSTATE & MESSAGE, only reset when an error occured.
* Speaking of which, has anyone tried to quantify the performance
impact of this patch? It might well be negligible, but I do not
think we should assume that without evidence.
My guess is negligible. Not sure how to measure this negligible, as many
very fast query should be executed to have something significant. Maybe
100,000 "SELECT 1;" in a script?
* I wonder why you didn't merge this processing into ProcessResult,
instead of inventing an extra function (whose call site seems rather
poorly chosen anyhow --- what's the justification for not counting this
overhead as part of the query runtime?). You could probably eliminate
the refactoring you did, since it wouldn't be necessary to recompute
AcceptResult's result that way.
Variable setting moved at then end of ProcessResult, no new functions,
result is clean, so I should have done it like that in the beginning.
Forgotten help stuff added.
--
Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 5bdbc1e..0bbe1c9 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3475,6 +3475,16 @@ bar
+ ERROR
+
+
+ Whether the last query failed, as a boolean.
+ See also SQLSTATE.
+
+
+
+
+
FETCH_COUNT
@@ -3611,6 +3621,18 @@ bar
+ LAST_ERROR_SQLSTATE
+ LAST_ERROR_MESSAGE
+
+
+ The error code and associated error message of the last
+ error, or empty strings if no error occured since the
+ beginning of the script.
+
+
+
+
+
ON_ERROR_ROLLBACK
@@ -3679,6 +3701,25 @@ bar
+ ROW_COUNT
+
+
+ How many rows were returned or affected by the last query.
+
+
+
+
+
+ SQLSTATE
+
+
+ The error code associated to the last query, or
+ 0 if no error occured.
+
+
+
+
+
QUIET
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index b997058..93950fd 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -493,7 +493,6 @@ ResetCancelConn(void)
#endif
}
-
/*
* AcceptResult
*
@@ -1107,6 +1106,35 @@ ProcessResult(PGresult **results)
first_cycle = false;
}
+ /*
+ * Set special variables
+ * - ERROR: TRUE/FALSE, whether an error occurred
+ * - SQLSTATE: code of error, or "0"
+ * - LAST_ERROR_SQLSTATE: same for last error
+ * - LAST_ERROR_MESSAGE: message of last error
+ * - ROW_COUNT: how many rows were returned or affected, or "0"
+ */
+ if (success)
+ {
+ char *ntuples = PQcmdTuples(*results);
+ SetVariable(pset.vars, "ERROR", "FALSE");
+ SetVariable(pset.vars, "SQLSTATE", "0");
+ SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0");
+ }
+ else
+ {
+ char *code = PQresultErrorField(*results, PG_DIAG_SQLSTATE);
+ char *mesg = PQresultErrorField(*results, PG_DIAG_MESSAGE_PRIMARY);
+
+ SetVariable(pset.vars, "ERROR", "TRUE");
+ /* if an error was detected, it must have a code! */
+ Assert(code != NULL);
+ SetVariable(pset.vars, "SQLSTATE", code);
+ SetVariable(pset.vars, "LAST_ERROR_SQLSTATE", code);
+ SetVariable(pset.vars, "LAST_ERROR_MESSAGE", mesg ? mesg : "");
+ SetVariable(pset.vars, "ROW_COUNT", "0");
+ }
+
/* may need this to recover from conn loss during COPY */
if (!first_cycle && !CheckConnection())
return false;
@@ -1214,7 +1242,6 @@ PrintQueryResults(PGresult *results)
return success;
}
-
/*
* SendQuery: send the query string to the backend
* (and print out results)
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 4d1c0ec..ae951f5 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -337,7 +337,7 @@ helpVariables(unsigned short int pager)
* Windows builds currently print one more line than non-Windows builds.
* Using the larger number is fine.
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Applies, compiles, works for me. Very very minor comments that I should have noticed before, sorry for this additional round trip. In the help line, move -I just after -i, to put short options in alphabetical and decreasing importance order. On this line, also add the information about the default, something like: -i, --ini... -I, --custom=[...]+ (default "ctgvp") ... When/if the pgbench tap test patch get through, the feature should be tested there as well. No action needed now. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] Zipfian distribution in pgbench
Hello Alik, Applies, compiles, works for me. Some minor comments and suggestions. Two typos: - "usinng" -> "using" - "a rejection method used" -> "a rejection method is used" I'm not sure of "least_recently_used_i", this naming style is not used in pgbench. "least_recently_used" would be ok. "..nb_cells.. != ZIPF_CACHE_SIZE", ISTM that "<" is more logical, even if the result is the same? I would put the parameter value check in getZipfianRand, so that if someone reuse the function elsewhere the check is also performed. That would also simplify a bit the already very large expression evaluation function. When/if the pgbench tap test patch get through, coverage tests should be added. Maybe the cache overflow could be counted, to allow for a possible warning message in the final report? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add special variable to reflect the last query status
* It might be better if SQLSTATE and ERROR_MESSAGE were left unchanged by a non-error query. Hmmm. I'm not sure. If so, ERROR_MESSAGE should be LAST_ERROR_MESSAGE maybe? Then what about LAST_ERROR_SQLSTATE to go with it, and let SQLSTATE & ERROR_MESSAGE reflect the last command, and ERROR is just the boolean to test if it occured? That would reduce the need to copy them into other variables just because you needed to do something else before printing them. It'd save a few cycles too. Well, my suggestion would mean that they would be copied when an error occurs, but only when it occurs, which would not be often. Uh ... what? Sorry if my sentence was not very clear. Time to go do bed:-) I just mean that a LAST_ERROR_* would be set when an error occurs. When there is no error, it is expected to remain the same, and it does not cost anything to let it as is. If an error occured then you had a problem, a transaction aborted, paying to set a few variables when it occurs does not look like a big performance issue. Script usually expect to run without error, errors are rare events. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add psql variables showing server version and psql version.
Huh? The variable will be set in any case. It should be correct for any server version we might find in the wild --- so far as I can tell from the commit history, every version supporting FE/BE protocol version 3 has sent server_version at connection start. With a pre-7.4 server, it looks like the variables would be set to "0.0.0" and "0" respectively. Scratch that: experimentation says it works fine with older servers too. The oldest one I have in captivity reports Ok, be it means a recent psql connecting to an old server. It does not work with an old psql. SERVER_VERSION_NAME = '7.0.3' SERVER_VERSION_NUM = '70003' Looking more closely, I see that when using protocol version 2, libpq will issue a "select version()" command at connection start to get this info. Then it could be used for free to set SERVER_VERSION if it can be extracted from libpq somehow?! -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add psql variables showing server version and psql version.
Hello Simon, Does raise the further question of how psql behaves when we connect to a pre-10 server, so we have SERVER_VERSION_NUM but yet it is not set. How does this \if SERVER_VERSION_NUM < x The if does not have expressions (yet), it just handles TRUE/ON/1 and FALSE/0/OFF. Do we need some macro or suggested special handling? If "SERVER_VERSION_NUM" is available in 10, then: -- exit if version < 10 (\if is ignored and \q is executed) \if false \echo "prior 10" \q \endif -- then test version through a server side expression, will work SELECT :SERVER_VERSION_NUM < 11 AS prior_11 \gset \if :prior_11 -- version 10 \else -- version 11 or more \endif -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql - add special variable to reflect the last query status
Hello Tom, * I think the ERROR_CODE variable should instead be named SQLSTATE. That is what the SQL standard calls this string, and it's also what just about all our documentation calls it; see PG_DIAG_SQLSTATE in libpq, or the SQLSTATE 'x' construct in pl/pgsql, or the sqlstate attribute of an exception object in plpython, etc etc. I choose ERROR_CODE because it matched the ERROR boolean. But is may be a misnomer if the status is that all is well. I'm okay with SQLSTATE. * I'm not exactly convinced that there's a use-case for STATUS that's not covered as well or better by ERROR. Client code that looks at PQresStatus for anything beyond error/not-error is usually doing that because it's library code that doesn't know what kind of query it's working on. It seems like a stretch that a psql script would not know that. Also, PQresultStatus memorializes some legacy distinctions, like "fatal" vs "nonfatal" error, that I think we'd be better off not exposing in psql scripting. Ok. * It might be better if SQLSTATE and ERROR_MESSAGE were left unchanged by a non-error query. Hmmm. I'm not sure. If so, ERROR_MESSAGE should be LAST_ERROR_MESSAGE maybe? Then what about LAST_ERROR_SQLSTATE to go with it, and let SQLSTATE & ERROR_MESSAGE reflect the last command, and ERROR is just the boolean to test if it occured? That would reduce the need to copy them into other variables just because you needed to do something else before printing them. It'd save a few cycles too. Well, my suggestion would mean that they would be copied when an error occurs, but only when it occurs, which would not be often. If you would like them, I'm not sure how these variable should be initialized. Undefined? Empty? * Speaking of which, has anyone tried to quantify the performance impact of this patch? It might well be negligible, but I do not think we should assume that without evidence. I think it should be negligible compared to network connections, aborting an ongoing transaction, reading the script... But I do not know libpq internals so I may be quite naive. * I wonder why you didn't merge this processing into ProcessResult, instead of inventing an extra function (whose call site seems rather poorly chosen anyhow --- what's the justification for not counting this overhead as part of the query runtime?). You could probably eliminate the refactoring you did, since it wouldn't be necessary to recompute AcceptResult's result that way. Hmmm. I assume that you are unhappy about ResultIsSuccess. The refactoring is because the function is used twice. I choose to do that because the functionality is clear and could be made as a function which improved readability. Ok, PQresultStatus is thus called twice, I assumed that it is just reading a field in a struct... it could be returned to the caller with an additional pointer to avoid that. The SendResult & ProcessResult functions are already quite heavy to my taste, I did not want to add significantly to that. The ProcessResult switch does not test all states cleanly, it is really about checking about copy, and not so clear, so I do not think that it would mix well to add the variable stuff in the middle of that. SendQuery is also pretty complex, including gotos everywhere. So I did want to add to these two functions beyond the minimum. Now, I can also inline everything coldly in ProcessResult, no problem. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add psql variables showing server version and psql version.
This is good. Please can we backpatch these are far as they will go (easily)? There is very little risk in doing so and significant benefits in being able to rely on scripts that know about versions. Hm. I think it would be a fine idea to push this change into v10, so that all psql versions that have \if would have these variables. However, I'm less enthused about adding them to the 9.x branches. Given the lack of \if, I'm not seeing a use-case that would justify taking any compatibility risk for. Opinions anyone? I think that it is harmless and useful for v10, and mostly useless for previous versions. ISTM that the hack looking like: \if false \echo BAD VERSION \q \endif Allow to test a prior 10 version and stop. However testing whether a variable is defined is not included yet, so differenciating between 10 and 11 would not be easy... thus having 10 => version available would be significantly helpful for scripting. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
[ psql-version-num-8.patch ] Pushed with some minor additional fiddling. Ok, Thanks. I also noticed the reformating. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Sorry, I don't follow that. You meant I should add a newline before pg_realloc()? That is, +initialize_cmds = +(char *) pg_realloc(initialize_cmds, +sizeof(char) * n_cmds + 1); Yes. Or maybe my terminal was doing tricks, because I had the impression that both argument where on the same line with many tabs in between, but maybe I just misinterpreted the diff file. My apology if it is the case. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Attached the latest patch. Please review it. Patch applies and compiles cleanly. Three very minor points: Typo added in the documentation: ".Each" -> ". Each". In "case 8:" there is a very long line which lacks a newline before pg_realloc second argument. I think that the check should silently accept spaces as they are ignored by init later, i.e.: sh> ./pgbench -i -I "ctg vpf" invalid custom initialization script command " " possible commands are: "c", "t", "g", "v", "p", "f" Could just work... -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
Hello Tom, sub pgbench($) My concern is basically about maintaining coding style consistency. Yes, I'm fine with that. I have removed it in the v12 patch. reasons why it's not like that already. I do have to say that many of the examples I've seen look more like line noise than readable code. Sure. I agree that the readability is debatable. The usefulness is only that an error is raised at "compile" time instead of having a strange behavior at runtime. I run the test, figure out the number it found in the resulting error message, and update the number in the source. Yeah, but then what error is all that work protecting you from? I'm not sure I understand your point. I agree that Perl doing the counting may hide issues. Now it is more of an incremental thing, if a test is added the counter is upgraded accordingly, and the local consistency can be checked. Anyway, as some tests may have to be skipped on some platforms, it seems that the done_testing approach is sounder. The v12 patch uses that. Thanks for your comments. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
Anyway, done with the addition of a "chomp" parameter, leaving only the
TAP test changes to consider.
Ok, thanks.
I'll set the CF entry back to "waiting on author" pending your
revisions of those.
See attached.
Removed scalar/array ref hack, plenty [] added everywhere to match.
Removed perl function prototypes.
Does not try to announce the number of tests, just tell when done.
--
Fabien.diff --git a/src/bin/pgbench/t/001_pgbench.pl b/src/bin/pgbench/t/001_pgbench.pl
deleted file mode 100644
index 34d686e..000
--- a/src/bin/pgbench/t/001_pgbench.pl
+++ /dev/null
@@ -1,25 +0,0 @@
-use strict;
-use warnings;
-
-use PostgresNode;
-use TestLib;
-use Test::More tests => 3;
-
-# Test concurrent insertion into table with UNIQUE oid column. DDL expects
-# GetNewOidWithIndex() to successfully avoid violating uniqueness for indexes
-# like pg_class_oid_index and pg_proc_oid_index. This indirectly exercises
-# LWLock and spinlock concurrency. This test makes a 5-MiB table.
-my $node = get_new_node('main');
-$node->init;
-$node->start;
-$node->safe_psql('postgres',
- 'CREATE UNLOGGED TABLE oid_tbl () WITH OIDS; '
- . 'ALTER TABLE oid_tbl ADD UNIQUE (oid);');
-my $script = $node->basedir . '/pgbench_script';
-append_to_file($script,
- 'INSERT INTO oid_tbl SELECT FROM generate_series(1,1000);');
-$node->command_like(
- [ qw(pgbench --no-vacuum --client=5 --protocol=prepared
- --transactions=25 --file), $script ],
- qr{processed: 125/125},
- 'concurrent OID generation');
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
new file mode 100644
index 000..5db2fd0
--- /dev/null
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -0,0 +1,441 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More;
+
+# start a pgbench specific server
+my $node = get_new_node('main');
+$node->init;
+$node->start;
+
+# invoke pgbench
+sub pgbench
+{
+ my ($opts, $stat, $out, $err, $name, $files) = @_;
+ my @cmd = ('pgbench', split /\s+/, $opts);
+ my @filenames = ();
+ if (defined $files)
+ {
+ # note: files are ordered for determinism
+ for my $fn (sort keys %$files)
+ {
+ my $filename = $node->basedir . '/' . $fn;
+ push @cmd, '-f', $filename;
+ # cleanup file weight
+ $filename =~ s/\@\d+$//;
+ #push @filenames, $filename;
+ append_to_file($filename, $$files{$fn});
+ }
+ }
+ $node->command_checks_all(\@cmd, $stat, $out, $err, $name);
+ # cleanup?
+ #unlink @filenames or die "cannot unlink files (@filenames): $!";
+}
+
+# Test concurrent insertion into table with UNIQUE oid column. DDL expects
+# GetNewOidWithIndex() to successfully avoid violating uniqueness for indexes
+# like pg_class_oid_index and pg_proc_oid_index. This indirectly exercises
+# LWLock and spinlock concurrency. This test makes a 5-MiB table.
+
+$node->safe_psql('postgres',
+ 'CREATE UNLOGGED TABLE oid_tbl () WITH OIDS; '
+ . 'ALTER TABLE oid_tbl ADD UNIQUE (oid);');
+
+pgbench('--no-vacuum --client=5 --protocol=prepared --transactions=25',
+ 0, [ qr{processed: 125/125} ], [ qr{^$} ], 'concurrency OID generation',
+ { '001_pgbench_concurrent_oid_generation' =>
+'INSERT INTO oid_tbl SELECT FROM generate_series(1,1000);' });
+
+# cleanup
+$node->safe_psql('postgres', 'DROP TABLE oid_tbl;');
+
+# Trigger various connection errors
+pgbench('no-such-database',
+ 1, [ qr{^$} ],
+ [ qr{connection to database "no-such-database" failed},
+qr{FATAL: database "no-such-database" does not exist} ],
+ 'no such database');
+
+pgbench('-U no-such-user template0',
+ 1, [ qr{^$} ],
+ [ qr{connection to database "template0" failed},
+qr{FATAL: role "no-such-user" does not exist} ],
+ 'no such user');
+
+pgbench('-h no-such-host.postgresql.org',
+ 1, [ qr{^$} ],
+ [ qr{connection to database "postgres" failed},
+# actual message may vary:
+# - Name or service not known
+# - Temporary failure in name resolution
+qr{could not translate host name "no-such-host.postgresql.org" to address: } ],
+ 'no such host');
+
+pgbench('-S -t 1',
+ 1, [ qr{^$} ], [ qr{Perhaps you need to do initialization} ],
+ 'run without init');
+
+# Initialize pgbench tables scale 1
+pgbench('-i',
+ 0, [ qr{^$} ],
+ [ qr{creating tables}, qr{vacuum}, qr{set primary keys}, qr{done\.} ],
+ 'pgbench scale 1 initialization',
+);
+
+# Again, with all possible options
+pgbench(
+ # unlogged => faster test
+ '--initialize --scale=1 --unlogged --fillfactor=98 --foreign-keys --quiet' .
+ ' --tablespace=pg_default --index-tablespace=pg_default',
+ 0, [ qr{^$}i ],
+ [ qr{creating tables}, qr{vacuum}, qr{set primary keys},
+qr{set foreign keys}, qr{done\.} ],
+ 'pgbench scale 1 initialization');
+
+# Run all builtins for a few transactions: 20 checks
+pgbench(
+ '--transactions=5 -Dfoo=bla --client=2 --protocol=simple --builtin=t' .
+ ' --connect -n -v -n',
+ 0,
+ [ qr{builtin: TPC-B}, qr{clients: 2\b}, qr{processed: 10/10},
+
Re: [HACKERS] pgbench - minor fix for meta command only scripts
Hello Jeff,
I have fixed a bug introduced in the patch by changing && by || in the
(min_sec > 0 && maxsock != -1) condition which was inducing errors with
multi-threads & clients...
Since this commit (12788ae49e1933f463bc5), if I use the --rate to throttle
the transaction rate, it does get throttled to about the indicated speed,
but the pg_bench consumes the entire CPU.
At the block of code starting
if (min_usec > 0 && maxsock != -1)
If maxsock == -1, then there is no sleep happening.
Argh, shame on me:-(
I cannot find the "induced errors" I was refering to in the message...
Sleeping is definitely needed to avoid a hard loop.
Patch attached fixes it and does not seem introduce any special issue...
Should probably be backpatched.
Thanks for the debug!
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 364e254..3e23a6a 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4587,7 +4587,7 @@ threadRun(void *arg)
* or it's time to print a progress report. Update input_mask to show
* which client(s) received data.
*/
- if (min_usec > 0 && maxsock != -1)
+ if (min_usec > 0 || maxsock != -1)
{
int nsocks; /* return from select(2) */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
I think we should go with Daniel's idea for all three parts. I'm okay with that, although probably it should be an independent patch. In the documentation, I do not think that both SERVER_VERSION_NAME and _NUM (or whatever their chosen name) deserve two independent explanations with heavy repeats just one after the other, and being treated differently from VERSION_*. I started with it that way, but it seemed pretty unreadable with the parenthetical examples added. And I think we need the examples, particularly the one pointing out that you might get something like "beta". Yes for "beta" which is also in the v8 patch I sent. One shared doc with different examples does not look too bad to me, and having things repeated so closely do not look good. Seeing it as is, it calls for having "SERVER_VERSION" as well, but I'm not sure of the better way to get it. I tried with "SELECT VERSION() AS SERVER_VERSION \gset" but varnames are lowerized. The problem there is you can't get version() without an extra round trip to the server --- and an extra logged query --- which people are going to complain about. Yep. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
Hello Tom, As for included bug fixes, I can do separate patches, but I think that it is enough to first commit the pgbench files and then the tap-test files, in that order. I'll see what committers say. Starting to look at this. I concur that the bug fixes ought to be committed separately, but since they're in separate files it's not hard to disassemble the patch. Ok. A couple of thoughts -- * I do not think we need expr_scanner_chomp_substring. Of the three existing callers of expr_scanner_get_substring, one is doing a manual chomp afterwards, and the other two need chomps per your patch. Seems to me we should just include the chomp logic in expr_scanner_get_substring. Maybe it'd be worth adding a "bool chomp" argument to it, but I think that would be more for clarity than usefulness. Ok. I thought that I would get a slap on the hand if I changed the initial function, but I get one not for changing it:-) * I do not like the API complexity of command_checks_all, specifically not the business of taking either a scalar or an array for the cmd, out, and err arguments. I think it'd be clearer and less bug-prone to just take arrays, full stop. Maybe it's just that I'm a crummy Perl programmer, but I do not see uses of this "ref ... eq 'ARRAY'" business elsewhere in our test scaffolding, and this doesn't seem like a great place to introduce it. At worst you'd need to add brackets around the arguments in a few callers. Hmmm. I find it quite elegant and harmless, but it can be removed. * In the same vein, I don't know what this does: sub pgbench($) and I see no existing instances of it elsewhere in our tree. I think it'd be better not to require advanced degrees in Perl programming in order to read the test cases. It just says that 5 scalars are expected, so it would complain if "type" or number do not match. Now, why give type hints if they are not mandatory, as devs can always detect their errors by extensive testing instead:-) But I agree that it is not a usual Perl stance and it can be removed. * Another way in which command_checks_all introduces API complexity is that it accounts for a variable number of tests depending on how many patterns are provided. This seems like a mess. I see that you have in some places (not very consistently) annotated call sites as to how many tests they account for, but who's going to do the math to make sure everything adds up? Perl:-) I run the test, figure out the number it found in the resulting error message, and update the number in the source. Not too hard:-) Maybe it'd be better to not use like(), or do something else so that each command_checks_all call counts as one Test::More test rather than N. Or just forget prespecifying a test count and use done_testing instead. Yep, "done_testing" looks fine, I'll investigate that, but other test seemed to insist on declaring the expected number. Now "done_testing" may be a nicer option if some tests need to be skipped on some platform. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
Hello Tom,
So I thought we were done bikeshedding the variable names for this
feature, but as I was reviewing the patch with intent to commit,
I noticed you hadn't updated helpVariables() to mention them.
Indeed.
Possibly you missed this because it doesn't mention VERSION either,
but that doesn't seem very defensible.
Long time ago. Maybe I greped for it to check where it was appearing and
did not find what does not exist...
I inserted text to describe all five variables --- but
"SERVER_VERSION_NAME" is too long to fit in the available column space.
Indeed.
In the attached updated patch, I moved all the descriptive text over one
column, and really should have moved it over two columns; but adding even
one space makes a couple of the lines longer than 80 columns when they
were not before. Since we've blown past 80 columns on some of the other
output, maybe that doesn't matter. Or maybe we should shorten this
variable name so it doesn't force reformatting of all this text.
It seems that PSQL_EDITOR_LINENUMBER_ARG (25 characters) has been accepted
before for an environment variable.
Possible ideas include "DB_VERSION_NAME", "SERVER_VER_NAME", or
"SERVER_VERSION_STR". (The last saves only one character, whereas
we really need to save two if we're trying not to be wider than any
other documented variable.)
Thoughts?
Like Pavel, I must admit that I do not like these options much, nor the
other ones down thread: I hate "hungarian" naming, ISTM that mixing abbrev
and full words is better avoided. These are really minor aethetical
preferences that I may break occasionally, eg with _NUM for the nice
similarity with _NAME.
ISTM that it needs to be consistent with the pre-existing VERSION, which
rules out "VER".
Now if this is a bloker, I think that anything is more useful than no
variable as it is useful to have them for simple scripting test through
server side expressions.
I also like Daniel's idea to update formatting rules, eg following what is
done for environment variables and accepting that it won't fit in one page
anyway.
SERVER_VERSION NAME
bla bla bla
Attached updated patch changes helpVariables() as we'd need to do if
not renaming, and does some minor doc/comment wordsmithing elsewhere.
Given my broken English, I'm fine with wordsmithing.
I like trying to keep the 80 (or 88 it seems) columns limit if possible,
because my getting older eyes water on long lines.
In the documentation, I do not think that both SERVER_VERSION_NAME and
_NUM (or whatever their chosen name) deserve two independent explanations
with heavy repeats just one after the other, and being treated differently
from VERSION_*.
The same together-ness approach can be used for helpVariables(), see v8
attached for instance.
Seeing it as is, it calls for having "SERVER_VERSION" as well, but I'm not
sure of the better way to get it. I tried with "SELECT VERSION() AS
SERVER_VERSION \gset" but varnames are lowerized.
--
Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index c592eda..79646fd 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3688,6 +3688,19 @@ bar
+SERVER_VERSION_NAME
+SERVER_VERSION_NUM
+
+
+The server's version as a string, for example 9.6.2, 10.1 or 11beta1,
+and in numeric form, for example 90602, 11.
+This is set every time you connect to a database
+(including program start-up), but can be changed or unset.
+
+
+
+
+
SINGLELINE
@@ -3733,10 +3746,15 @@ bar
VERSION
+VERSION_NAME
+VERSION_NUM
-This variable is set at program start-up to
-reflect psql's version. It can be changed or unset.
+ These variables are set at program start-up to reflect
+ psql's version, respectively as a verbose string,
+ a short string (e.g., 9.6.2, 10.1,
+ or 11beta1), and a number (e.g., 90602
+ or 11). They can be changed or unset.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 96f3a40..237e063 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3337,6 +3337,9 @@ checkWin32Codepage(void)
void
SyncVariables(void)
{
+ char vbuf[32];
+ const char *server_version;
+
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
pset.popt.topt.encoding = pset.encoding;
@@ -3348,6 +3351,20 @@ SyncVariables(void)
SetVariable(pset.vars, "PORT", PQport(pset.db));
SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encoding));
+ /* this bit should match connection_warnings(): */
+ /* Try to get full text form of version, might include "devel" etc */
+ server_version = PQparameterStatus(pset.db, "server_version");
+ /* Otherwise fall back on pset.sversion
Re: [HACKERS] pgbench - allow to store select results into variables
Looks good to me. I have marked the patch status as "Ready for committer". LGTM too. Pushed with a minor adjustment to make parseVariable() have exactly the same test as valid_variable_name(). \set ありがとうございました 1 \set 谢谢 2 \set dankeschön 3 :-) -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Hello Masahiko-san, Currently TRUNCATE pgbench_accounts command is executed within a transaction started immediately before it. If we move it out of the transaction, the table data will be truncated even if the copying data failed. Maybe we can do TRUNCATE pgbench_accounts, pgbench_history instead. Thought? Keep the truncate in the transaction, and truncate both (or all?) tables together. Attached latest patch incorporated the comments I got so far. Please review it. "g" does not work for me yet when after "f", only the message is slightly different, it chokes on another dependency, branches instead of accounts. sh> ./pgbench -i -I ctpfg cleaning up... creating tables... set primary keys... set foreign keys... ERROR: cannot truncate a table referenced in a foreign key constraint DETAIL: Table "pgbench_history" references "pgbench_branches". HINT: Truncate table "pgbench_history" at the same time, or use TRUNCATE ... CASCADE. I think that the whole data generation should be in *one* transaction which starts with "truncate pgbench_history, pgbench_branches, pgbench_tellers, pgbench_accounts;" In passing, I think that the documentation should tell explicitely what the default value is (aka "ctgvp"). -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
I'm wondering whether this truncation should be yet another available command? Hmmm... maybe not. Currently TRUNCATE pgbench_accounts command is executed within a transaction started immediately before it. If we move it out of the transaction, the table data will be truncated even if the copying data failed. Maybe we can do TRUNCATE pgbench_accounts, pgbench_history instead. Thought? Keep the truncate in the transaction, and truncate both (or all?) tables together. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Hello Masahiko-san, Patch applies and compiles. One bug found, and some minor points again. Sorry for this hopefully last iteration... I'm kind of an iterative person... I've generated the doc to look a it. Short option "-I" does not use a "=", it should be "-I custom_init_commands". Also maybe it would look nicer and clearer if the short mnemonic was outside the literal, that is with: c (cleanup) instead of: c (cleanup) But this is debatable. Do it the way you think is best. Command "g" does not work after "f", something I had not tested before: ./pgbench -i -I ctvpfg cleaning up... creating tables... vacuum... set primary keys... set foreign keys... ERROR: cannot truncate a table referenced in a foreign key constraint DETAIL: Table "pgbench_history" references "pgbench_accounts". HINT: Truncate table "pgbench_history" at the same time, or use TRUNCATE ... CASCADE. I think it should work. It probably just mean to TRUNCATE all tables as one command, or add the suggested CASCADE. I would favor the first option. I'm wondering whether this truncation should be yet another available command? Hmmm... maybe not. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench tap tests & minor fixes.
Hello Nikolay, Thanks for the review! As for function names, committers can have their say. I'm somehow not dissatisfied with the current version, but I also agree with you that they are imperfect. As for included bug fixes, I can do separate patches, but I think that it is enough to first commit the pgbench files and then the tap-test files, in that order. I'll see what committers say. When/if this patch is committed, it should enable to add more tests quite easily to the numerous pgbench patches already in the pipe for quite some time... In particular, adding the new functions and operators to pgbench expressions patch is waiting for this to go to ready to committers. A further caveat to committers: I'm unsure about what happens on Windows. I've done my best so that it should work. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization
Hello Masahiko-san, [...] Personally I prefer "t" for table creation because "c" for create is a generic word. We might want to have another initialization command that creates something. Ok, good point. About the patch: applies, compiles, works for me. A few minor comments. While re-reading the documentation, I think that it should be "Set custom initialization steps". It could be "Require ..." when -I implied -i, but since -i is still required the sentence does not seem to apply as such. "Destroying any existing tables: ..." -> "Destroy existing pgbench tables: ...". I would suggest to add short expanded explanations in the term definition, next to the triggering letter, to underline the mnemonic. Something like: c (cleanup) t (table creation) g (generate data) v (vacuum) p (primary key) f (foreign key) Also update the error message in checkCustomCommands to "ctgvpf". Cleanup should have a message when it is executed. I suggest "cleaning up...". Maybe add a comment in front of the array tables to say that the order is important, something like "tables in reverse foreign key dependencies order"? case 'I': ISTM that initialize_cmds is necessarily already allocated, thus I would not bother to test before pg_free. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
