Re: pgbench - add pseudo-random permutation function
Hello David, Attached is an attempt at improving things. I have added a explicit note and hijacked an existing example to better illustrate the purpose of the function. This patch does not build on Linux due to some unused functions and variables: http://cfbot.cputube.org/patch_27_1712.log This link is about some other patch, but indeed there is something amiss in v18. Attached a v19 which fixes that. The CF entry has been updated to Waiting on Author. I put it back to "needs review". A few committers have expressed doubts about whether this patch is needed Yep. The key point is that if you (think that you) do not need it, it is by definition useless. If you finally figure out that you need it (IMHO you must for any benchmark with non uniform randoms, otherwise performance result are biased and thus invalid) and it is not available, then you are just stuck. and it doesn't make sense to keep moving it from CF to CF. You do as you feel. IMO such a feature is useful and consistent with providing non-uniform random functions. I'm planning to mark this patch RwF on April 8 and I suggest you resubmit if you are able to get some consensus. People interested in non-uniform benchmarks would see the point. Why many people would be happy with uniform benchmarks only while life is not uniform at all fails me. -- Fabien.
Re: pgbench - add \aset to store results of a combined query
Bonjour Michaël,
Except for the addition of a test case to skip empty results when
\aset is used, I think that we are pretty good here.
While hacking on the patch more by myself, I found that mixing tests
for \gset and \aset was rather messy. A test for an empty result
leads also to a failure with the pgbench command as we want to make
sure that the variable does not exist in this case using debug().
ISTM that I submitted a patch to test whether a variable exists in
pgbench, like available in psql (:{?var} I think), but AFAICS it did not
pass. Maybe I should resurect it as it would allow to test simply whether
an empty result was returned to aset, which could make sense in a bench
script (get something, if it does not exist skip remainder… I can see some
interesting use cases).
So let's split the tests in three parts:
- the set for \get is left alone.
- addition of a new set for the valid cases of \aset.
- addition of an invalid test for \aset (the empty set one).
Ok.
Fabien, what do you think about the attached?
It does not need to create an UNLOGGED table, a mere "WHERE FALSE"
suffices.
I do not understand why you removed the comment about meta which makes it
false, so I added something minimal back.
Perhaps we should also have a test where we return more than 1 row for
\get? The last point is unrelated to this thread though.
Yes, but ISTM that it is not worth a dedicated patch… so I added a test
similar to the one about empty aset.
See v7 attached.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 41b3880c91..58a2aa3bf2 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1057,18 +1057,29 @@ pgbench options d
\gset [prefix]
+ \aset [prefix]
- This command may be used to end SQL queries, taking the place of the
+ These commands may be used to end SQL queries, taking the place of the
terminating semicolon (;).
- When this command is 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.
+ When the \gset command is 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.
+
+
+
+ When the \aset command is used, all combined SQL queries
+ (separated by \;) have their columns stored into variables
+ named after column names, and prefixed with prefix
+ if provided. If a query returns no row, no assignment is made and the variable
+ can be tested for existence to detect this. If a query returns more than one
+ row, the last value is kept.
@@ -1077,6 +1088,8 @@ pgbench options d
p_two and p_three
with integers from the third query.
The result of the second query is discarded.
+ The result of the two last combined queries are stored in variables
+ four and five.
UPDATE pgbench_accounts
SET abalance = abalance + :delta
@@ -1085,6 +1098,7 @@ UPDATE pgbench_accounts
-- compound of two queries
SELECT 1 \;
SELECT 2 AS two, 3 AS three \gset p_
+SELECT 4 AS four \; SELECT 5 AS five \aset
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b8864c6ae5..798a952a25 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -480,6 +480,7 @@ typedef enum MetaCommand
META_SHELL, /* \shell */
META_SLEEP, /* \sleep */
META_GSET, /* \gset */
+ META_ASET, /* \aset */
META_IF, /* \if */
META_ELIF, /* \elif */
META_ELSE, /* \else */
@@ -504,14 +505,15 @@ static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
*not applied.
* first_line A short, single-line extract of 'lines', for error reporting.
* type SQL_COMMAND or META_COMMAND
- * meta The type of meta-command, or META_NONE if command is SQL
+ * meta The type of meta-command. On SQL_COMMAND: META_NONE/GSET/ASET.
* argc Number of arguments of the command, 0 if not yet processed.
* argv Command arguments, the first of which is the command or SQL
*string itself. For SQL commands, after post-processing
*argv[0] is the same as 'lines' with variables substituted.
- * varprefix SQL commands terminated with \gset have this set
+ * varprefix SQL commands terminated with \gset or \aset have this set
*to a non NULL value. If nonempty, it's used to prefix the
*variable name that receives the value.
+ * aset do gset on all possible queries of a combined query (\;).
* expr Parsed expression, if needed.
* stats Time spent in this command.
*/
@@ -2489,6 +2491,8 @@ getMetaCommand(const char *cmd)
mc = META_ENDIF;
else if (pg_strcasecmp(cmd, "gset") == 0)
mc = META_GSET;
+
Re: Allow continuations in "pg_hba.conf" files
Hello, FWIW, I don't think so. Generally a trailing backspace is an escape character for the following newline. And '\ ' is a escaped space, which is usualy menas a space itself. In this case escape character doesn't work generally and I think it is natural that a backslash in the middle of a line is a backslash character itself. I concur: The backslash char is only a continuation as the very last character of the line, before cr/nl line ending markers. There are no assumption about backslash escaping, quotes and such, which seems reasonable given the lexing structure of the files, i.e. records of space-separated words, and # line comments. -- Fabien.
Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction
Hello, As I wrote about an earlier version of the patch, ISTM that instead of reinventing, extending, adapting various ls variants (with/without metadata, which show only files, which shows target of links, which shows directory, etc.) we would just need *one* postgres "ls" implementation which would be like "ls -la arg" (returns file type, dates), and then everything else is a wrapper around that with appropriate filtering that can be done at the SQL level, like you started with recurse. Yeah, I agree that some new function that can represent symlinks explicitly in its output is the place to deal with this, for people who want to deal with it. In the meantime, there's still the question of what pg_ls_dir_files should do exactly. Are we content to have it ignore symlinks? I remain inclined to think that's the right thing given its current brief. My 0.02€: I agree that it is enough to reproduce the current behavior of various existing pg_ls* functions, but on the other hand outputing a column type char like ls (-, d, l…) looks like really no big deal. I'd say that the only reason not to do it may be to pass this before feature freeze. -- Fabien.
Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction
Hello Justin, Well, the following comment says "ignore anything but regular files", so I'm supposing that that is the behavior that we actually want here and failed to implement correctly. There might be scope for additional directory-reading functions, but I'd think you'd want more information (such as the file type) returned from anything that doesn't act this way. Maybe pg_stat_file() deserves similar attention ? Right now, it'll fail on a broken link. If we changed it to lstat(), then it'd work, but it'd also show metadata for the *link* rather than its target. Yep. I think this traditional answer is the rational answer. As I wrote about an earlier version of the patch, ISTM that instead of reinventing, extending, adapting various ls variants (with/without metadata, which show only files, which shows target of links, which shows directory, etc.) we would just need *one* postgres "ls" implementation which would be like "ls -la arg" (returns file type, dates), and then everything else is a wrapper around that with appropriate filtering that can be done at the SQL level, like you started with recurse. It would reduce the amount of C code and I find the SQL-level approach quite elegant. -- Fabien.
Re: pgbench - refactor init functions with buffers
Hello Andres, That being the case, I'd think a better design principle is "make your new code look like the code around it", which would tend to weigh against introducing StringInfo uses into pgbench when there's none there now and a bunch of PQExpBuffer instead. So I can't help thinking the advice you're being given here is suspect. I don't agree with this. This is a "fresh" usage of StringInfo. That's different to adding one new printed line among others built with pqexpbuffer. If we continue adding large numbers of new uses of both pieces of infrastructure, we're just making things more confusing. My 0.02 € : - I'm in favor or having one tool for one purpose, so a fe/be common StringInfo interface is fine with me; - I prefer to avoid using both PQExpBuffer & StringInfo in the same file, because they do the exact same thing and it is locally confusing; - I'd be fine with switching all of pgbench to StringInfo, as there are only 31 uses; - But, pgbench relies on psql scanner, which uses PQExpBuffer in PsqlScanState, so mixing is unavoidable, unless PQExpBuffer & StringInfo are the same thing (i.e. typedef + cpp/inline/function wrappers); - There are 1260 uses of PQExpBuffer in psql that, although they are trivial, I'm in no hurry to update. -- Fabien.
Re: pgbench - refactor init functions with buffers
Hello Tom, I cannot say that I "want" to fix something which already works the same way, because it is against my coding principles. [...] I counted nearly 3500 calls under src/bin. Yeah, that's the problem. If someone does come forward with a patch to do that, I think it'd be summarily rejected, at least in high-traffic code like pg_dump. The pain it'd cause for back-patching would outweigh the value. What about "typedef StringInfoData PQExpBufferData" and replacing PQExpBuffer by StringInfo internally, just keeping the old interface around because it is there? That would remove a few hundreds clocs. ISTM that with inline and varargs macro the substition can be managed reasonably lightly, depending on what level of compatibility is required for libpq: should it be linkability, or requiring a recompilation is ok? A clear benefit is that there are quite a few utils for PQExpBuffer in "fe_utils/string_utils.c" which would become available for StringInfo, which would help using StringInfo without duplicating them. That being the case, I'd think a better design principle is "make your new code look like the code around it", Yep. which would tend to weigh against introducing StringInfo uses into pgbench when there's none there now and a bunch of PQExpBuffer instead. So I can't help thinking the advice you're being given here is suspect. Well, that is what I was saying, but at 2 against 1, I fold. -- Fabien.
Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)
Hello Tom, Thanks for your feedback, I'd be rather unclear about what the actual feedback is, though. I'd interpret it as "pg does not care much about code coverage". Most clients are in the red on coverage.postgresql.org. I'd like pgbench at least to be in the green, but it does not look that it will ever be the case. The reason why the first iteration failed was that it was insufficiently insensitive to timing. Yes. So the problem for a replacement patch is "how do you test fundamentally timing-sensitive behavior in a timing-insensitive way?". The answer is that it depends on the test objective, and there can be no miracle because the test environment is not controlled, in particular it cannot expect the execution to be responsive (i.e. the host not to be overloaded for some reason). As written in the added test comments, these tests mostly aim at coverage, so the time-related part checks are loose, although real. It's not really clear to me that that's possible, so I don't have a lot of faith that this patch wouldn't fail as well. AFAICR I think that it is pretty unlikely to fail. Many other pg test can fail but mostly don't: some rely on random stuff, and you can get unlucky once in a while; Other events can occur such as file system full or whatever. I'm also a bit disturbed that the patch seems to be changing pgbench's behavior for no reason other than to try to make the test produce the same results no matter what the actual machine performance is ... which seems, at best, rather contrary to pgbench's real purpose. No, not exactly. The behavior change is to remove a corner-case optimization which creates an exception to -T/-P compliance: when under very low test rate (with -R) and -T and -P, pgbench shortens the run (i.e. end before the prescribed time) if there is nothing to do in the future, so that progress is not shown. This optimization makes it impossible to test that pgbench complies to -T and -P, because it does not always complies. Using -R is useful to avoid too much assumption on the test host load. So I wonder, are those behavioral changes a net win from a user's standpoint? Would a user complain that they asked for -T 10 -P 1 but the test ran for 10 seconds and the got 10 progress reports along the way? The net win is that the feature they asked for has been tested a little before they actually ran it. It is small, but better than nothing. If not we're letting the tail wag the dog. (If they are a win, they ought to be submitted and defended independently, and maybe there ought to be some documentation changes alongside.) The shorten execution time is a non documented corner case that nobody really expects as a feature, IMHO. It is a side effect of the implementation. I do not think it is worth documenting. I'm not against having better test coverage numbers, but it's a means to an end not an end in itself. Sure, numbers are not an end in themselves. For me what is not tested should not be expected to work, so I like to have most/all lines run at least once by some tests, as a minimal insurance that it is not completely broken… which means at least green. It has to be balanced against the amount of effort to be put into testing (as opposed to actually improving our software). I'm all for balanced and meaningful testing, obviously. However, the current balance results in the coverage status being abysmal. I do not think it is the right one. -- Fabien.
Re: pgbench - refactor init functions with buffers
Ok. I find it strange to mix PQExpBuffer & StringInfo in the same file. Agreed, but we'd rather use StringInfo going forward. However, I don't think that puts you on the hook for updating all the PQExpBuffer references. Unless you want to... I cannot say that I "want" to fix something which already works the same way, because it is against my coding principles. However there may be some fun in writing a little script to replace one with the other automatically. I counted nearly 3500 calls under src/bin. -- Fabien.
Re: psql FETCH_COUNT feature does not work with combined queries
Assuming there's no one willing to fix the behavior (and that seems unlikely given we're in the middle of a 2020-01 commitfest) it makes sense to at least document the behavior. That being said, I think the proposed patch may be a tad too brief. I don't think we need to describe the exact behavior, of course, but maybe it'd be good to mention that the behavior depends on the type of the last command etc. Also, I don't think "combined query" is a term used anywhere in the code/docs - I think the proper term is "multi-query string". Any thoughts on Tomas' comments? Sorry, I did not notice them. I tried "multi-command", matching some wording use elsewhere in the file. I'm at lost about how to describe the insane behavior the feature has when they are used, which is really just an unfixed bug, so I expanded a minima in the attached v2. -- Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 402c4b1b26..b4a9fc3b60 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -3781,6 +3781,14 @@ bar fail after having already displayed some rows. + + +This feature does not works properly with multi-command +(\;) queries as it depends on the command +types encountered in the string. + + + Although you can use any output format with this feature,
Re: [HACKERS] Re: [COMMITTERS] pgsql: Remove pgbench "progress" test pending solution of its timing is (fwd)
This patch is registered in 2020-01, but the last message in the thread seems to be from 2019/05/23. The patch itself seems to be OK (it applies fine etc.) What do we need to get it over the line, instead of just moving it to the next one CF over and over? It does not look like the remainder of this patch is going to be committed and I don't think it makes sense to keep moving the patch indefinitely. Unless something changes by the end of this CF I'll mark it Returned With Feedback. I'd be rather unclear about what the actual feedback is, though. I'd interpret it as "pg does not care much about code coverage". Most clients are in the red on coverage.postgresql.org. I'd like pgbench at least to be in the green, but it does not look that it will ever be the case. -- Fabien.p
Re: pgbench - rework variable management
Patch v5 is a rebase with some adjustements.
This patch is failing on the Windows build:
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.85698
I'm not sure if this had been fixed in v3 and this is a new issue or if it
has been failing all along. Either way, it should be updated.
I don't do windows, so the mutex stuff for windows is just blind
programming.
Marked Waiting on Author.
BTW -- sorry if I seem to be picking on your patches but these happen to be
the patches with the longest time since any activity.
Basically, my areas of interest do not match committers' areas of
interest.
v6 is a yet-again blind attempt at fixing the windows mutex. If someone
with a windows could tell me if it is ok, and if not what to fix, it would
be great.
--
Fabien.diff --git a/src/bin/pgbench/Makefile b/src/bin/pgbench/Makefile
index f402fe7b91..e1d3ef9bb3 100644
--- a/src/bin/pgbench/Makefile
+++ b/src/bin/pgbench/Makefile
@@ -10,6 +10,7 @@ include $(top_builddir)/src/Makefile.global
OBJS = \
$(WIN32RES) \
exprparse.o \
+ symbol_table.o \
pgbench.o
override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index 85d61caa9f..ca21ee012e 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -211,7 +211,7 @@ make_variable(char *varname)
PgBenchExpr *expr = pg_malloc(sizeof(PgBenchExpr));
expr->etype = ENODE_VARIABLE;
- expr->u.variable.varname = varname;
+ expr->u.variable.varnum = getOrCreateSymbolId(symbol_table, varname);
return expr;
}
diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l
index 430bff38a6..a3c5ea348d 100644
--- a/src/bin/pgbench/exprscan.l
+++ b/src/bin/pgbench/exprscan.l
@@ -207,19 +207,19 @@ notnull [Nn][Oo][Tt][Nn][Uu][Ll][Ll]
{digit}+ {
if (!strtoint64(yytext, true, >ival))
expr_yyerror_more(yyscanner, "bigint constant overflow",
- strdup(yytext));
+ pg_strdup(yytext));
return INTEGER_CONST;
}
{digit}+(\.{digit}*)?([eE][-+]?{digit}+)? {
if (!strtodouble(yytext, true, >dval))
expr_yyerror_more(yyscanner, "double constant overflow",
- strdup(yytext));
+ pg_strdup(yytext));
return DOUBLE_CONST;
}
\.{digit}+([eE][-+]?{digit}+)? {
if (!strtodouble(yytext, true, >dval))
expr_yyerror_more(yyscanner, "double constant overflow",
- strdup(yytext));
+ pg_strdup(yytext));
return DOUBLE_CONST;
}
{alpha}{alnum}* {
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b8864c6ae5..3e4b0d7712 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -251,24 +251,32 @@ const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
/*
- * Variable definitions.
+ * Variable contents.
+ *
+ * Variable names are managed in symbol_table with a number.
*
* If a variable only has a string value, "svalue" is that value, and value is
- * "not set". If the value is known, "value" contains the value (in any
- * variant).
+ * not set.
+ *
+ * If the value is known, "value" contains the value (in any variant).
*
* In this case "svalue" contains the string equivalent of the value, if we've
- * had occasion to compute that, or NULL if we haven't.
+ * had occasion to compute that, or an empty string if we haven't.
+ *
+ * The string length is arbitrary limited to benefit from static allocation.
*/
+#define SVALUE_SIZE 128
typedef struct
{
- char *name; /* variable's name */
- char *svalue; /* its value in string form, if known */
- PgBenchValue value; /* actual variable's value */
+ int number;/* variable symbol number, -1 if unset */
+ char svalue[SVALUE_SIZE]; /* value in string form, if known */
+ PgBenchValue value; /* actual value, if known */
} Variable;
+#define undefined_variable_value(v) \
+ v.svalue[0] == '\0' && v.value.type == PGBT_NO_VALUE
+
#define MAX_SCRIPTS 128 /* max number of SQL scripts allowed */
-#define SHELL_COMMAND_SIZE 256 /* maximum size allowed for shell command */
/*
* Simple data structure to keep stats about something.
@@ -414,7 +422,6 @@ typedef struct
/* client variables */
Variable *variables; /* array of variable definitions */
int nvariables; /* number of variables */
- bool vars_sorted; /* are variables sorted by name? */
/* various times about current transaction */
int64 txn_scheduled; /* scheduled start time of transaction (usec) */
@@ -427,6 +434,9 @@ typedef struct
/* per client collected stats */
int64 cnt; /* client transaction count, for -t */
int ecnt; /* error count */
+
+ /* next buffer should be at thread level, but it would change functions... */
+ PQExpBufferData buf; /* persistant buffer to avoid malloc/free cycles */
} CState;
/*
@@ -512,6 +522,7 @@ static const char *QUERYMODE[] = {"simple",
Re: pgbench - refactor init functions with buffers
Hello David,
I'd prefer not to expand the use of pqexpbuffer in more places, and
instead rather see this use StringInfo, now that's also available to
frontend programs.
Franckly, one or the other does not matter much to me.
FWIW, I agree with Andres with regard to using StringInfo.
Ok. I find it strange to mix PQExpBuffer & StringInfo in the same file.
Also, the changes to executeStatementExpect() and adding executeStatement()
do not seem to fit in with the purpose of this patch.
Yep, that was in passing.
Attached a v6 which uses StringInfo, and the small refactoring as a
separate patch.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b8864c6ae5..40cae7b121 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -62,6 +62,7 @@
#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
#include "getopt_long.h"
+#include "lib/stringinfo.h"
#include "libpq-fe.h"
#include "pgbench.h"
#include "portability/instr_time.h"
@@ -599,7 +600,9 @@ static void doLog(TState *thread, CState *st,
StatsData *agg, bool skipped, double latency, double lag);
static void processXactStats(TState *thread, CState *st, instr_time *now,
bool skipped, StatsData *agg);
-static void append_fillfactor(char *opts, int len);
+static void append_fillfactor(StringInfo query);
+static void append_tablespace(PGconn *con, StringInfo query,
+ const char *kind, const char *tablespace);
static void addScript(ParsedScript script);
static void *threadRun(void *arg);
static void finishCon(CState *st);
@@ -3608,30 +3611,27 @@ initDropTables(PGconn *con)
static void
createPartitions(PGconn *con)
{
- char ff[64];
-
- ff[0] = '\0';
-
- /*
- * Per ddlinfo in initCreateTables, fillfactor is needed on table
- * pgbench_accounts.
- */
- append_fillfactor(ff, sizeof(ff));
+ StringInfoData query;
/* we must have to create some partitions */
Assert(partitions > 0);
fprintf(stderr, "creating %d partitions...\n", partitions);
+ initStringInfo();
+
for (int p = 1; p <= partitions; p++)
{
- char query[256];
-
if (partition_method == PART_RANGE)
{
int64 part_size = (naccounts * (int64) scale + partitions - 1) / partitions;
- char minvalue[32],
- maxvalue[32];
+
+ resetStringInfo();
+ appendStringInfo(,
+ "create%s table pgbench_accounts_%d\n"
+ " partition of pgbench_accounts\n"
+ " for values from (",
+ unlogged_tables ? " unlogged" : "", p);
/*
* For RANGE, we use open-ended partitions at the beginning and
@@ -3640,34 +3640,42 @@ createPartitions(PGconn *con)
* scale, it is more generic and the performance is better.
*/
if (p == 1)
-sprintf(minvalue, "minvalue");
+appendStringInfoString(, "minvalue");
else
-sprintf(minvalue, INT64_FORMAT, (p - 1) * part_size + 1);
+appendStringInfo(, INT64_FORMAT, (p - 1) * part_size + 1);
+
+ appendStringInfoString(, ") to (");
if (p < partitions)
-sprintf(maxvalue, INT64_FORMAT, p * part_size + 1);
+appendStringInfo(, INT64_FORMAT, p * part_size + 1);
else
-sprintf(maxvalue, "maxvalue");
+appendStringInfoString(, "maxvalue");
- snprintf(query, sizeof(query),
- "create%s table pgbench_accounts_%d\n"
- " partition of pgbench_accounts\n"
- " for values from (%s) to (%s)%s\n",
- unlogged_tables ? " unlogged" : "", p,
- minvalue, maxvalue, ff);
+ appendStringInfoChar(, ')');
}
else if (partition_method == PART_HASH)
- snprintf(query, sizeof(query),
- "create%s table pgbench_accounts_%d\n"
- " partition of pgbench_accounts\n"
- " for values with (modulus %d, remainder %d)%s\n",
- unlogged_tables ? " unlogged" : "", p,
- partitions, p - 1, ff);
+ {
+ resetStringInfo();
+ appendStringInfo(,
+ "create%s table pgbench_accounts_%d\n"
+ " partition of pgbench_accounts\n"
+ " for values with (modulus %d, remainder %d)",
+ unlogged_tables ? " unlogged" : "", p,
+ partitions, p - 1);
+ }
else /* cannot get there */
Assert(0);
- executeStatement(con, query);
+ /*
+ * Per ddlinfo in initCreateTables, fillfactor is needed on table
+ * pgbench_accounts.
+ */
+ append_fillfactor();
+
+ executeStatement(con, query.data);
}
+
+ pfree(query.data);
}
/*
@@ -3720,48 +3728,40 @@ initCreateTables(PGconn *con)
1
}
};
- int i;
+
+ StringInfoData query;
fprintf(stderr, "creating tables...\n");
- for (i = 0; i < lengthof(DDLs); i++)
+ initStringInfo();
+
+ for (int i = 0; i < lengthof(DDLs); i++)
{
- char opts[256];
- char buffer[256];
const struct ddlinfo *ddl = [i];
- const char *cols;
/* Construct new create table statement. */
- opts[0] = '\0';
+ resetStringInfo();
+ appendStringInfo(, "create%s table %s(%s)",
+ unlogged_tables ? " unlogged" : "",
+ ddl->table,
+ (scale >= SCALE_32BIT_THRESHOLD) ?
Re: pgbench - add \aset to store results of a combined query
Bonjour Michaël,
[...] Still sounds strange to me to invent a new variable to this
structure if it is possible to track the exact same thing with an
existing part of a Command, or it would make sense to split Command into
two different structures with an extra structure used after the parsing
for clarity?
Hmmm.
Your point is to store the gset/aset status into the meta field, even if
the command type is SQL. This is not done for gset, which relies on the
non-null prefix, and breaks the assumption that meta is set to something
only when the command is a meta command. Why not. I updated the comment,
so now meta is none/gset/aset when command type is sql, and I removed the
aset field.
Well, it still looks cleaner to me to just assign the meta field
properly within ParseScript(), and you get the same result. And it is
also possible to use "meta" to do more sanity checks around META_GSET
for some code paths. So I'd actually find the addition of a new
argument using a meta command within readCommandResponse() cleaner.
I tried to do that.
- * varprefix SQL commands terminated with \gset have this set
+ * varprefix SQL commands terminated with \gset or \aset have this set
Nit from v4: varprefix can be used for \gset and \aset, and the
comment was not updated.
It is now updated.
+ /* coldly skip empty result under \aset */
+ if (ntuples <= 0)
+ break;
Shouldn't this check after \aset? And it seems to me that this code
path is not taken, so a test would be nice.
Added (I think, if I understood what you suggested.).
- } while (res);
+ } while (res != NULL);
Useless diff.
Yep.
Attached an updated v5.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 41b3880c91..58a2aa3bf2 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1057,18 +1057,29 @@ pgbench options d
\gset [prefix]
+ \aset [prefix]
- This command may be used to end SQL queries, taking the place of the
+ These commands may be used to end SQL queries, taking the place of the
terminating semicolon (;).
- When this command is 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.
+ When the \gset command is 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.
+
+
+
+ When the \aset command is used, all combined SQL queries
+ (separated by \;) have their columns stored into variables
+ named after column names, and prefixed with prefix
+ if provided. If a query returns no row, no assignment is made and the variable
+ can be tested for existence to detect this. If a query returns more than one
+ row, the last value is kept.
@@ -1077,6 +1088,8 @@ pgbench options d
p_two and p_three
with integers from the third query.
The result of the second query is discarded.
+ The result of the two last combined queries are stored in variables
+ four and five.
UPDATE pgbench_accounts
SET abalance = abalance + :delta
@@ -1085,6 +1098,7 @@ UPDATE pgbench_accounts
-- compound of two queries
SELECT 1 \;
SELECT 2 AS two, 3 AS three \gset p_
+SELECT 4 AS four \; SELECT 5 AS five \aset
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b8864c6ae5..74aadeade0 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -480,6 +480,7 @@ typedef enum MetaCommand
META_SHELL, /* \shell */
META_SLEEP, /* \sleep */
META_GSET, /* \gset */
+ META_ASET, /* \aset */
META_IF, /* \if */
META_ELIF, /* \elif */
META_ELSE, /* \else */
@@ -504,14 +505,17 @@ static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
*not applied.
* first_line A short, single-line extract of 'lines', for error reporting.
* type SQL_COMMAND or META_COMMAND
- * meta The type of meta-command, or META_NONE if command is SQL
+ * meta The type of meta-command, if command is SQL META_NONE,
+ *META_GSET or META_ASET which dictate what to do with the
+ * SQL query result.
* argc Number of arguments of the command, 0 if not yet processed.
* argv Command arguments, the first of which is the command or SQL
*string itself. For SQL commands, after post-processing
*argv[0] is the same as 'lines' with variables substituted.
- * varprefix SQL commands terminated with \gset have this set
+ * varprefix SQL commands terminated with \gset or \aset have this set
*to a non NULL value. If nonempty, it's used to prefix the
*variable name that receives the value.
+ *
Re: Allow continuations in "pg_hba.conf" files
Hello Justin,
thanks for the feedback.
- Records cannot be continued across lines.
+ Records can be backslash-continued across lines.
Maybe say: "lines ending with a backslash are logically continued on the next
line", or similar.
I tried to change it along that.
Since it puts a blank there, it creates a "word" boundary, which I gather
worked for your use case. But I wonder whether it's needed to add a space (or
otherwise, document that lines cannot be split beween words?).
Hmmm. Ok, you are right. I hesitated while doing it. I removed the char
instead, so that it does not add a word break.
Note, that also appears to affect the "username maps" file. So mention
in that chapter, too.
https://www.postgresql.org/docs/current/auth-username-maps.html
Indeed, the same tokenizer is used. I updated a sentence to point on
continuations.
--
Fabien.diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 5f1eec78fb..4f947b0235 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -77,7 +77,8 @@
The general format of the pg_hba.conf file is
a set of records, one per line. Blank lines are ignored, as is any
text after the # comment character.
- Records cannot be continued across lines.
+ Lines ending with a backslash are logically continued on the next
+ line, so a record can span several lines.
A record is made
up of a number of fields which are separated by spaces and/or tabs.
Fields can contain white space if the field value is double-quoted.
@@ -821,7 +822,7 @@ local db1,db2,@demodbs all md5
map-name system-username database-username
- Comments and whitespace are handled in the same way as in
+ Comments, whitespace and continuations are handled in the same way as in
pg_hba.conf. The
map-name is an arbitrary name that will be used to
refer to this mapping in pg_hba.conf. The other
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index da5189a4fa..bae20dbc06 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -486,8 +486,43 @@ tokenize_file(const char *filename, FILE *file, List **tok_lines, int elevel)
char *lineptr;
List *current_line = NIL;
char *err_msg = NULL;
+ char *cur = rawline;
+ int len = sizeof(rawline);
+ int continuations = 0;
- if (!fgets(rawline, sizeof(rawline), file))
+ /* read input and handle simplistic backslash continuations */
+ while ((cur = fgets(cur, len, file)) != NULL)
+ {
+ int curlen = strlen(cur);
+ char *curend = cur + curlen - 1;
+
+ if (curlen == len - 1)
+ {
+/* Line too long! */
+ereport(elevel,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("authentication file line too long"),
+ errcontext("line %d of configuration file \"%s\"",
+ line_number + continuations, filename)));
+err_msg = "authentication file line too long";
+ }
+
+ /* Strip trailing linebreak from rawline */
+ while (curend >= cur && (*curend == '\n' || *curend == '\r'))
+*curend-- = '\0';
+
+ /* empty or not a continuation, we are done */
+ if (curend < cur || *curend != '\\')
+break;
+
+ /* else we have a continuation, just remove it and loop */
+ continuations++;
+ *curend = '\0';
+ len -= (curend - cur);
+ cur = curend;
+ }
+
+ if (cur == NULL)
{
int save_errno = errno;
@@ -501,21 +536,6 @@ tokenize_file(const char *filename, FILE *file, List **tok_lines, int elevel)
filename, strerror(save_errno));
rawline[0] = '\0';
}
- if (strlen(rawline) == MAX_LINE - 1)
- {
- /* Line too long! */
- ereport(elevel,
- (errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("authentication file line too long"),
- errcontext("line %d of configuration file \"%s\"",
-line_number, filename)));
- err_msg = "authentication file line too long";
- }
-
- /* Strip trailing linebreak from rawline */
- lineptr = rawline + strlen(rawline) - 1;
- while (lineptr >= rawline && (*lineptr == '\n' || *lineptr == '\r'))
- *lineptr-- = '\0';
/* Parse fields */
lineptr = rawline;
@@ -543,7 +563,7 @@ tokenize_file(const char *filename, FILE *file, List **tok_lines, int elevel)
*tok_lines = lappend(*tok_lines, tok_line);
}
- line_number++;
+ line_number += continuations + 1;
}
MemoryContextSwitchTo(oldcxt);
Allow continuations in "pg_hba.conf" files
Hello,
After writing an unreadable and stupidly long line for ldap
authentification in a "pg_hba.conf" file, I figured out that allowing
continuations looked simple enough and should just be done.
Patch attached.
--
Fabien.diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 5f1eec78fb..cf3b432c34 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -77,7 +77,7 @@
The general format of the pg_hba.conf file is
a set of records, one per line. Blank lines are ignored, as is any
text after the # comment character.
- Records cannot be continued across lines.
+ Records can be backslash-continued across lines.
A record is made
up of a number of fields which are separated by spaces and/or tabs.
Fields can contain white space if the field value is double-quoted.
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index da5189a4fa..95c2cfc8e4 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -486,8 +486,43 @@ tokenize_file(const char *filename, FILE *file, List **tok_lines, int elevel)
char *lineptr;
List *current_line = NIL;
char *err_msg = NULL;
+ char *cur = rawline;
+ int len = sizeof(rawline);
+ int continuations = 0;
- if (!fgets(rawline, sizeof(rawline), file))
+ /* read input and handle simplistic backslash continuations */
+ while ((cur = fgets(cur, len, file)) != NULL)
+ {
+ int curlen = strlen(cur);
+ char *curend = cur + curlen - 1;
+
+ if (curlen == len - 1)
+ {
+/* Line too long! */
+ereport(elevel,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("authentication file line too long"),
+ errcontext("line %d of configuration file \"%s\"",
+ line_number + continuations, filename)));
+err_msg = "authentication file line too long";
+ }
+
+ /* Strip trailing linebreak from rawline */
+ while (curend >= cur && (*curend == '\n' || *curend == '\r'))
+*curend-- = '\0';
+
+ /* empty or not a continuation, we are done */
+ if (curend < cur || *curend != '\\')
+break;
+
+ /* else we have a continuation, just blank it and loop */
+ continuations++;
+ *curend++ = ' ';
+ len -= (curend - cur);
+ cur = curend;
+ }
+
+ if (cur == NULL)
{
int save_errno = errno;
@@ -501,21 +536,6 @@ tokenize_file(const char *filename, FILE *file, List **tok_lines, int elevel)
filename, strerror(save_errno));
rawline[0] = '\0';
}
- if (strlen(rawline) == MAX_LINE - 1)
- {
- /* Line too long! */
- ereport(elevel,
- (errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("authentication file line too long"),
- errcontext("line %d of configuration file \"%s\"",
-line_number, filename)));
- err_msg = "authentication file line too long";
- }
-
- /* Strip trailing linebreak from rawline */
- lineptr = rawline + strlen(rawline) - 1;
- while (lineptr >= rawline && (*lineptr == '\n' || *lineptr == '\r'))
- *lineptr-- = '\0';
/* Parse fields */
lineptr = rawline;
@@ -543,7 +563,7 @@ tokenize_file(const char *filename, FILE *file, List **tok_lines, int elevel)
*tok_lines = lappend(*tok_lines, tok_line);
}
- line_number++;
+ line_number += continuations + 1;
}
MemoryContextSwitchTo(oldcxt);
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
About v13, seens as one patch:
Function "pg_ls_dir_metadata" documentation suggests a variable number of
arguments with brackets, but parameters are really mandatory.
postgres=# SELECT pg_ls_dir_metadata('.');
ERROR: function pg_ls_dir_metadata(unknown) does not exist
LINE 1: SELECT pg_ls_dir_metadata('.');
^
HINT: No function matches the given name and argument types. You might need
to add explicit type casts.
postgres=# SELECT pg_ls_dir_metadata('.', true, true);
…
The example in the documentation could be better indented. Also, ISTM that
there are two implicit laterals (format & pg_ls_dir_recurse) that I would
make explicit. I'd use the pcs alias explicitely. I'd use meaningful
aliases (eg ts instead of b, …).
On reflection, I think that a boolean "isdir" column is a bad idea because
it is not extensible. I'd propose to switch to the standard "ls" approach
of providing the type as one character: '-' for regular, 'd' for
directory, 'l' for link, 's' for socket, 'c' for character special…
ISTM that "lstat" is not available on windows, which suggests to call
"stat" always, and then "lstat" on un*x and pg ports stuff on win.
I'm wondering about the restriction on directories only. Why should it not
work on a file? Can it be easily extended to work on a simple file? If so,
it could be just "pg_ls".
--
Fabien.
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
Hello Justin,
psql> SELECT * FROM pg_ls_dir_recurse('.');
ERROR: could not stat file
"./base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo":
Too many levels of symbolic links
CONTEXT: SQL function "pg_ls_dir_recurse" statement 1
This probably means using lstat instead of (in supplement to?) stat, and
probably tell if something is a link, and if so not recurse in them.
Thanks for looking.
I think that opens up a can of worms. I don't want to go into the business of
re-implementing all of find(1) - I count ~128 flags (most of which take
arguments). You're referring to find -L vs find -P, and some people would want
one and some would want another. And don't forget about find -H...
This is not the point. The point is that a link can change a finite tree
into cyclic graph, and you do not want to delve into that, ever.
The "find" command, by default, does not recurse into a link because of
said problem, and the user *must* ask for it and assume the infinite loop
if any.
So if you implement one behavior, it should be not recursing into links.
Franckly, I would not provide the recurse into link alternative, but it
could be implemented if someone wants it, and the problem that come with
it.
pg_stat_file doesn't expose the file type (I guess because it's not portable?),
You are right that Un*x and Windows are not the same wrt link. It seems
that there is already something about that in port:
"./src/port/dirmod.c:pgwin32_is_junction(const char *path)"
So most of the details are already hidden.
and I think it's outside the scope of this patch to change that. Maybe it
suggests that the pg_ls_dir_recurse patch should be excluded.
IMHO, I really think that it should be included. Dealing with links is no
big deal, but you need an additional column in _metadata to tell it is a
link, and there is a ifdef because testing is a little different between
unix and windows. I'd guess around 10-20 lines of code added.
ISTM if someone wants to recursively list a directory, they should avoid
putting cycles there, or permission errors, or similar.
Hmmm. I'd say the user should like to be able to call the function and
never have a bad experience with it such as a failure on an infinite loop.
Or they should write their own C extension that borrows from
pg_ls_dir_files but handles more arguments.
ISTM that the point of your patch is to provide the basic tool needed to
list directories contents, and handling links somehow is a necessary part
of that.
--
Fabien.
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
About v11, ISTM that the recursive function should check for symbolic
links and possibly avoid them:
sh> cd data/base
sh> ln -s .. foo
psql> SELECT * FROM pg_ls_dir_recurse('.');
ERROR: could not stat file
"./base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo":
Too many levels of symbolic links
CONTEXT: SQL function "pg_ls_dir_recurse" statement 1
This probably means using lstat instead of (in supplement to?) stat, and
probably tell if something is a link, and if so not recurse in them.
--
Fabien.
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
Hello Justin,
Some feedback on v10:
All patches apply cleanly, one on top of the previous. I really wish there
would be less than 9 patches…
* v10.1 doc change: ok
* v10.2 doc change: ok, not sure why it is not merged with previous
* v10.3 test add: could be merge with both previous
Tests seem a little contrived. I'm wondering whether something more
straightforward could be proposed. For instance, once the tablespace is
just created but not used yet, probably we do know that the tmp file
exists and is empty?
* v10.4 at least, some code!
Compiles, make check ok.
pg_ls_dir_files: I'm fine with the flag approach given the number of
switches and the internal nature of the function.
I'm not sure of the "FLAG_" prefix which seems too generic, even if it is
local. I'd suggest "LS_DIR_*", maybe, as a more specific prefix.
ISTM that Pg style requires spaces around operators. I'd suggest some
parenthesis would help as well, eg: "flags_MISSING_OK" -> "(flags &
FLAG_MISSING_OK)" and other instances.
About:
if (S_ISDIR(attrib.st_mode)) {
if (flags_SKIP_DIRS)
continue;
}
and similars, why not the simpler:
if (S_ISDIR(attrib.st_mode) && (flags & FLAG_SKIP_DIRS))
continue;
Especially that it is done like that in previous cases.
Maybe I'd create defines for long and common flag specs, eg:
#define ..._LS_SIMPLE
(FLAG_SKIP_DIRS|FLAG_SKIP_HIDDEN|FLAG_SKIP_SPECIAL|FLAG_METADATA)
No attempt at recursing.
I'm not sure about these asserts:
/* isdir depends on metadata */
Assert(!(flags_ISDIR) || (flags_METADATA));
Hmmm. Why?
/* Unreasonable to show isdir and skip dirs */
Assert(!(flags_ISDIR) || !(flags_SKIP_DIRS));
Hmmm. Why would I prevent that, even if it has little sense, it should
work. I do not see having false on the isdir column as an actual issue.
* v10.5 add is_dir column, a few tests & doc.
Ok.
* v10.6 behavior change for existing functions, always show isdir column,
and removal of IS_DIR flag.
I'm unsure why the features are removed, some use case may benefit from
the more complete function?
Maybe flags defs should not be changed anyway?
I do not like much the "if (...) /* empty */;" code. Maybe it could be
caught more cleanly later in the conditional structure.
* v10.7 adds "pg_ls_dir_recurse" function
Using sql recurse to possibly to implement the feature is pretty elegant
and limits open directories to one at a time, which is pretty neat.
Doc looks incomplete and the example is very contrived and badly indented.
The function definition does not follow the style around: uppercase
whereas all others are lowercase, "" instead of '', no "as"…
I do not understand why oid 8511 is given to the new function.
I do not understand why UNION ALL and not UNION.
I would have put the definition after "pg_ls_dir_metadata" definition.
pg_ls_dir_metadata seems defined as (text,bool,bool) but called as
(text,bool,bool,bool).
Maybe a better alias could be given instead of x?
There are no tests for the new function. I'm not sure it would work.
* v10.8 change flags & add test on pg_ls_logdir().
I'm unsure why it is done at this stage.
* v10.9 change some ls functions and fix patch 10.7 issue
I'm unsure why it is done at this stage. "make check" ok.
--
Fabien.
Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
Hello Thomas, Thank you very much! I'm going to send a new patch set until the end of this week (I'm sorry I was very busy in the release of Postgres Pro 11...). Is anyone interested in rebasing this, and summarising what needs to be done to get it in? It's arguably a bug or at least quite unfortunate that pgbench doesn't work with SERIALIZABLE, and I heard that a couple of forks already ship Marina's patch set. I'm a reviewer on this patch, that I find a good thing (tm), and which was converging to a reasonable and simple enough addition, IMHO. If I proceed in place of Marina, who is going to do the reviews? -- Fabien.
Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
Hello Justin, Patch series applies cleanly. The last status compiles and passes "make check". A few more comments: * v8.[123] ok. * v8.4 Avoid using the type name as a field name? "enum dir_action dir_action;" -> "enum dir_action action", or maybe rename "dir_action" enum "dir_action_t". About pg_ls_dir: "if (!fctx->dirdesc)" I do not think that is true even if AllocateDir failed, the list exists anyway. ISTM it should be linitial which is NULL in that case. Given the overlap between pg_ls_dir and pg_ls_dir_files, ISTM that the former should call the slightly extended later with appropriate flags. About populate_paths: function is a little bit strange to me, ISTM it would deserve more comments. I'm not sure the name reflect what it does. For instance, ISTM that it does one thing, but the name is plural. Maybe "move_to_next_path" or "update_current_path" or something? It returns an int which can only be 0 or 1, which smells like an bool. What is this int/bool is not told in the function head comment. I guess it is whether the path was updated. When it returns false, the list length is down to one. Shouldn't AllocateDir be tested for bad result? Maybe it is a dir but you do not have perms to open it? Or give a comment about why it cannot happen? later, good, at least the function is called, even if it is only for an error case. Maybe some non empty coverage tests could be added with a "count(*) > 0" on not is_dir or maybe "count(*) = 0" on is_dir, for instance? (SELECT oid FROM pg_tablespace b WHERE b.spcname='regress_tblspace' UNION SELECT 0 ORDER BY 1 DESC LIMIT 1)b The 'b' glued to the ')' looks pretty strange. I'd suggest ") AS b". Reusing the same alias twice could be avoided for clarity, maybe. * v8.[56] I'd say that a behavior change which adds a column and a possibly a few rows is ok, especially as the tmpdir contains subdirs now. -- Fabien.
Re: pg_ls_tmpdir to show directories and shared filesets
It seems that lists are used as FIFO structures by appending, fetching & deleting last, all of which are O(n). ISTM it would be better to use the head of the list by inserting, getting and deleting first, which are O(1). I think you're referring to linked lists, but pglists are now arrays, Ok… I forgot about this change, so my point is void, you took the right one. -- Fabien.
Re: pg_ls_tmpdir to show directories and shared filesets
Hello Justin, Some feedback about the v7 patch set. About v7.1, seems ok. About v7.2 & v7.3 seems ok, altought the two could be merged. About v7.4: The documentation sentences could probably be improved "for for", "used ... used". Maybe: For the temporary directory for tablespace, ... -> For tablespace temporary directory, ... Directories are used for temporary files used by parallel processes, and are shown recursively. -> Directories holding temporary files used by parallel processes are shown recursively. It seems that lists are used as FIFO structures by appending, fetching & deleting last, all of which are O(n). ISTM it would be better to use the head of the list by inserting, getting and deleting first, which are O(1). ISTM that several instances of: "pg_ls_dir_files(..., true, false);" should be "pg_ls_dir_files(..., true, DIR_HIDE);". About v7.5 looks like a doc update which should be merged with v7.4. Alas, ISTM that there are no tests on any of these functions:-( -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
Hallo Andres,
Slight aside: Have you ever looked at moving pgbench to non-blocking
connection establishment? It seems weird to use non-blocking everywhere
but connection establishment.
Attached an attempt at doing that, mostly done for fun. It seems to be a
little slower on a local socket.
What do you think?
Maybe it would be worth having it with an option?
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b8864c6ae5..83ac2235a5 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -102,8 +102,9 @@ typedef struct socket_set
typedef struct socket_set
{
- int maxfd; /* largest FD currently set in fds */
- fd_set fds;
+ int maxfd; /* largest FD currently set in reads or writes */
+ fd_set reads;
+ fd_set writes;
} socket_set;
#endif /* POLL_USING_SELECT */
@@ -318,15 +319,32 @@ typedef enum
/*
* The client must first choose a script to execute. Once chosen, it can
* either be throttled (state CSTATE_PREPARE_THROTTLE under --rate), start
- * right away (state CSTATE_START_TX) or not start at all if the timer was
- * exceeded (state CSTATE_FINISHED).
+ * right away (state CSTATE_START_TX or CSTATE_CONNECT on --connect) or
+ * not start at all if the timer was exceeded (state CSTATE_FINISHED).
*/
CSTATE_CHOOSE_SCRIPT,
/*
- * CSTATE_START_TX performs start-of-transaction processing. Establishes
- * a new connection for the transaction in --connect mode, records the
- * transaction start time, and proceed to the first command.
+ * In CSTATE_PREPARE_THROTTLE state, we calculate when to begin the next
+ * transaction, and advance to CSTATE_THROTTLE. CSTATE_THROTTLE state
+ * sleeps until that moment, then advances to CSTATE_CONNECT (-C) or
+ * CSTATE_START_TX (not -C), or CSTATE_FINISHED if the next transaction
+ * would start beyond the end of the run.
+ */
+ CSTATE_PREPARE_THROTTLE,
+ CSTATE_THROTTLE,
+
+ /*
+ * CSTATE_CONNECT Establishes a connection asynchronously before starting
+ * a transaction, under -C. The state is then CSTATE_CONNECTING till the
+ * connection is established, and then CSTATE_START_TX.
+ */
+ CSTATE_CONNECT,
+ CSTATE_CONNECTING,
+
+ /*
+ * CSTATE_START_TX performs start-of-transaction processing.
+ * It records the transaction start time, and proceed to the first command.
*
* Note: once a script is started, it will either error or run till its
* end, where it may be interrupted. It is not interrupted while running,
@@ -335,16 +353,6 @@ typedef enum
*/
CSTATE_START_TX,
- /*
- * In CSTATE_PREPARE_THROTTLE state, we calculate when to begin the next
- * transaction, and advance to CSTATE_THROTTLE. CSTATE_THROTTLE state
- * sleeps until that moment, then advances to CSTATE_START_TX, or
- * CSTATE_FINISHED if the next transaction would start beyond the end of
- * the run.
- */
- CSTATE_PREPARE_THROTTLE,
- CSTATE_THROTTLE,
-
/*
* We loop through these states, to process each command in the script:
*
@@ -401,6 +409,7 @@ typedef struct
int id;/* client No. */
ConnectionStateEnum state; /* state machine's current state. */
ConditionalStack cstack; /* enclosing conditionals state */
+ PostgresPollingStatusType poll_state; /* async connection status */
/*
* Separate randomness for each client. This is used for random functions
@@ -421,6 +430,7 @@ typedef struct
int64 sleep_until; /* scheduled start time of next cmd (usec) */
instr_time txn_begin; /* used for measuring schedule lag times */
instr_time stmt_begin; /* used for measuring statement latencies */
+ instr_time conn_begin; /* start of asynchronous connection under -C */
bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */
@@ -607,9 +617,9 @@ static void setalarm(int seconds);
static socket_set *alloc_socket_set(int count);
static void free_socket_set(socket_set *sa);
static void clear_socket_set(socket_set *sa);
-static void add_socket_to_set(socket_set *sa, int fd, int idx);
+static void add_socket_to_set(socket_set *sa, int fd, int idx, bool reading);
static int wait_on_socket_set(socket_set *sa, int64 usecs);
-static bool socket_has_input(socket_set *sa, int fd, int idx);
+static bool socket_is_ready(socket_set *sa, int fd, int idx);
/* callback functions for our flex lexer */
@@ -1165,6 +1175,57 @@ tryExecuteStatement(PGconn *con, const char *sql)
PQclear(res);
}
+#define PARAMS_ARRAY_SIZE 7
+
+/* set connection parameters */
+static void
+setPQconnectionParams(const char *keywords[PARAMS_ARRAY_SIZE],
+ const char *values[PARAMS_ARRAY_SIZE],
+ const char *password)
+{
+ keywords[0] = "host";
+ values[0] = pghost;
+ keywords[1] = "port";
+ values[1] = pgport;
+ keywords[2] = "user";
+ values[2] = login;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = dbName;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] =
Re: pgbench: option delaying queries till connections establishment?
Hello Andres,
I've changed the performance calculations depending on -C or not. Ramp-up
effects are smoothed.
I've merged all time-related stuff (time_t, instr_time, int64) to use a
unique type (pg_time_usec_t) and set of functions/macros, which simplifies
the code somehow.
Hm. I'm not convinced it's a good idea for pgbench to do its own thing
here.
Given the unjustifiable heterogeneousness it induces and the simpler code
after the move, I think it is much better. Pgbench cloc is smaller after
barrier are added (4655 to 4650) thanks to that and a few other code
simplifications. Removing all INSTR_TIME_* costly macros is a relief in
itself…
+static int pthread_barrier_init(pthread_barrier_t *barrier, void *unused,
int nthreads);
How about using 'struct unknown_type *unused' instead of "unused"?
Haven't done it because I found no other instances in pg, and anyway this
code is only used once locally and NULL is passed.
+static pthread_barrier_t
+ start_barrier, /* all threads are started */
+ bench_barrier; /* benchmarking ready to start */
We don't really need two barriers here.
Indeed. Down to one.
/* print out results */
Given that we're changing the output (for the better) of pgbench again,
I wonder if we should add the pgbench version to the benchmark
output.
Not sure about it, but done anyway.
+pg_time_usec_t total_delay,/* benchmarking
time */
+pg_time_usec_t conn_total_delay, /* is_connect */
+pg_time_usec_t conn_elapsed_delay, /* !is_connect
*/
+int64 latency_late)
I'm not a fan of naming these 'delay'. To me that doesn't sounds like
it's about the time the total benchmark has taken.
I have used '_duration', and tried to clarify some field and variable
names depending on what data they actually hold.
+ printf("tps = %f (including reconnection times)\n", tps);
+ printf("tps = %f (without initial connection establishing)\n",
tps);
Keeping these separate makes sense to me, they're just so wildly
different.
Yep. I've added a comment about that.
+static inline pg_time_usec_t
+pg_time_get_usec(void)
For me the function name sounds like you're getting the usec out of a
pg_time. Not that it's getting a new timestamp.
I've used "pg_time_now()".
+#define PG_TIME_SET_CURRENT_LAZY(t)\
+ if ((t) == 0) \
+ (t) = pg_time_get_usec()
I'd make it an inline function instead of this.
Done "pg_time_now_lazy()"
I have also simplified the code around thread creation & join because it
was a mess: thread 0 was run in the middle of the stat collection loop…
I have updated the doc with actual current output, but excluding the
version display which would have to be changed between releases.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4c48a58ed2..b77c1089d8 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -55,8 +55,10 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-tps = 85.184871 (including connections establishing)
-tps = 85.296346 (excluding connections establishing)
+latency average = 11.013 ms
+latency stddev = 7.351 ms
+initial connection time = 45.758 ms
+tps = 896.967014 (without initial connection establishing)
The first six lines report some of the most important parameter
@@ -1835,7 +1837,6 @@ END;
For the default script, the output will look similar to this:
-starting vacuum...end.
transaction type: builtin: TPC-B (sort of)
scaling factor: 1
query mode: simple
@@ -1843,22 +1844,22 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-latency average = 15.844 ms
-latency stddev = 2.715 ms
-tps = 618.764555 (including connections establishing)
-tps = 622.977698 (excluding connections establishing)
+latency average = 10.870 ms
+latency stddev = 7.341 ms
+initial connection time = 30.954 ms
+tps = 907.949122 (without initial connection establishing)
statement latencies in milliseconds:
-0.002 \set aid random(1, 10 * :scale)
-0.005 \set bid random(1, 1 * :scale)
-0.002 \set tid random(1, 10 * :scale)
-0.001 \set delta random(-5000, 5000)
-0.326 BEGIN;
-0.603 UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
-0.454 SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
-5.528 UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
-7.335 UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
-0.371 INSERT INTO pgbench_history (tid, bid, aid, delta, mtime)
Re: pgbench: option delaying queries till connections establishment?
Hello Andres, Slight aside: Have you ever looked at moving pgbench to non-blocking connection establishment? It seems weird to use non-blocking everywhere but connection establishment. Nope. If there is some interest, why not. The reason for not doing it is that the typical use case is just to connect once at the beginning so that connections do not matter anyway. Now with -C it makes sense. I've changed the performance calculations depending on -C or not. Ramp-up effects are smoothed. I've merged all time-related stuff (time_t, instr_time, int64) to use a unique type (pg_time_usec_t) and set of functions/macros, which simplifies the code somehow. Hm. I'm not convinced it's a good idea for pgbench to do its own thing here. Having 3 time types (in fact, 4, double is used as well for some calculations) in just one file to deal with time does not help much to understand the code, and there is quite a few line to translate from one to the other. +static int pthread_barrier_init(pthread_barrier_t *barrier, void *unused, int nthreads); How about using 'struct unknown_type *unused' instead of "unused"? Because the void *unused will accept everything... Never encountered this pattern. It does not seem to be used anywhere in pg sources. I'd be afraid that some compilers would complain. I can try anyway. +/* Thread synchronization barriers */ +static pthread_barrier_t + start_barrier, /* all threads are started */ + bench_barrier; /* benchmarking ready to start */ + We don't really need two barriers here. The way that pthread barriers are defined is that they 'reset' after all the threads have arrived. You can argue we still want that, but ... Yes, one barrier could be reused. /* print out results */ static void -printResults(StatsData *total, instr_time total_time, -instr_time conn_total_time, int64 latency_late) +printResults(StatsData *total, Given that we're changing the output (for the better) of pgbench again, I wonder if we should add the pgbench version to the benchmark output. Dunno. Maybe. Otherwise it seems easy to end up e.g. seeing a performance difference between pg12 and pg14, where all that's actually happening is a different output because each run used the respective pgbench version. Yep. +pg_time_usec_t total_delay,/* benchmarking time */ +pg_time_usec_t conn_total_delay, /* is_connect */ +pg_time_usec_t conn_elapsed_delay, /* !is_connect */ +int64 latency_late) I'm not a fan of naming these 'delay'. To me that doesn't sounds like it's about the time the total benchmark has taken. Hmmm… I'd like to differentiate variable names which contain timestamp versus those which contain intervals, given that it is the same underlying type. That said, I'm not very happy with "delay" either. What would you suggest? +pg_time_get_usec(void) For me the function name sounds like you're getting the usec out of a pg_time. Not that it's getting a new timestamp. Ok, I'll think of something else, possibly "pg_now"? "pg_time_now"? +#define PG_TIME_SET_CURRENT_LAZY(t)\ + if ((t) == 0) \ + (t) = pg_time_get_usec() + +#define PG_TIME_GET_DOUBLE(t) (0.01 * (t)) #endif /* INSTR_TIME_H */ I'd make it an inline function instead of this. I did it that way because it was already done with defines on instr_time, but I'm fine with inline. I'll try to look at it over the week-end. -- Fabien.
Re: PG_COLOR not mentioned in docs of vacuumlo, oid2name and pgbench
Bonjour Michaël, All the tools mentioned in $subject have been switched recently to use the central logging infrastructure, which means that they have gained coloring output. However we (mostly I) forgot to update the docs. Attached is a patch to fix this issue. Please let me know if there are comments and/or objections. No objection. I did not know there was such a thing… Maybe a more detailed explanation about PG_COLOR could be stored somewhere, and all affected tools could link to it? Or not. For "pgbench", you could also add the standard sentence that it uses libpq environment variables, as it is also missing? -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
Hello Andres,
FWIW, leaving windows, error handling, and other annoyances aside, this
can be implemented fairly simply. See below.
Attached an attempt at improving things.
I've put 2 barriers: one so that all threads are up, one when all
connections are setup and the bench is ready to go.
I've done a blind attempt at implementing the barrier stuff on windows.
I've changed the performance calculations depending on -C or not. Ramp-up
effects are smoothed.
I've merged all time-related stuff (time_t, instr_time, int64) to use a
unique type (pg_time_usec_t) and set of functions/macros, which simplifies
the code somehow.
I've tried to do some variable renaming to distinguish timestamps and
intervals.
This is work in progress.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b8864c6ae5..a16d9d49e1 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -113,18 +113,29 @@ typedef struct socket_set
*/
#ifdef WIN32
+#define PTHREAD_BARRIER_SERIAL_THREAD (-1)
+
/* Use native win32 threads on Windows */
typedef struct win32_pthread *pthread_t;
typedef int pthread_attr_t;
+typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
static int pthread_create(pthread_t *thread, pthread_attr_t *attr, void *(*start_routine) (void *), void *arg);
static int pthread_join(pthread_t th, void **thread_return);
+
+static int pthread_barrier_init(pthread_barrier_t *barrier, void *unused, int nthreads);
+static int pthread_barrier_wait(pthread_barrier_t *barrier);
+static int pthread_barrier_destroy(pthread_barrier_t *barrier);
#elif defined(ENABLE_THREAD_SAFETY)
/* Use platform-dependent pthread capability */
#include
#else
/* No threads implementation, use none (-j 1) */
#define pthread_t void *
+#define pthread_barrier_t void *
+#define pthread_barrier_init(a, b, c) /* ignore */
+#define pthread_barrier_wait(a) /* ignore */
+#define pthread_barrier_destroy(a) /* ignore */
#endif
@@ -291,7 +302,7 @@ typedef struct SimpleStats
*/
typedef struct StatsData
{
- time_t start_time; /* interval start time, for aggregates */
+ pg_time_usec_t start_time; /* interval start time, for aggregates */
int64 cnt; /* number of transactions, including skipped */
int64 skipped; /* number of transactions skipped under --rate
* and --latency-limit */
@@ -310,6 +321,11 @@ typedef struct RandomState
/* Various random sequences are initialized from this one. */
static RandomState base_random_sequence;
+/* Thread synchronization barriers */
+static pthread_barrier_t
+ start_barrier, /* all threads are started */
+ bench_barrier; /* benchmarking ready to start */
+
/*
* Connection state machine states.
*/
@@ -417,10 +433,10 @@ typedef struct
bool vars_sorted; /* are variables sorted by name? */
/* various times about current transaction */
- int64 txn_scheduled; /* scheduled start time of transaction (usec) */
- int64 sleep_until; /* scheduled start time of next cmd (usec) */
- instr_time txn_begin; /* used for measuring schedule lag times */
- instr_time stmt_begin; /* used for measuring statement latencies */
+ pg_time_usec_t txn_scheduled; /* scheduled start time of transaction (usec) */
+ pg_time_usec_t sleep_until; /* scheduled start time of next cmd (usec) */
+ pg_time_usec_t txn_begin; /* used for measuring schedule lag times (usec) */
+ pg_time_usec_t stmt_begin; /* used for measuring statement latencies (usec) */
bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */
@@ -452,10 +468,13 @@ typedef struct
FILE *logfile; /* where to log, or NULL */
/* per thread collected stats */
- instr_time start_time; /* thread start time */
- instr_time conn_time;
+ pg_time_usec_t start_time; /* thread start (creation) time */
+ pg_time_usec_t started_time; /* thread is running after start barrier */
+ pg_time_usec_t bench_start_time; /* thread is really benchmarking */
+ pg_time_usec_t conn_delay; /* cumulated connection and deconnection delays (usec) */
+
StatsData stats;
- int64 latency_late; /* executed but late transactions */
+ int64 latency_late; /* count executed but late transactions */
} TState;
#define INVALID_THREAD ((pthread_t) 0)
@@ -594,10 +613,10 @@ static void setIntValue(PgBenchValue *pv, int64 ival);
static void setDoubleValue(PgBenchValue *pv, double dval);
static bool evaluateExpr(CState *st, PgBenchExpr *expr,
PgBenchValue *retval);
-static ConnectionStateEnum executeMetaCommand(CState *st, instr_time *now);
+static ConnectionStateEnum executeMetaCommand(CState *st, pg_time_usec_t *now);
static void doLog(TState *thread, CState *st,
StatsData *agg, bool skipped, double latency, double lag);
-static void processXactStats(TState *thread, CState *st, instr_time *now,
+static void processXactStats(TState *thread, CState *st, pg_time_usec_t *now,
bool skipped, StatsData *agg);
static void append_fillfactor(char *opts, int len);
static
Re: pgbench: option delaying queries till connections establishment?
Hello Kyotaro-san, I think this also shows that "including/excluding connections establishing" as well as some of the other stats reported pretty bogus. In the 'before' case a substantial numer of the connections had not yet been established until the end of the test run! I see it useful. In most cases we don't care connection time of pgbench. Especially in the mentioned case the result is just bogus. I think the reason for "including/excluding connection establishing" is not that people wants to see how long connection took to establish but that how long the substantial work took. If each client did run with continuously re-establishing new connections the connection time would be useful, but actually all the connections are established at once at the beginning. So FWIW I prefer that the barrier is applied by default Yep. (that is, it can be disabled) On reflection, I'm not sure I see a use case for not running the barrier if it is available. and the progress time starts at the time all clients has been established. Yep, the start time should be set after the connection barrier, and possibly before a start barrier to ensure that no transaction has started before the start time: although performance measures are expected to be slightly false because of how they are measured (measuring takes time), from a benchmarking perspective the displayed result should be <= the actual performance. Now, again, if long benchmarks are run, which for a db should more or less always be the case, this should not matter much. starting vacuum...end. + time to established 5000 connections: 1323ms Yep, maybe showing the initial connection time separately. -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
Hello Andres, Therefore I'd like to make pgbench wait till it has established all connections, before they run queries. Does anybody else see this as being useful? Yes, I think that having this behavior available would make sense. If so, should this be done unconditionally? Dunno. I should think about it. I'd say probably. Pgbench is more or less designed to run a long hopefully steady-state benchmark, so that the initial connection setup is always negligeable. Not complying with this hypothesis quite often leads to weird results. A new option? Maybe, if not unconditional. If there is an unconditional barrier, the excluding/including connection stuff does not make a lot of sense when not under -C, if it did make any sense before… Included in an existing one somehow? Which one would you suggest? Adding a synchronization barrier should be simple enough, I thought about it in the past. However, I'd still be wary that it is no silver bullet: if you start a lot of threads compared to the number of available cores, pgbench would basically overload the system, and you would experience a lot of waiting time which reflects that the client code has not got enough cpu time. Basically you would be testing the OS process/thread management performance. On my 4-core laptop, with a do-nothing script (\set i 0): sh> pgbench -T 10 -f nope.sql -P 1 -j 10 -c 10 latency average = 0.000 ms latency stddev = 0.049 ms tps = 21048841.630291 (including connections establishing) tps = 21075139.938887 (excluding connections establishing) sh> pgbench -T 10 -f nope.sql -P 1 -j 100 -c 100 latency average = 0.002 ms latency stddev = 0.470 ms tps = 23846777.241370 (including connections establishing) tps = 24132252.146257 (excluding connections establishing) Throughput is slightly better, latency average and variance explode because each thread is given stretches of cpu time to advance, then wait for the next round of cpu time. -- Fabien.
Re: Add %x to PROMPT1 and PROMPT2
+1 one for this change, it's something I also add to every .psqlrc I setup. So.. We have: +1: Vik, Ian, Daniel, Alvaro, Christoph +-0: Tom (?), Fabien (?) I did not know I had a vote. I'm "+1" on this change, if that matters. Just this morning I had a case where I wished I had the current transaction status under the eye under psql. -1: Michael P. So there is a clear majority in favor of changing the default. Any extra opinions from others? -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
Hello Tomas, This patch was marked as ready for committer, but clearly there's an ongoin discussion about what should be the default behavoir, if this breaks existing apps etc. So I've marked it as "needs review" and moved it to the next CF. The issue is that root (aka Tom) seems to be against the feature, and would like the keep it as current. Although my opinion is that the previous behavior is close to insane, I'm ready to resurect the guc to control the behavior so that it would be possible, or even the default. Right now I'm waiting for a "I will not veto it on principle" from Tom (I'm okay with a reject based on bad implementation) before spending more time on it: Although my time is given for free, it is not a good reason to send it down the drain if there is a reject coming whatever I do. Tom, would you consider the feature acceptable with a guc to control it? -- Fabien.
Re: Internal key management system
Hello Masahiko-san,
I've started a new separate thread from the previous long thread[1]
for internal key management system to PostgreSQL. As I mentioned in
the previous mail[2], we've decided to step back and focus on only
internal key management system for PG13. The internal key management
system introduces the functionality to PostgreSQL that allows user to
encrypt and decrypt data without knowing the actual key. Besides, it
will be able to be integrated with transparent data encryption in the
future.
The basic idea is that PostgreSQL generates the master encryption key
which is further protected by the user-provided passphrase. The key
management system provides two functions to wrap and unwrap the secret
by the master encryption key. A user generates a secret key locally
In understand that the secret key is sent in the clear for being encrypted
by a master key.
and send it to PostgreSQL to wrap it using by pg_kmgr_wrap() and save
it somewhere. Then the user can use the encrypted secret key to
encrypt data and decrypt data by something like:
INSERT INTO tbl VALUES (pg_encrypt('user data', pg_kmgr_unwrap('x'));
SELECT pg_decrypt(secret_column, pg_kmgr_unwrap('x')) FROM tbl;
Where 'x' is the result of pg_kmgr_wrap function.
I'm lost. If pg_{en,de}crypt and pg_kmgr_unwrap are functions, what
prevent users to:
SELECT pg_kmgr_unwrap('');
so as to recover the key, somehow in contradiction to "allows user to
encrypt and decrypt data without knowing the actual key".
When dealing with cryptography and key management, I can only recommand
extreme caution.
--
Fabien.
Re: pgbench - add pseudo-random permutation function
Hello Alvaro, I read the whole thread, I still don't know what this patch is supposed to do. I know what the words in the subject line mean, but I don't know how this helps a pgbench user run better benchmarks. I feel this is also the sentiment expressed by others earlier in the thread. You indicated that this functionality makes sense to those who want this functionality, but so far only two people, namely the patch author and the reviewer, have participated in the discussion on the substance of this patch. So either the feature is extremely niche, or nobody understands it. I think you ought to take about three steps back and explain this in more basic terms, even just in email at first so that we can then discuss what to put into the documentation. After re-reading one more time, it dawned on me that the point of this is similar in spirit to this one: https://wiki.postgresql.org/wiki/Pseudo_encrypt Indeed. The one in the wiki is useless because it is on all integers, whereas in a benchmark you want it for a given size and you want seeding, but otherwise the same correlation-avoidance problem is addressed. The idea seems to be to map the int4 domain into itself, so you can use a sequence to generate numbers that will not look like a sequence, allowing the user to hide some properties (such as the generation rate) that might be useful to an eavesdropper/attacker. In terms of writing benchmarks, it seems useful to destroy all locality of access, which changes the benchmark completely. Yes. (I'm not sure if this is something benchmark writers really want to have.) I do not get this sentence. I'm sure that a benchmark writer should really want to avoid unrealistic correlations that have a performance impact. If I'm right, then I agree that the documentation provided with the patch does a pretty bad job at explaining it, because until now I didn't at all realize this is what it was. The documentation is improvable, no doubt. Attached is an attempt at improving things. I have added a explicit note and hijacked an existing example to better illustrate the purpose of the function. -- Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 4c48a58ed2..d2344b029b 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -998,7 +998,7 @@ pgbench options d default_seed - seed used in hash functions by default + seed used in hash and pseudo-random permutation functions by default @@ -1494,6 +1494,13 @@ SELECT 2 AS two, 3 AS three \gset p_ pow(2.0, 10), power(2.0, 10) 1024.0 + + pr_perm(i, size [, seed ] ) + integer + pseudo-random permutation in [0,size) + pr_perm(0, 4) + 0, 1, 2 or 3 + random(lb, ub) integer @@ -1545,6 +1552,20 @@ SELECT 2 AS two, 3 AS three \gset p_ shape of the distribution. + + + When designing a benchmark which selects rows non-uniformly, be aware + that using pgbench non-uniform random functions + directly leads to unduly correlations: rows with close ids come out with + similar probability, skewing performance measures because they also reside + in close pages. + + + You must use pr_perm to shuffle the selected ids, or + some other additional step with similar effect, to avoid these skewing impacts. + + + @@ -1639,24 +1660,54 @@ f(x) = PHI(2.0 * parameter * (x - mu) / (max - min + 1)) / hash_fnv1a accept an input value and an optional seed parameter. In case the seed isn't provided the value of :default_seed is used, which is initialized randomly unless set by the command-line --D option. Hash functions can be used to scatter the -distribution of random functions such as random_zipfian or -random_exponential. For instance, the following pgbench -script simulates possible real world workload typical for social media and +-D option. + + + +Function pr_perm implements a pseudo-random permutation +on an arbitrary size. +It allows to shuffle output of non uniform random functions so that +values drawn more often are not trivially correlated. +It permutes integers in [0, size) using a seed by applying rounds of +simple invertible functions, similarly to an encryption function, +although beware that it is not at all cryptographically secure. +Compared to hash functions discussed above, the function +ensures that a perfect permutation is applied: there are neither collisions +nor holes in the output values. +Values outside the interval are interpreted modulo the size. +The function raises an error if size is not positive. +If no seed is provided, :default_seed is used. + +For instance, the following pgbench script +simulates possible real world workload typical for social media and
Re: pgbench - add pseudo-random permutation function
Hello Peter, This patch was marked as RFC on 2019-03-30, but since then there have been a couple more issues pointed out in a review by Thomas Munro, and it went through 2019-09 and 2019-11 without any attention. Is the RFC status still appropriate? Thomas review was about comments/documentation wording and asking for explanations, which I think I addressed, and the code did not actually change, so I'm not sure that the "needs review" is really needed, but do as you feel. I read the whole thread, I still don't know what this patch is supposed to do. I know what the words in the subject line mean, but I don't know how this helps a pgbench user run better benchmarks. I feel this is also the sentiment expressed by others earlier in the thread. You indicated that this functionality makes sense to those who want this functionality, but so far only two people, namely the patch author and the reviewer, have participated in the discussion on the substance of this patch. So either the feature is extremely niche, or nobody understands it. I think you ought to take about three steps back and explain this in more basic terms, even just in email at first so that we can then discuss what to put into the documentation. Here is the motivation for this feature: When you design a benchmark to test performance, you want to avoid unwanted correlation which may impact performance unduly, one way or another. For the default pgbench benchmarks, accounts are chosen uniformly, no problem. However, if you want to test a non uniform distribution, which is what most people would encounter in practice, things are different. For instance, you would replace random by random_exp in the default benchmarks. If you do that, then all accounts with lower ids would come out more often. However this creates an unwanted correlation and performance effect: why frequent accounts would just happen to be those with small ids? which just happen to reside together in the first few pages of the table? In order to avoid these effects, you need to shuffle the chosen account ids, so that frequent accounts are not specifically those at the beginning of the table. Basically, as soon as you have a non uniform random generator selecting step you want to add some shuffle, otherwise you are going to skew your benchmark measures. Nobody should use a non-uniform generator for selecting rows without some shuffling, ever. I have no doubt that many people do without realizing that they are skewing their performance data. Once you realize that, you can try to invent your own shuffling, but frankly this is not as easy as it looks to have something non trivial which would not generate unwanted correlation. I had a lot of (very) bad design before coming up with the one in the patch: You want something fast, you want good steering, which are contradictory objectives. There is also the question of being able to change the shuffling for a given size, for instance to check that there is no unwanted effects, hence the seeding. Good seeded shuffling is what an encryption algorithm do, but for these it is somehow simpler because they would usually work on power of two sizes. In conclusion, using a seeded shuffle step is needed as soon as you start using non random generators. Providing one in pgbench, a tool designed to run possibly non-uniform benchmarks against postgres, looks like common courtesy. Not providing one is helping people to design bad benchmarks, possibly without realizing it, so is outright thoughtlessness. I have no doubt that the documentation should be improved to explain that in a concise but clear way. -- Fabien.
Re: Add %x to PROMPT1 and PROMPT2
Hello Vik, Isn't there examples in the documentation which use the default prompts? If so, should they be updated accordingly? Good catch! I thought about the documentation but not the examples therein. Updated patch attached. Ok. Only one transaction prompt example in the whole documentation:-( No tests is troubled by the change:-( Sigh… Patch applies and compiles cleanly, global and psql make check ok. Doc build ok. Works for me. I'd be in favor of adding a non trivial session example in psql documentation at the end of the prompt stuff section, something like: BEGIN; CREATE TABLE Word(data TEXT PRIMARY KEY); COPY Word(data) FROM STDIN; hello \. SELECT 2+; ROLLBACK; but this is not necessary for this patch. -- Fabien.
Re: Add %x to PROMPT1 and PROMPT2
Hello Vik, I cannot ever think of a time when I don't want to know if I'm in a transaction or not (and what its state is). Every new setup I do, I add %x to the psql prompt. I think it should be part of the default prompt. Path attached. Isn't there examples in the documentation which use the default prompts? If so, should they be updated accordingly? -- Fabien.
Re: [PoC] Non-volatile WAL buffer
Hello, +1 on the idea. By quickly looking at the patch, I notice that there are no tests. Is it possible to emulate somthing without the actual hardware, at least for testing purposes? -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
My own vote would be to use the initial patch (after applying any unrelated changes per later review), ie. add the feature with its disable button. I can do that, but not if there is a veto from Tom on the feature. I wish definite negative opinions by senior committers would be expressed earlier, so that people do not spend time rewiewing dead code and developing even deader code. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
Hello Tom, This is one of the patches already marked as RFC (since September by Alvaro). Anyone interested in actually pushing it, so that it does not fall through to yet another commitfest? TBH, I think we'd be better off to reject it. This makes a nontrivial change in a very long-standing psql behavior, with AFAICS no way to get back the old semantics. (The thread title is completely misleading about that; there's no "option" in the patch as it stands.) The thread title was not misleading, the initial version of the patch did offer an option. Then I was said "the current behavior is stupid (which I agree), let us change it to the sane behavior without option", then I'm told the contrary. Sigh. I still have the patch with the option, though. Sure, in a green field this behavior would likely be more sensible ... but that has to be weighed against the fact that it's behaved the way it does for a long time, and any existing scripts that are affected by that behavior have presumably deliberately chosen to use it. I cannot imagine many people actually relying on the current insane behavior. I can't imagine that changing this will make very many people happier. It seems much more likely that people who are affected will be unhappy. The compatibility issue could be resolved by putting in the option that I suppose was there at the beginning. Indeed. But then we'd have to have a debate about which behavior would be default, The patch was keeping current behavior as the default because people do not like a change whatever. and there would still be the question of who would find this to be an improvement. If you're chaining together commands with \; then it's likely that you are happy with the way it behaves today. Certainly there's been no drumbeat of bug reports about it. Why would there be bug report if this is a feature? :-) The behavior has been irritating me for a long time. It is plain stupid to be able to send queries but not see their results. -- Fabien.
Re: Patch to document base64 encoding
Hello Karl,
New patch attached: doc_base64_v13.patch
This required surprisingly little re-wording.
Added word "binary" into the descriptions of convert(),
substring(), convert_from(), and convert_to().
I also added data types to the call syntax of set_bit()
and set_byte().
And this patch adds hyperlinks from the get_bit(), get_byte(),
set_bit(), and set_byte() descriptions to the note
that offsets are zero-based.
I also removed the hyperlinked asterisks about the hash
function results and instead hyperlinked the word "hash"
in the descriptions. (Links to the note about md5()
returning hex text and the others returning bytea and how
to convert between the two.)
Patch applies cleanly and compiles.
My 0.02€: The overall restructuration and cross references is an
improvement.
Some comments about v13:
The note about get_byte reads:
get_byte and set_byte number the first byte of a binary string as byte
0. get_bit and set_bit number bits from the right within each byte; for
example bit 0 is the least significant bit of the first byte, and bit 15
is the most significant bit of the second byte.
The two sentences starts with a lower case letter, which looks strange to
me. I'd suggest to put "Functions" at the beginning of the sentences:
Functions get_byte and set_byte number the first byte of a binary string
as byte 0. Functions get_bit and set_bit number bits from the right
within each byte; for example bit 0 is the least significant bit of the
first byte, and bit 15 is the most significant bit of the second byte.
The note about hash provides an example for getting the hex representation
out of sha*. I'd add an exemple to get the bytea representation from md5,
eg "DECODE(MD5('hello world'), 'hex')"…
Maybe the encode/decode in the note could be linked to the function
description? Well, they are just after, maybe it is not very useful.
The "Binary String Functions and Operators" 9.5 section has only one
subsection, "9.5.1", which is about at two thirds of the page. This
structure looks weird. ISTM that a subsection is missing for the beginning
of the page, or that the subsection should just be dropped, because it is
somehow redundant with the table title.
The "9.4" section has the same structural weirdness. Either remove the
subsection, or add some for the other parts?
--
Fabien.
Re: [PATCH v1] pg_ls_tmpdir to show directories
Hello Justin, I'm trying to think about how to get rid of the strange structure and hacks, and the arbitrary looking size 2 array. Also the recursion is one step, but I'm not sure why, ISTM it could/should go on always? Because tmpfiles only go one level deep. I'm not sure it is a general rule. ISTM that extensions can use tmp files, and we would have no control about what they would do there. Looking at the code, ISTM that relying on a stack/list would be much cleaner and easier to understand. The code could look like: I'm willing to change the implementation, but only after there's an agreement about the desired behavior (extra column, one level, etc). For the level, ISTM that the implementation should not make this assumption. If in practice there is just one level, then the function will not recurse deep, no problem. For the column, I'm not sure that "isdir" is necessary. You could put it implicitely in the file name by ending it with "/", and/or showing the directory contents is enough a hint that there is a directory? Also, I'm not fully sure why ".*" files should be skipped, maybe it should be an option? Or the user can filter it with SQL if it does not want them? -- Fabien.
Re: [PATCH v1] pg_ls_tmpdir to show directories
Hello Justin,
About this v4.
It applies cleanly.
I'm trying to think about how to get rid of the strange structure and
hacks, and the arbitrary looking size 2 array.
Also the recursion is one step, but I'm not sure why, ISTM it could/should
go on always?
Maybe the recursive implementation was not such a good idea, if I
suggested it is because I did not noticed the "return next" re-entrant
stuff, shame on me.
Looking at the code, ISTM that relying on a stack/list would be much
cleaner and easier to understand. The code could look like:
ls_func()
if (first_time)
initialize description
set next directory to visit
while (1)
if next directory
init/push next directory to visit as current
read current directory
if emty
pop/close current directory
elif is_a_dir and recursion allowed
set next directory to visit
else ...
return next tuple
cleanup
The point is to avoid a hack around the directory_fctx array, to have only
one place to push/init a new directory (i.e. call AllocateDir and play
around with the memory context) instead of two, and to allow deeper
recursion if useful.
Details : snprintf return is not checked. Maybe it should say why an
overflow cannot occur.
"bool nulls[3] = { 0,};" -> "bool nulls[3} = { false, false, false };"?
--
Fabien.
Re: doc: vacuum full, fillfactor, and "extra space"
Patch applies and compiles. Given that the paragraph begins with "Plain VACUUM (without FULL)", it is better to have the VACUUM FULL explanations on a separate paragraph, and the The original patch does that (Fabien agreed when I asked off list) Indeed. I may have looked at it in reverse, dunno. I switched it to ready. -- Fabien.
Re: pgbench - use pg logging capabilities
Michaël,
So I think that the current situation is a good thing at least for debug.
If you look at some of my messages on other threads, you would likely
notice that my mood of the day is to not design things which try to
outsmart a user's expectations :)
I'm not following you.
ISTM that user expectations is that the message is printed when the level
requires it, and that the performance impact is minimal otherwise.
I'm not aiming at anything different.
So I would stand on the position to just remove those likely/unlikely
parts if we want this logging to be generic.
It is unclear to me whether your point is about the whole "if", or only
the compiler directive itself (i.e. "likely" and "unlikely").
I'll assume the former. I do not think we should "want" logging to be
generic per se, but only if it makes sense from a performance and feature
point of view.
For the normal case (standard level, no debug), there is basically no
difference because the message is going to be printed anyway: either it is
check+call+work, or call+check+work. Anything is fine. The directive would
help the compiler reorder instructions so that usual case does not inccur
a jump.
For debug messages, things are different: removing the external test &
unlikely would have a detrimental effect on performance when not
debugging, which is most of the time, because you would pay the cost of
evaluating arguments and call/return cycle on each message anyway. That
can be bad if a debug message is place in some critical place.
So the right place of the the debug check is early. Once this is done,
then why not doing that for all other level for consistency? This is the
current situation.
If the check is moved inside the call, then there is a performance benefit
to repeat it for debug, which is a pain because then it would be there
twice in that case, and it creates an exception. The fact that some macro
are simplified is not very useful because this is not really user visible.
So IMHO the current situation is fine, but the __variable name. So ISTM
that the attached is the simplest and more reasonable option to fix this.
For other levels, they are on by default AND would not be placed at critical
performance points, so the whole effort of avoiding the call are moot.
I agree with Tom that __pg_log_level variable name violates usages.
My own taste would be to still keep the variable local to logging.c,
and use a "get"-like routine to be consistent with the "set" part. I
don't have to be right, let's see where this discussion leads us.
This would defeat the point of avoiding a function call, if a function
call is needed to check whether the other function call is needed:-)
Hence the macro.
(I mentioned that upthread, but I don't think it is a good idea to
discuss about a redesign of those routines on a thread about pgbench
based on $subject. All the main players are here so it likely does
not matter, but..)
Yep. I hesitated to be the one to do it, and ISTM that the problem is
small enough so that it can be resolved without a new thread. I may be
naïvely wrong:-)
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 39c1a243d5..e7b33bb8e0 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3295,7 +3295,7 @@ executeMetaCommand(CState *st, instr_time *now)
argc = command->argc;
argv = command->argv;
- if (unlikely(__pg_log_level <= PG_LOG_DEBUG))
+ if (unlikely(pg_log_current_level <= PG_LOG_DEBUG))
{
PQExpBufferData buf;
diff --git a/src/common/logging.c b/src/common/logging.c
index c78ae793b8..05827c52cc 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -13,7 +13,7 @@
#include "common/logging.h"
-enum pg_log_level __pg_log_level;
+enum pg_log_level pg_log_current_level;
static const char *progname;
static int log_flags;
@@ -45,7 +45,7 @@ pg_logging_init(const char *argv0)
setvbuf(stderr, NULL, _IONBF, 0);
progname = get_progname(argv0);
- __pg_log_level = PG_LOG_INFO;
+ pg_log_current_level = PG_LOG_INFO;
if (pg_color_env)
{
@@ -107,7 +107,7 @@ pg_logging_config(int new_flags)
void
pg_logging_set_level(enum pg_log_level new_level)
{
- __pg_log_level = new_level;
+ pg_log_current_level = new_level;
}
void
diff --git a/src/include/common/logging.h b/src/include/common/logging.h
index 028149c7a1..487ce21cf7 100644
--- a/src/include/common/logging.h
+++ b/src/include/common/logging.h
@@ -55,7 +55,7 @@ enum pg_log_level
PG_LOG_OFF,
};
-extern enum pg_log_level __pg_log_level;
+extern enum pg_log_level pg_log_current_level;
/*
* Kind of a hack to be able to produce the psql output exactly as required by
@@ -73,23 +73,23 @@ void pg_log_generic(enum pg_log_level level, const char *pg_restrict fmt,...) p
void pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_list ap) pg_attribute_printf(2, 0);
#define pg_log_fatal(...) do { \
- if
Re: pgbench - use pg logging capabilities
Bonjour Michaël,
TBH, my recommendation would be to drop *all* of these likely()
and unlikely() calls. What evidence have you got that those are
meaningfully improving the quality of the generated code? And if
they're buried inside macros, they certainly aren't doing anything
useful in terms of documenting the code.
Yes. I am wondering if we should not rework this part of the logging
with something like the attached. My 2c, thoughts welcome.
ISTM that the intent is to minimise the performance impact of ignored
pg_log calls, especially when under debug where it is most likely to be
the case AND that they may be in critical places.
Compared to dealing with the level inside the call, the use of the level
variable avoids a call-test-return cycle in this case, and the unlikely
should help the compiler reorder instructions so that no actual branch is
taken under the common case.
So I think that the current situation is a good thing at least for debug.
For other levels, they are on by default AND would not be placed at
critical performance points, so the whole effort of avoiding the call are
moot.
I agree with Tom that __pg_log_level variable name violates usages.
ISTM that switching the variable to explicitely global solves the issues,
and that possible the level test can be moved to inside the function for
all but the debug level. See attached which reprises some of your idea,
but keep the outside filtering for the debug level.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 39c1a243d5..e7b33bb8e0 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3295,7 +3295,7 @@ executeMetaCommand(CState *st, instr_time *now)
argc = command->argc;
argv = command->argv;
- if (unlikely(__pg_log_level <= PG_LOG_DEBUG))
+ if (unlikely(pg_log_current_level <= PG_LOG_DEBUG))
{
PQExpBufferData buf;
diff --git a/src/common/logging.c b/src/common/logging.c
index c78ae793b8..ce12593e32 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -13,7 +13,7 @@
#include "common/logging.h"
-enum pg_log_level __pg_log_level;
+enum pg_log_level pg_log_current_level;
static const char *progname;
static int log_flags;
@@ -45,7 +45,7 @@ pg_logging_init(const char *argv0)
setvbuf(stderr, NULL, _IONBF, 0);
progname = get_progname(argv0);
- __pg_log_level = PG_LOG_INFO;
+ pg_log_current_level = PG_LOG_INFO;
if (pg_color_env)
{
@@ -107,7 +107,7 @@ pg_logging_config(int new_flags)
void
pg_logging_set_level(enum pg_log_level new_level)
{
- __pg_log_level = new_level;
+ pg_log_current_level = new_level;
}
void
@@ -142,6 +142,9 @@ pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_list a
size_t required_len;
char *buf;
+ if (unlikely(pg_log_current_level > level))
+ return;
+
Assert(progname);
Assert(level);
Assert(fmt);
diff --git a/src/include/common/logging.h b/src/include/common/logging.h
index 028149c7a1..c73c4ee76c 100644
--- a/src/include/common/logging.h
+++ b/src/include/common/logging.h
@@ -55,7 +55,7 @@ enum pg_log_level
PG_LOG_OFF,
};
-extern enum pg_log_level __pg_log_level;
+extern enum pg_log_level pg_log_current_level;
/*
* Kind of a hack to be able to produce the psql output exactly as required by
@@ -72,24 +72,16 @@ void pg_logging_set_locus_callback(void (*cb) (const char **filename, uint64 *l
void pg_log_generic(enum pg_log_level level, const char *pg_restrict fmt,...) pg_attribute_printf(2, 3);
void pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_list ap) pg_attribute_printf(2, 0);
-#define pg_log_fatal(...) do { \
- if (likely(__pg_log_level <= PG_LOG_FATAL)) pg_log_generic(PG_LOG_FATAL, __VA_ARGS__); \
- } while(0)
+#define pg_log_fatal(...) pg_log_generic(PG_LOG_FATAL, __VA_ARGS__)
-#define pg_log_error(...) do { \
- if (likely(__pg_log_level <= PG_LOG_ERROR)) pg_log_generic(PG_LOG_ERROR, __VA_ARGS__); \
- } while(0)
+#define pg_log_error(...) pg_log_generic(PG_LOG_ERROR, __VA_ARGS__)
-#define pg_log_warning(...) do { \
- if (likely(__pg_log_level <= PG_LOG_WARNING)) pg_log_generic(PG_LOG_WARNING, __VA_ARGS__); \
- } while(0)
+#define pg_log_warning(...) pg_log_generic(PG_LOG_WARNING, __VA_ARGS__)
-#define pg_log_info(...) do { \
- if (likely(__pg_log_level <= PG_LOG_INFO)) pg_log_generic(PG_LOG_INFO, __VA_ARGS__); \
- } while(0)
+#define pg_log_info(...) pg_log_generic(PG_LOG_INFO, __VA_ARGS__)
#define pg_log_debug(...) do { \
- if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \
+ if (unlikely(pg_log_current_level <= PG_LOG_DEBUG)) pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \
} while(0)
#endif /* COMMON_LOGGING_H */
Re: pgbench - rework variable management
Patch v4 is a just a rebase.
Patch v5 is a rebase with some adjustements.
--
Fabien.diff --git a/src/bin/pgbench/Makefile b/src/bin/pgbench/Makefile
index f402fe7b91..e1d3ef9bb3 100644
--- a/src/bin/pgbench/Makefile
+++ b/src/bin/pgbench/Makefile
@@ -10,6 +10,7 @@ include $(top_builddir)/src/Makefile.global
OBJS = \
$(WIN32RES) \
exprparse.o \
+ symbol_table.o \
pgbench.o
override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index 85d61caa9f..ca21ee012e 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -211,7 +211,7 @@ make_variable(char *varname)
PgBenchExpr *expr = pg_malloc(sizeof(PgBenchExpr));
expr->etype = ENODE_VARIABLE;
- expr->u.variable.varname = varname;
+ expr->u.variable.varnum = getOrCreateSymbolId(symbol_table, varname);
return expr;
}
diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l
index 430bff38a6..a3c5ea348d 100644
--- a/src/bin/pgbench/exprscan.l
+++ b/src/bin/pgbench/exprscan.l
@@ -207,19 +207,19 @@ notnull [Nn][Oo][Tt][Nn][Uu][Ll][Ll]
{digit}+ {
if (!strtoint64(yytext, true, >ival))
expr_yyerror_more(yyscanner, "bigint constant overflow",
- strdup(yytext));
+ pg_strdup(yytext));
return INTEGER_CONST;
}
{digit}+(\.{digit}*)?([eE][-+]?{digit}+)? {
if (!strtodouble(yytext, true, >dval))
expr_yyerror_more(yyscanner, "double constant overflow",
- strdup(yytext));
+ pg_strdup(yytext));
return DOUBLE_CONST;
}
\.{digit}+([eE][-+]?{digit}+)? {
if (!strtodouble(yytext, true, >dval))
expr_yyerror_more(yyscanner, "double constant overflow",
- strdup(yytext));
+ pg_strdup(yytext));
return DOUBLE_CONST;
}
{alpha}{alnum}* {
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ee1134aea2..22ca43738a 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -251,24 +251,32 @@ const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
/*
- * Variable definitions.
+ * Variable contents.
+ *
+ * Variable names are managed in symbol_table with a number.
*
* If a variable only has a string value, "svalue" is that value, and value is
- * "not set". If the value is known, "value" contains the value (in any
- * variant).
+ * not set.
+ *
+ * If the value is known, "value" contains the value (in any variant).
*
* In this case "svalue" contains the string equivalent of the value, if we've
- * had occasion to compute that, or NULL if we haven't.
+ * had occasion to compute that, or an empty string if we haven't.
+ *
+ * The string length is arbitrary limited to benefit from static allocation.
*/
+#define SVALUE_SIZE 128
typedef struct
{
- char *name; /* variable's name */
- char *svalue; /* its value in string form, if known */
- PgBenchValue value; /* actual variable's value */
+ int number;/* variable symbol number, -1 if unset */
+ char svalue[SVALUE_SIZE]; /* value in string form, if known */
+ PgBenchValue value; /* actual value, if known */
} Variable;
+#define undefined_variable_value(v) \
+ v.svalue[0] == '\0' && v.value.type == PGBT_NO_VALUE
+
#define MAX_SCRIPTS 128 /* max number of SQL scripts allowed */
-#define SHELL_COMMAND_SIZE 256 /* maximum size allowed for shell command */
/*
* Simple data structure to keep stats about something.
@@ -414,7 +422,6 @@ typedef struct
/* client variables */
Variable *variables; /* array of variable definitions */
int nvariables; /* number of variables */
- bool vars_sorted; /* are variables sorted by name? */
/* various times about current transaction */
int64 txn_scheduled; /* scheduled start time of transaction (usec) */
@@ -427,6 +434,9 @@ typedef struct
/* per client collected stats */
int64 cnt; /* client transaction count, for -t */
int ecnt; /* error count */
+
+ /* next buffer should be at thread level, but it would change functions... */
+ PQExpBufferData buf; /* persistant buffer to avoid malloc/free cycles */
} CState;
/*
@@ -512,6 +522,7 @@ static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
* varprefix SQL commands terminated with \gset have this set
*to a non NULL value. If nonempty, it's used to prefix the
*variable name that receives the value.
+ * set_varnum variable symbol number for \set and \setshell.
* expr Parsed expression, if needed.
* stats Time spent in this command.
*/
@@ -524,6 +535,7 @@ typedef struct Command
int argc;
char *argv[MAX_ARGS];
char *varprefix;
+ int set_varnum;
PgBenchExpr *expr;
SimpleStats stats;
} Command;
@@ -540,6 +552,9 @@ static ParsedScript sql_script[MAX_SCRIPTS]; /* SQL script files */
static int num_scripts; /* number of scripts in sql_script[] */
static int64 total_weight = 0;
+#define
Re: pgbench - use pg logging capabilities
Bonjour Michaël,
I don't follow what you mean by that.
The first versions of the patch did not change syntax_error(), and the
version committed has switched to use PQExpBufferData there. I think
that we should just do the same for the debug logs executing the meta
commands. This way, we get an output consistent with what's printed
out for sending or receiving stuff. Please see the attached.
Yep, I thought of it, but I was not very keen on having a malloc/free
cycle just for one debug message. However under debug this is probably not
an issue.
Your patch works for me. IT can avoid some level of format interpretation
overheads by switching to Char/Str functions, see first attachement.
The other point is the test on __pg_log_level, see second attached.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ee1134aea2..9e1c31413a 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3297,10 +3297,20 @@ executeMetaCommand(CState *st, instr_time *now)
if (unlikely(__pg_log_level <= PG_LOG_DEBUG))
{
- fprintf(stderr, "client %d executing \\%s", st->id, argv[0]);
+ PQExpBufferData buf;
+
+ initPQExpBuffer();
+
+ printfPQExpBuffer(, "client %d executing \\%s", st->id, argv[0]);
for (int i = 1; i < argc; i++)
- fprintf(stderr, " %s", argv[i]);
- fprintf(stderr, "\n");
+ {
+ appendPQExpBufferChar(, ' ');
+ appendPQExpBufferStr(, argv[i]);
+ }
+
+ pg_log_debug("%s", buf.data);
+
+ termPQExpBuffer();
}
if (command->meta == META_SLEEP)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ee1134aea2..44bebed717 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3295,7 +3295,7 @@ executeMetaCommand(CState *st, instr_time *now)
argc = command->argc;
argv = command->argv;
- if (unlikely(__pg_log_level <= PG_LOG_DEBUG))
+ if (pg_log_debug_level)
{
fprintf(stderr, "client %d executing \\%s", st->id, argv[0]);
for (int i = 1; i < argc; i++)
diff --git a/src/include/common/logging.h b/src/include/common/logging.h
index 028149c7a1..c4af92a4d9 100644
--- a/src/include/common/logging.h
+++ b/src/include/common/logging.h
@@ -72,24 +72,29 @@ void pg_logging_set_locus_callback(void (*cb) (const char **filename, uint64 *l
void pg_log_generic(enum pg_log_level level, const char *pg_restrict fmt,...) pg_attribute_printf(2, 3);
void pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_list ap) pg_attribute_printf(2, 0);
+#define pg_log_fatal_level (likely(__pg_log_level <= PG_LOG_FATAL))
#define pg_log_fatal(...) do { \
- if (likely(__pg_log_level <= PG_LOG_FATAL)) pg_log_generic(PG_LOG_FATAL, __VA_ARGS__); \
+ if (pg_log_fatal_level) pg_log_generic(PG_LOG_FATAL, __VA_ARGS__); \
} while(0)
+#define pg_log_error_level (likely(__pg_log_level <= PG_LOG_ERROR))
#define pg_log_error(...) do { \
- if (likely(__pg_log_level <= PG_LOG_ERROR)) pg_log_generic(PG_LOG_ERROR, __VA_ARGS__); \
+ if (pg_log_error_level) pg_log_generic(PG_LOG_ERROR, __VA_ARGS__); \
} while(0)
+#define pg_log_warning_level (likely(__pg_log_level <= PG_LOG_WARNING))
#define pg_log_warning(...) do { \
- if (likely(__pg_log_level <= PG_LOG_WARNING)) pg_log_generic(PG_LOG_WARNING, __VA_ARGS__); \
+ if (pg_log_warning_level) pg_log_generic(PG_LOG_WARNING, __VA_ARGS__); \
} while(0)
+#define pg_log_info_level (likely(__pg_log_level <= PG_LOG_INFO))
#define pg_log_info(...) do { \
- if (likely(__pg_log_level <= PG_LOG_INFO)) pg_log_generic(PG_LOG_INFO, __VA_ARGS__); \
+ if (pg_log_info_level) pg_log_generic(PG_LOG_INFO, __VA_ARGS__); \
} while(0)
+#define pg_log_debug_level (unlikely(__pg_log_level <= PG_LOG_DEBUG))
#define pg_log_debug(...) do { \
- if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \
+ if (pg_log_debug_level) pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \
} while(0)
#endif /* COMMON_LOGGING_H */
Re: Patch to document base64 encoding
Hello Karl, Another option would be to use "bytes bytea". (The current patch uses "string bytea".) This would probably also require some re-wording throughout. Please let me know your preference. I like it, but this is only my own limited opinion, and I'm not a native English speaker. -- Fabien.
Re: pgbench - use pg logging capabilities
Patch v6 makes syntax error location code more compact and tests the case. Committed. Thanks! -- Fabien.
Re: pgbench - use pg logging capabilities
Patch v5 attached tries to follow your above suggestions.
Patch v6 makes syntax error location code more compact and tests the case.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index a1e0663c8b..cfb5110608 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -70,7 +70,6 @@
#define M_PI 3.14159265358979323846
#endif
-
#define ERRCODE_UNDEFINED_TABLE "42P01"
/*
@@ -541,8 +540,6 @@ static ParsedScript sql_script[MAX_SCRIPTS]; /* SQL script files */
static int num_scripts; /* number of scripts in sql_script[] */
static int64 total_weight = 0;
-static int debug = 0; /* debug flag */
-
/* Builtin test scripts */
typedef struct BuiltinScript
{
@@ -787,14 +784,12 @@ strtoint64(const char *str, bool errorOK, int64 *result)
out_of_range:
if (!errorOK)
- fprintf(stderr,
-"value \"%s\" is out of range for type bigint\n", str);
+ pg_log_error("value \"%s\" is out of range for type bigint", str);
return false;
invalid_syntax:
if (!errorOK)
- fprintf(stderr,
-"invalid input syntax for type bigint: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type bigint: \"%s\"", str);
return false;
}
@@ -810,16 +805,14 @@ strtodouble(const char *str, bool errorOK, double *dv)
if (unlikely(errno != 0))
{
if (!errorOK)
- fprintf(stderr,
- "value \"%s\" is out of range for type double\n", str);
+ pg_log_error("value \"%s\" is out of range for type double", str);
return false;
}
if (unlikely(end == str || *end != '\0'))
{
if (!errorOK)
- fprintf(stderr,
- "invalid input syntax for type double: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type double: \"%s\"", str);
return false;
}
return true;
@@ -1149,7 +1142,8 @@ executeStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
+ pg_log_fatal("query failed: %s", PQerrorMessage(con));
+ pg_log_info("query was: %s", sql);
exit(1);
}
PQclear(res);
@@ -1164,8 +1158,8 @@ tryExecuteStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
- fprintf(stderr, "(ignoring this error and continuing anyway)\n");
+ pg_log_error("%s", PQerrorMessage(con));
+ pg_log_info("(ignoring this error and continuing anyway)");
}
PQclear(res);
}
@@ -1211,8 +1205,7 @@ doConnect(void)
if (!conn)
{
- fprintf(stderr, "connection to database \"%s\" failed\n",
- dbName);
+ pg_log_error("connection to database \"%s\" failed", dbName);
return NULL;
}
@@ -1230,8 +1223,8 @@ doConnect(void)
/* check to see that the backend connection was successfully made */
if (PQstatus(conn) == CONNECTION_BAD)
{
- fprintf(stderr, "connection to database \"%s\" failed:\n%s",
-dbName, PQerrorMessage(conn));
+ pg_log_error("connection to database \"%s\" failed: %s",
+ dbName, PQerrorMessage(conn));
PQfinish(conn);
return NULL;
}
@@ -1360,9 +1353,8 @@ makeVariableValue(Variable *var)
if (!strtodouble(var->svalue, true, ))
{
- fprintf(stderr,
- "malformed variable \"%s\" value: \"%s\"\n",
- var->name, var->svalue);
+ pg_log_error("malformed variable \"%s\" value: \"%s\"",
+ var->name, var->svalue);
return false;
}
setDoubleValue(>value, dv);
@@ -1425,8 +1417,7 @@ lookupCreateVariable(CState *st, const char *context, char *name)
*/
if (!valid_variable_name(name))
{
- fprintf(stderr, "%s: invalid variable name: \"%s\"\n",
- context, name);
+ pg_log_error("%s: invalid variable name: \"%s\"", context, name);
return NULL;
}
@@ -1635,7 +1626,7 @@ coerceToBool(PgBenchValue *pval, bool *bval)
}
else /* NULL, INT or DOUBLE */
{
- fprintf(stderr, "cannot coerce %s to boolean\n", valueTypeName(pval));
+ pg_log_error("cannot coerce %s to boolean", valueTypeName(pval));
*bval = false; /* suppress uninitialized-variable warnings */
return false;
}
@@ -1680,7 +1671,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
if (isnan(dval) || !FLOAT8_FITS_IN_INT64(dval))
{
- fprintf(stderr, "double to int overflow for %f\n", dval);
+ pg_log_error("double to int overflow for %f", dval);
return false;
}
*ival = (int64) dval;
@@ -1688,7 +1679,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
}
else /* BOOLEAN or NULL */
{
- fprintf(stderr, "cannot coerce %s to int\n", valueTypeName(pval));
+ pg_log_error("cannot coerce %s to int", valueTypeName(pval));
return false;
}
}
@@ -1709,7 +1700,7 @@ coerceToDouble(PgBenchValue *pval, double *dval)
}
else /* BOOLEAN or NULL */
{
- fprintf(stderr, "cannot coerce %s to double\n", valueTypeName(pval));
+ pg_log_error("cannot coerce %s to double", valueTypeName(pval));
return false;
}
}
@@ -1890,8 +1881,7 @@ evalStandardFunc(CState *st,
if
Re: pgbench - use pg logging capabilities
Bonjour Michaël,
I think that I would just remove the "debug" variable defined in
pgbench.c all together, and switch the messages for the duration and the
one in executeMetaCommand to use info-level logging..
Ok, done.
Thanks. The output then gets kind of inconsistent when using --debug:
pgbench: client 2 executing script ""
client 2 executing \set aid
client 2 executing \set bid
client 2 executing \set tid
client 2 executing \set delta
My point was to just modify the code so as this uses pg_log_debug(),
with a routine doing some reverse-engineering of the Command data to
generate a string to show up in the logs. Sorry for the confusion..
And there is no need to use __pg_log_level either which should remain
internal to logging.h IMO.
For the first case with the output you point out, there is a loop to
generate the output. I do not think that we want to pay the cost of
generating the string and then throw it away afterwards when not under
debug, esp as string manipulation is not that cheap, so we need to enter
the thing only when under debug. However, there is no easy way to do that
without accessing __pg_log_level. It could be hidden in a macro to create,
but that's it.
For the second case I called pg_log_debug just once.
We'd likely want a similar business in syntax_error() to be fully
consistent with all other code paths dealing with an error showing up
before exiting.
The syntax error is kind of complicated because there is the location
information which is better left as is, IMHO. I moved remainder to a
PQExpBuffer and pg_log_fatal.
No idea what others think here. I may be too much pedantic.
Maybe a little:-)
Note that I submitted another patch to use PQExpBuffer wherever possible
in pgbench, especially to get rid of doubtful snprintf/strlen patterns.
Patch v5 attached tries to follow your above suggestions.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index a1e0663c8b..2e7f873702 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -70,7 +70,6 @@
#define M_PI 3.14159265358979323846
#endif
-
#define ERRCODE_UNDEFINED_TABLE "42P01"
/*
@@ -541,8 +540,6 @@ static ParsedScript sql_script[MAX_SCRIPTS]; /* SQL script files */
static int num_scripts; /* number of scripts in sql_script[] */
static int64 total_weight = 0;
-static int debug = 0; /* debug flag */
-
/* Builtin test scripts */
typedef struct BuiltinScript
{
@@ -787,14 +784,12 @@ strtoint64(const char *str, bool errorOK, int64 *result)
out_of_range:
if (!errorOK)
- fprintf(stderr,
-"value \"%s\" is out of range for type bigint\n", str);
+ pg_log_error("value \"%s\" is out of range for type bigint", str);
return false;
invalid_syntax:
if (!errorOK)
- fprintf(stderr,
-"invalid input syntax for type bigint: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type bigint: \"%s\"", str);
return false;
}
@@ -810,16 +805,14 @@ strtodouble(const char *str, bool errorOK, double *dv)
if (unlikely(errno != 0))
{
if (!errorOK)
- fprintf(stderr,
- "value \"%s\" is out of range for type double\n", str);
+ pg_log_error("value \"%s\" is out of range for type double", str);
return false;
}
if (unlikely(end == str || *end != '\0'))
{
if (!errorOK)
- fprintf(stderr,
- "invalid input syntax for type double: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type double: \"%s\"", str);
return false;
}
return true;
@@ -1149,7 +1142,8 @@ executeStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
+ pg_log_fatal("query failed: %s", PQerrorMessage(con));
+ pg_log_info("query was: %s", sql);
exit(1);
}
PQclear(res);
@@ -1164,8 +1158,8 @@ tryExecuteStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
- fprintf(stderr, "(ignoring this error and continuing anyway)\n");
+ pg_log_error("%s", PQerrorMessage(con));
+ pg_log_info("(ignoring this error and continuing anyway)");
}
PQclear(res);
}
@@ -1211,8 +1205,7 @@ doConnect(void)
if (!conn)
{
- fprintf(stderr, "connection to database \"%s\" failed\n",
- dbName);
+ pg_log_error("connection to database \"%s\" failed", dbName);
return NULL;
}
@@ -1230,8 +1223,8 @@ doConnect(void)
/* check to see that the backend connection was successfully made */
if (PQstatus(conn) == CONNECTION_BAD)
{
- fprintf(stderr, "connection to database \"%s\" failed:\n%s",
-dbName, PQerrorMessage(conn));
+ pg_log_error("connection to database \"%s\" failed: %s",
+ dbName, PQerrorMessage(conn));
PQfinish(conn);
return NULL;
}
@@ -1360,9 +1353,8 @@ makeVariableValue(Variable *var)
if (!strtodouble(var->svalue, true, ))
{
- fprintf(stderr,
- "malformed variable \"%s\"
Re: Greatest Common Divisor
Hello Merlin,
For low-level arithmetic code like this one, with tests and loops
containing very few hardware instructions, I think that helping compiler
optimizations is a good idea.
Do you have any performance data to back that up?
Yep.
A generic data is the woes about speculative execution related CVE (aka
Spectre) fixes and their impact on performance, which is in percents,
possibly tens of them, when the thing is more or less desactivated to
mitigate the security issue:
https://www.nextplatform.com/2018/03/16/how-spectre-and-meltdown-mitigation-hits-xeon-performance/
Some data about the __builtin_expect compiler hints:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0479r0.html
Basically, they are talking about percents, up to tens in some cases,
which is consistent with the previous example.
As I said, helping the compiler is a good idea, and pg has been doing that
with the likely/unlikely macros for some time, there are over an hundred
of them, including in headers which get expanded ("logging.h", "float.h",
"simplehash.h", …), which is a good thing.
I do not see any good reason to stop doing that, especially in low-level
arithmetic functions.
Now, I do not have specific data about the performance impact on a gcd
implementation. Mileage may vary depending on hardware, compiler, options
and other overheads.
--
Fabien.
Re: Greatest Common Divisor
Hello Robert, if (arg1 == PG_INT32_MIN) if (arg2 == 0 || arg2 == PG_INT32_MIN) And possibly a "likely" on the while. I don't think decoration the code with likely() and unlikely() all over the place is a very good idea. Odds are good that we'll end up with a bunch that are actually non-optimal, and nobody will ever figure it out because it's hard to figure out. My 0.02€: I'd tend to disagree. Modern pipelined processors can take advantage of speculative execution on branches, so if you know which branch is the more likely it can help. Obviously if you get it wrong it does not, but for the above cases it seems to me that they are rather straightforward. It also provides some "this case is expected to be exceptional" semantics to people reading the code. I have a hard time believing that we're going to be much worse off if we just write the code normally. I think that your point applies to more general programming in postgres, but this is not the context here. For low-level arithmetic code like this one, with tests and loops containing very few hardware instructions, I think that helping compiler optimizations is a good idea. Maybe in the "while" the compiler would assume that it is going to loop anyway by default, so it may be less useful there. -- Fabien.
Re: pgbench - use pg logging capabilities
Bonjour Michaël,
Without looking at the context I thought that argv[0] was the program name,
which is not the case here. I put it back everywhere, including the DEBUG
message.
The variable names in Command are confusing IMO...
Somehow, yes. Note that there is a logic, it will indeed be the argv of
the called shell command… And I do not think it is the point of this patch
to solve this possible confusion.
+ pg_log_error("gaussian parameter must be at least "
+ "%f (not %f)", MIN_GAUSSIAN_PARAM, param);
I would keep all the error message strings to be on the same line.
That makes grepping for them easier on the same, and that's the usual
convention even if these are larger than 72-80 characters.
Ok. I also did other similar cases accordingly.
#ifdef DEBUG
Worth removing this ifdef?
Yep, especially as it is the only instance. Done.
+ pg_log_fatal("unexpected copy in result");
+ pg_log_error("%s", PQerrorMessage(con));
+ pg_log_fatal("cannot count number of branches");
+ pg_log_error("%s", PQerrorMessage(con));
These are inconsistent with the rest, why not combining both?
Ok, done.
I think that I would just remove the "debug" variable defined in
pgbench.c all together, and switch the messages for the duration and the
one in executeMetaCommand to use info-level logging..
Ok, done.
Patch v4 attached addresses all these points.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index a1e0663c8b..11b701b359 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -70,7 +70,6 @@
#define M_PI 3.14159265358979323846
#endif
-
#define ERRCODE_UNDEFINED_TABLE "42P01"
/*
@@ -541,8 +540,6 @@ static ParsedScript sql_script[MAX_SCRIPTS]; /* SQL script files */
static int num_scripts; /* number of scripts in sql_script[] */
static int64 total_weight = 0;
-static int debug = 0; /* debug flag */
-
/* Builtin test scripts */
typedef struct BuiltinScript
{
@@ -787,14 +784,12 @@ strtoint64(const char *str, bool errorOK, int64 *result)
out_of_range:
if (!errorOK)
- fprintf(stderr,
-"value \"%s\" is out of range for type bigint\n", str);
+ pg_log_error("value \"%s\" is out of range for type bigint", str);
return false;
invalid_syntax:
if (!errorOK)
- fprintf(stderr,
-"invalid input syntax for type bigint: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type bigint: \"%s\"", str);
return false;
}
@@ -810,16 +805,14 @@ strtodouble(const char *str, bool errorOK, double *dv)
if (unlikely(errno != 0))
{
if (!errorOK)
- fprintf(stderr,
- "value \"%s\" is out of range for type double\n", str);
+ pg_log_error("value \"%s\" is out of range for type double", str);
return false;
}
if (unlikely(end == str || *end != '\0'))
{
if (!errorOK)
- fprintf(stderr,
- "invalid input syntax for type double: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type double: \"%s\"", str);
return false;
}
return true;
@@ -1149,7 +1142,8 @@ executeStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
+ pg_log_fatal("query failed: %s", PQerrorMessage(con));
+ pg_log_info("query was: %s", sql);
exit(1);
}
PQclear(res);
@@ -1164,8 +1158,8 @@ tryExecuteStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
- fprintf(stderr, "(ignoring this error and continuing anyway)\n");
+ pg_log_error("%s", PQerrorMessage(con));
+ pg_log_info("(ignoring this error and continuing anyway)");
}
PQclear(res);
}
@@ -1211,8 +1205,7 @@ doConnect(void)
if (!conn)
{
- fprintf(stderr, "connection to database \"%s\" failed\n",
- dbName);
+ pg_log_error("connection to database \"%s\" failed", dbName);
return NULL;
}
@@ -1230,8 +1223,8 @@ doConnect(void)
/* check to see that the backend connection was successfully made */
if (PQstatus(conn) == CONNECTION_BAD)
{
- fprintf(stderr, "connection to database \"%s\" failed:\n%s",
-dbName, PQerrorMessage(conn));
+ pg_log_error("connection to database \"%s\" failed: %s",
+ dbName, PQerrorMessage(conn));
PQfinish(conn);
return NULL;
}
@@ -1360,9 +1353,8 @@ makeVariableValue(Variable *var)
if (!strtodouble(var->svalue, true, ))
{
- fprintf(stderr,
- "malformed variable \"%s\" value: \"%s\"\n",
- var->name, var->svalue);
+ pg_log_error("malformed variable \"%s\" value: \"%s\"",
+ var->name, var->svalue);
return false;
}
setDoubleValue(>value, dv);
@@ -1425,8 +1417,7 @@ lookupCreateVariable(CState *st, const char *context, char *name)
*/
if (!valid_variable_name(name))
{
- fprintf(stderr, "%s: invalid variable name: \"%s\"\n",
- context, name);
+ pg_log_error("%s: invalid variable name:
Re: Patch to document base64 encoding
Hello Karl, Attached is doc_base64_v11.patch Patch applies cleanly and compiles. I'm in favor of moving and reorganizing these function descriptions, as they are somehow scattered with a unclear logic when you are looking for them. + bytea || +bytea bytea String concatenation Bytea concatenation? I'm not keen on calling the parameter the name of its type. I'd suggest to keep "string" as a name everywhere, which is not a type name in Pg. The functions descriptions are not homogeneous. Some have parameter name & type "btrim(string bytea, bytes bytea)" and others only type or parameter with tagged as a parameter "get_bit(bytea, offset)" (first param), "sha224(bytea)". I'd suggest to be consistent, eg use "string bytea" everywhere appropriate. -- Fabien.
Re: pgbench - add pseudo-random permutation function
This patch was marked as RFC on 2019-03-30, but since then there have been a couple more issues pointed out in a review by Thomas Munro, and it went through 2019-09 and 2019-11 without any attention. Is the RFC status still appropriate? Thomas review was about comments/documentation wording and asking for explanations, which I think I addressed, and the code did not actually change, so I'm not sure that the "needs review" is really needed, but do as you feel. -- Fabien
Re: Greatest Common Divisor
Hello Vik, Add unlikely() where appropriate. Any particular place in mind where I didn't already put it? In GCD implementations, for instance: if (arg1 == PG_INT32_MIN) if (arg2 == 0 || arg2 == PG_INT32_MIN) And possibly a "likely" on the while. In LCM implementations, for instance: if (arg1 == 0 || arg2 == 0) if (arg1 == arg2) The later is partially redundant with preceeding case BTW, which could be managed inside this one, reducing the number of tests? Something like: if (arg1 == arg2) if (arg1 == MIN_INT) error else return abs(arg1) I'm not sure why you want to deal with a1 == a2 case separately, could it not just work with the main code? If you want to deal with it separately, then why not doing arg1 == -arg2 as well? Please stop suggesting it. Fine, fine! Tom also suggested to align implementations as much as possible, and I do agree with him. Also, I'd suggest to add a comment to explain that the precise C99 modulo semantics is required to make the algorithm work, and that it may not work with C89 semantics for instance. Note: in the numeric code you abs the value, ISTM consistent to do it as well in the other implementations. As noted in the comments, numeric does not have the INT_MIN problem. Sure, but there are special cases at the beginning all the same: NAN, INTEGRAL… Would it make sense that NAN is returned on NUMERIC when the computation cannot be performed, eg on non integer values? I don't think so, no. Ok. Why? I do not have an opinion, but ISTM that there is a choice and it should be explained. Could be consistency with other cases, whatever. Why the positive constraint on LCM(NUMERIC, NUMERIC)? Why not absoluting? I didn't see a definition for negative inputs, but now I see one so I've lifted the restriction. Good. About tests: again, I'd check the LCM overflow on smaller values. I'm not convinced by the handling of fractional numerics in gcd, eg: +SELECT gcd(330.3::numeric, 462::numeric); + gcd +- + 0.3 +(1 row) ISTM that the average user, including myself, would expect an integer result from gcd. If this is kept, the documentation should be clear about what it does and what it means, because the least to say is that it is surprising. Somehow I could have expected the arguments to be casted to int, so that it would lead to 66. Python does a type error, which I find even better. I'd vote for erroring. If this fractional gcd makes some sense and is desirable, then ISTM that lcm(a,b) = a / gcd(a, b) * b should make as much sense and should be allowed as well, for consistency. -- Fabien.
Re: Greatest Common Divisor
Bonjour Vik, Here is an updated patch fixing that. As I said, welcome to arithmetic:-) Patch v5 applies cleanly. Doc: I'd consider using an example the result of which is 42 instead of 21, for obvious geek motivations. Possibly gcd(2142, 462) should be ok. I got it wrong with my previous comment on gcd(int_min, 0). I'm okay with erroring on int_min. Code comments: gcd(n, 0) = abs(n), not n? About the code. Add unlikely() where appropriate. I'd deal with int_min and 0 at the beginning and then proceed with absoluting the values, rather than the dance around a1/arg1, a2/arg2, and other arg2 = -arg2, and arg1 = -arg1 anyway in various places, which does not make the code that easy to understand. Pseudo code could be: if ((a1 == min && (a2 == min || a2 == 0)) || (a2 == min && a1 == 0)) error; a1 = abs(a1), a2 = abs(a2); euclids; return; Note: in the numeric code you abs the value, ISTM consistent to do it as well in the other implementations. Would it make sense that NAN is returned on NUMERIC when the computation cannot be performed, eg on non integer values? Why the positive constraint on LCM(NUMERIC, NUMERIC)? Why not absoluting? Tests: you can make LCM fail on much smaller values for int2/4/8, you do not need to start around max_int. -- Fabien.
Re: Greatest Common Divisor
Hello Tom, Which architecture has single cycle division? I think it's way above that, based on profiles I've seen. And Agner seems to back me up: https://www.agner.org/optimize/instruction_tables.pdf That lists a 32/64 idiv with a latency of ~26/~42-95 cycles, even on a moder uarch like skylake-x. Huh. I figured Intel would have thrown sufficient transistors at that problem by now. It is not just a problem of number of transistors, division is intrisically iterative (with various kind of iterations used in division algorithms), involving some level of guessing and other arithmetics, so the latency can only be bad, and the possibility of implementing that in 1 cycle at 3 GHz looks pretty remote. -- Fabien.
Re: Greatest Common Divisor
Bonjour Vik, The point of swapping is to a void possibly expensive modulo, but this should be done on absolute values, otherwise it may not achieve its purpose as stated by the comment? Ah, true. How widespread are these architectures that need this special treatment? Is it really worth handling? Dunno. AFAICR it was with sparc architectures 25 years ago. Also I do not like much relying on the subtleties of C99 % wrt negative numbers to have the algorithm work, I'd be much at ease to deal with sign and special values at the beginning of the function and proceed with positive numbers afterwards. I'm unsure about gcd(INT_MIN, 0) should error. Possibly 0 would be nicer? What justification for that do you have? ISTM that the current implementation has: \forall int4 n, n \neq MIN_INT4, \gcd(n, 0) = 0 ? In which case applying the same rule for min int seems ok. -- Fabien Coelho - CRI, MINES ParisTech
Re: Greatest Common Divisor
Bonsoir Vik,
+int4gcd_internal(int32 arg1, int32 arg2)
+{
+ int32 swap;
+
+ /*
+* Put the greater value in arg1.
+* This would happen automatically in the loop below, but avoids an
+* expensive modulo simulation on some architectures.
+*/
+ if (arg1 < arg2)
+ {
+ swap = arg1;
+ arg1 = arg2;
+ arg2 = swap;
+ }
The point of swapping is to a void possibly expensive modulo, but this
should be done on absolute values, otherwise it may not achieve its
purpose as stated by the comment?
gcd() is now strictly positive, so INT_MIN is no longer a valid result.
Ok.
I'm unsure about gcd(INT_MIN, 0) should error. Possibly 0 would be
nicer?
--
Fabien.
Re: Allow an alias to be attached directly to a JOIN ... USING
Hello Peter, I took another crack at this. Attached is a new patch that addresses the semantic comments from this and the other thread. It's all a bit tricky, comments welcome. It seems that this patch does not apply anymore after Tom's 5815696. -- Fabien.
Re: pgbench - allow to create partitioned tables
Hello Peter, The documentation and pgbench --help output that accompanied this patch claims that the argument to pgbench --partition-method is optional and defaults to "range", but that is not actually the case, as the implementation requires an argument. Could you please sort this out? AFAICS, if the user omits this argument, then the default is range as specified in docs. I tried by using something like 'pgbench.exe -i -s 1 --partitions=2 postgres' and then run 'pgbench -S postgres'. Ah, the way I interpreted this is that the argument to --partition-method itself is optional. Yep. Optionnal stuff would be in [], where () is used for choices. Would the attached have improved your understanding? It is somehow more consistent with other help lines. -- Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index a1e0663c8b..8d4f5f0866 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -639,7 +639,7 @@ usage(void) " --index-tablespace=TABLESPACE\n" " create indexes in the specified tablespace\n" " --partitions=NUM partition pgbench_accounts in NUM parts (default: 0)\n" - " --partition-method=(range|hash)\n" + " --partition-method=range|hash\n" " partition pgbench_accounts with this method (default: range)\n" " --tablespace=TABLESPACE create tables in the specified tablespace\n" " --unlogged-tablescreate tables as unlogged tables\n"
Re: pgbench - use pg logging capabilities
Hello Peter,
Compared to v1 I have also made a few changes to be more consistent when
using fatal/error/info.
The renaming of debug to debug_level seems unnecessary and unrelated.
Indeed. It was when I used "debug" as a shorthand for "pg_log_debug".
In runShellCommand(), you removed some but not all argv[0] from the output
messages. I'm not sure what the intent was there.
Without looking at the context I thought that argv[0] was the program
name, which is not the case here. I put it back everywhere, including the
DEBUG message.
I would also recommend these changes:
- pg_log_fatal("query failed: %s", sql);
- pg_log_error("%s", PQerrorMessage(con));
+ pg_log_fatal("query failed: %s", PQerrorMessage(con));
+ pg_log_info("query was: %s", sql);
This puts the most important information first.
Ok.
- pg_log_error("connection to database \"%s\" failed", dbName);
- pg_log_error("%s", PQerrorMessage(conn));
+ pg_log_error("connection to database \"%s\" failed: %s",
+dbName, PQerrorMessage(conn));
Line break here is unnecessary.
Ok. I homogeneised another similar message.
Patch v3 attached hopefully fixes all of the above.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index a1e0663c8b..e2dbdf4da4 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -70,7 +70,6 @@
#define M_PI 3.14159265358979323846
#endif
-
#define ERRCODE_UNDEFINED_TABLE "42P01"
/*
@@ -787,14 +786,12 @@ strtoint64(const char *str, bool errorOK, int64 *result)
out_of_range:
if (!errorOK)
- fprintf(stderr,
-"value \"%s\" is out of range for type bigint\n", str);
+ pg_log_error("value \"%s\" is out of range for type bigint", str);
return false;
invalid_syntax:
if (!errorOK)
- fprintf(stderr,
-"invalid input syntax for type bigint: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type bigint: \"%s\"", str);
return false;
}
@@ -810,16 +807,14 @@ strtodouble(const char *str, bool errorOK, double *dv)
if (unlikely(errno != 0))
{
if (!errorOK)
- fprintf(stderr,
- "value \"%s\" is out of range for type double\n", str);
+ pg_log_error("value \"%s\" is out of range for type double", str);
return false;
}
if (unlikely(end == str || *end != '\0'))
{
if (!errorOK)
- fprintf(stderr,
- "invalid input syntax for type double: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type double: \"%s\"", str);
return false;
}
return true;
@@ -1149,7 +1144,8 @@ executeStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
+ pg_log_fatal("query failed: %s", PQerrorMessage(con));
+ pg_log_info("query was: %s", sql);
exit(1);
}
PQclear(res);
@@ -1164,8 +1160,8 @@ tryExecuteStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
- fprintf(stderr, "(ignoring this error and continuing anyway)\n");
+ pg_log_error("%s", PQerrorMessage(con));
+ pg_log_info("(ignoring this error and continuing anyway)");
}
PQclear(res);
}
@@ -1211,8 +1207,7 @@ doConnect(void)
if (!conn)
{
- fprintf(stderr, "connection to database \"%s\" failed\n",
- dbName);
+ pg_log_error("connection to database \"%s\" failed", dbName);
return NULL;
}
@@ -1230,8 +1225,8 @@ doConnect(void)
/* check to see that the backend connection was successfully made */
if (PQstatus(conn) == CONNECTION_BAD)
{
- fprintf(stderr, "connection to database \"%s\" failed:\n%s",
-dbName, PQerrorMessage(conn));
+ pg_log_error("connection to database \"%s\" failed: %s",
+ dbName, PQerrorMessage(conn));
PQfinish(conn);
return NULL;
}
@@ -1360,9 +1355,8 @@ makeVariableValue(Variable *var)
if (!strtodouble(var->svalue, true, ))
{
- fprintf(stderr,
- "malformed variable \"%s\" value: \"%s\"\n",
- var->name, var->svalue);
+ pg_log_error("malformed variable \"%s\" value: \"%s\"",
+ var->name, var->svalue);
return false;
}
setDoubleValue(>value, dv);
@@ -1425,8 +1419,7 @@ lookupCreateVariable(CState *st, const char *context, char *name)
*/
if (!valid_variable_name(name))
{
- fprintf(stderr, "%s: invalid variable name: \"%s\"\n",
- context, name);
+ pg_log_error("%s: invalid variable name: \"%s\"", context, name);
return NULL;
}
@@ -1635,7 +1628,7 @@ coerceToBool(PgBenchValue *pval, bool *bval)
}
else /* NULL, INT or DOUBLE */
{
- fprintf(stderr, "cannot coerce %s to boolean\n", valueTypeName(pval));
+ pg_log_error("cannot coerce %s to boolean", valueTypeName(pval));
*bval = false; /* suppress uninitialized-variable warnings */
return false;
}
@@ -1680,7 +1673,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
if (isnan(dval) ||
Re: Greatest Common Divisor
Normally gcd() returns a positive integer, and gcd(a,0) = gcd(a,a) = abs(a). But since abs(INT_MIN) cannot be represented as a 32-bit integer, both those cases should throw an integer-out-of-range error. I'm also in favor of that option, rather than sending a negative result as a result. About lcm(a, b): a / gcd(a, b) * b, at least if a & b are positive. If not, some thoughts are needed:-) Returning a NUMERIC as suggested by Tom would solve the overflow problem by sending it back to the user who has to cast. This looks ok to me. Maybe we could provide "int4 lcm(int2, int2)", "int8 lcm(int4, int4)", as ISTM that there cannot be overflows on those (eg for the later: lcm <= a*b, a & b are 31 non-signed bits, 62 bits are needed, 63 are available). -- Fabien.
Re: pgbench - use pg logging capabilities
Hello Peter,
The patch seems pretty straightforward, but this
+/*
+ * Convenient shorcuts
+ */
+#define fatal pg_log_fatal
+#define error pg_log_error
+#define warning pg_log_warning
+#define info pg_log_info
+#define debug pg_log_debug
seems counterproductive. Let's just use the normal function names.
I'm trying to keep the column width under control, but if you like it
wider, here it is.
Compared to v1 I have also made a few changes to be more consistent when
using fatal/error/info.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index a1e0663c8b..c35b871045 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -70,7 +70,6 @@
#define M_PI 3.14159265358979323846
#endif
-
#define ERRCODE_UNDEFINED_TABLE "42P01"
/*
@@ -541,7 +540,7 @@ static ParsedScript sql_script[MAX_SCRIPTS]; /* SQL script files */
static int num_scripts; /* number of scripts in sql_script[] */
static int64 total_weight = 0;
-static int debug = 0; /* debug flag */
+static int debug_level = 0; /* debug flag */
/* Builtin test scripts */
typedef struct BuiltinScript
@@ -787,14 +786,12 @@ strtoint64(const char *str, bool errorOK, int64 *result)
out_of_range:
if (!errorOK)
- fprintf(stderr,
-"value \"%s\" is out of range for type bigint\n", str);
+ pg_log_error("value \"%s\" is out of range for type bigint", str);
return false;
invalid_syntax:
if (!errorOK)
- fprintf(stderr,
-"invalid input syntax for type bigint: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type bigint: \"%s\"", str);
return false;
}
@@ -810,16 +807,14 @@ strtodouble(const char *str, bool errorOK, double *dv)
if (unlikely(errno != 0))
{
if (!errorOK)
- fprintf(stderr,
- "value \"%s\" is out of range for type double\n", str);
+ pg_log_error("value \"%s\" is out of range for type double", str);
return false;
}
if (unlikely(end == str || *end != '\0'))
{
if (!errorOK)
- fprintf(stderr,
- "invalid input syntax for type double: \"%s\"\n", str);
+ pg_log_error("invalid input syntax for type double: \"%s\"", str);
return false;
}
return true;
@@ -1149,7 +1144,8 @@ executeStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
+ pg_log_fatal("query failed: %s", sql);
+ pg_log_error("%s", PQerrorMessage(con));
exit(1);
}
PQclear(res);
@@ -1164,8 +1160,8 @@ tryExecuteStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
- fprintf(stderr, "(ignoring this error and continuing anyway)\n");
+ pg_log_error("%s", PQerrorMessage(con));
+ pg_log_info("(ignoring this error and continuing anyway)");
}
PQclear(res);
}
@@ -1211,8 +1207,7 @@ doConnect(void)
if (!conn)
{
- fprintf(stderr, "connection to database \"%s\" failed\n",
- dbName);
+ pg_log_error("connection to database \"%s\" failed", dbName);
return NULL;
}
@@ -1230,8 +1225,8 @@ doConnect(void)
/* check to see that the backend connection was successfully made */
if (PQstatus(conn) == CONNECTION_BAD)
{
- fprintf(stderr, "connection to database \"%s\" failed:\n%s",
-dbName, PQerrorMessage(conn));
+ pg_log_error("connection to database \"%s\" failed", dbName);
+ pg_log_error("%s", PQerrorMessage(conn));
PQfinish(conn);
return NULL;
}
@@ -1360,9 +1355,8 @@ makeVariableValue(Variable *var)
if (!strtodouble(var->svalue, true, ))
{
- fprintf(stderr,
- "malformed variable \"%s\" value: \"%s\"\n",
- var->name, var->svalue);
+ pg_log_error("malformed variable \"%s\" value: \"%s\"",
+ var->name, var->svalue);
return false;
}
setDoubleValue(>value, dv);
@@ -1425,8 +1419,7 @@ lookupCreateVariable(CState *st, const char *context, char *name)
*/
if (!valid_variable_name(name))
{
- fprintf(stderr, "%s: invalid variable name: \"%s\"\n",
- context, name);
+ pg_log_error("%s: invalid variable name: \"%s\"", context, name);
return NULL;
}
@@ -1635,7 +1628,7 @@ coerceToBool(PgBenchValue *pval, bool *bval)
}
else /* NULL, INT or DOUBLE */
{
- fprintf(stderr, "cannot coerce %s to boolean\n", valueTypeName(pval));
+ pg_log_error("cannot coerce %s to boolean", valueTypeName(pval));
*bval = false; /* suppress uninitialized-variable warnings */
return false;
}
@@ -1680,7 +1673,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
if (isnan(dval) || !FLOAT8_FITS_IN_INT64(dval))
{
- fprintf(stderr, "double to int overflow for %f\n", dval);
+ pg_log_error("double to int overflow for %f", dval);
return false;
}
*ival = (int64) dval;
@@ -1688,7 +1681,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
}
else /* BOOLEAN or NULL */
{
- fprintf(stderr, "cannot coerce %s to int\n", valueTypeName(pval));
+
Re: pgbench - use pg logging capabilities
Bonjour Michaël, et excellente année 2020 ! Hmm. Wouldn't it make sense to output the log generated as information from the test using pg_log_info() instead of using fprintf(stderr) (the logs of the initial data load, progress report)? For the progress report, the reason I decided against is that the lines are already long enough with data (for the progress report: tps, latency, etc.), and prepending "pgbench info" or equivalent in front of every line does not look very useful and make it more likely that actually useful data could be pushed out of the terminal width. For data load, ISTM that people are used to it like that. Moreover, I do not think that the \r recently-added trick can work with the logging stuff, so I left it out as well altogether. It seems to me that this would be consistent with the other tools we have, and being able to make a difference with the level of logs is kind of a nice property of logging.c as you can grep easily for one problems instead of looking at multiple patterns matching an error in the logs. Note also an error in the scripts does not report an error. Another thing is that messages logged would need to be translated. I think that's nice, but perhaps others don't like that or may think that's not a good idea. Who knows.. Dunno about translation. ISTM that pgbench is mostly not translated, not sure why. -- Fabien.
Re: TAP testing for psql's tab completion code
Hello Tom, I do not think it is a good idea, because help output is quite large, there are many of them, and we should certainly not want it stored repeatedly in output files for diffs. Hm, I don't follow --- we are most certainly not going to exercise \help for every possible SQL keyword, that'd just be silly. I am silly. Price is pretty low, it helps with coverage in "sql_help.c, it checks that the help files returns adequate results so that adding new help contents does not hide existing stuff. I do not see why we should not do it, in TAP tests. The alternative is that the project tolerates substandard test coverage. The "psql" command is currently around 40-44%. Having said that, the fact that \help now includes a version-dependent URL in its output is probably enough to break the idea of testing it with a conventional expected-output test, so maybe TAP is the only way for that. The URL is a good thing, though. -- Fabien.
Re: TAP testing for psql's tab completion code
That is what my patch does: it tests prompts, tab completion, help, command options… and I added tests till I covered most psql source. Well, I think that where possible we ought to test using the existing test infrastructure -- help, for example, seems like it could perfectly well be tested in src/test/regress/sql/psql.sql, or we could move stuff out to a new set of SQL test scripts under src/bin/psql/sql/, I do not think it is a good idea, because help output is quite large, there are many of them, and we should certainly not want it stored repeatedly in output files for diffs. I rather trigger the output and only check for some related keywords, so that it fits TAP tests reasonably well. -- Fabien.
Re: TAP testing for psql's tab completion code
I'm not fan of relying on the configure stuff ("with_readline"), in my
Expect version I tested if history capabilities are available from psql
itself.
No, I disagree with that. If configure thinks it built with readline,
and then the actual binary acts like it doesn't have readline, that's
a bug that we'd like the tests to detect.
Hmmm. Sure, that's a point.
What about running some tests on an installed version?
For the psql coverage patch, I was more ambitious and needed less
assumption about the configuration, I only forced -X.
I mainly just duplicated the environment set up by PostgresNode::psql
as much as it seemed reasonable to. The -At options are kind of
irrelevant for what we're going to test here, probably, but why not
keep the default behavior the same? I did drop -q since that
suppresses prompting, and we probably want to test prompt.c using
this infrastructure.
That is what my patch does: it tests prompts, tab completion, help,
command options… and I added tests till I covered most psql source.
--
Fabien.
Re: TAP testing for psql's tab completion code
Hello Tom,
If you have to install IO::Pty anyway, ISTM you can also install Expect.
My point is precisely that buildfarm owners *won't* have to install
IO::Pty; it comes in a default Perl install almost everywhere.
I'm afraid that's not true of Expect.
Hmmm. That is a good argument.
Now in both cases we could avoid raising the bar by allowing the
script to "skip" if the module isn't there.
Yep.
IO::Pty documentation says that it is "mainly used by Expect", which is a
clue that IO::Pty is not much better than Expect as a dependency.
You're just guessing, not looking at facts on the ground. [...]
I'm not guessing what the documentation says:-) But for the consequences,
indeed I was guessing.
Well, actually, it's possible that on some of these boxes it was pulled
in by the IPC::Run package,
Ah, you are guessing right, IPC::Run requires IO::Pty, so it should be
available everywhere the buildfarm scripts already run. Maybe.
I've looked at your PoC implementation:
I'm not fan of relying on the configure stuff ("with_readline"), in my
Expect version I tested if history capabilities are available from psql
itself.
I did not paid attention not to overwrite the psql history file, though.
For the psql coverage patch, I was more ambitious and needed less
assumption about the configuration, I only forced -X.
--
Fabien.
Re: Greatest Common Divisor
Hello, Because I do not trust C modulo as I had a lot of problems with it?:-) If I recall correctly (and I'm traveling and away from those notes), the exact semantics of C's % with negative operands was left implementation-defined until, was it, C99 ? Indeed, my woes with C % started before that date:-) By Googling the C99 spec, I found: "When integers are divided, the result of the / operator is the algebraic quotient with any fractional part discarded (aka truncation toward zero). If the quotient a/b is representable, the expression (a/b)*b + a%b shall equal a." Let a = 2 and b = -3, then a/b == 0 (-0.666 truncated toward zero), then (a/b)*b + a%b == a => 0 * -3 + (2 % -3) == 2 => 2 % -3 == 2 Then with a = -2, b = 3, then a/b == 0 (same as above), and the same reasoning leads to -2 % 3 == -2 Which is indeed what was produced with C, but not with Python. The good news is that the absolute value of the modulo is the module in the usual sense, which is what is needed for the Euclidian descent and allows fixing the sign afterwards, as Vik was doing. So it might be ok to rely on the specified C99 behavior (whichever behavior that is, he wrote, notelessly) for PG 12 and later, where C99 is expected. Yep, probably with a comment. -- Fabien.
Re: Greatest Common Divisor
Bonjour Vik, Should there be a NUMERIC version as well? I'd say maybe yes. I thought about that, too, but also decided against it for this patch. Hmmm. ISTM that int functions are available for numeric? I'm wondering what it should do on N, 0 and 0, 0. Raise an error? Return 0? Return 1? return N? There should be some logic and comments explaining it. Well, gcd(N, 0) is N, and gcd(0, 0) is 0, so I don't see an issue here? I think that there should be a comment. I'd test with INT_MIN and INT_MAX. Okay, I'll add tests for those, instead of the pretty much random values I have now. C modulo operator (%) is a pain because it is not positive remainder (2 % -3 == -1 vs 2 % 3 == 2, AFAICR). This does not seem to be the case... Indeed, I tested quickly with python, but it has yet another behavior as shown above, what a laugh! So with C: 2 % -3 == 2, -2 % 3 == -2 Note that AFAICS there is no integer i so that 3 * i - (-2) == -2. It does not seem that fixing the sign afterwards is the right thing to do. I'd rather turn both arguments positive before doing the descent. Why isn't it the right thing to do? Because I do not trust C modulo as I had a lot of problems with it? :-) If it works, but it should deserve a clear comment explaining why. Which raises an issue with INT_MIN by the way, which has no positive:-( That's an argument against abs-ing the input values. It's only an issue with gcd(INT_MIN, INT_MIN) which currently returns INT_MIN. That should be an error instead, because -INT_MIN cannot be represented? Any other value with INT_MIN will be 1 or something lower than INT_MAX. Looks ok. Also, the usual approach is to exchange args so that the largest is first, because there may be a software emulation if the hardware does not implement modulo. At least it was the case with some sparc processors 25 years ago:-) The args will exchange themselves. Yep, but after a possibly expensive software-emulated modulo operation? -- Fabien.
Re: TAP testing for psql's tab completion code
Hello Tom, We've often talked about the problem that we have no regression test coverage for psql's tab completion code. I got interested in this issue while messing with the filename completion logic therein [1], so here is a draft patch that adds some testing for that code. After you raised the issue, I submitted something last August, which did not attract much attention. https://commitfest.postgresql.org/26/2262/ It covers some tab-completion stuff. It uses Expect for the interactive stuff (tab completion, \h, ...). Now that you mention it, I seem to recall looking at that and not being happy with the additional dependency on Expect. Possibly. You did not say it out very loud. Expect is *not* a standard module; Somehow. It is an old one, though. on the machines I have handy, the only one in which it appears in the default Perl installation is macOS. (Huh, what's Apple doing out ahead of the pack?) I'm pretty sure that Expect also relies on IO::Pty, Indeed, it does. so it's a strictly worse dependency than what I've got here. If you have to install IO::Pty anyway, ISTM you can also install Expect. IO::Pty documentation says that it is "mainly used by Expect", which is a clue that IO::Pty is not much better than Expect as a dependency. For installation, "apt install libexpect-perl" did the trick for me. "cpan install Expect" should work as well on most setup. I guess it is possible to check whether Expect is available and to skip the corresponding tests if not. Can we recast what you did into something like this patch's methods? Basically it means reimplementing some expect functionality in the script, including new bugs. Modules were invented to avert that, so I cannot say I'm happy with the prospect of re-inventing the wheel. Note that Expect is a pure-perl 1600-LOC module. Anyway, I'll have a look. At least I used a very limited subset of Expect capabilities which should help matters along. -- Fabien.
Re: TAP testing for psql's tab completion code
Hello Tom, We've often talked about the problem that we have no regression test coverage for psql's tab completion code. I got interested in this issue while messing with the filename completion logic therein [1], so here is a draft patch that adds some testing for that code. This is just preliminary investigation, so I've made no attempt to test tab-complete.c comprehensively (and I'm not sure there would be much point in covering every last code path in it anyway). Still, it does get us from zero to 43% coverage of that file already, and it does good things for the coverage of input.c and prompt.c as well. What I think is actually interesting at this stage is portability of the tests. There are a number of issues: * The script requires Perl's IO::Pty module (indirectly, in that IPC::Run requires that to make pty connections), which isn't installed everywhere. I just made the script skip if that's not available, so that we're not moving the goalposts for what has to be installed to run the TAP tests. Is that the right answer? * It seems pretty likely that this won't work on Windows, given all the caveats in the IPC::Run documentation about nonfunctionality of the pty support there. Maybe we don't care, seeing that we don't really support readline on Windows anyway. For the moment I assumed that the skip conditions for --without-readline and/or missing-IO::Pty would cover this, but we might need an explicit check for Windows too. Or maybe somebody wants to try to make it work on Windows; but that won't be me. * What's even more likely to be problematic is small variations in the output behavior of different readline and libedit versions. According to my tests so far, though, all modern versions of them do pass these test cases. I noted failures on very old Apple versions of libedit: 1. macOS 10.5's version of libedit seems not to honor rl_completion_append_character; it never emits the trailing space we're expecting. This seems like a plain old libedit bug, especially since 10.4's version works as expected. 2. Both 10.4 and 10.5 emit the alternative table names in the wrong order, suggesting that they're not internally sorting the completion results. If this proves to be more widespread, we could likely fix it by adding ORDER BY to the completion queries, but I'm not sure that it's worth doing if only these dead macOS versions have the issue. (On the other hand, it seems like bad practice to be issuing queries that have LIMIT without ORDER BY, so maybe we should fix them anyway.) I'm strongly tempted to just push this and see what the buildfarm thinks of it. If it fails in lots of places, maybe the idea is a dead end. If it works, I'd look into extending the tests --- in particular, I'd like to have some coverage for the filename completion logic. Thoughts? After you raised the issue, I submitted something last August, which did not attract much attention. https://commitfest.postgresql.org/26/2262/ It covers some tab-completion stuff. It uses Expect for the interactive stuff (tab completion, \h, ...). -- Fabien.
Re: Greatest Common Divisor
Bonsoir Vik, I recently came across the need for a gcd function (actually I needed lcm) and was surprised that we didn't have one. Why not. So here one is, using the basic Euclidean algorithm. I made one for smallint, integer, and bigint. Should pg provide the LCM as well? Hmmm, probably not, too likely to overflow. Should there be a NUMERIC version as well? I'd say maybe yes. I'm wondering what it should do on N, 0 and 0, 0. Raise an error? Return 0? Return 1? return N? There should be some logic and comments explaining it. I'd test with INT_MIN and INT_MAX. Given that there are no overflows risk with the Euclidian descent, would it make sense that the int2 version call the int4 implementation? C modulo operator (%) is a pain because it is not positive remainder (2 % -3 == -1 vs 2 % 3 == 2, AFAICR). It does not seem that fixing the sign afterwards is the right thing to do. I'd rather turn both arguments positive before doing the descent. Which raises an issue with INT_MIN by the way, which has no positive:-( Also, the usual approach is to exchange args so that the largest is first, because there may be a software emulation if the hardware does not implement modulo. At least it was the case with some sparc processors 25 years ago:-) See for instance (the int min value is probably not well handled): https://svn.cri.ensmp.fr/svn/linear/trunk/src/arithmetique/pgcd.c Basically, welcome to arithmetic:-) -- Fabien.
Re: [PATCH v1] pg_ls_tmpdir to show directories
Hello Justin,
Ok, so this suggests recursing into subdirs, which requires to make a
separate function of the inner loop.
Yea, it suggests that; but, SRF_RETURN_NEXT doesn't make that very easy.
It'd need to accept the fcinfo argument, and pg_ls_dir_files would call it once
for every tuple to be returned. So it's recursive and saves its state...
The attached is pretty ugly, but I can't see how to do better.
Hmmm… I do agree with pretty ugly:-)
If it really neads to be in the structure, why not save the open directory
field in the caller and restore it after the recursive call, and pass the
rest of the structure as a pointer? Something like:
... root_function(...)
{
struct status_t status = initialization();
...
call rec_function(, path)
...
cleanup();
}
... rec_function(struct *status, path)
{
status->dir = opendir(path);
for (dir contents)
{
if (it_is_a_dir)
{
/* some comment about why we do that… */
dir_t saved = status->dir;
status->dir = NULL;
rec_function(status, subpath);
status->dir = saved;
}
}
closedir(status->dir), status->dir = NULL;
}
The alternative seems to be to build up a full list of pathnames in the SRF
initial branch, and stat them all later. Or stat them all in the INITIAL case,
and keep a list of stat structures to be emited during future calls.
--
Fabien.
Re: [PATCH v1] pg_ls_tmpdir to show directories
Hello Justin, Why not simply showing the files underneath their directories? /path/to/tmp/file1 /path/to/tmp/subdir1/file2 In which case probably showing the directory itself is not useful, and the is_dir column could be dropped? The names are expected to look like this: $ sudo find /var/lib/pgsql/12/data/base/pgsql_tmp -ls 1429774 drwxr-x--- 3 postgres postgres 4096 Dec 27 13:51 /var/lib/pgsql/12/data/base/pgsql_tmp 1698684 drwxr-x--- 2 postgres postgres 4096 Dec 7 01:35 /var/lib/pgsql/12/data/base/pgsql_tmp/pgsql_tmp11025.0.sharedfileset 169347 5492 -rw-r- 1 postgres postgres 5619712 Dec 7 01:35 /var/lib/pgsql/12/data/base/pgsql_tmp/pgsql_tmp11025.0.sharedfileset/0.0 169346 5380 -rw-r- 1 postgres postgres 5505024 Dec 7 01:35 /var/lib/pgsql/12/data/base/pgsql_tmp/pgsql_tmp11025.0.sharedfileset/1.0 I think we'd have to show sudbdir/file1, subdir/file2, not just file1, file2. It doesn't seem useful or nice to show a bunch of files called 0.0 or 1.0. Actually the results should be unique, either on filename or (dir,file). Ok, so this suggests recursing into subdirs, which requires to make a separate function of the inner loop. It's worth thinking if subdir should be a separate column. My 0.02 €: I would rather simply keep the full path and just add subdir contents, so that the function output does not change at all. -- Fabien.
Re: [PATCH v1] pg_ls_tmpdir to show directories
Re-added -hackers. Indeed, I left it out by accident. I tried to bounce the original mail but it did not work. Thanks for reviewing. On Fri, Dec 27, 2019 at 05:22:47PM +0100, Fabien COELHO wrote: The implementation simply extends an existing functions with a boolean to allow for sub-directories. However, the function does not seem to show subdir contents recursively. Should it be the case? STM that "//"-comments are not project policy. Sure, but the patch is less important than the design, which needs to be addressed first. The goal is to somehow show tmpfiles (or at least dirs) used by parallel workers. I mentioned a few possible ways, of which this was the simplest to implement. Showing files beneath the dir is probably good, but need to decide how to present it. Should there be a column for the dir (null if not a shared filesets)? Or some other presentation, like a boolean column "is_shared_fileset". Why not simply showing the files underneath their directories? /path/to/tmp/file1 /path/to/tmp/subdir1/file2 In which case probably showing the directory itself is not useful, and the is_dir column could be dropped? I'm unconvinced by the skipping condition: + if (!S_ISREG(attrib.st_mode) && + (!dir_ok && S_ISDIR(attrib.st_mode))) continue; which is pretty hard to read. ISTM you meant "not A and not (B and C)" instead? I can write it as two ifs. Hmmm. Not sure it would help that much. At least the condition must be right. Also, the comment should be updated. And, it's probably better to say: if (!ISDIR() || !dir_ok) I cannot say I like it. dir_ok is cheaper to test so could come first. ..which is same as: !(B && C), as you said. Ok, so you confirm that the condition was wrong. Catalog update should be a date + number? Maybe this is best left to the committer? Yes, but I preferred to include it, causing a deliberate conflict, to ensure it's not forgotten. Ok. -- Fabien.
Re: doc: vacuum full, fillfactor, and "extra space"
Hello Justin, I started writing this patch to avoid the possibly-misleading phrase: "with no extra space" (since it's expected to typically take ~2x space, or 1x "extra" space). But the original phrase "with no extra space" seems to be wrong anyway, since it actually follows fillfactor, so say that. Possibly should be backpatched. Patch applies and compiles. Given that the paragraph begins with "Plain VACUUM (without FULL)", it is better to have the VACUUM FULL explanations on a separate paragraph, and the fillfactor precision makes it explicit about what it does, although it could also be material for the NOTES section below. -- Fabien.
Re: Allow an alias to be attached directly to a JOIN ... USING
On Tue, 17 Sep 2019, Alvaro Herrera wrote: Indeed, that seems like a problem, and it's a good question. You can see this on unpatched master with SELECT x.filler FROM (pgbench_tellers AS t JOIN b USING (bid)) AS x. I'm not sure I understand why that problem is a blocker for this patch. As discussed on another thread, https://www.postgresql.org/message-id/flat/2aa57950-b1d7-e9b6-0770-fa592d565...@2ndquadrant.com the patch does not conform to spec SQL:2016 Part 2 Foundation Section 7.10 Basically "x" is expected to include *ONLY* joined attributes with USING, i.e. above only x.bid should exists, and per-table aliases are expected to still work for other attributes. ISTM that this patch could be "returned with feedback". -- Fabien.
Re: Should we rename amapi.h and amapi.c?
Bonjour Michaël, the syscache mainly, so instead the attached patch does the following changes: - amapi.h -> indexam.h - amapi.c -> indexamapi.c. Here we have an equivalent in access/table/ as tableamapi.c. - amvalidate.c -> indexamvalidate.c - amvalidate.h -> indexamvalidate.h - genam.c -> indexgenam.c Patch applies cleanly, compiles, make check-world ok. The change does not attempt to keep included files in ab order. Should it do that, or is it fixed later by some reindentation phase? -- Fabien.
pgbench - use pg logging capabilities
Bonjour Michaël, hello devs,
As suggested in "cce64a51", this patch make pgbench use postgres logging
capabilities.
I tried to use fatal/error/warning/info/debug where appropriate.
Some printing to stderr remain for some pgbench specific output.
The patch fixes a inconsistent test case name that I noticed in passing.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 6ab79eef99..a95f238127 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -70,6 +70,14 @@
#define M_PI 3.14159265358979323846
#endif
+/*
+ * Convenient shorcuts
+ */
+#define fatal pg_log_fatal
+#define error pg_log_error
+#define warning pg_log_warning
+#define info pg_log_info
+#define debug pg_log_debug
#define ERRCODE_UNDEFINED_TABLE "42P01"
@@ -541,7 +549,7 @@ static ParsedScript sql_script[MAX_SCRIPTS]; /* SQL script files */
static int num_scripts; /* number of scripts in sql_script[] */
static int64 total_weight = 0;
-static int debug = 0; /* debug flag */
+static int debug_level = 0; /* debug flag */
/* Builtin test scripts */
typedef struct BuiltinScript
@@ -787,14 +795,12 @@ strtoint64(const char *str, bool errorOK, int64 *result)
out_of_range:
if (!errorOK)
- fprintf(stderr,
-"value \"%s\" is out of range for type bigint\n", str);
+ error("value \"%s\" is out of range for type bigint", str);
return false;
invalid_syntax:
if (!errorOK)
- fprintf(stderr,
-"invalid input syntax for type bigint: \"%s\"\n", str);
+ error("invalid input syntax for type bigint: \"%s\"", str);
return false;
}
@@ -810,16 +816,14 @@ strtodouble(const char *str, bool errorOK, double *dv)
if (unlikely(errno != 0))
{
if (!errorOK)
- fprintf(stderr,
- "value \"%s\" is out of range for type double\n", str);
+ error("value \"%s\" is out of range for type double", str);
return false;
}
if (unlikely(end == str || *end != '\0'))
{
if (!errorOK)
- fprintf(stderr,
- "invalid input syntax for type double: \"%s\"\n", str);
+ error("invalid input syntax for type double: \"%s\"", str);
return false;
}
return true;
@@ -1149,7 +1153,7 @@ executeStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
+ fatal("%s", PQerrorMessage(con));
exit(1);
}
PQclear(res);
@@ -1164,8 +1168,8 @@ tryExecuteStatement(PGconn *con, const char *sql)
res = PQexec(con, sql);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
- fprintf(stderr, "%s", PQerrorMessage(con));
- fprintf(stderr, "(ignoring this error and continuing anyway)\n");
+ error("%s", PQerrorMessage(con));
+ info("(ignoring this error and continuing anyway)");
}
PQclear(res);
}
@@ -1211,8 +1215,7 @@ doConnect(void)
if (!conn)
{
- fprintf(stderr, "connection to database \"%s\" failed\n",
- dbName);
+ error("connection to database \"%s\" failed", dbName);
return NULL;
}
@@ -1230,8 +1233,8 @@ doConnect(void)
/* check to see that the backend connection was successfully made */
if (PQstatus(conn) == CONNECTION_BAD)
{
- fprintf(stderr, "connection to database \"%s\" failed:\n%s",
-dbName, PQerrorMessage(conn));
+ error("connection to database \"%s\" failed", dbName);
+ error("%s", PQerrorMessage(conn));
PQfinish(conn);
return NULL;
}
@@ -1360,9 +1363,8 @@ makeVariableValue(Variable *var)
if (!strtodouble(var->svalue, true, ))
{
- fprintf(stderr,
- "malformed variable \"%s\" value: \"%s\"\n",
- var->name, var->svalue);
+ error("malformed variable \"%s\" value: \"%s\"",
+ var->name, var->svalue);
return false;
}
setDoubleValue(>value, dv);
@@ -1425,8 +1427,7 @@ lookupCreateVariable(CState *st, const char *context, char *name)
*/
if (!valid_variable_name(name))
{
- fprintf(stderr, "%s: invalid variable name: \"%s\"\n",
- context, name);
+ error("%s: invalid variable name: \"%s\"", context, name);
return NULL;
}
@@ -1635,7 +1636,7 @@ coerceToBool(PgBenchValue *pval, bool *bval)
}
else /* NULL, INT or DOUBLE */
{
- fprintf(stderr, "cannot coerce %s to boolean\n", valueTypeName(pval));
+ error("cannot coerce %s to boolean", valueTypeName(pval));
*bval = false; /* suppress uninitialized-variable warnings */
return false;
}
@@ -1680,7 +1681,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
if (isnan(dval) || !FLOAT8_FITS_IN_INT64(dval))
{
- fprintf(stderr, "double to int overflow for %f\n", dval);
+ error("double to int overflow for %f", dval);
return false;
}
*ival = (int64) dval;
@@ -1688,7 +1689,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
}
else /* BOOLEAN or NULL */
{
- fprintf(stderr, "cannot coerce %s to int\n", valueTypeName(pval));
+ error("cannot coerce %s to int", valueTypeName(pval));
return false;
}
}
@@ -1709,7 +1710,7 @@ coerceToDouble(PgBenchValue *pval, double *dval)
}
Re: psql's \watch is broken
Hello Tom, My 0.02 €: Given the rather small number of existing uses of CancelRequested, I wonder if it wouldn't be a better idea to rename it to cancel_pressed? I prefer the former because it is more functional (a cancellation has been requested, however the mean to do so) while "pressed" rather suggest a particular operation. Also, perhaps I am missing something, but I do not see anyplace in the current code base that ever *clears* CancelRequested. This was already like that in the initial version before the refactoring. ./src/bin/scripts/common.h:extern bool CancelRequested; ./src/bin/scripts/common.c:bool CancelRequested = false; ./src/bin/scripts/common.c: CancelRequested = true; ./src/bin/scripts/common.c: CancelRequested = true; ./src/bin/scripts/common.c: CancelRequested = true; ./src/bin/scripts/common.c: CancelRequested = true; ./src/bin/scripts/vacuumdb.c: if (CancelRequested) ./src/bin/scripts/vacuumdb.c: if (CancelRequested) ./src/bin/scripts/vacuumdb.c: if (i < 0 || CancelRequested) However "cancel_request" resets are in "psql/mainloop.c", and are changed to "CancelRequest = false" resets by Michaël patch, so all seems fine. How much has this code been tested? Is it really sane to remove the setting of that flag from psql_cancel_callback, as this patch does? ISTM that the callback is called from a function which sets CancelRequest? Is it sane that CancelRequested isn't declared volatile? I agree that it would seem appropriate, but the initial version I refactored was not declaring CancelRequested as volatile, so I did not change that. However, "cancel_pressed" is volatile, so merging the two would indeed suggest to declare it as volatile. -- Fabien.
Re: psql's \watch is broken
I've not dug into code itself, I just bisected it. Thanks for the report. I'll look into it. Looked at it already. Ah, the magic of timezones! And yes, I can see the difference. This comes from the switch from cancel_pressed to CancelRequested in psql, especially PSQLexecWatch() in this case. And actually, now that I look at it, I think that we should simply get rid of cancel_pressed in psql completely and replace it with CancelRequested. This also removes the need of having cancel_pressed defined in print.c, which was not really wanted originally. Attached is a patch which addresses the issue for me, and cleans up the code while on it. Fabien, Jeff, can you confirm please? Yep. Patch applies cleanly, compiles, works for me as well. -- Fabien.
Re: psql's \watch is broken
explain (analyze) select * from pgbench_accounts \watch 1 It behaves as expected. But once I break out of the loop with ctrl-C, then if I execute the same thing again it executes the command once, but shows no output and doesn't loop. It seems like some flag is getting set with ctrl-C, but then never gets reset. It was broken in this commit: commit a4fd3aa719e8f97299dfcf1a8f79b3017e2b8d8b Author: Michael Paquier Date: Mon Dec 2 11:18:56 2019 +0900 Refactor query cancellation code into src/fe_utils/ I've not dug into code itself, I just bisected it. Thanks for the report. I'll look into it. -- Fabien.
Re: [PATCH] print help from psql when user tries to run pg_restore, pg_dump etc
Hello Craig, New users frequently attempt to run PostgreSQL's command line utilities from the psql prompt. Alas, that is true. I also have the reverse, i.e. SQL commands fed to bash, which does not like it much. They tend to be confused when this appears to do absolutely nothing: psql=> pg_restore psql-> since they're generally not going to semicolon-terminate the command either. The attached patch detects common command names when they appear first on a new input line prints a help message. If the buffer is empty a more detailed message is printed and the input is swallowed. Otherwise, much like how we handle "help" etc, a short message is printed and the input is still added to the buffer. psql=> pg_restore "pg_restore" is a command line utility program. Use it from the system terminal or command prompt not from psql. … prompt, not from psql. (added comma?) psql=> psql=> select 1 psql-> pg_restore "pg_restore" is a command-line utility program not a psql command. See "help". psql-> Wording advice would be welcome. I'd be tempted to backpatch this, since it's one of the things I see users confused by most often now - right up there with pg_hba.conf issues, forgetting a semicolon in psql, etc. I doubt that backpathing is reasonable. Are we that sure that there is no legitimate reason to enter such lines on psql, eg: psql=> SELECT ' psql'> pg_whatever ...' psql-> ... Although I can confirm that the problem exists, I'm unsure about whether psql should fix it. What are the opinions around? -- Fabien.
Re: pgbench -i progress output on terminal
Attached v4. Patch applies cleanly, compiles, works for me. Put it back to ready. -- Fabien.
Re: fe-utils - share query cancellation code
Bonjour Michaël, Committed the patch after splitting things into two commits and after testing things from Linux and from a Windows console: the actual refactoring and the pgbench changes. I have found that we have a useless declaration of CancelRequested in common.h, which is already part of cancel.h. Ok. On top of that I think that we need to rework a bit the header inclusions of bin/scripts/, as per the attached. Looks fine to me: patch applies, compiles, runs. -- Fabien.
Re: pgbench -i progress output on terminal
Another question I have is why doing only that for the data initialization phase? Wouldn't it make sense to be consistent with the other tools having --progress and do the same dance in pgbench's printProgressReport()? I thought of it but did not suggest it. When running a bench I like seeing the last few seconds status to see the dynamic evolution at a glance, and overwriting the previous line would hide that. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
The below addition can be removed, it seems to be duplicate: Indeed. I'm unsure how this got into the patch, probably some rebase mix-up. Attached v7 removes the duplicates. Attached patch v8 is a rebase. -- Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index 6787ec1efd..de59a5cf65 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -49,17 +49,42 @@ BEGIN \; SELECT 2.0 AS "float" \; SELECT 'world' AS "text" \; COMMIT; + float +--- + 2.0 +(1 row) + + text +--- + world +(1 row) + -- compound with empty statements and spurious leading spacing -\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;; - ?column? --- -5 +\;\; SELECT 3 + 3 AS "+" \;\;\; SELECT ' ' || ' !' AS "||" \;\; SELECT 1 + 4 AS "+" \;; + + +--- + 6 +(1 row) + + || +- + ! +(1 row) + + + +--- + 5 (1 row) -- non ;-terminated statements SELECT 1 + 1 + 1 AS "add" \gset SELECT :add + 1 + 1 AS "add" \; SELECT :add + 1 + 1 AS "add" \gset + add +- + 5 +(1 row) + -- set operator SELECT 1 AS i UNION SELECT 2 ORDER BY i; i @@ -102,12 +127,12 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"; SELECT $1+| 4 |4 +| | AS "text" | | - SELECT $1 + $2| 2 |2 SELECT $1 + $2 + $3 AS "add" | 3 |3 + SELECT $1 + $2 AS "+" | 2 |2 SELECT $1 AS "float" | 1 |1 SELECT $1 AS "int"| 2 |2 SELECT $1 AS i UNION SELECT $2 ORDER BY i | 1 |2 - SELECT $1 || $2 | 1 |1 + SELECT $1 || $2 AS "||" | 1 |1 SELECT pg_stat_statements_reset() | 1 |1 WITH t(f) AS ( +| 1 |2 VALUES ($1), ($2) +| | diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql index 8b527070d4..ea3a31176e 100644 --- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql +++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql @@ -27,7 +27,7 @@ SELECT 'world' AS "text" \; COMMIT; -- compound with empty statements and spurious leading spacing -\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;; +\;\; SELECT 3 + 3 AS "+" \;\;\; SELECT ' ' || ' !' AS "||" \;\; SELECT 1 + 4 AS "+" \;; -- non ;-terminated statements SELECT 1 + 1 + 1 AS "add" \gset diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 48b081fd58..924ba93df2 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql commands included in the string to divide it into multiple transactions. (See for more details about how the server handles multi-query strings.) - Also, psql only prints the - result of the last SQL command in the string. - This is different from the behavior when the same string is read from - a file or fed to psql's standard input, - because then psql sends - each SQL command separately. - Because of this behavior, putting more than one SQL command in a - single -c string often has unexpected results. - It's better to use repeated -c commands or feed - multiple commands to psql's standard input, + If having several commands executed in one transaction is not desired, + use repeated -c commands or feed multiple commands to + psql's standard input, either using echo as illustrated above, or via a shell here-document, for example: @@ -3361,10 +3354,8 @@ select 1\; select 2\; select 3; commands included in the string to divide it into multiple transactions. (See for more details about how the server handles multi-query strings.) -psql prints only the last query result -it receives for each request; in this example, although all -three SELECTs are indeed executed, psql -only prints the 3. +psql prints all results it receives, one +after the other. diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 9c0d53689e..4876ac132c 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -348,6 +348,19 @@ CheckConnection(void) } +/* + * Show error message from result, if any, and check connection in passing. + */ +static void
Re: pgbench -i progress output on terminal
I wrote the v1 patch on CentOS Linux, and now on MacOS. It would be great if someone can volunteer to test on Windows terminal. I do not have that. Attached v3. Patch applies, compiles, works for me. No further comments. I switched the patch as ready. -- Fabien.
Re: pgbench -i progress output on terminal
Hello Amit, I have updated the patch based on these observations. Attached v2. Patch v2 applies & compiles cleanly, works for me. I'm not partial to Hungarian notation conventions, which is not widely used elsewhere in pg. I'd suggest eolchar -> eol or line_end or whatever, but others may have different opinion. Maybe having a char variable is a rare enough occurence which warrants advertising it. Maybe use fputc instead of fprintf in the closing output? I'm unsure about what happens on MacOS and Windows terminal, but if it works for other commands progress options, it is probably all right. -- Fabien.
Re: pgbench -i progress output on terminal
I wonder why we don't use the same style for $subject as pg_basebackup --progress, that is, use a carriage return instead of a newline after each line reporting the number of tuples copied? Patch applies cleanly, compiles, and works for me. My 0.02€: fprintf -> fputs or fputc to avoid a format parsing, or maybe use %c in the formats. As the format is not constant, ISTM that vfprintf should be called, not fprintf (even if in practice fprintf does call vfprintf internally). I'm not sure what the compilers does with isatty(fileno(stderr)), maybe the eol could be precomputed: char eol = isatty(...) ? '\r' : '\n'; and reused afterwards in the loop: fprintf(stderr, " %c", ..., eol); that would remove the added in-loop printing. -- Fabien.
Re: pgbench - add \aset to store results of a combined query
Michaël,
+ boolaset;
It seems to me that there is no point to have the variable aset in
Command because this structure includes already MetaCommand, so the
information is duplicated. [...] Perhaps I am missing something?
Yep. ISTM that you are missing that aset is not an independent meta
command like most others but really changes the state of the previous SQL
command, so that it needs to be stored into that with some additional
fields. This is the same with "gset" which is tagged by a non-null
"varprefix".
So I cannot remove the "aset" field.
And I would suggest to change readCommandResponse() to use a MetaCommand
in argument.
MetaCommand is not enough: we need varprefix, and then distinguishing
between aset and gset. Although this last point can be done with a
MetaCommand, ISTM that a bool is_aset is clear and good enough. It is
possible to switch if you insist on it, but I do not think it is
desirable.
Attached v4 removes an unwanted rebased comment duplication and does minor
changes while re-reading the code.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4c48a58ed2..2c1110c054 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1026,18 +1026,29 @@ pgbench options d
\gset [prefix]
+ \aset [prefix]
- This command may be used to end SQL queries, taking the place of the
+ These commands may be used to end SQL queries, taking the place of the
terminating semicolon (;).
- When this command is 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.
+ When the \gset command is 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.
+
+
+
+ When the \aset command is used, all combined SQL queries
+ (separated by \;) have their columns stored into variables
+ named after column names, and prefixed with prefix
+ if provided. If a query returns no row, no assignment is made and the variable
+ can be tested for existence to detect this. If a query returns more than one
+ row, the last value is kept.
@@ -1046,6 +1057,8 @@ pgbench options d
p_two and p_three
with integers from the third query.
The result of the second query is discarded.
+ The result of the two last combined queries are stored in variables
+ four and five.
UPDATE pgbench_accounts
SET abalance = abalance + :delta
@@ -1054,6 +1067,7 @@ UPDATE pgbench_accounts
-- compound of two queries
SELECT 1 \;
SELECT 2 AS two, 3 AS three \gset p_
+SELECT 4 AS four \; SELECT 5 AS five \aset
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4a7ac1f821..da67846814 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -480,6 +480,7 @@ typedef enum MetaCommand
META_SHELL, /* \shell */
META_SLEEP, /* \sleep */
META_GSET, /* \gset */
+ META_ASET, /* \aset */
META_IF, /* \if */
META_ELIF, /* \elif */
META_ELSE, /* \else */
@@ -512,6 +513,7 @@ static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
* varprefix SQL commands terminated with \gset have this set
*to a non NULL value. If nonempty, it's used to prefix the
*variable name that receives the value.
+ * aset do gset on all possible queries of a combined query (\;).
* expr Parsed expression, if needed.
* stats Time spent in this command.
*/
@@ -524,6 +526,7 @@ typedef struct Command
int argc;
char *argv[MAX_ARGS];
char *varprefix;
+ bool aset;
PgBenchExpr *expr;
SimpleStats stats;
} Command;
@@ -2503,6 +2506,8 @@ getMetaCommand(const char *cmd)
mc = META_ENDIF;
else if (pg_strcasecmp(cmd, "gset") == 0)
mc = META_GSET;
+ else if (pg_strcasecmp(cmd, "aset") == 0)
+ mc = META_ASET;
else
mc = META_NONE;
return mc;
@@ -2734,12 +2739,12 @@ sendCommand(CState *st, Command *command)
* Process query response from the backend.
*
* If varprefix is not NULL, it's the variable name prefix where to store
- * the results of the *last* command.
+ * the results of the *last* command (gset) or *all* commands (aset).
*
* Returns true if everything is A-OK, false if any error occurs.
*/
static bool
-readCommandResponse(CState *st, char *varprefix)
+readCommandResponse(CState *st, char *varprefix, bool is_aset)
{
PGresult *res;
PGresult *next_res;
@@ -2759,7 +2764,7 @@ readCommandResponse(CState *st, char *varprefix)
{
case PGRES_COMMAND_OK: /* non-SELECT commands */
case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */
-if (is_last && varprefix != NULL)
+if
Re: fe-utils - share query cancellation code
Bonjour Michaël,
The query cancellation added to pgbench is different than the actual
refactoring, and it is a result of the refactoring, so I would rather
split that into two different commits for clarity. The split is easy
enough, so that's fine not to send two different patches.
Yep, different file set.
Compilation of the patch fails for me on Windows for psql:
unresolved external symbol sigint_interrupt_jmp
Please note that Mr Robot complains as well at build time:
http://commitfest.cputube.org/fabien-coelho.html
Visibly the problem here is that sigint_interrupt_jmp is declared in
common.h, but you have moved it to a non-WIN32 section of the code in
psql/common.c. And actually, note that copy.c and mainloop.c make use
of it...
Indeed.
I would not worry much about SIGTERM as you mentioned in the comments,
query cancellations are associated to SIGINT now. There could be an
argument possible later to allow passing down a custom signal though.
Ok.
Attached is an updated patch with a couple of edits I have done,
including the removal of some noise diffs and the previous edits.
Thanks!
I am switching the patch as waiting on author, bumping it to next CF at
the same time. Could you please fix the Windows issue?
I do not have a Windows host, so I can only do blind tests. I just moved
the declaration out of !WIN32 scope in attached v7, which might solve the
resolution error. All other issues pointed out above seem fixed in the v6
you sent.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4a7ac1f821..5129aea516 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -59,6 +59,7 @@
#include "common/int.h"
#include "common/logging.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -3894,6 +3895,9 @@ initGenerateDataClientSide(PGconn *con)
exit(1);
}
+ if (CancelRequested)
+ break;
+
/*
* If we want to stick with the original logging, print a message each
* 100k inserted rows.
@@ -4109,6 +4113,9 @@ runInitSteps(const char *initialize_steps)
if ((con = doConnect()) == NULL)
exit(1);
+ setup_cancel_handler(NULL);
+ SetCancelConn(con);
+
for (step = initialize_steps; *step != '\0'; step++)
{
instr_time start;
@@ -4176,6 +4183,7 @@ runInitSteps(const char *initialize_steps)
}
fprintf(stderr, "done in %.2f s (%s).\n", run_time, stats.data);
+ ResetCancelConn();
PQfinish(con);
termPQExpBuffer();
}
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 0a2597046d..48b6279403 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -29,6 +29,7 @@
#include "copy.h"
#include "crosstabview.h"
#include "describe.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/print.h"
#include "fe_utils/string_utils.h"
#include "help.h"
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 53a1ea2bdb..442ff2fe5d 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -23,6 +23,7 @@
#include "common/logging.h"
#include "copy.h"
#include "crosstabview.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/mbprint.h"
#include "fe_utils/string_utils.h"
#include "portability/instr_time.h"
@@ -241,7 +242,7 @@ NoticeProcessor(void *arg, const char *message)
*
* SIGINT is supposed to abort all long-running psql operations, not only
* database queries. In most places, this is accomplished by checking
- * cancel_pressed during long-running loops. However, that won't work when
+ * CancelRequested during long-running loops. However, that won't work when
* blocked on user input (in readline() or fgets()). In those places, we
* set sigint_interrupt_enabled true while blocked, instructing the signal
* catcher to longjmp through sigint_interrupt_jmp. We assume readline and
@@ -252,34 +253,11 @@ volatile bool sigint_interrupt_enabled = false;
sigjmp_buf sigint_interrupt_jmp;
-static PGcancel *volatile cancelConn = NULL;
-
-#ifdef WIN32
-static CRITICAL_SECTION cancelConnLock;
-#endif
-
-/*
- * Write a simple string to stderr --- must be safe in a signal handler.
- * We ignore the write() result since there's not much we could do about it.
- * Certain compilers make that harder than it ought to be.
- */
-#define write_stderr(str) \
- do { \
- const char *str_ = (str); \
- int rc_; \
- rc_ = write(fileno(stderr), str_, strlen(str_)); \
- (void) rc_; \
- } while (0)
-
-
#ifndef WIN32
static void
-handle_sigint(SIGNAL_ARGS)
+psql_sigint_callback(void)
{
- int save_errno = errno;
- char errbuf[256];
-
/* if we are waiting for input, longjmp out of it */
if (sigint_interrupt_enabled)
{
@@ -288,74 +266,20 @@ handle_sigint(SIGNAL_ARGS)
}
/* else, set cancel flag to stop any long-running loops */
- cancel_pressed = true;
-
- /* and send QueryCancel if we are processing a database query */
- if (cancelConn != NULL)
- {
- if (PQcancel(cancelConn, errbuf,
Re: pgbench -i progress output on terminal
Hello Amit, I wonder why we don't use the same style for $subject as pg_basebackup --progress, that is, use a carriage return instead of a newline after each line reporting the number of tuples copied? Why not. Attached patch for that. I'll look into it. Could you add it to the CF app? -- Fabien.
Re: [patch]socket_timeout in interfaces/libpq
Michaël, Not this round. You have registered yourself as a reviewer of this patch since the end of June. Could you please avoid that? Sometimes people skips patches when they see someone already registered to review it. Yep. ISTM that I did a few reviews on early versions of the patch, which was really a set of 3 patches. The patch applies cleanly so I am movingit to next CF. (FWIW, I still have the same impression as upthread, looking again at the patch, but let's see if there are other opinions.) AFAICR, I was partly dissuated to pursue reviews by your comment that somehow the feature had no clear consensus, so I thought that the patch was implicitely rejected. Although I work for free, I try to avoid working for nothing:-) It is still unclear from your above comment whether the patch would ever get committed, so this does not motivate spending time on it. -- Fabien.
