public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v14 1/2] Pgbench errors: use the Variables structure for client variables
26+ messages / 7 participants
[nested] [flat]
* [PATCH v14 1/2] Pgbench errors: use the Variables structure for client variables
@ 2021-05-26 07:58 Yugo Nagata <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Yugo Nagata @ 2021-05-26 07:58 UTC (permalink / raw)
This is most important when it is used to reset client variables during the
repeating of transactions after serialization/deadlock failures.
Don't allocate Variable structs one by one. Instead, add a constant margin each
time it overflows.
---
src/bin/pgbench/pgbench.c | 163 +++++++++++++++++++++++---------------
1 file changed, 100 insertions(+), 63 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4aeccd93af..3629caba42 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -287,6 +287,12 @@ const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
+/*
+ * We don't want to allocate variables one by one; for efficiency, add a
+ * constant margin each time it overflows.
+ */
+#define VARIABLES_ALLOC_MARGIN 8
+
/*
* Variable definitions.
*
@@ -304,6 +310,24 @@ typedef struct
PgBenchValue value; /* actual variable's value */
} Variable;
+/*
+ * Data structure for client variables.
+ */
+typedef struct
+{
+ Variable *vars; /* array of variable definitions */
+ int nvars; /* number of variables */
+
+ /*
+ * The maximum number of variables that we can currently store in 'vars'
+ * without having to reallocate more space. We must always have max_vars >=
+ * nvars.
+ */
+ int max_vars;
+
+ bool vars_sorted; /* are variables sorted by name? */
+} Variables;
+
#define MAX_SCRIPTS 128 /* max number of SQL scripts allowed */
#define SHELL_COMMAND_SIZE 256 /* maximum size allowed for shell command */
@@ -460,9 +484,7 @@ typedef struct
int command; /* command number in script */
/* client variables */
- Variable *variables; /* array of variable definitions */
- int nvariables; /* number of variables */
- bool vars_sorted; /* are variables sorted by name? */
+ Variables variables;
/* various times about current transaction in microseconds */
pg_time_usec_t txn_scheduled; /* scheduled start time of transaction */
@@ -1418,39 +1440,39 @@ compareVariableNames(const void *v1, const void *v2)
/* Locate a variable by name; returns NULL if unknown */
static Variable *
-lookupVariable(CState *st, char *name)
+lookupVariable(Variables *variables, char *name)
{
Variable key;
/* On some versions of Solaris, bsearch of zero items dumps core */
- if (st->nvariables <= 0)
+ if (variables->nvars <= 0)
return NULL;
/* Sort if we have to */
- if (!st->vars_sorted)
+ if (!variables->vars_sorted)
{
- qsort((void *) st->variables, st->nvariables, sizeof(Variable),
+ qsort((void *) variables->vars, variables->nvars, sizeof(Variable),
compareVariableNames);
- st->vars_sorted = true;
+ variables->vars_sorted = true;
}
/* Now we can search */
key.name = name;
return (Variable *) bsearch((void *) &key,
- (void *) st->variables,
- st->nvariables,
+ (void *) variables->vars,
+ variables->nvars,
sizeof(Variable),
compareVariableNames);
}
/* Get the value of a variable, in string form; returns NULL if unknown */
static char *
-getVariable(CState *st, char *name)
+getVariable(Variables *variables, char *name)
{
Variable *var;
char stringform[64];
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
return NULL; /* not found */
@@ -1582,21 +1604,37 @@ valid_variable_name(const char *name)
return true;
}
+/*
+ * Make sure there is enough space for 'needed' more variable in the variables
+ * array.
+ */
+static void
+enlargeVariables(Variables *variables, int needed)
+{
+ /* total number of variables required now */
+ needed += variables->nvars;
+
+ if (variables->max_vars < needed)
+ {
+ variables->max_vars = needed + VARIABLES_ALLOC_MARGIN;
+ variables->vars = (Variable *)
+ pg_realloc(variables->vars, variables->max_vars * sizeof(Variable));
+ }
+}
+
/*
* Lookup a variable by name, creating it if need be.
* Caller is expected to assign a value to the variable.
* Returns NULL on failure (bad name).
*/
static Variable *
-lookupCreateVariable(CState *st, const char *context, char *name)
+lookupCreateVariable(Variables *variables, const char *context, char *name)
{
Variable *var;
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
{
- Variable *newvars;
-
/*
* Check for the name only when declaring a new variable to avoid
* overhead.
@@ -1608,23 +1646,17 @@ lookupCreateVariable(CState *st, const char *context, char *name)
}
/* Create variable at the end of the array */
- if (st->variables)
- newvars = (Variable *) pg_realloc(st->variables,
- (st->nvariables + 1) * sizeof(Variable));
- else
- newvars = (Variable *) pg_malloc(sizeof(Variable));
-
- st->variables = newvars;
+ enlargeVariables(variables, 1);
- var = &newvars[st->nvariables];
+ var = &(variables->vars[variables->nvars]);
var->name = pg_strdup(name);
var->svalue = NULL;
/* caller is expected to initialize remaining fields */
- st->nvariables++;
+ variables->nvars++;
/* we don't re-sort the array till we have to */
- st->vars_sorted = false;
+ variables->vars_sorted = false;
}
return var;
@@ -1633,12 +1665,13 @@ lookupCreateVariable(CState *st, const char *context, char *name)
/* Assign a string value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariable(CState *st, const char *context, char *name, const char *value)
+putVariable(Variables *variables, const char *context, char *name,
+ const char *value)
{
Variable *var;
char *val;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1656,12 +1689,12 @@ putVariable(CState *st, const char *context, char *name, const char *value)
/* Assign a value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableValue(CState *st, const char *context, char *name,
+putVariableValue(Variables *variables, const char *context, char *name,
const PgBenchValue *value)
{
Variable *var;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1676,12 +1709,13 @@ putVariableValue(CState *st, const char *context, char *name,
/* Assign an integer value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableInt(CState *st, const char *context, char *name, int64 value)
+putVariableInt(Variables *variables, const char *context, char *name,
+ int64 value)
{
PgBenchValue val;
setIntValue(&val, value);
- return putVariableValue(st, context, name, &val);
+ return putVariableValue(variables, context, name, &val);
}
/*
@@ -1740,7 +1774,7 @@ replaceVariable(char **sql, char *param, int len, char *value)
}
static char *
-assignVariables(CState *st, char *sql)
+assignVariables(Variables *variables, char *sql)
{
char *p,
*name,
@@ -1761,7 +1795,7 @@ assignVariables(CState *st, char *sql)
continue;
}
- val = getVariable(st, name);
+ val = getVariable(variables, name);
free(name);
if (val == NULL)
{
@@ -1776,12 +1810,13 @@ assignVariables(CState *st, char *sql)
}
static void
-getQueryParams(CState *st, const Command *command, const char **params)
+getQueryParams(Variables *variables, const Command *command,
+ const char **params)
{
int i;
for (i = 0; i < command->argc - 1; i++)
- params[i] = getVariable(st, command->argv[i + 1]);
+ params[i] = getVariable(variables, command->argv[i + 1]);
}
static char *
@@ -2649,7 +2684,7 @@ evaluateExpr(CState *st, PgBenchExpr *expr, PgBenchValue *retval)
{
Variable *var;
- if ((var = lookupVariable(st, expr->u.variable.varname)) == NULL)
+ if ((var = lookupVariable(&st->variables, expr->u.variable.varname)) == NULL)
{
pg_log_error("undefined variable \"%s\"", expr->u.variable.varname);
return false;
@@ -2719,7 +2754,7 @@ getMetaCommand(const char *cmd)
* Return true if succeeded, or false on error.
*/
static bool
-runShellCommand(CState *st, char *variable, char **argv, int argc)
+runShellCommand(Variables *variables, char *variable, char **argv, int argc)
{
char command[SHELL_COMMAND_SIZE];
int i,
@@ -2750,7 +2785,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
{
arg = argv[i] + 1; /* a string literal starting with colons */
}
- else if ((arg = getVariable(st, argv[i] + 1)) == NULL)
+ else if ((arg = getVariable(variables, argv[i] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[i]);
return false;
@@ -2811,7 +2846,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
pg_log_error("%s: shell command must return an integer (not \"%s\")", argv[0], res);
return false;
}
- if (!putVariableInt(st, "setshell", variable, retval))
+ if (!putVariableInt(variables, "setshell", variable, retval))
return false;
pg_log_debug("%s: shell parameter name: \"%s\", value: \"%s\"", argv[0], argv[1], res);
@@ -2863,7 +2898,7 @@ sendCommand(CState *st, Command *command)
char *sql;
sql = pg_strdup(command->argv[0]);
- sql = assignVariables(st, sql);
+ sql = assignVariables(&st->variables, sql);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQuery(st->con, sql);
@@ -2874,7 +2909,7 @@ sendCommand(CState *st, Command *command)
const char *sql = command->argv[0];
const char *params[MAX_ARGS];
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQueryParams(st->con, sql, command->argc - 1,
@@ -2921,7 +2956,7 @@ sendCommand(CState *st, Command *command)
st->prepared[st->use_file] = true;
}
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
preparedStatementName(name, st->use_file, st->command);
pg_log_debug("client %d sending %s", st->id, name);
@@ -3014,7 +3049,7 @@ readCommandResponse(CState *st, MetaCommand meta, char *varprefix)
varname = psprintf("%s%s", varprefix, varname);
/* store last row result as a string */
- if (!putVariable(st, meta == META_ASET ? "aset" : "gset", varname,
+ if (!putVariable(&st->variables, meta == META_ASET ? "aset" : "gset", varname,
PQgetvalue(res, ntuples - 1, fld)))
{
/* internal error */
@@ -3075,14 +3110,14 @@ error:
* of delay, in microseconds. Returns true on success, false on error.
*/
static bool
-evaluateSleep(CState *st, int argc, char **argv, int *usecs)
+evaluateSleep(Variables *variables, int argc, char **argv, int *usecs)
{
char *var;
int usec;
if (*argv[1] == ':')
{
- if ((var = getVariable(st, argv[1] + 1)) == NULL)
+ if ((var = getVariable(variables, argv[1] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[1] + 1);
return false;
@@ -3614,7 +3649,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
* latency will be recorded in CSTATE_SLEEP state, not here, after the
* delay has elapsed.)
*/
- if (!evaluateSleep(st, argc, argv, &usec))
+ if (!evaluateSleep(&st->variables, argc, argv, &usec))
{
commandFailed(st, "sleep", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3635,7 +3670,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
return CSTATE_ABORTED;
}
- if (!putVariableValue(st, argv[0], argv[1], &result))
+ if (!putVariableValue(&st->variables, argv[0], argv[1], &result))
{
commandFailed(st, "set", "assignment of meta-command failed");
return CSTATE_ABORTED;
@@ -3705,7 +3740,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SETSHELL)
{
- if (!runShellCommand(st, argv[1], argv + 2, argc - 2))
+ if (!runShellCommand(&st->variables, argv[1], argv + 2, argc - 2))
{
commandFailed(st, "setshell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3713,7 +3748,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SHELL)
{
- if (!runShellCommand(st, NULL, argv + 1, argc - 1))
+ if (!runShellCommand(&st->variables, NULL, argv + 1, argc - 1))
{
commandFailed(st, "shell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -5995,7 +6030,7 @@ main(int argc, char **argv)
}
*p++ = '\0';
- if (!putVariable(&state[0], "option", optarg, p))
+ if (!putVariable(&state[0].variables, "option", optarg, p))
exit(1);
}
break;
@@ -6335,19 +6370,19 @@ main(int argc, char **argv)
int j;
state[i].id = i;
- for (j = 0; j < state[0].nvariables; j++)
+ for (j = 0; j < state[0].variables.nvars; j++)
{
- Variable *var = &state[0].variables[j];
+ Variable *var = &state[0].variables.vars[j];
if (var->value.type != PGBT_NO_VALUE)
{
- if (!putVariableValue(&state[i], "startup",
+ if (!putVariableValue(&state[i].variables, "startup",
var->name, &var->value))
exit(1);
}
else
{
- if (!putVariable(&state[i], "startup",
+ if (!putVariable(&state[i].variables, "startup",
var->name, var->svalue))
exit(1);
}
@@ -6382,11 +6417,11 @@ main(int argc, char **argv)
* :scale variables normally get -s or database scale, but don't override
* an explicit -D switch
*/
- if (lookupVariable(&state[0], "scale") == NULL)
+ if (lookupVariable(&state[0].variables, "scale") == NULL)
{
for (i = 0; i < nclients; i++)
{
- if (!putVariableInt(&state[i], "startup", "scale", scale))
+ if (!putVariableInt(&state[i].variables, "startup", "scale", scale))
exit(1);
}
}
@@ -6395,30 +6430,32 @@ main(int argc, char **argv)
* Define a :client_id variable that is unique per connection. But don't
* override an explicit -D switch.
*/
- if (lookupVariable(&state[0], "client_id") == NULL)
+ if (lookupVariable(&state[0].variables, "client_id") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "client_id", i))
+ if (!putVariableInt(&state[i].variables, "startup", "client_id", i))
exit(1);
}
/* set default seed for hash functions */
- if (lookupVariable(&state[0], "default_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "default_seed") == NULL)
{
uint64 seed =
((uint64) pg_jrand48(base_random_sequence.xseed) & 0xFFFFFFFF) |
(((uint64) pg_jrand48(base_random_sequence.xseed) & 0xFFFFFFFF) << 32);
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "default_seed", (int64) seed))
+ if (!putVariableInt(&state[i].variables, "startup", "default_seed",
+ (int64) seed))
exit(1);
}
/* set random seed unless overwritten */
- if (lookupVariable(&state[0], "random_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "random_seed") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "random_seed", random_seed))
+ if (!putVariableInt(&state[i].variables, "startup", "random_seed",
+ random_seed))
exit(1);
}
--
2.17.1
--Multipart=_Thu__8_Jul_2021_08_35_06_+0900_rt42K=fIffwe5=xI--
^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v16 1/2] Pgbench errors: use the Variables structure for client variables
@ 2021-05-26 07:58 Yugo Nagata <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Yugo Nagata @ 2021-05-26 07:58 UTC (permalink / raw)
This is most important when it is used to reset client variables during the
repeating of transactions after serialization/deadlock failures.
Don't allocate Variable structs one by one. Instead, add a constant margin each
time it overflows.
---
src/bin/pgbench/pgbench.c | 163 +++++++++++++++++++++++---------------
1 file changed, 100 insertions(+), 63 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index f166a77e3a..3743e36dac 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -289,6 +289,12 @@ const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
+/*
+ * We don't want to allocate variables one by one; for efficiency, add a
+ * constant margin each time it overflows.
+ */
+#define VARIABLES_ALLOC_MARGIN 8
+
/*
* Variable definitions.
*
@@ -306,6 +312,24 @@ typedef struct
PgBenchValue value; /* actual variable's value */
} Variable;
+/*
+ * Data structure for client variables.
+ */
+typedef struct
+{
+ Variable *vars; /* array of variable definitions */
+ int nvars; /* number of variables */
+
+ /*
+ * The maximum number of variables that we can currently store in 'vars'
+ * without having to reallocate more space. We must always have max_vars >=
+ * nvars.
+ */
+ int max_vars;
+
+ bool vars_sorted; /* are variables sorted by name? */
+} Variables;
+
#define MAX_SCRIPTS 128 /* max number of SQL scripts allowed */
#define SHELL_COMMAND_SIZE 256 /* maximum size allowed for shell command */
@@ -460,9 +484,7 @@ typedef struct
int command; /* command number in script */
/* client variables */
- Variable *variables; /* array of variable definitions */
- int nvariables; /* number of variables */
- bool vars_sorted; /* are variables sorted by name? */
+ Variables variables;
/* various times about current transaction in microseconds */
pg_time_usec_t txn_scheduled; /* scheduled start time of transaction */
@@ -1398,39 +1420,39 @@ compareVariableNames(const void *v1, const void *v2)
/* Locate a variable by name; returns NULL if unknown */
static Variable *
-lookupVariable(CState *st, char *name)
+lookupVariable(Variables *variables, char *name)
{
Variable key;
/* On some versions of Solaris, bsearch of zero items dumps core */
- if (st->nvariables <= 0)
+ if (variables->nvars <= 0)
return NULL;
/* Sort if we have to */
- if (!st->vars_sorted)
+ if (!variables->vars_sorted)
{
- qsort((void *) st->variables, st->nvariables, sizeof(Variable),
+ qsort((void *) variables->vars, variables->nvars, sizeof(Variable),
compareVariableNames);
- st->vars_sorted = true;
+ variables->vars_sorted = true;
}
/* Now we can search */
key.name = name;
return (Variable *) bsearch((void *) &key,
- (void *) st->variables,
- st->nvariables,
+ (void *) variables->vars,
+ variables->nvars,
sizeof(Variable),
compareVariableNames);
}
/* Get the value of a variable, in string form; returns NULL if unknown */
static char *
-getVariable(CState *st, char *name)
+getVariable(Variables *variables, char *name)
{
Variable *var;
char stringform[64];
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
return NULL; /* not found */
@@ -1562,21 +1584,37 @@ valid_variable_name(const char *name)
return true;
}
+/*
+ * Make sure there is enough space for 'needed' more variable in the variables
+ * array.
+ */
+static void
+enlargeVariables(Variables *variables, int needed)
+{
+ /* total number of variables required now */
+ needed += variables->nvars;
+
+ if (variables->max_vars < needed)
+ {
+ variables->max_vars = needed + VARIABLES_ALLOC_MARGIN;
+ variables->vars = (Variable *)
+ pg_realloc(variables->vars, variables->max_vars * sizeof(Variable));
+ }
+}
+
/*
* Lookup a variable by name, creating it if need be.
* Caller is expected to assign a value to the variable.
* Returns NULL on failure (bad name).
*/
static Variable *
-lookupCreateVariable(CState *st, const char *context, char *name)
+lookupCreateVariable(Variables *variables, const char *context, char *name)
{
Variable *var;
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
{
- Variable *newvars;
-
/*
* Check for the name only when declaring a new variable to avoid
* overhead.
@@ -1588,23 +1626,17 @@ lookupCreateVariable(CState *st, const char *context, char *name)
}
/* Create variable at the end of the array */
- if (st->variables)
- newvars = (Variable *) pg_realloc(st->variables,
- (st->nvariables + 1) * sizeof(Variable));
- else
- newvars = (Variable *) pg_malloc(sizeof(Variable));
-
- st->variables = newvars;
+ enlargeVariables(variables, 1);
- var = &newvars[st->nvariables];
+ var = &(variables->vars[variables->nvars]);
var->name = pg_strdup(name);
var->svalue = NULL;
/* caller is expected to initialize remaining fields */
- st->nvariables++;
+ variables->nvars++;
/* we don't re-sort the array till we have to */
- st->vars_sorted = false;
+ variables->vars_sorted = false;
}
return var;
@@ -1613,12 +1645,13 @@ lookupCreateVariable(CState *st, const char *context, char *name)
/* Assign a string value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariable(CState *st, const char *context, char *name, const char *value)
+putVariable(Variables *variables, const char *context, char *name,
+ const char *value)
{
Variable *var;
char *val;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1636,12 +1669,12 @@ putVariable(CState *st, const char *context, char *name, const char *value)
/* Assign a value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableValue(CState *st, const char *context, char *name,
+putVariableValue(Variables *variables, const char *context, char *name,
const PgBenchValue *value)
{
Variable *var;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1656,12 +1689,13 @@ putVariableValue(CState *st, const char *context, char *name,
/* Assign an integer value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableInt(CState *st, const char *context, char *name, int64 value)
+putVariableInt(Variables *variables, const char *context, char *name,
+ int64 value)
{
PgBenchValue val;
setIntValue(&val, value);
- return putVariableValue(st, context, name, &val);
+ return putVariableValue(variables, context, name, &val);
}
/*
@@ -1720,7 +1754,7 @@ replaceVariable(char **sql, char *param, int len, char *value)
}
static char *
-assignVariables(CState *st, char *sql)
+assignVariables(Variables *variables, char *sql)
{
char *p,
*name,
@@ -1741,7 +1775,7 @@ assignVariables(CState *st, char *sql)
continue;
}
- val = getVariable(st, name);
+ val = getVariable(variables, name);
free(name);
if (val == NULL)
{
@@ -1756,12 +1790,13 @@ assignVariables(CState *st, char *sql)
}
static void
-getQueryParams(CState *st, const Command *command, const char **params)
+getQueryParams(Variables *variables, const Command *command,
+ const char **params)
{
int i;
for (i = 0; i < command->argc - 1; i++)
- params[i] = getVariable(st, command->argv[i + 1]);
+ params[i] = getVariable(variables, command->argv[i + 1]);
}
static char *
@@ -2629,7 +2664,7 @@ evaluateExpr(CState *st, PgBenchExpr *expr, PgBenchValue *retval)
{
Variable *var;
- if ((var = lookupVariable(st, expr->u.variable.varname)) == NULL)
+ if ((var = lookupVariable(&st->variables, expr->u.variable.varname)) == NULL)
{
pg_log_error("undefined variable \"%s\"", expr->u.variable.varname);
return false;
@@ -2699,7 +2734,7 @@ getMetaCommand(const char *cmd)
* Return true if succeeded, or false on error.
*/
static bool
-runShellCommand(CState *st, char *variable, char **argv, int argc)
+runShellCommand(Variables *variables, char *variable, char **argv, int argc)
{
char command[SHELL_COMMAND_SIZE];
int i,
@@ -2730,7 +2765,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
{
arg = argv[i] + 1; /* a string literal starting with colons */
}
- else if ((arg = getVariable(st, argv[i] + 1)) == NULL)
+ else if ((arg = getVariable(variables, argv[i] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[i]);
return false;
@@ -2791,7 +2826,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
pg_log_error("%s: shell command must return an integer (not \"%s\")", argv[0], res);
return false;
}
- if (!putVariableInt(st, "setshell", variable, retval))
+ if (!putVariableInt(variables, "setshell", variable, retval))
return false;
pg_log_debug("%s: shell parameter name: \"%s\", value: \"%s\"", argv[0], argv[1], res);
@@ -2843,7 +2878,7 @@ sendCommand(CState *st, Command *command)
char *sql;
sql = pg_strdup(command->argv[0]);
- sql = assignVariables(st, sql);
+ sql = assignVariables(&st->variables, sql);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQuery(st->con, sql);
@@ -2854,7 +2889,7 @@ sendCommand(CState *st, Command *command)
const char *sql = command->argv[0];
const char *params[MAX_ARGS];
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQueryParams(st->con, sql, command->argc - 1,
@@ -2901,7 +2936,7 @@ sendCommand(CState *st, Command *command)
st->prepared[st->use_file] = true;
}
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
preparedStatementName(name, st->use_file, st->command);
pg_log_debug("client %d sending %s", st->id, name);
@@ -2994,7 +3029,7 @@ readCommandResponse(CState *st, MetaCommand meta, char *varprefix)
varname = psprintf("%s%s", varprefix, varname);
/* store last row result as a string */
- if (!putVariable(st, meta == META_ASET ? "aset" : "gset", varname,
+ if (!putVariable(&st->variables, meta == META_ASET ? "aset" : "gset", varname,
PQgetvalue(res, ntuples - 1, fld)))
{
/* internal error */
@@ -3055,14 +3090,14 @@ error:
* of delay, in microseconds. Returns true on success, false on error.
*/
static bool
-evaluateSleep(CState *st, int argc, char **argv, int *usecs)
+evaluateSleep(Variables *variables, int argc, char **argv, int *usecs)
{
char *var;
int usec;
if (*argv[1] == ':')
{
- if ((var = getVariable(st, argv[1] + 1)) == NULL)
+ if ((var = getVariable(variables, argv[1] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[1] + 1);
return false;
@@ -3627,7 +3662,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
* latency will be recorded in CSTATE_SLEEP state, not here, after the
* delay has elapsed.)
*/
- if (!evaluateSleep(st, argc, argv, &usec))
+ if (!evaluateSleep(&st->variables, argc, argv, &usec))
{
commandFailed(st, "sleep", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3648,7 +3683,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
return CSTATE_ABORTED;
}
- if (!putVariableValue(st, argv[0], argv[1], &result))
+ if (!putVariableValue(&st->variables, argv[0], argv[1], &result))
{
commandFailed(st, "set", "assignment of meta-command failed");
return CSTATE_ABORTED;
@@ -3718,7 +3753,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SETSHELL)
{
- if (!runShellCommand(st, argv[1], argv + 2, argc - 2))
+ if (!runShellCommand(&st->variables, argv[1], argv + 2, argc - 2))
{
commandFailed(st, "setshell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3726,7 +3761,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SHELL)
{
- if (!runShellCommand(st, NULL, argv + 1, argc - 1))
+ if (!runShellCommand(&st->variables, NULL, argv + 1, argc - 1))
{
commandFailed(st, "shell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -6020,7 +6055,7 @@ main(int argc, char **argv)
}
*p++ = '\0';
- if (!putVariable(&state[0], "option", optarg, p))
+ if (!putVariable(&state[0].variables, "option", optarg, p))
exit(1);
}
break;
@@ -6348,19 +6383,19 @@ main(int argc, char **argv)
int j;
state[i].id = i;
- for (j = 0; j < state[0].nvariables; j++)
+ for (j = 0; j < state[0].variables.nvars; j++)
{
- Variable *var = &state[0].variables[j];
+ Variable *var = &state[0].variables.vars[j];
if (var->value.type != PGBT_NO_VALUE)
{
- if (!putVariableValue(&state[i], "startup",
+ if (!putVariableValue(&state[i].variables, "startup",
var->name, &var->value))
exit(1);
}
else
{
- if (!putVariable(&state[i], "startup",
+ if (!putVariable(&state[i].variables, "startup",
var->name, var->svalue))
exit(1);
}
@@ -6398,11 +6433,11 @@ main(int argc, char **argv)
* :scale variables normally get -s or database scale, but don't override
* an explicit -D switch
*/
- if (lookupVariable(&state[0], "scale") == NULL)
+ if (lookupVariable(&state[0].variables, "scale") == NULL)
{
for (i = 0; i < nclients; i++)
{
- if (!putVariableInt(&state[i], "startup", "scale", scale))
+ if (!putVariableInt(&state[i].variables, "startup", "scale", scale))
exit(1);
}
}
@@ -6411,28 +6446,30 @@ main(int argc, char **argv)
* Define a :client_id variable that is unique per connection. But don't
* override an explicit -D switch.
*/
- if (lookupVariable(&state[0], "client_id") == NULL)
+ if (lookupVariable(&state[0].variables, "client_id") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "client_id", i))
+ if (!putVariableInt(&state[i].variables, "startup", "client_id", i))
exit(1);
}
/* set default seed for hash functions */
- if (lookupVariable(&state[0], "default_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "default_seed") == NULL)
{
uint64 seed = pg_prng_uint64(&base_random_sequence);
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "default_seed", (int64) seed))
+ if (!putVariableInt(&state[i].variables, "startup", "default_seed",
+ (int64) seed))
exit(1);
}
/* set random seed unless overwritten */
- if (lookupVariable(&state[0], "random_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "random_seed") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "random_seed", random_seed))
+ if (!putVariableInt(&state[i].variables, "startup", "random_seed",
+ random_seed))
exit(1);
}
--
2.17.1
--Multipart=_Fri__18_Feb_2022_23_45_49_+0900_/e7EPEHuui1P_=wo--
^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v17 1/2] Pgbench errors: use the Variables structure for client variables
@ 2021-05-26 07:58 Yugo Nagata <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Yugo Nagata @ 2021-05-26 07:58 UTC (permalink / raw)
This is most important when it is used to reset client variables during the
repeating of transactions after serialization/deadlock failures.
Don't allocate Variable structs one by one. Instead, add a constant margin each
time it overflows.
---
src/bin/pgbench/pgbench.c | 163 +++++++++++++++++++++++---------------
1 file changed, 100 insertions(+), 63 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 000ffc4a5c..ab2c5dfc5f 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -289,6 +289,12 @@ const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
+/*
+ * We don't want to allocate variables one by one; for efficiency, add a
+ * constant margin each time it overflows.
+ */
+#define VARIABLES_ALLOC_MARGIN 8
+
/*
* Variable definitions.
*
@@ -306,6 +312,24 @@ typedef struct
PgBenchValue value; /* actual variable's value */
} Variable;
+/*
+ * Data structure for client variables.
+ */
+typedef struct
+{
+ Variable *vars; /* array of variable definitions */
+ int nvars; /* number of variables */
+
+ /*
+ * The maximum number of variables that we can currently store in 'vars'
+ * without having to reallocate more space. We must always have max_vars >=
+ * nvars.
+ */
+ int max_vars;
+
+ bool vars_sorted; /* are variables sorted by name? */
+} Variables;
+
#define MAX_SCRIPTS 128 /* max number of SQL scripts allowed */
#define SHELL_COMMAND_SIZE 256 /* maximum size allowed for shell command */
@@ -460,9 +484,7 @@ typedef struct
int command; /* command number in script */
/* client variables */
- Variable *variables; /* array of variable definitions */
- int nvariables; /* number of variables */
- bool vars_sorted; /* are variables sorted by name? */
+ Variables variables;
/* various times about current transaction in microseconds */
pg_time_usec_t txn_scheduled; /* scheduled start time of transaction */
@@ -1398,39 +1420,39 @@ compareVariableNames(const void *v1, const void *v2)
/* Locate a variable by name; returns NULL if unknown */
static Variable *
-lookupVariable(CState *st, char *name)
+lookupVariable(Variables *variables, char *name)
{
Variable key;
/* On some versions of Solaris, bsearch of zero items dumps core */
- if (st->nvariables <= 0)
+ if (variables->nvars <= 0)
return NULL;
/* Sort if we have to */
- if (!st->vars_sorted)
+ if (!variables->vars_sorted)
{
- qsort((void *) st->variables, st->nvariables, sizeof(Variable),
+ qsort((void *) variables->vars, variables->nvars, sizeof(Variable),
compareVariableNames);
- st->vars_sorted = true;
+ variables->vars_sorted = true;
}
/* Now we can search */
key.name = name;
return (Variable *) bsearch((void *) &key,
- (void *) st->variables,
- st->nvariables,
+ (void *) variables->vars,
+ variables->nvars,
sizeof(Variable),
compareVariableNames);
}
/* Get the value of a variable, in string form; returns NULL if unknown */
static char *
-getVariable(CState *st, char *name)
+getVariable(Variables *variables, char *name)
{
Variable *var;
char stringform[64];
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
return NULL; /* not found */
@@ -1562,21 +1584,37 @@ valid_variable_name(const char *name)
return true;
}
+/*
+ * Make sure there is enough space for 'needed' more variable in the variables
+ * array.
+ */
+static void
+enlargeVariables(Variables *variables, int needed)
+{
+ /* total number of variables required now */
+ needed += variables->nvars;
+
+ if (variables->max_vars < needed)
+ {
+ variables->max_vars = needed + VARIABLES_ALLOC_MARGIN;
+ variables->vars = (Variable *)
+ pg_realloc(variables->vars, variables->max_vars * sizeof(Variable));
+ }
+}
+
/*
* Lookup a variable by name, creating it if need be.
* Caller is expected to assign a value to the variable.
* Returns NULL on failure (bad name).
*/
static Variable *
-lookupCreateVariable(CState *st, const char *context, char *name)
+lookupCreateVariable(Variables *variables, const char *context, char *name)
{
Variable *var;
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
{
- Variable *newvars;
-
/*
* Check for the name only when declaring a new variable to avoid
* overhead.
@@ -1588,23 +1626,17 @@ lookupCreateVariable(CState *st, const char *context, char *name)
}
/* Create variable at the end of the array */
- if (st->variables)
- newvars = (Variable *) pg_realloc(st->variables,
- (st->nvariables + 1) * sizeof(Variable));
- else
- newvars = (Variable *) pg_malloc(sizeof(Variable));
-
- st->variables = newvars;
+ enlargeVariables(variables, 1);
- var = &newvars[st->nvariables];
+ var = &(variables->vars[variables->nvars]);
var->name = pg_strdup(name);
var->svalue = NULL;
/* caller is expected to initialize remaining fields */
- st->nvariables++;
+ variables->nvars++;
/* we don't re-sort the array till we have to */
- st->vars_sorted = false;
+ variables->vars_sorted = false;
}
return var;
@@ -1613,12 +1645,13 @@ lookupCreateVariable(CState *st, const char *context, char *name)
/* Assign a string value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariable(CState *st, const char *context, char *name, const char *value)
+putVariable(Variables *variables, const char *context, char *name,
+ const char *value)
{
Variable *var;
char *val;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1636,12 +1669,12 @@ putVariable(CState *st, const char *context, char *name, const char *value)
/* Assign a value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableValue(CState *st, const char *context, char *name,
+putVariableValue(Variables *variables, const char *context, char *name,
const PgBenchValue *value)
{
Variable *var;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1656,12 +1689,13 @@ putVariableValue(CState *st, const char *context, char *name,
/* Assign an integer value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableInt(CState *st, const char *context, char *name, int64 value)
+putVariableInt(Variables *variables, const char *context, char *name,
+ int64 value)
{
PgBenchValue val;
setIntValue(&val, value);
- return putVariableValue(st, context, name, &val);
+ return putVariableValue(variables, context, name, &val);
}
/*
@@ -1720,7 +1754,7 @@ replaceVariable(char **sql, char *param, int len, char *value)
}
static char *
-assignVariables(CState *st, char *sql)
+assignVariables(Variables *variables, char *sql)
{
char *p,
*name,
@@ -1741,7 +1775,7 @@ assignVariables(CState *st, char *sql)
continue;
}
- val = getVariable(st, name);
+ val = getVariable(variables, name);
free(name);
if (val == NULL)
{
@@ -1756,12 +1790,13 @@ assignVariables(CState *st, char *sql)
}
static void
-getQueryParams(CState *st, const Command *command, const char **params)
+getQueryParams(Variables *variables, const Command *command,
+ const char **params)
{
int i;
for (i = 0; i < command->argc - 1; i++)
- params[i] = getVariable(st, command->argv[i + 1]);
+ params[i] = getVariable(variables, command->argv[i + 1]);
}
static char *
@@ -2629,7 +2664,7 @@ evaluateExpr(CState *st, PgBenchExpr *expr, PgBenchValue *retval)
{
Variable *var;
- if ((var = lookupVariable(st, expr->u.variable.varname)) == NULL)
+ if ((var = lookupVariable(&st->variables, expr->u.variable.varname)) == NULL)
{
pg_log_error("undefined variable \"%s\"", expr->u.variable.varname);
return false;
@@ -2699,7 +2734,7 @@ getMetaCommand(const char *cmd)
* Return true if succeeded, or false on error.
*/
static bool
-runShellCommand(CState *st, char *variable, char **argv, int argc)
+runShellCommand(Variables *variables, char *variable, char **argv, int argc)
{
char command[SHELL_COMMAND_SIZE];
int i,
@@ -2730,7 +2765,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
{
arg = argv[i] + 1; /* a string literal starting with colons */
}
- else if ((arg = getVariable(st, argv[i] + 1)) == NULL)
+ else if ((arg = getVariable(variables, argv[i] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[i]);
return false;
@@ -2791,7 +2826,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
pg_log_error("%s: shell command must return an integer (not \"%s\")", argv[0], res);
return false;
}
- if (!putVariableInt(st, "setshell", variable, retval))
+ if (!putVariableInt(variables, "setshell", variable, retval))
return false;
pg_log_debug("%s: shell parameter name: \"%s\", value: \"%s\"", argv[0], argv[1], res);
@@ -2843,7 +2878,7 @@ sendCommand(CState *st, Command *command)
char *sql;
sql = pg_strdup(command->argv[0]);
- sql = assignVariables(st, sql);
+ sql = assignVariables(&st->variables, sql);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQuery(st->con, sql);
@@ -2854,7 +2889,7 @@ sendCommand(CState *st, Command *command)
const char *sql = command->argv[0];
const char *params[MAX_ARGS];
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQueryParams(st->con, sql, command->argc - 1,
@@ -2901,7 +2936,7 @@ sendCommand(CState *st, Command *command)
st->prepared[st->use_file] = true;
}
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
preparedStatementName(name, st->use_file, st->command);
pg_log_debug("client %d sending %s", st->id, name);
@@ -2994,7 +3029,7 @@ readCommandResponse(CState *st, MetaCommand meta, char *varprefix)
varname = psprintf("%s%s", varprefix, varname);
/* store last row result as a string */
- if (!putVariable(st, meta == META_ASET ? "aset" : "gset", varname,
+ if (!putVariable(&st->variables, meta == META_ASET ? "aset" : "gset", varname,
PQgetvalue(res, ntuples - 1, fld)))
{
/* internal error */
@@ -3055,14 +3090,14 @@ error:
* of delay, in microseconds. Returns true on success, false on error.
*/
static bool
-evaluateSleep(CState *st, int argc, char **argv, int *usecs)
+evaluateSleep(Variables *variables, int argc, char **argv, int *usecs)
{
char *var;
int usec;
if (*argv[1] == ':')
{
- if ((var = getVariable(st, argv[1] + 1)) == NULL)
+ if ((var = getVariable(variables, argv[1] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[1] + 1);
return false;
@@ -3627,7 +3662,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
* latency will be recorded in CSTATE_SLEEP state, not here, after the
* delay has elapsed.)
*/
- if (!evaluateSleep(st, argc, argv, &usec))
+ if (!evaluateSleep(&st->variables, argc, argv, &usec))
{
commandFailed(st, "sleep", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3648,7 +3683,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
return CSTATE_ABORTED;
}
- if (!putVariableValue(st, argv[0], argv[1], &result))
+ if (!putVariableValue(&st->variables, argv[0], argv[1], &result))
{
commandFailed(st, "set", "assignment of meta-command failed");
return CSTATE_ABORTED;
@@ -3718,7 +3753,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SETSHELL)
{
- if (!runShellCommand(st, argv[1], argv + 2, argc - 2))
+ if (!runShellCommand(&st->variables, argv[1], argv + 2, argc - 2))
{
commandFailed(st, "setshell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3726,7 +3761,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SHELL)
{
- if (!runShellCommand(st, NULL, argv + 1, argc - 1))
+ if (!runShellCommand(&st->variables, NULL, argv + 1, argc - 1))
{
commandFailed(st, "shell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -6020,7 +6055,7 @@ main(int argc, char **argv)
}
*p++ = '\0';
- if (!putVariable(&state[0], "option", optarg, p))
+ if (!putVariable(&state[0].variables, "option", optarg, p))
exit(1);
}
break;
@@ -6348,19 +6383,19 @@ main(int argc, char **argv)
int j;
state[i].id = i;
- for (j = 0; j < state[0].nvariables; j++)
+ for (j = 0; j < state[0].variables.nvars; j++)
{
- Variable *var = &state[0].variables[j];
+ Variable *var = &state[0].variables.vars[j];
if (var->value.type != PGBT_NO_VALUE)
{
- if (!putVariableValue(&state[i], "startup",
+ if (!putVariableValue(&state[i].variables, "startup",
var->name, &var->value))
exit(1);
}
else
{
- if (!putVariable(&state[i], "startup",
+ if (!putVariable(&state[i].variables, "startup",
var->name, var->svalue))
exit(1);
}
@@ -6398,11 +6433,11 @@ main(int argc, char **argv)
* :scale variables normally get -s or database scale, but don't override
* an explicit -D switch
*/
- if (lookupVariable(&state[0], "scale") == NULL)
+ if (lookupVariable(&state[0].variables, "scale") == NULL)
{
for (i = 0; i < nclients; i++)
{
- if (!putVariableInt(&state[i], "startup", "scale", scale))
+ if (!putVariableInt(&state[i].variables, "startup", "scale", scale))
exit(1);
}
}
@@ -6411,28 +6446,30 @@ main(int argc, char **argv)
* Define a :client_id variable that is unique per connection. But don't
* override an explicit -D switch.
*/
- if (lookupVariable(&state[0], "client_id") == NULL)
+ if (lookupVariable(&state[0].variables, "client_id") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "client_id", i))
+ if (!putVariableInt(&state[i].variables, "startup", "client_id", i))
exit(1);
}
/* set default seed for hash functions */
- if (lookupVariable(&state[0], "default_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "default_seed") == NULL)
{
uint64 seed = pg_prng_uint64(&base_random_sequence);
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "default_seed", (int64) seed))
+ if (!putVariableInt(&state[i].variables, "startup", "default_seed",
+ (int64) seed))
exit(1);
}
/* set random seed unless overwritten */
- if (lookupVariable(&state[0], "random_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "random_seed") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "random_seed", random_seed))
+ if (!putVariableInt(&state[i].variables, "startup", "random_seed",
+ random_seed))
exit(1);
}
--
2.17.1
--Multipart=_Sun__20_Mar_2022_03_23_21_+0900_8dUZljuxkcYgB2tW
Content-Type: text/x-diff;
name="v17-0002-Pgbench-errors-and-serialization-deadlock-retrie.patch"
Content-Disposition: attachment;
filename="v17-0002-Pgbench-errors-and-serialization-deadlock-retrie.patch"
Content-Transfer-Encoding: 7bit
^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v18 1/2] Pgbench errors: use the Variables structure for client variables
@ 2021-05-26 07:58 Yugo Nagata <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Yugo Nagata @ 2021-05-26 07:58 UTC (permalink / raw)
This is most important when it is used to reset client variables during the
repeating of transactions after serialization/deadlock failures.
Don't allocate Variable structs one by one. Instead, add a constant margin each
time it overflows.
---
src/bin/pgbench/pgbench.c | 163 +++++++++++++++++++++++---------------
1 file changed, 100 insertions(+), 63 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 000ffc4a5c..ab2c5dfc5f 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -289,6 +289,12 @@ const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
+/*
+ * We don't want to allocate variables one by one; for efficiency, add a
+ * constant margin each time it overflows.
+ */
+#define VARIABLES_ALLOC_MARGIN 8
+
/*
* Variable definitions.
*
@@ -306,6 +312,24 @@ typedef struct
PgBenchValue value; /* actual variable's value */
} Variable;
+/*
+ * Data structure for client variables.
+ */
+typedef struct
+{
+ Variable *vars; /* array of variable definitions */
+ int nvars; /* number of variables */
+
+ /*
+ * The maximum number of variables that we can currently store in 'vars'
+ * without having to reallocate more space. We must always have max_vars >=
+ * nvars.
+ */
+ int max_vars;
+
+ bool vars_sorted; /* are variables sorted by name? */
+} Variables;
+
#define MAX_SCRIPTS 128 /* max number of SQL scripts allowed */
#define SHELL_COMMAND_SIZE 256 /* maximum size allowed for shell command */
@@ -460,9 +484,7 @@ typedef struct
int command; /* command number in script */
/* client variables */
- Variable *variables; /* array of variable definitions */
- int nvariables; /* number of variables */
- bool vars_sorted; /* are variables sorted by name? */
+ Variables variables;
/* various times about current transaction in microseconds */
pg_time_usec_t txn_scheduled; /* scheduled start time of transaction */
@@ -1398,39 +1420,39 @@ compareVariableNames(const void *v1, const void *v2)
/* Locate a variable by name; returns NULL if unknown */
static Variable *
-lookupVariable(CState *st, char *name)
+lookupVariable(Variables *variables, char *name)
{
Variable key;
/* On some versions of Solaris, bsearch of zero items dumps core */
- if (st->nvariables <= 0)
+ if (variables->nvars <= 0)
return NULL;
/* Sort if we have to */
- if (!st->vars_sorted)
+ if (!variables->vars_sorted)
{
- qsort((void *) st->variables, st->nvariables, sizeof(Variable),
+ qsort((void *) variables->vars, variables->nvars, sizeof(Variable),
compareVariableNames);
- st->vars_sorted = true;
+ variables->vars_sorted = true;
}
/* Now we can search */
key.name = name;
return (Variable *) bsearch((void *) &key,
- (void *) st->variables,
- st->nvariables,
+ (void *) variables->vars,
+ variables->nvars,
sizeof(Variable),
compareVariableNames);
}
/* Get the value of a variable, in string form; returns NULL if unknown */
static char *
-getVariable(CState *st, char *name)
+getVariable(Variables *variables, char *name)
{
Variable *var;
char stringform[64];
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
return NULL; /* not found */
@@ -1562,21 +1584,37 @@ valid_variable_name(const char *name)
return true;
}
+/*
+ * Make sure there is enough space for 'needed' more variable in the variables
+ * array.
+ */
+static void
+enlargeVariables(Variables *variables, int needed)
+{
+ /* total number of variables required now */
+ needed += variables->nvars;
+
+ if (variables->max_vars < needed)
+ {
+ variables->max_vars = needed + VARIABLES_ALLOC_MARGIN;
+ variables->vars = (Variable *)
+ pg_realloc(variables->vars, variables->max_vars * sizeof(Variable));
+ }
+}
+
/*
* Lookup a variable by name, creating it if need be.
* Caller is expected to assign a value to the variable.
* Returns NULL on failure (bad name).
*/
static Variable *
-lookupCreateVariable(CState *st, const char *context, char *name)
+lookupCreateVariable(Variables *variables, const char *context, char *name)
{
Variable *var;
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
{
- Variable *newvars;
-
/*
* Check for the name only when declaring a new variable to avoid
* overhead.
@@ -1588,23 +1626,17 @@ lookupCreateVariable(CState *st, const char *context, char *name)
}
/* Create variable at the end of the array */
- if (st->variables)
- newvars = (Variable *) pg_realloc(st->variables,
- (st->nvariables + 1) * sizeof(Variable));
- else
- newvars = (Variable *) pg_malloc(sizeof(Variable));
-
- st->variables = newvars;
+ enlargeVariables(variables, 1);
- var = &newvars[st->nvariables];
+ var = &(variables->vars[variables->nvars]);
var->name = pg_strdup(name);
var->svalue = NULL;
/* caller is expected to initialize remaining fields */
- st->nvariables++;
+ variables->nvars++;
/* we don't re-sort the array till we have to */
- st->vars_sorted = false;
+ variables->vars_sorted = false;
}
return var;
@@ -1613,12 +1645,13 @@ lookupCreateVariable(CState *st, const char *context, char *name)
/* Assign a string value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariable(CState *st, const char *context, char *name, const char *value)
+putVariable(Variables *variables, const char *context, char *name,
+ const char *value)
{
Variable *var;
char *val;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1636,12 +1669,12 @@ putVariable(CState *st, const char *context, char *name, const char *value)
/* Assign a value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableValue(CState *st, const char *context, char *name,
+putVariableValue(Variables *variables, const char *context, char *name,
const PgBenchValue *value)
{
Variable *var;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1656,12 +1689,13 @@ putVariableValue(CState *st, const char *context, char *name,
/* Assign an integer value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableInt(CState *st, const char *context, char *name, int64 value)
+putVariableInt(Variables *variables, const char *context, char *name,
+ int64 value)
{
PgBenchValue val;
setIntValue(&val, value);
- return putVariableValue(st, context, name, &val);
+ return putVariableValue(variables, context, name, &val);
}
/*
@@ -1720,7 +1754,7 @@ replaceVariable(char **sql, char *param, int len, char *value)
}
static char *
-assignVariables(CState *st, char *sql)
+assignVariables(Variables *variables, char *sql)
{
char *p,
*name,
@@ -1741,7 +1775,7 @@ assignVariables(CState *st, char *sql)
continue;
}
- val = getVariable(st, name);
+ val = getVariable(variables, name);
free(name);
if (val == NULL)
{
@@ -1756,12 +1790,13 @@ assignVariables(CState *st, char *sql)
}
static void
-getQueryParams(CState *st, const Command *command, const char **params)
+getQueryParams(Variables *variables, const Command *command,
+ const char **params)
{
int i;
for (i = 0; i < command->argc - 1; i++)
- params[i] = getVariable(st, command->argv[i + 1]);
+ params[i] = getVariable(variables, command->argv[i + 1]);
}
static char *
@@ -2629,7 +2664,7 @@ evaluateExpr(CState *st, PgBenchExpr *expr, PgBenchValue *retval)
{
Variable *var;
- if ((var = lookupVariable(st, expr->u.variable.varname)) == NULL)
+ if ((var = lookupVariable(&st->variables, expr->u.variable.varname)) == NULL)
{
pg_log_error("undefined variable \"%s\"", expr->u.variable.varname);
return false;
@@ -2699,7 +2734,7 @@ getMetaCommand(const char *cmd)
* Return true if succeeded, or false on error.
*/
static bool
-runShellCommand(CState *st, char *variable, char **argv, int argc)
+runShellCommand(Variables *variables, char *variable, char **argv, int argc)
{
char command[SHELL_COMMAND_SIZE];
int i,
@@ -2730,7 +2765,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
{
arg = argv[i] + 1; /* a string literal starting with colons */
}
- else if ((arg = getVariable(st, argv[i] + 1)) == NULL)
+ else if ((arg = getVariable(variables, argv[i] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[i]);
return false;
@@ -2791,7 +2826,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
pg_log_error("%s: shell command must return an integer (not \"%s\")", argv[0], res);
return false;
}
- if (!putVariableInt(st, "setshell", variable, retval))
+ if (!putVariableInt(variables, "setshell", variable, retval))
return false;
pg_log_debug("%s: shell parameter name: \"%s\", value: \"%s\"", argv[0], argv[1], res);
@@ -2843,7 +2878,7 @@ sendCommand(CState *st, Command *command)
char *sql;
sql = pg_strdup(command->argv[0]);
- sql = assignVariables(st, sql);
+ sql = assignVariables(&st->variables, sql);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQuery(st->con, sql);
@@ -2854,7 +2889,7 @@ sendCommand(CState *st, Command *command)
const char *sql = command->argv[0];
const char *params[MAX_ARGS];
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQueryParams(st->con, sql, command->argc - 1,
@@ -2901,7 +2936,7 @@ sendCommand(CState *st, Command *command)
st->prepared[st->use_file] = true;
}
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
preparedStatementName(name, st->use_file, st->command);
pg_log_debug("client %d sending %s", st->id, name);
@@ -2994,7 +3029,7 @@ readCommandResponse(CState *st, MetaCommand meta, char *varprefix)
varname = psprintf("%s%s", varprefix, varname);
/* store last row result as a string */
- if (!putVariable(st, meta == META_ASET ? "aset" : "gset", varname,
+ if (!putVariable(&st->variables, meta == META_ASET ? "aset" : "gset", varname,
PQgetvalue(res, ntuples - 1, fld)))
{
/* internal error */
@@ -3055,14 +3090,14 @@ error:
* of delay, in microseconds. Returns true on success, false on error.
*/
static bool
-evaluateSleep(CState *st, int argc, char **argv, int *usecs)
+evaluateSleep(Variables *variables, int argc, char **argv, int *usecs)
{
char *var;
int usec;
if (*argv[1] == ':')
{
- if ((var = getVariable(st, argv[1] + 1)) == NULL)
+ if ((var = getVariable(variables, argv[1] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[1] + 1);
return false;
@@ -3627,7 +3662,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
* latency will be recorded in CSTATE_SLEEP state, not here, after the
* delay has elapsed.)
*/
- if (!evaluateSleep(st, argc, argv, &usec))
+ if (!evaluateSleep(&st->variables, argc, argv, &usec))
{
commandFailed(st, "sleep", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3648,7 +3683,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
return CSTATE_ABORTED;
}
- if (!putVariableValue(st, argv[0], argv[1], &result))
+ if (!putVariableValue(&st->variables, argv[0], argv[1], &result))
{
commandFailed(st, "set", "assignment of meta-command failed");
return CSTATE_ABORTED;
@@ -3718,7 +3753,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SETSHELL)
{
- if (!runShellCommand(st, argv[1], argv + 2, argc - 2))
+ if (!runShellCommand(&st->variables, argv[1], argv + 2, argc - 2))
{
commandFailed(st, "setshell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3726,7 +3761,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SHELL)
{
- if (!runShellCommand(st, NULL, argv + 1, argc - 1))
+ if (!runShellCommand(&st->variables, NULL, argv + 1, argc - 1))
{
commandFailed(st, "shell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -6020,7 +6055,7 @@ main(int argc, char **argv)
}
*p++ = '\0';
- if (!putVariable(&state[0], "option", optarg, p))
+ if (!putVariable(&state[0].variables, "option", optarg, p))
exit(1);
}
break;
@@ -6348,19 +6383,19 @@ main(int argc, char **argv)
int j;
state[i].id = i;
- for (j = 0; j < state[0].nvariables; j++)
+ for (j = 0; j < state[0].variables.nvars; j++)
{
- Variable *var = &state[0].variables[j];
+ Variable *var = &state[0].variables.vars[j];
if (var->value.type != PGBT_NO_VALUE)
{
- if (!putVariableValue(&state[i], "startup",
+ if (!putVariableValue(&state[i].variables, "startup",
var->name, &var->value))
exit(1);
}
else
{
- if (!putVariable(&state[i], "startup",
+ if (!putVariable(&state[i].variables, "startup",
var->name, var->svalue))
exit(1);
}
@@ -6398,11 +6433,11 @@ main(int argc, char **argv)
* :scale variables normally get -s or database scale, but don't override
* an explicit -D switch
*/
- if (lookupVariable(&state[0], "scale") == NULL)
+ if (lookupVariable(&state[0].variables, "scale") == NULL)
{
for (i = 0; i < nclients; i++)
{
- if (!putVariableInt(&state[i], "startup", "scale", scale))
+ if (!putVariableInt(&state[i].variables, "startup", "scale", scale))
exit(1);
}
}
@@ -6411,28 +6446,30 @@ main(int argc, char **argv)
* Define a :client_id variable that is unique per connection. But don't
* override an explicit -D switch.
*/
- if (lookupVariable(&state[0], "client_id") == NULL)
+ if (lookupVariable(&state[0].variables, "client_id") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "client_id", i))
+ if (!putVariableInt(&state[i].variables, "startup", "client_id", i))
exit(1);
}
/* set default seed for hash functions */
- if (lookupVariable(&state[0], "default_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "default_seed") == NULL)
{
uint64 seed = pg_prng_uint64(&base_random_sequence);
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "default_seed", (int64) seed))
+ if (!putVariableInt(&state[i].variables, "startup", "default_seed",
+ (int64) seed))
exit(1);
}
/* set random seed unless overwritten */
- if (lookupVariable(&state[0], "random_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "random_seed") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "random_seed", random_seed))
+ if (!putVariableInt(&state[i].variables, "startup", "random_seed",
+ random_seed))
exit(1);
}
--
2.17.1
--Multipart=_Tue__22_Mar_2022_17_27_14_+0900_PTQR43_rmqjQDpKM
Content-Type: text/x-diff;
name="v18-0002-Pgbench-errors-and-serialization-deadlock-retrie.patch"
Content-Disposition: attachment;
filename="v18-0002-Pgbench-errors-and-serialization-deadlock-retrie.patch"
Content-Transfer-Encoding: 7bit
^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v15 1/2] Pgbench errors: use the Variables structure for client variables
@ 2021-05-26 07:58 Yugo Nagata <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Yugo Nagata @ 2021-05-26 07:58 UTC (permalink / raw)
This is most important when it is used to reset client variables during the
repeating of transactions after serialization/deadlock failures.
Don't allocate Variable structs one by one. Instead, add a constant margin each
time it overflows.
---
src/bin/pgbench/pgbench.c | 163 +++++++++++++++++++++++---------------
1 file changed, 100 insertions(+), 63 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 364b5a2e47..c4c2fd3566 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -287,6 +287,12 @@ const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
+/*
+ * We don't want to allocate variables one by one; for efficiency, add a
+ * constant margin each time it overflows.
+ */
+#define VARIABLES_ALLOC_MARGIN 8
+
/*
* Variable definitions.
*
@@ -304,6 +310,24 @@ typedef struct
PgBenchValue value; /* actual variable's value */
} Variable;
+/*
+ * Data structure for client variables.
+ */
+typedef struct
+{
+ Variable *vars; /* array of variable definitions */
+ int nvars; /* number of variables */
+
+ /*
+ * The maximum number of variables that we can currently store in 'vars'
+ * without having to reallocate more space. We must always have max_vars >=
+ * nvars.
+ */
+ int max_vars;
+
+ bool vars_sorted; /* are variables sorted by name? */
+} Variables;
+
#define MAX_SCRIPTS 128 /* max number of SQL scripts allowed */
#define SHELL_COMMAND_SIZE 256 /* maximum size allowed for shell command */
@@ -466,9 +490,7 @@ typedef struct
int command; /* command number in script */
/* client variables */
- Variable *variables; /* array of variable definitions */
- int nvariables; /* number of variables */
- bool vars_sorted; /* are variables sorted by name? */
+ Variables variables;
/* various times about current transaction in microseconds */
pg_time_usec_t txn_scheduled; /* scheduled start time of transaction */
@@ -1424,39 +1446,39 @@ compareVariableNames(const void *v1, const void *v2)
/* Locate a variable by name; returns NULL if unknown */
static Variable *
-lookupVariable(CState *st, char *name)
+lookupVariable(Variables *variables, char *name)
{
Variable key;
/* On some versions of Solaris, bsearch of zero items dumps core */
- if (st->nvariables <= 0)
+ if (variables->nvars <= 0)
return NULL;
/* Sort if we have to */
- if (!st->vars_sorted)
+ if (!variables->vars_sorted)
{
- qsort((void *) st->variables, st->nvariables, sizeof(Variable),
+ qsort((void *) variables->vars, variables->nvars, sizeof(Variable),
compareVariableNames);
- st->vars_sorted = true;
+ variables->vars_sorted = true;
}
/* Now we can search */
key.name = name;
return (Variable *) bsearch((void *) &key,
- (void *) st->variables,
- st->nvariables,
+ (void *) variables->vars,
+ variables->nvars,
sizeof(Variable),
compareVariableNames);
}
/* Get the value of a variable, in string form; returns NULL if unknown */
static char *
-getVariable(CState *st, char *name)
+getVariable(Variables *variables, char *name)
{
Variable *var;
char stringform[64];
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
return NULL; /* not found */
@@ -1588,21 +1610,37 @@ valid_variable_name(const char *name)
return true;
}
+/*
+ * Make sure there is enough space for 'needed' more variable in the variables
+ * array.
+ */
+static void
+enlargeVariables(Variables *variables, int needed)
+{
+ /* total number of variables required now */
+ needed += variables->nvars;
+
+ if (variables->max_vars < needed)
+ {
+ variables->max_vars = needed + VARIABLES_ALLOC_MARGIN;
+ variables->vars = (Variable *)
+ pg_realloc(variables->vars, variables->max_vars * sizeof(Variable));
+ }
+}
+
/*
* Lookup a variable by name, creating it if need be.
* Caller is expected to assign a value to the variable.
* Returns NULL on failure (bad name).
*/
static Variable *
-lookupCreateVariable(CState *st, const char *context, char *name)
+lookupCreateVariable(Variables *variables, const char *context, char *name)
{
Variable *var;
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
{
- Variable *newvars;
-
/*
* Check for the name only when declaring a new variable to avoid
* overhead.
@@ -1614,23 +1652,17 @@ lookupCreateVariable(CState *st, const char *context, char *name)
}
/* Create variable at the end of the array */
- if (st->variables)
- newvars = (Variable *) pg_realloc(st->variables,
- (st->nvariables + 1) * sizeof(Variable));
- else
- newvars = (Variable *) pg_malloc(sizeof(Variable));
-
- st->variables = newvars;
+ enlargeVariables(variables, 1);
- var = &newvars[st->nvariables];
+ var = &(variables->vars[variables->nvars]);
var->name = pg_strdup(name);
var->svalue = NULL;
/* caller is expected to initialize remaining fields */
- st->nvariables++;
+ variables->nvars++;
/* we don't re-sort the array till we have to */
- st->vars_sorted = false;
+ variables->vars_sorted = false;
}
return var;
@@ -1639,12 +1671,13 @@ lookupCreateVariable(CState *st, const char *context, char *name)
/* Assign a string value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariable(CState *st, const char *context, char *name, const char *value)
+putVariable(Variables *variables, const char *context, char *name,
+ const char *value)
{
Variable *var;
char *val;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1662,12 +1695,12 @@ putVariable(CState *st, const char *context, char *name, const char *value)
/* Assign a value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableValue(CState *st, const char *context, char *name,
+putVariableValue(Variables *variables, const char *context, char *name,
const PgBenchValue *value)
{
Variable *var;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1682,12 +1715,13 @@ putVariableValue(CState *st, const char *context, char *name,
/* Assign an integer value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableInt(CState *st, const char *context, char *name, int64 value)
+putVariableInt(Variables *variables, const char *context, char *name,
+ int64 value)
{
PgBenchValue val;
setIntValue(&val, value);
- return putVariableValue(st, context, name, &val);
+ return putVariableValue(variables, context, name, &val);
}
/*
@@ -1746,7 +1780,7 @@ replaceVariable(char **sql, char *param, int len, char *value)
}
static char *
-assignVariables(CState *st, char *sql)
+assignVariables(Variables *variables, char *sql)
{
char *p,
*name,
@@ -1767,7 +1801,7 @@ assignVariables(CState *st, char *sql)
continue;
}
- val = getVariable(st, name);
+ val = getVariable(variables, name);
free(name);
if (val == NULL)
{
@@ -1782,12 +1816,13 @@ assignVariables(CState *st, char *sql)
}
static void
-getQueryParams(CState *st, const Command *command, const char **params)
+getQueryParams(Variables *variables, const Command *command,
+ const char **params)
{
int i;
for (i = 0; i < command->argc - 1; i++)
- params[i] = getVariable(st, command->argv[i + 1]);
+ params[i] = getVariable(variables, command->argv[i + 1]);
}
static char *
@@ -2655,7 +2690,7 @@ evaluateExpr(CState *st, PgBenchExpr *expr, PgBenchValue *retval)
{
Variable *var;
- if ((var = lookupVariable(st, expr->u.variable.varname)) == NULL)
+ if ((var = lookupVariable(&st->variables, expr->u.variable.varname)) == NULL)
{
pg_log_error("undefined variable \"%s\"", expr->u.variable.varname);
return false;
@@ -2725,7 +2760,7 @@ getMetaCommand(const char *cmd)
* Return true if succeeded, or false on error.
*/
static bool
-runShellCommand(CState *st, char *variable, char **argv, int argc)
+runShellCommand(Variables *variables, char *variable, char **argv, int argc)
{
char command[SHELL_COMMAND_SIZE];
int i,
@@ -2756,7 +2791,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
{
arg = argv[i] + 1; /* a string literal starting with colons */
}
- else if ((arg = getVariable(st, argv[i] + 1)) == NULL)
+ else if ((arg = getVariable(variables, argv[i] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[i]);
return false;
@@ -2817,7 +2852,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
pg_log_error("%s: shell command must return an integer (not \"%s\")", argv[0], res);
return false;
}
- if (!putVariableInt(st, "setshell", variable, retval))
+ if (!putVariableInt(variables, "setshell", variable, retval))
return false;
pg_log_debug("%s: shell parameter name: \"%s\", value: \"%s\"", argv[0], argv[1], res);
@@ -2869,7 +2904,7 @@ sendCommand(CState *st, Command *command)
char *sql;
sql = pg_strdup(command->argv[0]);
- sql = assignVariables(st, sql);
+ sql = assignVariables(&st->variables, sql);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQuery(st->con, sql);
@@ -2880,7 +2915,7 @@ sendCommand(CState *st, Command *command)
const char *sql = command->argv[0];
const char *params[MAX_ARGS];
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQueryParams(st->con, sql, command->argc - 1,
@@ -2927,7 +2962,7 @@ sendCommand(CState *st, Command *command)
st->prepared[st->use_file] = true;
}
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
preparedStatementName(name, st->use_file, st->command);
pg_log_debug("client %d sending %s", st->id, name);
@@ -3020,7 +3055,7 @@ readCommandResponse(CState *st, MetaCommand meta, char *varprefix)
varname = psprintf("%s%s", varprefix, varname);
/* store last row result as a string */
- if (!putVariable(st, meta == META_ASET ? "aset" : "gset", varname,
+ if (!putVariable(&st->variables, meta == META_ASET ? "aset" : "gset", varname,
PQgetvalue(res, ntuples - 1, fld)))
{
/* internal error */
@@ -3081,14 +3116,14 @@ error:
* of delay, in microseconds. Returns true on success, false on error.
*/
static bool
-evaluateSleep(CState *st, int argc, char **argv, int *usecs)
+evaluateSleep(Variables *variables, int argc, char **argv, int *usecs)
{
char *var;
int usec;
if (*argv[1] == ':')
{
- if ((var = getVariable(st, argv[1] + 1)) == NULL)
+ if ((var = getVariable(variables, argv[1] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[1] + 1);
return false;
@@ -3620,7 +3655,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
* latency will be recorded in CSTATE_SLEEP state, not here, after the
* delay has elapsed.)
*/
- if (!evaluateSleep(st, argc, argv, &usec))
+ if (!evaluateSleep(&st->variables, argc, argv, &usec))
{
commandFailed(st, "sleep", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3641,7 +3676,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
return CSTATE_ABORTED;
}
- if (!putVariableValue(st, argv[0], argv[1], &result))
+ if (!putVariableValue(&st->variables, argv[0], argv[1], &result))
{
commandFailed(st, "set", "assignment of meta-command failed");
return CSTATE_ABORTED;
@@ -3711,7 +3746,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SETSHELL)
{
- if (!runShellCommand(st, argv[1], argv + 2, argc - 2))
+ if (!runShellCommand(&st->variables, argv[1], argv + 2, argc - 2))
{
commandFailed(st, "setshell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3719,7 +3754,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SHELL)
{
- if (!runShellCommand(st, NULL, argv + 1, argc - 1))
+ if (!runShellCommand(&st->variables, NULL, argv + 1, argc - 1))
{
commandFailed(st, "shell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -6013,7 +6048,7 @@ main(int argc, char **argv)
}
*p++ = '\0';
- if (!putVariable(&state[0], "option", optarg, p))
+ if (!putVariable(&state[0].variables, "option", optarg, p))
exit(1);
}
break;
@@ -6353,19 +6388,19 @@ main(int argc, char **argv)
int j;
state[i].id = i;
- for (j = 0; j < state[0].nvariables; j++)
+ for (j = 0; j < state[0].variables.nvars; j++)
{
- Variable *var = &state[0].variables[j];
+ Variable *var = &state[0].variables.vars[j];
if (var->value.type != PGBT_NO_VALUE)
{
- if (!putVariableValue(&state[i], "startup",
+ if (!putVariableValue(&state[i].variables, "startup",
var->name, &var->value))
exit(1);
}
else
{
- if (!putVariable(&state[i], "startup",
+ if (!putVariable(&state[i].variables, "startup",
var->name, var->svalue))
exit(1);
}
@@ -6400,11 +6435,11 @@ main(int argc, char **argv)
* :scale variables normally get -s or database scale, but don't override
* an explicit -D switch
*/
- if (lookupVariable(&state[0], "scale") == NULL)
+ if (lookupVariable(&state[0].variables, "scale") == NULL)
{
for (i = 0; i < nclients; i++)
{
- if (!putVariableInt(&state[i], "startup", "scale", scale))
+ if (!putVariableInt(&state[i].variables, "startup", "scale", scale))
exit(1);
}
}
@@ -6413,30 +6448,32 @@ main(int argc, char **argv)
* Define a :client_id variable that is unique per connection. But don't
* override an explicit -D switch.
*/
- if (lookupVariable(&state[0], "client_id") == NULL)
+ if (lookupVariable(&state[0].variables, "client_id") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "client_id", i))
+ if (!putVariableInt(&state[i].variables, "startup", "client_id", i))
exit(1);
}
/* set default seed for hash functions */
- if (lookupVariable(&state[0], "default_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "default_seed") == NULL)
{
uint64 seed =
((uint64) pg_jrand48(base_random_sequence.xseed) & 0xFFFFFFFF) |
(((uint64) pg_jrand48(base_random_sequence.xseed) & 0xFFFFFFFF) << 32);
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "default_seed", (int64) seed))
+ if (!putVariableInt(&state[i].variables, "startup", "default_seed",
+ (int64) seed))
exit(1);
}
/* set random seed unless overwritten */
- if (lookupVariable(&state[0], "random_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "random_seed") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "random_seed", random_seed))
+ if (!putVariableInt(&state[i].variables, "startup", "random_seed",
+ random_seed))
exit(1);
}
--
2.17.1
--Multipart=_Fri__16_Jul_2021_12_05_16_+0900_jCtOsvpoRs.oEJzk--
^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v13 1/2] Pgbench errors: use the Variables structure for client variables
@ 2021-05-26 07:58 Yugo Nagata <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Yugo Nagata @ 2021-05-26 07:58 UTC (permalink / raw)
This is most important when it is used to reset client variables during the
repeating of transactions after serialization/deadlock failures.
Don't allocate Variable structs one by one. Instead, add a constant margin each
time it overflows.
---
src/bin/pgbench/pgbench.c | 163 +++++++++++++++++++++++---------------
1 file changed, 100 insertions(+), 63 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4aeccd93af..3629caba42 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -287,6 +287,12 @@ const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
+/*
+ * We don't want to allocate variables one by one; for efficiency, add a
+ * constant margin each time it overflows.
+ */
+#define VARIABLES_ALLOC_MARGIN 8
+
/*
* Variable definitions.
*
@@ -304,6 +310,24 @@ typedef struct
PgBenchValue value; /* actual variable's value */
} Variable;
+/*
+ * Data structure for client variables.
+ */
+typedef struct
+{
+ Variable *vars; /* array of variable definitions */
+ int nvars; /* number of variables */
+
+ /*
+ * The maximum number of variables that we can currently store in 'vars'
+ * without having to reallocate more space. We must always have max_vars >=
+ * nvars.
+ */
+ int max_vars;
+
+ bool vars_sorted; /* are variables sorted by name? */
+} Variables;
+
#define MAX_SCRIPTS 128 /* max number of SQL scripts allowed */
#define SHELL_COMMAND_SIZE 256 /* maximum size allowed for shell command */
@@ -460,9 +484,7 @@ typedef struct
int command; /* command number in script */
/* client variables */
- Variable *variables; /* array of variable definitions */
- int nvariables; /* number of variables */
- bool vars_sorted; /* are variables sorted by name? */
+ Variables variables;
/* various times about current transaction in microseconds */
pg_time_usec_t txn_scheduled; /* scheduled start time of transaction */
@@ -1418,39 +1440,39 @@ compareVariableNames(const void *v1, const void *v2)
/* Locate a variable by name; returns NULL if unknown */
static Variable *
-lookupVariable(CState *st, char *name)
+lookupVariable(Variables *variables, char *name)
{
Variable key;
/* On some versions of Solaris, bsearch of zero items dumps core */
- if (st->nvariables <= 0)
+ if (variables->nvars <= 0)
return NULL;
/* Sort if we have to */
- if (!st->vars_sorted)
+ if (!variables->vars_sorted)
{
- qsort((void *) st->variables, st->nvariables, sizeof(Variable),
+ qsort((void *) variables->vars, variables->nvars, sizeof(Variable),
compareVariableNames);
- st->vars_sorted = true;
+ variables->vars_sorted = true;
}
/* Now we can search */
key.name = name;
return (Variable *) bsearch((void *) &key,
- (void *) st->variables,
- st->nvariables,
+ (void *) variables->vars,
+ variables->nvars,
sizeof(Variable),
compareVariableNames);
}
/* Get the value of a variable, in string form; returns NULL if unknown */
static char *
-getVariable(CState *st, char *name)
+getVariable(Variables *variables, char *name)
{
Variable *var;
char stringform[64];
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
return NULL; /* not found */
@@ -1582,21 +1604,37 @@ valid_variable_name(const char *name)
return true;
}
+/*
+ * Make sure there is enough space for 'needed' more variable in the variables
+ * array.
+ */
+static void
+enlargeVariables(Variables *variables, int needed)
+{
+ /* total number of variables required now */
+ needed += variables->nvars;
+
+ if (variables->max_vars < needed)
+ {
+ variables->max_vars = needed + VARIABLES_ALLOC_MARGIN;
+ variables->vars = (Variable *)
+ pg_realloc(variables->vars, variables->max_vars * sizeof(Variable));
+ }
+}
+
/*
* Lookup a variable by name, creating it if need be.
* Caller is expected to assign a value to the variable.
* Returns NULL on failure (bad name).
*/
static Variable *
-lookupCreateVariable(CState *st, const char *context, char *name)
+lookupCreateVariable(Variables *variables, const char *context, char *name)
{
Variable *var;
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
{
- Variable *newvars;
-
/*
* Check for the name only when declaring a new variable to avoid
* overhead.
@@ -1608,23 +1646,17 @@ lookupCreateVariable(CState *st, const char *context, char *name)
}
/* Create variable at the end of the array */
- if (st->variables)
- newvars = (Variable *) pg_realloc(st->variables,
- (st->nvariables + 1) * sizeof(Variable));
- else
- newvars = (Variable *) pg_malloc(sizeof(Variable));
-
- st->variables = newvars;
+ enlargeVariables(variables, 1);
- var = &newvars[st->nvariables];
+ var = &(variables->vars[variables->nvars]);
var->name = pg_strdup(name);
var->svalue = NULL;
/* caller is expected to initialize remaining fields */
- st->nvariables++;
+ variables->nvars++;
/* we don't re-sort the array till we have to */
- st->vars_sorted = false;
+ variables->vars_sorted = false;
}
return var;
@@ -1633,12 +1665,13 @@ lookupCreateVariable(CState *st, const char *context, char *name)
/* Assign a string value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariable(CState *st, const char *context, char *name, const char *value)
+putVariable(Variables *variables, const char *context, char *name,
+ const char *value)
{
Variable *var;
char *val;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1656,12 +1689,12 @@ putVariable(CState *st, const char *context, char *name, const char *value)
/* Assign a value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableValue(CState *st, const char *context, char *name,
+putVariableValue(Variables *variables, const char *context, char *name,
const PgBenchValue *value)
{
Variable *var;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1676,12 +1709,13 @@ putVariableValue(CState *st, const char *context, char *name,
/* Assign an integer value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableInt(CState *st, const char *context, char *name, int64 value)
+putVariableInt(Variables *variables, const char *context, char *name,
+ int64 value)
{
PgBenchValue val;
setIntValue(&val, value);
- return putVariableValue(st, context, name, &val);
+ return putVariableValue(variables, context, name, &val);
}
/*
@@ -1740,7 +1774,7 @@ replaceVariable(char **sql, char *param, int len, char *value)
}
static char *
-assignVariables(CState *st, char *sql)
+assignVariables(Variables *variables, char *sql)
{
char *p,
*name,
@@ -1761,7 +1795,7 @@ assignVariables(CState *st, char *sql)
continue;
}
- val = getVariable(st, name);
+ val = getVariable(variables, name);
free(name);
if (val == NULL)
{
@@ -1776,12 +1810,13 @@ assignVariables(CState *st, char *sql)
}
static void
-getQueryParams(CState *st, const Command *command, const char **params)
+getQueryParams(Variables *variables, const Command *command,
+ const char **params)
{
int i;
for (i = 0; i < command->argc - 1; i++)
- params[i] = getVariable(st, command->argv[i + 1]);
+ params[i] = getVariable(variables, command->argv[i + 1]);
}
static char *
@@ -2649,7 +2684,7 @@ evaluateExpr(CState *st, PgBenchExpr *expr, PgBenchValue *retval)
{
Variable *var;
- if ((var = lookupVariable(st, expr->u.variable.varname)) == NULL)
+ if ((var = lookupVariable(&st->variables, expr->u.variable.varname)) == NULL)
{
pg_log_error("undefined variable \"%s\"", expr->u.variable.varname);
return false;
@@ -2719,7 +2754,7 @@ getMetaCommand(const char *cmd)
* Return true if succeeded, or false on error.
*/
static bool
-runShellCommand(CState *st, char *variable, char **argv, int argc)
+runShellCommand(Variables *variables, char *variable, char **argv, int argc)
{
char command[SHELL_COMMAND_SIZE];
int i,
@@ -2750,7 +2785,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
{
arg = argv[i] + 1; /* a string literal starting with colons */
}
- else if ((arg = getVariable(st, argv[i] + 1)) == NULL)
+ else if ((arg = getVariable(variables, argv[i] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[i]);
return false;
@@ -2811,7 +2846,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
pg_log_error("%s: shell command must return an integer (not \"%s\")", argv[0], res);
return false;
}
- if (!putVariableInt(st, "setshell", variable, retval))
+ if (!putVariableInt(variables, "setshell", variable, retval))
return false;
pg_log_debug("%s: shell parameter name: \"%s\", value: \"%s\"", argv[0], argv[1], res);
@@ -2863,7 +2898,7 @@ sendCommand(CState *st, Command *command)
char *sql;
sql = pg_strdup(command->argv[0]);
- sql = assignVariables(st, sql);
+ sql = assignVariables(&st->variables, sql);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQuery(st->con, sql);
@@ -2874,7 +2909,7 @@ sendCommand(CState *st, Command *command)
const char *sql = command->argv[0];
const char *params[MAX_ARGS];
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQueryParams(st->con, sql, command->argc - 1,
@@ -2921,7 +2956,7 @@ sendCommand(CState *st, Command *command)
st->prepared[st->use_file] = true;
}
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
preparedStatementName(name, st->use_file, st->command);
pg_log_debug("client %d sending %s", st->id, name);
@@ -3014,7 +3049,7 @@ readCommandResponse(CState *st, MetaCommand meta, char *varprefix)
varname = psprintf("%s%s", varprefix, varname);
/* store last row result as a string */
- if (!putVariable(st, meta == META_ASET ? "aset" : "gset", varname,
+ if (!putVariable(&st->variables, meta == META_ASET ? "aset" : "gset", varname,
PQgetvalue(res, ntuples - 1, fld)))
{
/* internal error */
@@ -3075,14 +3110,14 @@ error:
* of delay, in microseconds. Returns true on success, false on error.
*/
static bool
-evaluateSleep(CState *st, int argc, char **argv, int *usecs)
+evaluateSleep(Variables *variables, int argc, char **argv, int *usecs)
{
char *var;
int usec;
if (*argv[1] == ':')
{
- if ((var = getVariable(st, argv[1] + 1)) == NULL)
+ if ((var = getVariable(variables, argv[1] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[1] + 1);
return false;
@@ -3614,7 +3649,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
* latency will be recorded in CSTATE_SLEEP state, not here, after the
* delay has elapsed.)
*/
- if (!evaluateSleep(st, argc, argv, &usec))
+ if (!evaluateSleep(&st->variables, argc, argv, &usec))
{
commandFailed(st, "sleep", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3635,7 +3670,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
return CSTATE_ABORTED;
}
- if (!putVariableValue(st, argv[0], argv[1], &result))
+ if (!putVariableValue(&st->variables, argv[0], argv[1], &result))
{
commandFailed(st, "set", "assignment of meta-command failed");
return CSTATE_ABORTED;
@@ -3705,7 +3740,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SETSHELL)
{
- if (!runShellCommand(st, argv[1], argv + 2, argc - 2))
+ if (!runShellCommand(&st->variables, argv[1], argv + 2, argc - 2))
{
commandFailed(st, "setshell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3713,7 +3748,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SHELL)
{
- if (!runShellCommand(st, NULL, argv + 1, argc - 1))
+ if (!runShellCommand(&st->variables, NULL, argv + 1, argc - 1))
{
commandFailed(st, "shell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -5995,7 +6030,7 @@ main(int argc, char **argv)
}
*p++ = '\0';
- if (!putVariable(&state[0], "option", optarg, p))
+ if (!putVariable(&state[0].variables, "option", optarg, p))
exit(1);
}
break;
@@ -6335,19 +6370,19 @@ main(int argc, char **argv)
int j;
state[i].id = i;
- for (j = 0; j < state[0].nvariables; j++)
+ for (j = 0; j < state[0].variables.nvars; j++)
{
- Variable *var = &state[0].variables[j];
+ Variable *var = &state[0].variables.vars[j];
if (var->value.type != PGBT_NO_VALUE)
{
- if (!putVariableValue(&state[i], "startup",
+ if (!putVariableValue(&state[i].variables, "startup",
var->name, &var->value))
exit(1);
}
else
{
- if (!putVariable(&state[i], "startup",
+ if (!putVariable(&state[i].variables, "startup",
var->name, var->svalue))
exit(1);
}
@@ -6382,11 +6417,11 @@ main(int argc, char **argv)
* :scale variables normally get -s or database scale, but don't override
* an explicit -D switch
*/
- if (lookupVariable(&state[0], "scale") == NULL)
+ if (lookupVariable(&state[0].variables, "scale") == NULL)
{
for (i = 0; i < nclients; i++)
{
- if (!putVariableInt(&state[i], "startup", "scale", scale))
+ if (!putVariableInt(&state[i].variables, "startup", "scale", scale))
exit(1);
}
}
@@ -6395,30 +6430,32 @@ main(int argc, char **argv)
* Define a :client_id variable that is unique per connection. But don't
* override an explicit -D switch.
*/
- if (lookupVariable(&state[0], "client_id") == NULL)
+ if (lookupVariable(&state[0].variables, "client_id") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "client_id", i))
+ if (!putVariableInt(&state[i].variables, "startup", "client_id", i))
exit(1);
}
/* set default seed for hash functions */
- if (lookupVariable(&state[0], "default_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "default_seed") == NULL)
{
uint64 seed =
((uint64) pg_jrand48(base_random_sequence.xseed) & 0xFFFFFFFF) |
(((uint64) pg_jrand48(base_random_sequence.xseed) & 0xFFFFFFFF) << 32);
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "default_seed", (int64) seed))
+ if (!putVariableInt(&state[i].variables, "startup", "default_seed",
+ (int64) seed))
exit(1);
}
/* set random seed unless overwritten */
- if (lookupVariable(&state[0], "random_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "random_seed") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "random_seed", random_seed))
+ if (!putVariableInt(&state[i].variables, "startup", "random_seed",
+ random_seed))
exit(1);
}
--
2.17.1
--Multipart=_Thu__1_Jul_2021_00_02_13_+0900_iyONG0RfOE79H0vs--
^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH v12 1/2] Pgbench errors: use the Variables structure for client variables
@ 2021-05-26 07:58 Yugo Nagata <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Yugo Nagata @ 2021-05-26 07:58 UTC (permalink / raw)
This is most important when it is used to reset client variables during the
repeating of transactions after serialization/deadlock failures.
Don't allocate Variable structs one by one. Instead, add a constant margin each
time it overflows.
---
src/bin/pgbench/pgbench.c | 169 ++++++++++++++++++++++++--------------
1 file changed, 106 insertions(+), 63 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e61055b6b7..8acda86cad 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -287,6 +287,12 @@ const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
+/*
+ * We don't want to allocate variables one by one; for efficiency, add a
+ * constant margin each time it overflows.
+ */
+#define VARIABLES_ALLOC_MARGIN 8
+
/*
* Variable definitions.
*
@@ -304,6 +310,24 @@ typedef struct
PgBenchValue value; /* actual variable's value */
} Variable;
+/*
+ * Data structure for client variables.
+ */
+typedef struct
+{
+ Variable *vars; /* array of variable definitions */
+ int nvars; /* number of variables */
+
+ /*
+ * The maximum number of variables that we can currently store in 'vars'
+ * without having to reallocate more space. We must always have max_vars >=
+ * nvars.
+ */
+ int max_vars;
+
+ bool vars_sorted; /* are variables sorted by name? */
+} Variables;
+
#define MAX_SCRIPTS 128 /* max number of SQL scripts allowed */
#define SHELL_COMMAND_SIZE 256 /* maximum size allowed for shell command */
@@ -460,9 +484,7 @@ typedef struct
int command; /* command number in script */
/* client variables */
- Variable *variables; /* array of variable definitions */
- int nvariables; /* number of variables */
- bool vars_sorted; /* are variables sorted by name? */
+ Variables variables;
/* various times about current transaction in microseconds */
pg_time_usec_t txn_scheduled; /* scheduled start time of transaction */
@@ -1418,39 +1440,39 @@ compareVariableNames(const void *v1, const void *v2)
/* Locate a variable by name; returns NULL if unknown */
static Variable *
-lookupVariable(CState *st, char *name)
+lookupVariable(Variables *variables, char *name)
{
Variable key;
/* On some versions of Solaris, bsearch of zero items dumps core */
- if (st->nvariables <= 0)
+ if (variables->nvars <= 0)
return NULL;
/* Sort if we have to */
- if (!st->vars_sorted)
+ if (!variables->vars_sorted)
{
- qsort((void *) st->variables, st->nvariables, sizeof(Variable),
+ qsort((void *) variables->vars, variables->nvars, sizeof(Variable),
compareVariableNames);
- st->vars_sorted = true;
+ variables->vars_sorted = true;
}
/* Now we can search */
key.name = name;
return (Variable *) bsearch((void *) &key,
- (void *) st->variables,
- st->nvariables,
+ (void *) variables->vars,
+ variables->nvars,
sizeof(Variable),
compareVariableNames);
}
/* Get the value of a variable, in string form; returns NULL if unknown */
static char *
-getVariable(CState *st, char *name)
+getVariable(Variables *variables, char *name)
{
Variable *var;
char stringform[64];
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
return NULL; /* not found */
@@ -1582,21 +1604,43 @@ valid_variable_name(const char *name)
return true;
}
+/*
+ * Make sure there is enough space for 'needed' more variable in the variables
+ * array. It is assumed that the sum of the number of current variables and the
+ * number of needed variables is less than or equal to (INT_MAX -
+ * VARIABLES_ALLOC_MARGIN).
+ */
+static void
+enlargeVariables(Variables *variables, int needed)
+{
+ /* total number of variables required now */
+ needed += variables->nvars;
+
+ if (variables->max_vars < needed)
+ {
+ /*
+ * We don't want to allocate variables one by one; for efficiency, add a
+ * constant margin each time it overflows.
+ */
+ variables->max_vars = needed + VARIABLES_ALLOC_MARGIN;
+ variables->vars = (Variable *)
+ pg_realloc(variables->vars, variables->max_vars * sizeof(Variable));
+ }
+}
+
/*
* Lookup a variable by name, creating it if need be.
* Caller is expected to assign a value to the variable.
* Returns NULL on failure (bad name).
*/
static Variable *
-lookupCreateVariable(CState *st, const char *context, char *name)
+lookupCreateVariable(Variables *variables, const char *context, char *name)
{
Variable *var;
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
{
- Variable *newvars;
-
/*
* Check for the name only when declaring a new variable to avoid
* overhead.
@@ -1608,23 +1652,17 @@ lookupCreateVariable(CState *st, const char *context, char *name)
}
/* Create variable at the end of the array */
- if (st->variables)
- newvars = (Variable *) pg_realloc(st->variables,
- (st->nvariables + 1) * sizeof(Variable));
- else
- newvars = (Variable *) pg_malloc(sizeof(Variable));
-
- st->variables = newvars;
+ enlargeVariables(variables, 1);
- var = &newvars[st->nvariables];
+ var = &(variables->vars[variables->nvars]);
var->name = pg_strdup(name);
var->svalue = NULL;
/* caller is expected to initialize remaining fields */
- st->nvariables++;
+ variables->nvars++;
/* we don't re-sort the array till we have to */
- st->vars_sorted = false;
+ variables->vars_sorted = false;
}
return var;
@@ -1633,12 +1671,13 @@ lookupCreateVariable(CState *st, const char *context, char *name)
/* Assign a string value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariable(CState *st, const char *context, char *name, const char *value)
+putVariable(Variables *variables, const char *context, char *name,
+ const char *value)
{
Variable *var;
char *val;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1656,12 +1695,12 @@ putVariable(CState *st, const char *context, char *name, const char *value)
/* Assign a value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableValue(CState *st, const char *context, char *name,
+putVariableValue(Variables *variables, const char *context, char *name,
const PgBenchValue *value)
{
Variable *var;
- var = lookupCreateVariable(st, context, name);
+ var = lookupCreateVariable(variables, context, name);
if (!var)
return false;
@@ -1676,12 +1715,13 @@ putVariableValue(CState *st, const char *context, char *name,
/* Assign an integer value to a variable, creating it if need be */
/* Returns false on failure (bad name) */
static bool
-putVariableInt(CState *st, const char *context, char *name, int64 value)
+putVariableInt(Variables *variables, const char *context, char *name,
+ int64 value)
{
PgBenchValue val;
setIntValue(&val, value);
- return putVariableValue(st, context, name, &val);
+ return putVariableValue(variables, context, name, &val);
}
/*
@@ -1740,7 +1780,7 @@ replaceVariable(char **sql, char *param, int len, char *value)
}
static char *
-assignVariables(CState *st, char *sql)
+assignVariables(Variables *variables, char *sql)
{
char *p,
*name,
@@ -1761,7 +1801,7 @@ assignVariables(CState *st, char *sql)
continue;
}
- val = getVariable(st, name);
+ val = getVariable(variables, name);
free(name);
if (val == NULL)
{
@@ -1776,12 +1816,13 @@ assignVariables(CState *st, char *sql)
}
static void
-getQueryParams(CState *st, const Command *command, const char **params)
+getQueryParams(Variables *variables, const Command *command,
+ const char **params)
{
int i;
for (i = 0; i < command->argc - 1; i++)
- params[i] = getVariable(st, command->argv[i + 1]);
+ params[i] = getVariable(variables, command->argv[i + 1]);
}
static char *
@@ -2647,7 +2688,7 @@ evaluateExpr(CState *st, PgBenchExpr *expr, PgBenchValue *retval)
{
Variable *var;
- if ((var = lookupVariable(st, expr->u.variable.varname)) == NULL)
+ if ((var = lookupVariable(&st->variables, expr->u.variable.varname)) == NULL)
{
pg_log_error("undefined variable \"%s\"", expr->u.variable.varname);
return false;
@@ -2717,7 +2758,7 @@ getMetaCommand(const char *cmd)
* Return true if succeeded, or false on error.
*/
static bool
-runShellCommand(CState *st, char *variable, char **argv, int argc)
+runShellCommand(Variables *variables, char *variable, char **argv, int argc)
{
char command[SHELL_COMMAND_SIZE];
int i,
@@ -2748,7 +2789,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
{
arg = argv[i] + 1; /* a string literal starting with colons */
}
- else if ((arg = getVariable(st, argv[i] + 1)) == NULL)
+ else if ((arg = getVariable(variables, argv[i] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[i]);
return false;
@@ -2809,7 +2850,7 @@ runShellCommand(CState *st, char *variable, char **argv, int argc)
pg_log_error("%s: shell command must return an integer (not \"%s\")", argv[0], res);
return false;
}
- if (!putVariableInt(st, "setshell", variable, retval))
+ if (!putVariableInt(variables, "setshell", variable, retval))
return false;
pg_log_debug("%s: shell parameter name: \"%s\", value: \"%s\"", argv[0], argv[1], res);
@@ -2861,7 +2902,7 @@ sendCommand(CState *st, Command *command)
char *sql;
sql = pg_strdup(command->argv[0]);
- sql = assignVariables(st, sql);
+ sql = assignVariables(&st->variables, sql);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQuery(st->con, sql);
@@ -2872,7 +2913,7 @@ sendCommand(CState *st, Command *command)
const char *sql = command->argv[0];
const char *params[MAX_ARGS];
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
pg_log_debug("client %d sending %s", st->id, sql);
r = PQsendQueryParams(st->con, sql, command->argc - 1,
@@ -2919,7 +2960,7 @@ sendCommand(CState *st, Command *command)
st->prepared[st->use_file] = true;
}
- getQueryParams(st, command, params);
+ getQueryParams(&st->variables, command, params);
preparedStatementName(name, st->use_file, st->command);
pg_log_debug("client %d sending %s", st->id, name);
@@ -3012,7 +3053,7 @@ readCommandResponse(CState *st, MetaCommand meta, char *varprefix)
varname = psprintf("%s%s", varprefix, varname);
/* store last row result as a string */
- if (!putVariable(st, meta == META_ASET ? "aset" : "gset", varname,
+ if (!putVariable(&st->variables, meta == META_ASET ? "aset" : "gset", varname,
PQgetvalue(res, ntuples - 1, fld)))
{
/* internal error */
@@ -3073,14 +3114,14 @@ error:
* of delay, in microseconds. Returns true on success, false on error.
*/
static bool
-evaluateSleep(CState *st, int argc, char **argv, int *usecs)
+evaluateSleep(Variables *variables, int argc, char **argv, int *usecs)
{
char *var;
int usec;
if (*argv[1] == ':')
{
- if ((var = getVariable(st, argv[1] + 1)) == NULL)
+ if ((var = getVariable(variables, argv[1] + 1)) == NULL)
{
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[1] + 1);
return false;
@@ -3612,7 +3653,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
* latency will be recorded in CSTATE_SLEEP state, not here, after the
* delay has elapsed.)
*/
- if (!evaluateSleep(st, argc, argv, &usec))
+ if (!evaluateSleep(&st->variables, argc, argv, &usec))
{
commandFailed(st, "sleep", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3633,7 +3674,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
return CSTATE_ABORTED;
}
- if (!putVariableValue(st, argv[0], argv[1], &result))
+ if (!putVariableValue(&st->variables, argv[0], argv[1], &result))
{
commandFailed(st, "set", "assignment of meta-command failed");
return CSTATE_ABORTED;
@@ -3703,7 +3744,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SETSHELL)
{
- if (!runShellCommand(st, argv[1], argv + 2, argc - 2))
+ if (!runShellCommand(&st->variables, argv[1], argv + 2, argc - 2))
{
commandFailed(st, "setshell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -3711,7 +3752,7 @@ executeMetaCommand(CState *st, pg_time_usec_t *now)
}
else if (command->meta == META_SHELL)
{
- if (!runShellCommand(st, NULL, argv + 1, argc - 1))
+ if (!runShellCommand(&st->variables, NULL, argv + 1, argc - 1))
{
commandFailed(st, "shell", "execution of meta-command failed");
return CSTATE_ABORTED;
@@ -5993,7 +6034,7 @@ main(int argc, char **argv)
}
*p++ = '\0';
- if (!putVariable(&state[0], "option", optarg, p))
+ if (!putVariable(&state[0].variables, "option", optarg, p))
exit(1);
}
break;
@@ -6333,19 +6374,19 @@ main(int argc, char **argv)
int j;
state[i].id = i;
- for (j = 0; j < state[0].nvariables; j++)
+ for (j = 0; j < state[0].variables.nvars; j++)
{
- Variable *var = &state[0].variables[j];
+ Variable *var = &state[0].variables.vars[j];
if (var->value.type != PGBT_NO_VALUE)
{
- if (!putVariableValue(&state[i], "startup",
+ if (!putVariableValue(&state[i].variables, "startup",
var->name, &var->value))
exit(1);
}
else
{
- if (!putVariable(&state[i], "startup",
+ if (!putVariable(&state[i].variables, "startup",
var->name, var->svalue))
exit(1);
}
@@ -6380,11 +6421,11 @@ main(int argc, char **argv)
* :scale variables normally get -s or database scale, but don't override
* an explicit -D switch
*/
- if (lookupVariable(&state[0], "scale") == NULL)
+ if (lookupVariable(&state[0].variables, "scale") == NULL)
{
for (i = 0; i < nclients; i++)
{
- if (!putVariableInt(&state[i], "startup", "scale", scale))
+ if (!putVariableInt(&state[i].variables, "startup", "scale", scale))
exit(1);
}
}
@@ -6393,30 +6434,32 @@ main(int argc, char **argv)
* Define a :client_id variable that is unique per connection. But don't
* override an explicit -D switch.
*/
- if (lookupVariable(&state[0], "client_id") == NULL)
+ if (lookupVariable(&state[0].variables, "client_id") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "client_id", i))
+ if (!putVariableInt(&state[i].variables, "startup", "client_id", i))
exit(1);
}
/* set default seed for hash functions */
- if (lookupVariable(&state[0], "default_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "default_seed") == NULL)
{
uint64 seed =
((uint64) pg_jrand48(base_random_sequence.xseed) & 0xFFFFFFFF) |
(((uint64) pg_jrand48(base_random_sequence.xseed) & 0xFFFFFFFF) << 32);
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "default_seed", (int64) seed))
+ if (!putVariableInt(&state[i].variables, "startup", "default_seed",
+ (int64) seed))
exit(1);
}
/* set random seed unless overwritten */
- if (lookupVariable(&state[0], "random_seed") == NULL)
+ if (lookupVariable(&state[0].variables, "random_seed") == NULL)
{
for (i = 0; i < nclients; i++)
- if (!putVariableInt(&state[i], "startup", "random_seed", random_seed))
+ if (!putVariableInt(&state[i].variables, "startup", "random_seed",
+ random_seed))
exit(1);
}
--
2.17.1
--Multipart=_Tue__22_Jun_2021_23_54_59_+0900_JJJkapGYdP+ALwtM--
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
@ 2024-12-12 02:32 Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: Peter Smith @ 2024-12-12 02:32 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Thu, Dec 12, 2024 at 12:41 PM Tom Lane <[email protected]> wrote:
>
> Peter Smith <[email protected]> writes:
> > While reviewing a patch for another pg_createsubscriber thread I found
> > that multiple TAP tests have a terrible wrapping where the command
> > options and their associated oparg are separated on different lines
> > instead of paired together nicely. This makes it unnecessarily
> > difficult to see what the test is doing.
>
> I think that is mostly the fault of pgperltidy. We did change
> the options we use with it awhile back, so maybe now it will honor
> your manual changes to its line-wrapping choices. But I wouldn't
> bet on that. Did you check what happens if you run the modified
> code through pgperltidy?
>
> (If the answer is bad, we could look into making further changes so
> that pgperltidy won't override these decisions. But there's no point
> in manually patching this if it'll just get undone.)
>
Thanks for your suggestion. As you probably suspected, the answer is bad.
I ran pgperltidy on the "fixed" file:
[postgres@CentOS7-x64 oss_postgres_misc]$
src/tools/pgindent/pgperltidy
src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
This reverted it to how it currently looks on master.
The strange thing is there are other commands in that file very
similar to the ones I had changed but those already looked good, yet
they remained unaffected by the pgperltidy. Why?
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
@ 2024-12-12 02:46 ` Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: Tom Lane @ 2024-12-12 02:46 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Peter Smith <[email protected]> writes:
> The strange thing is there are other commands in that file very
> similar to the ones I had changed but those already looked good, yet
> they remained unaffected by the pgperltidy. Why?
You sure it's not just luck-of-the-draw? I think that perltidy
is just splitting the lines based on length, so sometimes related
options would be kept together and sometimes not.
regards, tom lane
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
@ 2024-12-12 03:13 ` Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: Peter Smith @ 2024-12-12 03:13 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Thu, Dec 12, 2024 at 1:46 PM Tom Lane <[email protected]> wrote:
>
> Peter Smith <[email protected]> writes:
> > The strange thing is there are other commands in that file very
> > similar to the ones I had changed but those already looked good, yet
> > they remained unaffected by the pgperltidy. Why?
>
> You sure it's not just luck-of-the-draw? I think that perltidy
> is just splitting the lines based on length, so sometimes related
> options would be kept together and sometimes not.
>
TBH, I have no idea what logic perltidy uses. I did find some
configurations here [1] (are those what it pgperltidy uses?) but those
claim max line length is 78 which I didn;t come anywhere near
exceeding.
After some more experimentation, I've noticed that it is trying to
keep only 2 items on each line. So whether it looks good or not seems
to depend if there is an even or odd number of options without
arguments up-front. Maybe those perltidy "tightness" switches?
So, AFAICT I can workaround the perltidy wrapping just by putting all
the noarg options at the bottom of the command, then all the
option/optarg pairs (ie 2s) will stay together. I can post another
patch to do it this way unless you think it is too hacky.
======
[1] https://github.com/postgres/postgres/blob/master/src/tools/pgindent/perltidyrc
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
@ 2024-12-12 03:53 ` Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: Michael Paquier @ 2024-12-12 03:53 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Dec 12, 2024 at 02:13:40PM +1100, Peter Smith wrote:
> TBH, I have no idea what logic perltidy uses. I did find some
> configurations here [1] (are those what it pgperltidy uses?) but those
> claim max line length is 78 which I didn't come anywhere near
> exceeding.
Gave up trying to understand its internals and its rules years ago.
It's complicated enough that we require a specific version of the tool
for the tree with a custom PATH :D
> After some more experimentation, I've noticed that it is trying to
> keep only 2 items on each line. So whether it looks good or not seems
> to depend if there is an even or odd number of options without
> arguments up-front. Maybe those perltidy "tightness" switches?
Don't recall so even under Perl-Tidy-20230309, because it comes down
to the number of elements in these arrays and the length of their
values, not the specific values in each element of the array.
> So, AFAICT I can workaround the perltidy wrapping just by putting all
> the noarg options at the bottom of the command, then all the
> option/optarg pairs (ie 2s) will stay together. I can post another
> patch to do it this way unless you think it is too hacky.
This trick works for me if that makes the long list of option easier
to read. With two elements of the array perl line, I would just put
some --dry-run or --verbose at the end of their respective arrays.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
@ 2024-12-12 04:24 ` Peter Smith <[email protected]>
2024-12-12 05:02 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
0 siblings, 2 replies; 26+ messages in thread
From: Peter Smith @ 2024-12-12 04:24 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Dec 12, 2024 at 2:53 PM Michael Paquier <[email protected]> wrote:
...
> > So, AFAICT I can workaround the perltidy wrapping just by putting all
> > the noarg options at the bottom of the command, then all the
> > option/optarg pairs (ie 2s) will stay together. I can post another
> > patch to do it this way unless you think it is too hacky.
>
> This trick works for me if that makes the long list of option easier
> to read. With two elements of the array perl line, I would just put
> some --dry-run or --verbose at the end of their respective arrays.
> --
> Michael
Hi Michael.
Yes, that is the workaround that I was proposing.
PSA v2-0001. This time it can survive pgperltidy unchanged.
In passing I also removed the duplicated '--verbose' and changed the
'Pub2' case mentioned in the original post.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
[application/octet-stream] v2-0001-Modify-wrapping-to-make-command-options-easier-to.patch (5.8K, ../../CAHut+PvYsVhF5k-mYHVEFdpijdgkYRtoeWVxdspxio0ePRpkug@mail.gmail.com/2-v2-0001-Modify-wrapping-to-make-command-options-easier-to.patch)
download | inline diff:
From 0631df34f93ae70942b06e7d66d420416f0da253 Mon Sep 17 00:00:00 2001
From: Peter Smith <[email protected]>
Date: Thu, 12 Dec 2024 15:20:42 +1100
Subject: [PATCH v2] Modify wrapping to make command options easier to read.
---
src/bin/pg_basebackup/t/040_pg_createsubscriber.pl | 125 ++++++++++-----------
1 file changed, 62 insertions(+), 63 deletions(-)
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 0a900ed..6ef105f 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -169,13 +169,13 @@ $node_t->stop;
command_fails(
[
'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_t->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_t->host, '--subscriber-port',
- $node_t->port, '--database',
- $db1, '--database',
- $db2
+ '--pgdata', $node_t->data_dir,
+ '--publisher-server', $node_p->connstr($db1),
+ '--socketdir', $node_t->host,
+ '--subscriber-port', $node_t->port,
+ '--database', $db1,
+ '--database', $db2,
+ '--dry-run'
],
'target server is not in recovery');
@@ -183,13 +183,13 @@ command_fails(
command_fails(
[
'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ '--pgdata', $node_s->data_dir,
+ '--publisher-server', $node_p->connstr($db1),
+ '--socketdir', $node_s->host,
+ '--subscriber-port', $node_s->port,
+ '--database', $db1,
+ '--database', $db2,
+ '--dry-run'
],
'standby is up and running');
@@ -217,13 +217,13 @@ $node_c->set_standby_mode();
command_fails(
[
'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_c->data_dir, '--publisher-server',
- $node_s->connstr($db1), '--socketdir',
- $node_c->host, '--subscriber-port',
- $node_c->port, '--database',
- $db1, '--database',
- $db2
+ '--pgdata', $node_c->data_dir,
+ '--publisher-server', $node_s->connstr($db1),
+ '--socketdir', $node_c->host,
+ '--subscriber-port', $node_c->port,
+ '--database', $db1,
+ '--database', $db2,
+ '--dry-run'
],
'primary server is in recovery');
@@ -240,13 +240,13 @@ $node_s->stop;
command_fails(
[
'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ '--pgdata', $node_s->data_dir,
+ '--publisher-server', $node_p->connstr($db1),
+ '--socketdir', $node_s->host,
+ '--subscriber-port', $node_s->port,
+ '--database', $db1,
+ '--database', $db2,
+ '--dry-run'
],
'primary contains unmet conditions on node P');
# Restore default settings here but only apply it after testing standby. Some
@@ -269,13 +269,13 @@ max_worker_processes = 2
command_fails(
[
'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ '--pgdata', $node_s->data_dir,
+ '--publisher-server', $node_p->connstr($db1),
+ '--socketdir', $node_s->host,
+ '--subscriber-port', $node_s->port,
+ '--database', $db1,
+ '--database', $db2,
+ '--dry-run'
],
'standby contains unmet conditions on node S');
$node_s->append_conf(
@@ -323,17 +323,17 @@ command_ok(
[
'pg_createsubscriber', '--verbose',
'--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'pub2', '--subscription',
- 'sub1', '--subscription',
- 'sub2', '--database',
- $db1, '--database',
- $db2
+ '--pgdata', $node_s->data_dir,
+ '--publisher-server', $node_p->connstr($db1),
+ '--socketdir', $node_s->host,
+ '--subscriber-port', $node_s->port,
+ '--publication', 'pub1',
+ '--publication', 'pub2',
+ '--subscription', 'sub1',
+ '--subscription', 'sub2',
+ '--database', $db1,
+ '--database', $db2,
+ '--dry-run'
],
'run pg_createsubscriber --dry-run on node S');
@@ -347,12 +347,12 @@ $node_s->stop;
command_ok(
[
'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--replication-slot',
- 'replslot1'
+ '--pgdata', $node_s->data_dir,
+ '--publisher-server', $node_p->connstr($db1),
+ '--socketdir', $node_s->host,
+ '--subscriber-port', $node_s->port,
+ '--replication-slot', 'replslot1',
+ '--dry-run'
],
'run pg_createsubscriber without --databases');
@@ -361,17 +361,16 @@ command_ok(
[
'pg_createsubscriber', '--verbose',
'--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--verbose', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'Pub2', '--replication-slot',
- 'replslot1', '--replication-slot',
- 'replslot2', '--database',
- $db1, '--database',
- $db2
+ '--pgdata', $node_s->data_dir,
+ '--publisher-server', $node_p->connstr($db1),
+ '--socketdir', $node_s->host,
+ '--subscriber-port', $node_s->port,
+ '--publication', 'pub1',
+ '--publication', 'pub2',
+ '--replication-slot', 'replslot1',
+ '--replication-slot', 'replslot2',
+ '--database', $db1,
+ '--database', $db2
],
'run pg_createsubscriber on node S');
--
1.8.3.1
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
@ 2024-12-12 05:02 ` Michael Paquier <[email protected]>
2024-12-12 10:02 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Alvaro Herrera <[email protected]>
1 sibling, 1 reply; 26+ messages in thread
From: Michael Paquier @ 2024-12-12 05:02 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Dec 12, 2024 at 03:24:42PM +1100, Peter Smith wrote:
> PSA v2-0001. This time it can survive pgperltidy unchanged.
Confirmed. It looks to apply cleanly to v17 as well. Better to
backpatch to avoid conflict frictions, even if it's cosmetic.
> In passing I also removed the duplicated '--verbose' and changed the
> 'Pub2' case mentioned in the original post.
Right. I didn't notice this dup in "run pg_createsubscriber on node
S". All the commands of the test seem to be in order with what you've
attached, so LGTM.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 05:02 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
@ 2024-12-12 10:02 ` Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Alvaro Herrera @ 2024-12-12 10:02 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Peter Smith <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>
On 2024-Dec-12, Michael Paquier wrote:
> On Thu, Dec 12, 2024 at 03:24:42PM +1100, Peter Smith wrote:
> > PSA v2-0001. This time it can survive pgperltidy unchanged.
>
> Confirmed. It looks to apply cleanly to v17 as well. Better to
> backpatch to avoid conflict frictions, even if it's cosmetic.
I'd rather use #<<< and #>>> markers.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
@ 2024-12-12 12:04 ` Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
1 sibling, 1 reply; 26+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2024-12-12 12:04 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: Michael Paquier <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>
Peter Smith <[email protected]> writes:
> On Thu, Dec 12, 2024 at 2:53 PM Michael Paquier <[email protected]> wrote:
> ...
>
>> > So, AFAICT I can workaround the perltidy wrapping just by putting all
>> > the noarg options at the bottom of the command, then all the
>> > option/optarg pairs (ie 2s) will stay together. I can post another
>> > patch to do it this way unless you think it is too hacky.
>>
>> This trick works for me if that makes the long list of option easier
>> to read. With two elements of the array perl line, I would just put
>> some --dry-run or --verbose at the end of their respective arrays.
>> --
>> Michael
>
> Hi Michael.
>
> Yes, that is the workaround that I was proposing.
A better option, IMO, is to use the fat comma (=>) between options and
their values. This makes it clear both to humans and perltidy that they
belong together, and we can put all the valueless options first without
things being rewrapped.
- ilmari
Attachments:
[text/x-diff] v3-0001-Modify-wrapping-to-make-command-options-easier-to.patch (7.4K, ../../[email protected]/2-v3-0001-Modify-wrapping-to-make-command-options-easier-to.patch)
download | inline diff:
From e972f1a5f87499390f401e7db2c079fb87533553 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <[email protected]>
Date: Thu, 12 Dec 2024 11:56:15 +0000
Subject: [PATCH v3] Modify wrapping to make command options easier to read
Use fat comma after options that take values, both to make it clearer
to humans, and to stop perltidy from re-wrapping them.
---
.../t/040_pg_createsubscriber.pl | 169 +++++++++---------
1 file changed, 89 insertions(+), 80 deletions(-)
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 0a900edb656..0f7bb103177 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -168,41 +168,44 @@ sub generate_db
# Run pg_createsubscriber on a promoted server
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_t->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_t->host, '--subscriber-port',
- $node_t->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_t->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_t->host,
+ '--subscriber-port' => $node_t->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'target server is not in recovery');
# Run pg_createsubscriber when standby is running
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'standby is up and running');
# Run pg_createsubscriber on about-to-fail node F
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $node_f->data_dir,
- '--publisher-server', $node_p->connstr($db1),
- '--socketdir', $node_f->host,
- '--subscriber-port', $node_f->port,
- '--database', $db1,
- '--database', $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $node_f->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_f->host,
+ '--subscriber-port' => $node_f->port,
+ '--database' => $db1,
+ '--database' => $db2
],
'subscriber data directory is not a copy of the source database cluster');
@@ -216,14 +219,15 @@ sub generate_db
# Run pg_createsubscriber on node C (P -> S -> C)
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_c->data_dir, '--publisher-server',
- $node_s->connstr($db1), '--socketdir',
- $node_c->host, '--subscriber-port',
- $node_c->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_c->data_dir,
+ '--publisher-server' => $node_s->connstr($db1),
+ '--socketdir' => $node_c->host,
+ '--subscriber-port' => $node_c->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'primary server is in recovery');
@@ -239,14 +243,16 @@ sub generate_db
$node_s->stop;
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
+
],
'primary contains unmet conditions on node P');
# Restore default settings here but only apply it after testing standby. Some
@@ -268,14 +274,15 @@ sub generate_db
});
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'standby contains unmet conditions on node S');
$node_s->append_conf(
@@ -321,19 +328,20 @@ sub generate_db
# dry run mode on node S
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'pub2', '--subscription',
- 'sub1', '--subscription',
- 'sub2', '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--publication' => 'pub1',
+ '--publication' => 'pub2',
+ '--subscription' => 'sub1',
+ '--subscription' => 'sub2',
+ '--database' => $db1,
+ '--database' => $db2,
],
'run pg_createsubscriber --dry-run on node S');
@@ -346,32 +354,33 @@ sub generate_db
# pg_createsubscriber can run without --databases option
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--replication-slot',
- 'replslot1'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--replication-slot' => 'replslot1',
],
'run pg_createsubscriber without --databases');
# Run pg_createsubscriber on node S
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--verbose', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'Pub2', '--replication-slot',
- 'replslot1', '--replication-slot',
- 'replslot2', '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--recovery-timeout' => "$PostgreSQL::Test::Utils::timeout_default",
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--publication' => 'pub1',
+ '--publication' => 'pub2',
+ '--replication-slot' => 'replslot1',
+ '--replication-slot' => 'replslot2',
+ '--database' => $db1,
+ '--database' => $db2,
],
'run pg_createsubscriber on node S');
--
2.47.1
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
@ 2024-12-12 13:17 ` Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 15:28 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2025-01-14 07:03 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
0 siblings, 2 replies; 26+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2024-12-12 13:17 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: Michael Paquier <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>
Dagfinn Ilmari Mannsåker <[email protected]> writes:
> Peter Smith <[email protected]> writes:
>
>> On Thu, Dec 12, 2024 at 2:53 PM Michael Paquier <[email protected]> wrote:
>> ...
>>
>>> > So, AFAICT I can workaround the perltidy wrapping just by putting all
>>> > the noarg options at the bottom of the command, then all the
>>> > option/optarg pairs (ie 2s) will stay together. I can post another
>>> > patch to do it this way unless you think it is too hacky.
>>>
>>> This trick works for me if that makes the long list of option easier
>>> to read. With two elements of the array perl line, I would just put
>>> some --dry-run or --verbose at the end of their respective arrays.
>>> --
>>> Michael
>>
>> Hi Michael.
>>
>> Yes, that is the workaround that I was proposing.
>
> A better option, IMO, is to use the fat comma (=>) between options and
> their values. This makes it clear both to humans and perltidy that they
> belong together, and we can put all the valueless options first without
> things being rewrapped.
Here's a more thorough patch, that also applies the fat comma treatment
to other pg_createsubscriber invocations in the same file that don't
currently happen to be mangled by perltidy. It also adds trailing
commas to the last item in multi-line command arrays, which is common
perl style.
- ilmari
Attachments:
[text/x-diff] v4-0001-Modify-wrapping-to-make-command-options-easier-to.patch (10.5K, ../../[email protected]/2-v4-0001-Modify-wrapping-to-make-command-options-easier-to.patch)
download | inline diff:
From 953d0c8ca8202d6f53af833fd53e3f6b1929fa77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <[email protected]>
Date: Thu, 12 Dec 2024 11:56:15 +0000
Subject: [PATCH v4] Modify wrapping to make command options easier to read
Use fat comma after options that take values, both to make it clearer
to humans, and to stop perltidy from re-wrapping them.
Also remove pointless quoting of $PostgreSQL::Test::Utils::timeout_default.
---
.../t/040_pg_createsubscriber.pl | 255 +++++++++---------
1 file changed, 135 insertions(+), 120 deletions(-)
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 0a900edb656..369846db0d0 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -46,69 +46,75 @@ sub generate_db
command_fails(['pg_createsubscriber'],
'no subscriber data directory specified');
command_fails(
- [ 'pg_createsubscriber', '--pgdata', $datadir ],
+ [ 'pg_createsubscriber', '--pgdata' => $datadir ],
'no publisher connection string specified');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
],
'no database name specified');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432',
- '--database', 'pg1',
- '--database', 'pg1'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
+ '--database' => 'pg1',
+ '--database' => 'pg1',
],
'duplicate database name');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432',
- '--publication', 'foo1',
- '--publication', 'foo1',
- '--database', 'pg1',
- '--database', 'pg2'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
+ '--publication' => 'foo1',
+ '--publication' => 'foo1',
+ '--database' => 'pg1',
+ '--database' => 'pg2',
],
'duplicate publication name');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432',
- '--publication', 'foo1',
- '--database', 'pg1',
- '--database', 'pg2'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
+ '--publication' => 'foo1',
+ '--database' => 'pg1',
+ '--database' => 'pg2',
],
'wrong number of publication names');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432',
- '--publication', 'foo1',
- '--publication', 'foo2',
- '--subscription', 'bar1',
- '--database', 'pg1',
- '--database', 'pg2'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
+ '--publication' => 'foo1',
+ '--publication' => 'foo2',
+ '--subscription' => 'bar1',
+ '--database' => 'pg1',
+ '--database' => 'pg2',
],
'wrong number of subscription names');
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $datadir,
- '--publisher-server', 'port=5432',
- '--publication', 'foo1',
- '--publication', 'foo2',
- '--subscription', 'bar1',
- '--subscription', 'bar2',
- '--replication-slot', 'baz1',
- '--database', 'pg1',
- '--database', 'pg2'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $datadir,
+ '--publisher-server' => 'port=5432',
+ '--publication' => 'foo1',
+ '--publication' => 'foo2',
+ '--subscription' => 'bar1',
+ '--subscription' => 'bar2',
+ '--replication-slot' => 'baz1',
+ '--database' => 'pg1',
+ '--database' => 'pg2',
],
'wrong number of replication slot names');
@@ -168,41 +174,44 @@ sub generate_db
# Run pg_createsubscriber on a promoted server
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_t->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_t->host, '--subscriber-port',
- $node_t->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_t->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_t->host,
+ '--subscriber-port' => $node_t->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'target server is not in recovery');
# Run pg_createsubscriber when standby is running
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'standby is up and running');
# Run pg_createsubscriber on about-to-fail node F
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--pgdata', $node_f->data_dir,
- '--publisher-server', $node_p->connstr($db1),
- '--socketdir', $node_f->host,
- '--subscriber-port', $node_f->port,
- '--database', $db1,
- '--database', $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--pgdata' => $node_f->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_f->host,
+ '--subscriber-port' => $node_f->port,
+ '--database' => $db1,
+ '--database' => $db2
],
'subscriber data directory is not a copy of the source database cluster');
@@ -216,14 +225,15 @@ sub generate_db
# Run pg_createsubscriber on node C (P -> S -> C)
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_c->data_dir, '--publisher-server',
- $node_s->connstr($db1), '--socketdir',
- $node_c->host, '--subscriber-port',
- $node_c->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_c->data_dir,
+ '--publisher-server' => $node_s->connstr($db1),
+ '--socketdir' => $node_c->host,
+ '--subscriber-port' => $node_c->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'primary server is in recovery');
@@ -239,14 +249,16 @@ sub generate_db
$node_s->stop;
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
+
],
'primary contains unmet conditions on node P');
# Restore default settings here but only apply it after testing standby. Some
@@ -268,14 +280,15 @@ sub generate_db
});
command_fails(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--database' => $db2,
],
'standby contains unmet conditions on node S');
$node_s->append_conf(
@@ -321,19 +334,20 @@ sub generate_db
# dry run mode on node S
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'pub2', '--subscription',
- 'sub1', '--subscription',
- 'sub2', '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--publication' => 'pub1',
+ '--publication' => 'pub2',
+ '--subscription' => 'sub1',
+ '--subscription' => 'sub2',
+ '--database' => $db1,
+ '--database' => $db2,
],
'run pg_createsubscriber --dry-run on node S');
@@ -346,32 +360,33 @@ sub generate_db
# pg_createsubscriber can run without --databases option
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--dry-run', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--replication-slot',
- 'replslot1'
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--replication-slot' => 'replslot1',
],
'run pg_createsubscriber without --databases');
# Run pg_createsubscriber on node S
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--verbose', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'Pub2', '--replication-slot',
- 'replslot1', '--replication-slot',
- 'replslot2', '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--publication' => 'pub1',
+ '--publication' => 'pub2',
+ '--replication-slot' => 'replslot1',
+ '--replication-slot' => 'replslot2',
+ '--database' => $db1,
+ '--database' => $db2,
],
'run pg_createsubscriber on node S');
--
2.47.1
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
@ 2024-12-12 15:28 ` Andrew Dunstan <[email protected]>
2024-12-12 15:35 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
1 sibling, 1 reply; 26+ messages in thread
From: Andrew Dunstan @ 2024-12-12 15:28 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; Peter Smith <[email protected]>; +Cc: Michael Paquier <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>
On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker wrote:
> Dagfinn Ilmari Mannsåker<[email protected]> writes:
>
>> Peter Smith<[email protected]> writes:
>>
>>> On Thu, Dec 12, 2024 at 2:53 PM Michael Paquier<[email protected]> wrote:
>>> ...
>>>
>>>>> So, AFAICT I can workaround the perltidy wrapping just by putting all
>>>>> the noarg options at the bottom of the command, then all the
>>>>> option/optarg pairs (ie 2s) will stay together. I can post another
>>>>> patch to do it this way unless you think it is too hacky.
>>>> This trick works for me if that makes the long list of option easier
>>>> to read. With two elements of the array perl line, I would just put
>>>> some --dry-run or --verbose at the end of their respective arrays.
>>>> --
>>>> Michael
>>> Hi Michael.
>>>
>>> Yes, that is the workaround that I was proposing.
>> A better option, IMO, is to use the fat comma (=>) between options and
>> their values. This makes it clear both to humans and perltidy that they
>> belong together, and we can put all the valueless options first without
>> things being rewrapped.
> Here's a more thorough patch, that also applies the fat comma treatment
> to other pg_createsubscriber invocations in the same file that don't
> currently happen to be mangled by perltidy. It also adds trailing
> commas to the last item in multi-line command arrays, which is common
> perl style.
>
+1 for this approach.
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 15:28 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
@ 2024-12-12 15:35 ` Tom Lane <[email protected]>
2024-12-12 17:08 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: Tom Lane @ 2024-12-12 15:35 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; Peter Smith <[email protected]>; Michael Paquier <[email protected]>; PostgreSQL Hackers <[email protected]>
Andrew Dunstan <[email protected]> writes:
> On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker wrote:
>> Here's a more thorough patch, that also applies the fat comma treatment
>> to other pg_createsubscriber invocations in the same file that don't
>> currently happen to be mangled by perltidy. It also adds trailing
>> commas to the last item in multi-line command arrays, which is common
>> perl style.
> +1 for this approach.
Indeed, this is much nicer if it's something perltidy knows about.
However, I know we have the same issue in many other places.
Anyone feel like running through all the TAP scripts?
regards, tom lane
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 15:28 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2024-12-12 15:35 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
@ 2024-12-12 17:08 ` Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 17:52 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2024-12-12 17:08 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Peter Smith <[email protected]>; Michael Paquier <[email protected]>; PostgreSQL Hackers <[email protected]>
Tom Lane <[email protected]> writes:
> Andrew Dunstan <[email protected]> writes:
>> On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker wrote:
>>> Here's a more thorough patch, that also applies the fat comma treatment
>>> to other pg_createsubscriber invocations in the same file that don't
>>> currently happen to be mangled by perltidy. It also adds trailing
>>> commas to the last item in multi-line command arrays, which is common
>>> perl style.
>
>> +1 for this approach.
>
> Indeed, this is much nicer if it's something perltidy knows about.
>
> However, I know we have the same issue in many other places.
> Anyone feel like running through all the TAP scripts?
I can have a go in the next few days. A quick grep spotted another
workaround in 027_stream_regress.pl: using paretheses around the option
and its value:
command_ok(
[
'pg_dump',
('--schema', 'pg_catalog'),
('-f', $outputdir . '/catalogs_primary.dump'),
'--no-sync',
('-p', $node_primary->port),
'--no-unlogged-table-data',
'regression'
],
'dump catalogs of primary server');
I think the fat comma is much nicer than this, so I'd like to convert
these too (and replace some of the concatenations with interpolation).
Technically the quotes aren't necessary around single-dash options
before => since unary minus works on strings as well as numbers, but
I'll leave them in for consistency.
- ilmari
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 15:28 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2024-12-12 15:35 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 17:08 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
@ 2024-12-12 17:52 ` Andrew Dunstan <[email protected]>
2024-12-12 18:03 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: Andrew Dunstan @ 2024-12-12 17:52 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; Tom Lane <[email protected]>; +Cc: Peter Smith <[email protected]>; Michael Paquier <[email protected]>; PostgreSQL Hackers <[email protected]>
On 2024-12-12 Th 12:08 PM, Dagfinn Ilmari Mannsåker wrote:
> Tom Lane <[email protected]> writes:
>
>> Andrew Dunstan <[email protected]> writes:
>>> On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker wrote:
>>>> Here's a more thorough patch, that also applies the fat comma treatment
>>>> to other pg_createsubscriber invocations in the same file that don't
>>>> currently happen to be mangled by perltidy. It also adds trailing
>>>> commas to the last item in multi-line command arrays, which is common
>>>> perl style.
>>> +1 for this approach.
>> Indeed, this is much nicer if it's something perltidy knows about.
>>
>> However, I know we have the same issue in many other places.
>> Anyone feel like running through all the TAP scripts?
> I can have a go in the next few days. A quick grep spotted another
> workaround in 027_stream_regress.pl: using paretheses around the option
> and its value:
>
> command_ok(
> [
> 'pg_dump',
> ('--schema', 'pg_catalog'),
> ('-f', $outputdir . '/catalogs_primary.dump'),
> '--no-sync',
> ('-p', $node_primary->port),
> '--no-unlogged-table-data',
> 'regression'
> ],
> 'dump catalogs of primary server');
>
> I think the fat comma is much nicer than this, so I'd like to convert
> these too (and replace some of the concatenations with interpolation).
>
> Technically the quotes aren't necessary around single-dash options
> before => since unary minus works on strings as well as numbers, but
> I'll leave them in for consistency.
>
I'd rather get rid of those and just use the long options.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 15:28 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2024-12-12 15:35 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 17:08 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 17:52 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
@ 2024-12-12 18:03 ` Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-13 00:40 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-13 00:55 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2025-01-13 01:37 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
0 siblings, 3 replies; 26+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2024-12-12 18:03 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; +Cc: Peter Smith <[email protected]>; Michael Paquier <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, 12 Dec 2024, at 17:52, Andrew Dunstan wrote:
> On 2024-12-12 Th 12:08 PM, Dagfinn Ilmari Mannsåker wrote:
>>
>> command_ok(
>> [
>> 'pg_dump',
>> ('--schema', 'pg_catalog'),
>> ('-f', $outputdir . '/catalogs_primary.dump'),
>> '--no-sync',
>> ('-p', $node_primary->port),
>> '--no-unlogged-table-data',
>> 'regression'
>> ],
>> 'dump catalogs of primary server');
>>
>> I think the fat comma is much nicer than this, so I'd like to convert
>> these too (and replace some of the concatenations with interpolation).
>>
>> Technically the quotes aren't necessary around single-dash options
>> before => since unary minus works on strings as well as numbers, but
>> I'll leave them in for consistency.
>
> I'd rather get rid of those and just use the long options.
Yeah, that is more self-documenting, so I'll do that while I'm at it.
--
- ilmari
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 15:28 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2024-12-12 15:35 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 17:08 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 17:52 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2024-12-12 18:03 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
@ 2024-12-13 00:40 ` Michael Paquier <[email protected]>
2 siblings, 0 replies; 26+ messages in thread
From: Michael Paquier @ 2024-12-13 00:40 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; Peter Smith <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Dec 12, 2024 at 06:03:33PM +0000, Dagfinn Ilmari Mannsåker wrote:
> On Thu, 12 Dec 2024, at 17:52, Andrew Dunstan wrote:
>> I'd rather get rid of those and just use the long options.
>
> Yeah, that is more self-documenting, so I'll do that while I'm at it.
Agreed. I tend to prefer long options too. That's less mapping to
think about the intentions behind some tests when reading their code.
The idea of using fat commas is neat, Ilmari. I didn't suspect that
this one was possible to trick perltidy.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 15:28 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2024-12-12 15:35 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 17:08 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 17:52 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2024-12-12 18:03 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
@ 2024-12-13 00:55 ` Peter Smith <[email protected]>
2 siblings, 0 replies; 26+ messages in thread
From: Peter Smith @ 2024-12-13 00:55 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Dec 13, 2024 at 5:03 AM Dagfinn Ilmari Mannsåker
<[email protected]> wrote:
>
> On Thu, 12 Dec 2024, at 17:52, Andrew Dunstan wrote:
> > On 2024-12-12 Th 12:08 PM, Dagfinn Ilmari Mannsåker wrote:
> >>
> >> command_ok(
> >> [
> >> 'pg_dump',
> >> ('--schema', 'pg_catalog'),
> >> ('-f', $outputdir . '/catalogs_primary.dump'),
> >> '--no-sync',
> >> ('-p', $node_primary->port),
> >> '--no-unlogged-table-data',
> >> 'regression'
> >> ],
> >> 'dump catalogs of primary server');
> >>
> >> I think the fat comma is much nicer than this, so I'd like to convert
> >> these too (and replace some of the concatenations with interpolation).
> >>
> >> Technically the quotes aren't necessary around single-dash options
> >> before => since unary minus works on strings as well as numbers, but
> >> I'll leave them in for consistency.
> >
> > I'd rather get rid of those and just use the long options.
>
> Yeah, that is more self-documenting, so I'll do that while I'm at it.
>
> --
Your fat-comma solution is much better than something I could have
come up with. Thanks for taking this on.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 15:28 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2024-12-12 15:35 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 17:08 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 17:52 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2024-12-12 18:03 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
@ 2025-01-13 01:37 ` Peter Smith <[email protected]>
2 siblings, 0 replies; 26+ messages in thread
From: Peter Smith @ 2025-01-13 01:37 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Dec 13, 2024 at 5:03 AM Dagfinn Ilmari Mannsåker
<[email protected]> wrote:
>
> On Thu, 12 Dec 2024, at 17:52, Andrew Dunstan wrote:
> > On 2024-12-12 Th 12:08 PM, Dagfinn Ilmari Mannsåker wrote:
> >>
> >> command_ok(
> >> [
> >> 'pg_dump',
> >> ('--schema', 'pg_catalog'),
> >> ('-f', $outputdir . '/catalogs_primary.dump'),
> >> '--no-sync',
> >> ('-p', $node_primary->port),
> >> '--no-unlogged-table-data',
> >> 'regression'
> >> ],
> >> 'dump catalogs of primary server');
> >>
> >> I think the fat comma is much nicer than this, so I'd like to convert
> >> these too (and replace some of the concatenations with interpolation).
> >>
> >> Technically the quotes aren't necessary around single-dash options
> >> before => since unary minus works on strings as well as numbers, but
> >> I'll leave them in for consistency.
> >
> > I'd rather get rid of those and just use the long options.
>
> Yeah, that is more self-documenting, so I'll do that while I'm at it.
>
Hi. This thread has been silent for 1 month. Just wondering, has any
progress been made on these changes?
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
@ 2025-01-14 07:03 ` Peter Smith <[email protected]>
2025-01-20 20:42 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
1 sibling, 1 reply; 26+ messages in thread
From: Peter Smith @ 2025-01-14 07:03 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: Michael Paquier <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Dec 13, 2024 at 12:17 AM Dagfinn Ilmari Mannsåker
<[email protected]> wrote:
>
> Dagfinn Ilmari Mannsåker <[email protected]> writes:
>
> > Peter Smith <[email protected]> writes:
> >
> >> On Thu, Dec 12, 2024 at 2:53 PM Michael Paquier <[email protected]> wrote:
> >> ...
> >>
> >>> > So, AFAICT I can workaround the perltidy wrapping just by putting all
> >>> > the noarg options at the bottom of the command, then all the
> >>> > option/optarg pairs (ie 2s) will stay together. I can post another
> >>> > patch to do it this way unless you think it is too hacky.
> >>>
> >>> This trick works for me if that makes the long list of option easier
> >>> to read. With two elements of the array perl line, I would just put
> >>> some --dry-run or --verbose at the end of their respective arrays.
> >>> --
> >>> Michael
> >>
> >> Hi Michael.
> >>
> >> Yes, that is the workaround that I was proposing.
> >
> > A better option, IMO, is to use the fat comma (=>) between options and
> > their values. This makes it clear both to humans and perltidy that they
> > belong together, and we can put all the valueless options first without
> > things being rewrapped.
>
> Here's a more thorough patch, that also applies the fat comma treatment
> to other pg_createsubscriber invocations in the same file that don't
> currently happen to be mangled by perltidy. It also adds trailing
> commas to the last item in multi-line command arrays, which is common
> perl style.
>
> - ilmari
>
Hi,
In your v4 patch, there is a fragment (below) that replaces a double
'--verbose' switch with just a single '--verbose'.
As I have only recently learned, the '--verbose'' switch has a
cumulative effect [1], so the original double '--verbose' was probably
deliberate so it should be kept that way.
~~
# Run pg_createsubscriber on node S
command_ok(
[
- 'pg_createsubscriber', '--verbose',
- '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
- '--verbose', '--pgdata',
- $node_s->data_dir, '--publisher-server',
- $node_p->connstr($db1), '--socketdir',
- $node_s->host, '--subscriber-port',
- $node_s->port, '--publication',
- 'pub1', '--publication',
- 'Pub2', '--replication-slot',
- 'replslot1', '--replication-slot',
- 'replslot2', '--database',
- $db1, '--database',
- $db2
+ 'pg_createsubscriber',
+ '--verbose',
+ '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--publication' => 'pub1',
+ '--publication' => 'pub2',
+ '--replication-slot' => 'replslot1',
+ '--replication-slot' => 'replslot2',
+ '--database' => $db1,
+ '--database' => $db2,
],
'run pg_createsubscriber on node S');
======
[1] https://www.postgresql.org/docs/devel/app-pgcreatesubscriber.html
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2025-01-14 07:03 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
@ 2025-01-20 20:42 ` Dagfinn Ilmari Mannsåker <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2025-01-20 20:42 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: Michael Paquier <[email protected]>; Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]>; Peter Eisentraut <[email protected]>; Euler Taveira <[email protected]>
Hi Peter,
Peter Smith <[email protected]> writes:
> Hi,
>
> In your v4 patch, there is a fragment (below) that replaces a double
> '--verbose' switch with just a single '--verbose'.
>
> As I have only recently learned, the '--verbose'' switch has a
> cumulative effect [1], so the original double '--verbose' was probably
> deliberate so it should be kept that way.
I was going to say that if it were deliberate, I would have expected the
two `--verbose` swithces to be together, but then I noticed that the
--recovery-timeout switch was added later (commit 04c8634c0c4d), rather
haphazardly between them. Still, I would have expected a comment as to
why this command was being invoked extra-verbosely, but I'll restore it
in the next version.
I've Cc-ed both Peter (the committer) and Euler (the author) in case
they have any insight.
> ~~
>
> # Run pg_createsubscriber on node S
> command_ok(
> [
> - 'pg_createsubscriber', '--verbose',
> - '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
> - '--verbose', '--pgdata',
> - $node_s->data_dir, '--publisher-server',
> - $node_p->connstr($db1), '--socketdir',
> - $node_s->host, '--subscriber-port',
> - $node_s->port, '--publication',
> - 'pub1', '--publication',
> - 'Pub2', '--replication-slot',
> - 'replslot1', '--replication-slot',
> - 'replslot2', '--database',
> - $db1, '--database',
> - $db2
> + 'pg_createsubscriber',
> + '--verbose',
> + '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
> + '--pgdata' => $node_s->data_dir,
> + '--publisher-server' => $node_p->connstr($db1),
> + '--socketdir' => $node_s->host,
> + '--subscriber-port' => $node_s->port,
> + '--publication' => 'pub1',
> + '--publication' => 'pub2',
> + '--replication-slot' => 'replslot1',
> + '--replication-slot' => 'replslot2',
> + '--database' => $db1,
> + '--database' => $db2,
> ],
> 'run pg_createsubscriber on node S');
>
>
> ======
> [1] https://www.postgresql.org/docs/devel/app-pgcreatesubscriber.html
>
> Kind Regards,
> Peter Smith.
> Fujitsu Australia
^ permalink raw reply [nested|flat] 26+ messages in thread
end of thread, other threads:[~2025-01-20 20:42 UTC | newest]
Thread overview: 26+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-05-26 07:58 [PATCH v14 1/2] Pgbench errors: use the Variables structure for client variables Yugo Nagata <[email protected]>
2021-05-26 07:58 [PATCH v16 1/2] Pgbench errors: use the Variables structure for client variables Yugo Nagata <[email protected]>
2021-05-26 07:58 [PATCH v17 1/2] Pgbench errors: use the Variables structure for client variables Yugo Nagata <[email protected]>
2021-05-26 07:58 [PATCH v18 1/2] Pgbench errors: use the Variables structure for client variables Yugo Nagata <[email protected]>
2021-05-26 07:58 [PATCH v15 1/2] Pgbench errors: use the Variables structure for client variables Yugo Nagata <[email protected]>
2021-05-26 07:58 [PATCH v13 1/2] Pgbench errors: use the Variables structure for client variables Yugo Nagata <[email protected]>
2021-05-26 07:58 [PATCH v12 1/2] Pgbench errors: use the Variables structure for client variables Yugo Nagata <[email protected]>
2024-12-12 02:32 Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 02:46 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 03:13 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 03:53 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 04:24 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2024-12-12 05:02 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-12 10:02 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Alvaro Herrera <[email protected]>
2024-12-12 12:04 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 13:17 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 15:28 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2024-12-12 15:35 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Tom Lane <[email protected]>
2024-12-12 17:08 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-12 17:52 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Andrew Dunstan <[email protected]>
2024-12-12 18:03 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
2024-12-13 00:40 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Michael Paquier <[email protected]>
2024-12-13 00:55 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2025-01-13 01:37 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2025-01-14 07:03 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Peter Smith <[email protected]>
2025-01-20 20:42 ` Re: pg_createsubscriber TAP test wrapping makes command options hard to read. Dagfinn Ilmari Mannsåker <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox