Обсуждение: Rethinking the parameter access hooks for plpgsql's benefit
We already knew that plpgsql's setup_param_list() is a bit of a
performance hog. Trying to improve that was the primary motivation
behind commit f4e031c662a6b600b786c4849968a099c58fcce7 (the
bms_next_member business), though that was just micro-optimization
rather than any fundamental rethink of how things are done.
Some benchmarking at Salesforce has shown that there's a really
significant penalty in plpgsql functions that have a lot of local
variables, because large "ndatums" increases the size of the
ParamListInfo array that has to be allocated (and zeroed!) for
each expression invocation.
Thinking about this, it struck me that the real problem is that
the parameter list APIs are still too stuck in the last century,
in that they're optimized for static lists of parameters which
is not what plpgsql needs at all. In fact, if we redefined the
usage of ParamFetchHook so that it would be called on every parameter
fetch, plpgsql could be perfectly happy with just *one* ParamListInfo
slot that it would re-fill each time. This would entirely eliminate
the need for any per-expression-execution ParamListInfo allocation,
because a function could just use one single-entry ParamListInfo array
for all purposes.
The attached proposed patch represents an exploration of this idea.
I've also attached a SQL script with some simple functions for testing
its performance. The first one is for exploring what happens with more
or fewer local variables. I compared current HEAD against the patch
with asserts off, for a call like "SELECT timingcheck(10000000);".
It looks like this (times in ms):
HEAD patch
no extra variables 8695 8390
10 extra variables 9218 8419
30 extra variables 9424 8255
100 extra variables 9433 8202
These times are only repeatable to within a few percent, but they do show
that there is a gain rather than loss of performance even in the base
case, and that the patched code's performance is pretty much independent
of the number of local variables whereas HEAD isn't.
The case where the patch potentially loses is where a single expression
contains multiple references to the same plpgsql variable, since with
HEAD, additional references to a variable don't incur any extra trips
through the ParamFetchHook, whereas with the patch they do. I set up
the reusecheck10() and reusecheck5() functions to see how bad that is.
HEAD patch
5 uses of same variable 4105 3962
10 uses of same variable 5738 6076
So somewhere between 5 and 10 uses of the same variable in a single
expression, you start to lose.
I claim that that's a very unlikely scenario and so this patch should
be an unconditional win for just about everybody.
The patch does create an API break for any code that is using
ParamFetchHooks, but it's an easy fix (just return the address of
the array element you stored data into), and any decent C compiler
will complain about the function type mismatch so the API break
should not go unnoticed.
Objections? Even better ideas?
regards, tom lane
diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c
index 1c8be25..1504c92 100644
*** a/src/backend/executor/execCurrent.c
--- b/src/backend/executor/execCurrent.c
*************** fetch_cursor_param_value(ExprContext *ec
*** 216,226 ****
if (paramInfo &&
paramId > 0 && paramId <= paramInfo->numParams)
{
! ParamExternData *prm = ¶mInfo->params[paramId - 1];
/* give hook a chance in case parameter is dynamic */
! if (!OidIsValid(prm->ptype) && paramInfo->paramFetch != NULL)
! (*paramInfo->paramFetch) (paramInfo, paramId);
if (OidIsValid(prm->ptype) && !prm->isnull)
{
--- 216,228 ----
if (paramInfo &&
paramId > 0 && paramId <= paramInfo->numParams)
{
! ParamExternData *prm;
/* give hook a chance in case parameter is dynamic */
! if (paramInfo->paramFetch != NULL)
! prm = (*paramInfo->paramFetch) (paramInfo, paramId, 0);
! else
! prm = ¶mInfo->params[paramId - 1];
if (OidIsValid(prm->ptype) && !prm->isnull)
{
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index d94fe58..f558d40 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
*************** ExecEvalParamExtern(ExprState *exprstate
*** 1137,1147 ****
if (paramInfo &&
thisParamId > 0 && thisParamId <= paramInfo->numParams)
{
! ParamExternData *prm = ¶mInfo->params[thisParamId - 1];
/* give hook a chance in case parameter is dynamic */
! if (!OidIsValid(prm->ptype) && paramInfo->paramFetch != NULL)
! (*paramInfo->paramFetch) (paramInfo, thisParamId);
if (OidIsValid(prm->ptype))
{
--- 1137,1149 ----
if (paramInfo &&
thisParamId > 0 && thisParamId <= paramInfo->numParams)
{
! ParamExternData *prm;
/* give hook a chance in case parameter is dynamic */
! if (paramInfo->paramFetch != NULL)
! prm = (*paramInfo->paramFetch) (paramInfo, thisParamId, 0);
! else
! prm = ¶mInfo->params[thisParamId - 1];
if (OidIsValid(prm->ptype))
{
diff --git a/src/backend/nodes/params.c b/src/backend/nodes/params.c
index fb803f8..b4916d8 100644
*** a/src/backend/nodes/params.c
--- b/src/backend/nodes/params.c
*************** copyParamList(ParamListInfo from)
*** 52,65 ****
for (i = 0; i < from->numParams; i++)
{
! ParamExternData *oprm = &from->params[i];
ParamExternData *nprm = &retval->params[i];
int16 typLen;
bool typByVal;
/* give hook a chance in case parameter is dynamic */
! if (!OidIsValid(oprm->ptype) && from->paramFetch != NULL)
! (*from->paramFetch) (from, i + 1);
/* flat-copy the parameter info */
*nprm = *oprm;
--- 52,67 ----
for (i = 0; i < from->numParams; i++)
{
! ParamExternData *oprm;
ParamExternData *nprm = &retval->params[i];
int16 typLen;
bool typByVal;
/* give hook a chance in case parameter is dynamic */
! if (from->paramFetch != NULL)
! oprm = (*from->paramFetch) (from, i + 1, PARAM_CHECK_VALID);
! else
! oprm = &from->params[i];
/* flat-copy the parameter info */
*nprm = *oprm;
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 84d58ae..c0e086b 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** eval_const_expressions_mutator(Node *nod
*** 2343,2356 ****
case T_Param:
{
Param *param = (Param *) node;
/* Look to see if we've been given a value for this Param */
if (param->paramkind == PARAM_EXTERN &&
! context->boundParams != NULL &&
! param->paramid > 0 &&
! param->paramid <= context->boundParams->numParams)
{
! ParamExternData *prm = &context->boundParams->params[param->paramid - 1];
if (OidIsValid(prm->ptype))
{
--- 2343,2365 ----
case T_Param:
{
Param *param = (Param *) node;
+ int thisParamId = param->paramid;
+ ParamListInfo paramInfo = context->boundParams;
/* Look to see if we've been given a value for this Param */
if (param->paramkind == PARAM_EXTERN &&
! paramInfo != NULL &&
! thisParamId > 0 &&
! thisParamId <= paramInfo->numParams)
{
! ParamExternData *prm;
!
! /* give hook a chance in case parameter is dynamic */
! if (paramInfo->paramFetch != NULL)
! prm = (*paramInfo->paramFetch) (paramInfo, thisParamId,
! PARAM_CHECK_SAFE);
! else
! prm = ¶mInfo->params[thisParamId - 1];
if (OidIsValid(prm->ptype))
{
diff --git a/src/include/nodes/params.h b/src/include/nodes/params.h
index a0f7dd0..268876a 100644
*** a/src/include/nodes/params.h
--- b/src/include/nodes/params.h
*************** struct ParseState;
*** 36,53 ****
*
* There are two hook functions that can be associated with a ParamListInfo
* array to support dynamic parameter handling. First, if paramFetch
! * isn't null and the executor requires a value for an invalid parameter
! * (one with ptype == InvalidOid), the paramFetch hook is called to give
! * it a chance to fill in the parameter value. Second, a parserSetup
! * hook can be supplied to re-instantiate the original parsing hooks if
! * a query needs to be re-parsed/planned (as a substitute for supposing
! * that the current ptype values represent a fixed set of parameter types).
!
* Although the data structure is really an array, not a list, we keep
* the old typedef name to avoid unnecessary code changes.
* ----------------
*/
#define PARAM_FLAG_CONST 0x0001 /* parameter is constant */
typedef struct ParamExternData
--- 36,63 ----
*
* There are two hook functions that can be associated with a ParamListInfo
* array to support dynamic parameter handling. First, if paramFetch
! * isn't null, the paramFetch hook is called whenever a parameter value
! * is required, to give it a chance to fill in the parameter value.
! * (If the hook can't supply a value, it should set ptype to InvalidOid to
! * specify that the parameter is missing, rather than merely being null.)
! * Second, a parserSetup hook can be supplied to re-instantiate the
! * original parsing hooks if a query needs to be re-parsed/planned
! * (otherwise, we assume that the ptype values represent a fixed
! * set of parameter types).
! *
! * Note that if the paramFetch hook is set, the params[] array is
! * really entirely virtual and can be of any length, since the hook
! * has full control of which entry is accessed. (As an example,
! * PL/pgSQL uses only a single params[] entry that is multiplexed
! * among all parameters.) However, numParams must still be valid
! * as it defines the largest possibly-valid parameter number.
! *
* Although the data structure is really an array, not a list, we keep
* the old typedef name to avoid unnecessary code changes.
* ----------------
*/
+ /* flag bits in ParamExternData.pflags: */
#define PARAM_FLAG_CONST 0x0001 /* parameter is constant */
typedef struct ParamExternData
*************** typedef struct ParamExternData
*** 60,66 ****
typedef struct ParamListInfoData *ParamListInfo;
! typedef void (*ParamFetchHook) (ParamListInfo params, int paramid);
typedef void (*ParserSetupHook) (struct ParseState *pstate, void *arg);
--- 70,81 ----
typedef struct ParamListInfoData *ParamListInfo;
! /* flag bits for ParamFetchHook's flags argument: */
! #define PARAM_CHECK_VALID 0x0001 /* check paramid validity */
! #define PARAM_CHECK_SAFE 0x0002 /* skip evals that might fail */
!
! typedef ParamExternData *(*ParamFetchHook) (ParamListInfo params, int paramid,
! int flags);
typedef void (*ParserSetupHook) (struct ParseState *pstate, void *arg);
*************** typedef struct ParamListInfoData
*** 70,76 ****
void *paramFetchArg;
ParserSetupHook parserSetup; /* parser setup hook */
void *parserSetupArg;
! int numParams; /* number of ParamExternDatas following */
ParamExternData params[FLEXIBLE_ARRAY_MEMBER];
} ParamListInfoData;
--- 85,92 ----
void *paramFetchArg;
ParserSetupHook parserSetup; /* parser setup hook */
void *parserSetupArg;
! int numParams; /* maximum allowable Param number */
! /* typically, params[] is of length numParams */
ParamExternData params[FLEXIBLE_ARRAY_MEMBER];
} ParamListInfoData;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index f24f55a..5b9fe37 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** static int exec_for_query(PLpgSQL_execst
*** 211,217 ****
Portal portal, bool prefetch_ok);
static ParamListInfo setup_param_list(PLpgSQL_execstate *estate,
PLpgSQL_expr *expr);
! static void plpgsql_param_fetch(ParamListInfo params, int paramid);
static void exec_move_row(PLpgSQL_execstate *estate,
PLpgSQL_rec *rec,
PLpgSQL_row *row,
--- 211,218 ----
Portal portal, bool prefetch_ok);
static ParamListInfo setup_param_list(PLpgSQL_execstate *estate,
PLpgSQL_expr *expr);
! static ParamExternData *plpgsql_param_fetch(ParamListInfo params, int paramid,
! int flags);
static void exec_move_row(PLpgSQL_execstate *estate,
PLpgSQL_rec *rec,
PLpgSQL_row *row,
*************** exec_stmt_forc(PLpgSQL_execstate *estate
*** 2197,2206 ****
elog(ERROR, "could not open cursor: %s",
SPI_result_code_string(SPI_result));
- /* don't need paramlist any more */
- if (paramLI)
- pfree(paramLI);
-
/*
* If cursor variable was NULL, store the generated portal name in it
*/
--- 2198,2203 ----
*************** plpgsql_estate_setup(PLpgSQL_execstate *
*** 3169,3174 ****
--- 3166,3186 ----
estate->datums = palloc(sizeof(PLpgSQL_datum *) * estate->ndatums);
/* caller is expected to fill the datums array */
+ /*
+ * Create ParamListInfo with one entry (we only need one because
+ * plpgsql_param_fetch() funnels all requests through entry zero). This
+ * is shared by all SPI invocations from this estate. We would just make
+ * this a static part of the estate struct, except that doesn't work for
+ * structs containing flexible arrays.
+ */
+ estate->paramLI = (ParamListInfo)
+ palloc0(offsetof(ParamListInfoData, params) + sizeof(ParamExternData));
+ estate->paramLI->paramFetch = plpgsql_param_fetch;
+ estate->paramLI->paramFetchArg = (void *) estate;
+ estate->paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
+ estate->paramLI->parserSetupArg = NULL; /* filled during use */
+ estate->paramLI->numParams = estate->ndatums;
+
/* set up for use of appropriate simple-expression EState */
if (simple_eval_estate)
estate->simple_eval_estate = simple_eval_estate;
*************** plpgsql_estate_setup(PLpgSQL_execstate *
*** 3179,3185 ****
estate->eval_processed = 0;
estate->eval_lastoid = InvalidOid;
estate->eval_econtext = NULL;
- estate->cur_expr = NULL;
estate->err_stmt = NULL;
estate->err_text = NULL;
--- 3191,3196 ----
*************** exec_stmt_execsql(PLpgSQL_execstate *est
*** 3495,3503 ****
(rc == SPI_OK_SELECT) ? errhint("If you want to discard the results of a SELECT, use PERFORM
instead."): 0));
}
- if (paramLI)
- pfree(paramLI);
-
return PLPGSQL_RC_OK;
}
--- 3506,3511 ----
*************** exec_stmt_open(PLpgSQL_execstate *estate
*** 3864,3871 ****
if (curname)
pfree(curname);
- if (paramLI)
- pfree(paramLI);
return PLPGSQL_RC_OK;
}
--- 3872,3877 ----
*************** exec_run_select(PLpgSQL_execstate *estat
*** 4909,4916 ****
if (*portalP == NULL)
elog(ERROR, "could not open implicit cursor for query \"%s\": %s",
expr->query, SPI_result_code_string(SPI_result));
- if (paramLI)
- pfree(paramLI);
return SPI_OK_CURSOR;
}
--- 4915,4920 ----
*************** exec_run_select(PLpgSQL_execstate *estat
*** 4930,4938 ****
estate->eval_processed = SPI_processed;
estate->eval_lastoid = SPI_lastoid;
- if (paramLI)
- pfree(paramLI);
-
return rc;
}
--- 4934,4939 ----
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5140,5146 ****
LocalTransactionId curlxid = MyProc->lxid;
CachedPlan *cplan;
ParamListInfo paramLI;
- PLpgSQL_expr *save_cur_expr;
MemoryContext oldcontext;
/*
--- 5141,5146 ----
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5216,5230 ****
}
/*
! * Create the param list in econtext's temporary memory context. We won't
! * need to free it explicitly, since it will go away at the next reset of
! * that context.
! *
! * Just for paranoia's sake, save and restore the prior value of
! * estate->cur_expr, which setup_param_list() sets.
*/
- save_cur_expr = estate->cur_expr;
-
paramLI = setup_param_list(estate, expr);
econtext->ecxt_param_list_info = paramLI;
--- 5216,5223 ----
}
/*
! * Set up param list.
*/
paramLI = setup_param_list(estate, expr);
econtext->ecxt_param_list_info = paramLI;
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5244,5251 ****
/* Assorted cleanup */
expr->expr_simple_in_use = false;
- estate->cur_expr = save_cur_expr;
-
if (!estate->readonly_func)
PopActiveSnapshot();
--- 5237,5242 ----
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5266,5285 ****
/*
! * Create a ParamListInfo to pass to SPI
! *
! * We fill in the values for any expression parameters that are plain
! * PLpgSQL_var datums; these are cheap and safe to evaluate, and by setting
! * them with PARAM_FLAG_CONST flags, we allow the planner to use those values
! * in custom plans. However, parameters that are not plain PLpgSQL_vars
! * should not be evaluated here, because they could throw errors (for example
! * "no such record field") and we do not want that to happen in a part of
! * the expression that might never be evaluated at runtime. To handle those
! * parameters, we set up a paramFetch hook for the executor to call when it
! * wants a not-presupplied value.
! *
! * The result is a locally palloc'd array that should be pfree'd after use;
! * but note it can be NULL.
*/
static ParamListInfo
setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
--- 5257,5263 ----
/*
! * Set up a ParamListInfo to pass to SPI for this expression
*/
static ParamListInfo
setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
*************** setup_param_list(PLpgSQL_execstate *esta
*** 5292,5344 ****
*/
Assert(expr->plan != NULL);
! /*
! * Could we re-use these arrays instead of palloc'ing a new one each time?
! * However, we'd have to re-fill the array each time anyway, since new
! * values might have been assigned to the variables.
! */
if (!bms_is_empty(expr->paramnos))
{
! int dno;
! paramLI = (ParamListInfo)
! palloc0(offsetof(ParamListInfoData, params) +
! estate->ndatums * sizeof(ParamExternData));
! paramLI->paramFetch = plpgsql_param_fetch;
! paramLI->paramFetchArg = (void *) estate;
! paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
paramLI->parserSetupArg = (void *) expr;
- paramLI->numParams = estate->ndatums;
-
- /* Instantiate values for "safe" parameters of the expression */
- dno = -1;
- while ((dno = bms_next_member(expr->paramnos, dno)) >= 0)
- {
- PLpgSQL_datum *datum = estate->datums[dno];
-
- if (datum->dtype == PLPGSQL_DTYPE_VAR)
- {
- PLpgSQL_var *var = (PLpgSQL_var *) datum;
- ParamExternData *prm = ¶mLI->params[dno];
-
- prm->value = var->value;
- prm->isnull = var->isnull;
- prm->pflags = PARAM_FLAG_CONST;
- prm->ptype = var->datatype->typoid;
- }
- }
/*
! * Set up link to active expr where the hook functions can find it.
! * Callers must save and restore cur_expr if there is any chance that
! * they are interrupting an active use of parameters.
! */
! estate->cur_expr = expr;
!
! /*
! * Also make sure this is set before parser hooks need it. There is
! * no need to save and restore, since the value is always correct once
! * set. (Should be set already, but let's be sure.)
*/
expr->func = estate->func;
}
--- 5270,5287 ----
*/
Assert(expr->plan != NULL);
! /* We only need a ParamListInfo if the expression has parameters */
if (!bms_is_empty(expr->paramnos))
{
! /* Use the common ParamListInfo for all evals in this estate */
! paramLI = estate->paramLI;
! /* Make sure it points to this expr while we use it */
paramLI->parserSetupArg = (void *) expr;
/*
! * Make sure this is set before parser hooks need it. (Should be set
! * already, but let's be sure.)
*/
expr->func = estate->func;
}
*************** setup_param_list(PLpgSQL_execstate *esta
*** 5357,5370 ****
/*
* plpgsql_param_fetch paramFetch callback for dynamic parameter fetch
*/
! static void
! plpgsql_param_fetch(ParamListInfo params, int paramid)
{
int dno;
PLpgSQL_execstate *estate;
PLpgSQL_expr *expr;
PLpgSQL_datum *datum;
- ParamExternData *prm;
int32 prmtypmod;
/* paramid's are 1-based, but dnos are 0-based */
--- 5300,5313 ----
/*
* plpgsql_param_fetch paramFetch callback for dynamic parameter fetch
*/
! static ParamExternData *
! plpgsql_param_fetch(ParamListInfo params, int paramid, int flags)
{
+ ParamExternData *prm = ¶ms->params[0];
int dno;
PLpgSQL_execstate *estate;
PLpgSQL_expr *expr;
PLpgSQL_datum *datum;
int32 prmtypmod;
/* paramid's are 1-based, but dnos are 0-based */
*************** plpgsql_param_fetch(ParamListInfo params
*** 5373,5395 ****
/* fetch back the hook data */
estate = (PLpgSQL_execstate *) params->paramFetchArg;
! expr = estate->cur_expr;
Assert(params->numParams == estate->ndatums);
/*
! * Do nothing if asked for a value that's not supposed to be used by this
! * SQL expression. This avoids unwanted evaluations when functions such
! * as copyParamList try to materialize all the values.
*/
! if (!bms_is_member(dno, expr->paramnos))
! return;
! /* OK, evaluate the value and store into the appropriate paramlist slot */
! datum = estate->datums[dno];
! prm = ¶ms->params[dno];
exec_eval_datum(estate, datum,
&prm->ptype, &prmtypmod,
&prm->value, &prm->isnull);
}
--- 5316,5361 ----
/* fetch back the hook data */
estate = (PLpgSQL_execstate *) params->paramFetchArg;
! expr = (PLpgSQL_expr *) params->parserSetupArg;
Assert(params->numParams == estate->ndatums);
+ /* now we can look up the datum */
+ datum = estate->datums[dno];
+
/*
! * We return null/invalid in two cases:
! *
! * 1. If PARAM_CHECK_VALID is set and we're asked for a value that's not
! * supposed to be used by this SQL expression. This avoids unwanted
! * evaluations when functions such as copyParamList() try to materialize
! * all the parameter slots.
! *
! * 2. If PARAM_CHECK_SAFE is set and we're not sure evaluation will
! * succeed; the caller would rather we not return a value than cause an
! * unexpected error.
*/
! if (flags)
! {
! if (((flags & PARAM_CHECK_VALID) &&
! !bms_is_member(dno, expr->paramnos)) ||
! ((flags & PARAM_CHECK_SAFE) &&
! datum->dtype != PLPGSQL_DTYPE_VAR))
! {
! prm->value = (Datum) 0;
! prm->isnull = true;
! prm->pflags = 0;
! prm->ptype = InvalidOid;
! return prm;
! }
! }
! /* OK, evaluate the value and store into the single paramlist slot */
exec_eval_datum(estate, datum,
&prm->ptype, &prmtypmod,
&prm->value, &prm->isnull);
+ /* If we're being called by the planner, OK to treat value as constant */
+ prm->pflags = PARAM_FLAG_CONST;
+ return prm;
}
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 4ec4628..032f283 100644
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
*************** typedef struct PLpgSQL_execstate
*** 784,789 ****
--- 784,792 ----
int ndatums;
PLpgSQL_datum **datums;
+ /* Struct for passing datum values to executor */
+ ParamListInfo paramLI;
+
/* EState to use for "simple" expression evaluation */
EState *simple_eval_estate;
*************** typedef struct PLpgSQL_execstate
*** 792,798 ****
uint32 eval_processed;
Oid eval_lastoid;
ExprContext *eval_econtext; /* for executing simple expressions */
- PLpgSQL_expr *cur_expr; /* current query/expr being evaluated */
/* status information for error context reporting */
PLpgSQL_stmt *err_stmt; /* current stmt */
--- 795,800 ----
create or replace function timingcheck(n int) returns bigint as $$
declare arr int[] := '{}';
res bigint := 0;
dummyvar0 int := 0;
dummyvar1 int := 0;
dummyvar2 int := 0;
dummyvar3 int := 0;
dummyvar4 int := 0;
dummyvar5 int := 0;
dummyvar6 int := 0;
dummyvar7 int := 0;
dummyvar8 int := 0;
dummyvar9 int := 0;
/*
dummyvar10 int := 0;
dummyvar11 int := 0;
dummyvar12 int := 0;
dummyvar13 int := 0;
dummyvar14 int := 0;
dummyvar15 int := 0;
dummyvar16 int := 0;
dummyvar17 int := 0;
dummyvar18 int := 0;
dummyvar19 int := 0;
dummyvar20 int := 0;
dummyvar21 int := 0;
dummyvar22 int := 0;
dummyvar23 int := 0;
dummyvar24 int := 0;
dummyvar25 int := 0;
dummyvar26 int := 0;
dummyvar27 int := 0;
dummyvar28 int := 0;
dummyvar29 int := 0;
dummyvar30 int := 0;
dummyvar31 int := 0;
dummyvar32 int := 0;
dummyvar33 int := 0;
dummyvar34 int := 0;
dummyvar35 int := 0;
dummyvar36 int := 0;
dummyvar37 int := 0;
dummyvar38 int := 0;
dummyvar39 int := 0;
dummyvar40 int := 0;
dummyvar41 int := 0;
dummyvar42 int := 0;
dummyvar43 int := 0;
dummyvar44 int := 0;
dummyvar45 int := 0;
dummyvar46 int := 0;
dummyvar47 int := 0;
dummyvar48 int := 0;
dummyvar49 int := 0;
dummyvar50 int := 0;
dummyvar51 int := 0;
dummyvar52 int := 0;
dummyvar53 int := 0;
dummyvar54 int := 0;
dummyvar55 int := 0;
dummyvar56 int := 0;
dummyvar57 int := 0;
dummyvar58 int := 0;
dummyvar59 int := 0;
dummyvar60 int := 0;
dummyvar61 int := 0;
dummyvar62 int := 0;
dummyvar63 int := 0;
dummyvar64 int := 0;
dummyvar65 int := 0;
dummyvar66 int := 0;
dummyvar67 int := 0;
dummyvar68 int := 0;
dummyvar69 int := 0;
dummyvar70 int := 0;
dummyvar71 int := 0;
dummyvar72 int := 0;
dummyvar73 int := 0;
dummyvar74 int := 0;
dummyvar75 int := 0;
dummyvar76 int := 0;
dummyvar77 int := 0;
dummyvar78 int := 0;
dummyvar79 int := 0;
dummyvar80 int := 0;
dummyvar81 int := 0;
dummyvar82 int := 0;
dummyvar83 int := 0;
dummyvar84 int := 0;
dummyvar85 int := 0;
dummyvar86 int := 0;
dummyvar87 int := 0;
dummyvar88 int := 0;
dummyvar89 int := 0;
dummyvar90 int := 0;
dummyvar91 int := 0;
dummyvar92 int := 0;
dummyvar93 int := 0;
dummyvar94 int := 0;
dummyvar95 int := 0;
dummyvar96 int := 0;
dummyvar97 int := 0;
dummyvar98 int := 0;
dummyvar99 int := 0;
*/
begin
for i in 1 .. n loop
arr[10] := i;
res := res + arr[10];
end loop;
return res;
end
$$ language plpgsql strict stable;
create or replace function reusecheck10(n int) returns bigint as $$
declare res bigint := 0;
begin
for i in 1 .. n loop
res := res + i + i + i + i + i + i + i + i + i + i;
end loop;
return res;
end
$$ language plpgsql strict stable;
create or replace function reusecheck5(n int) returns bigint as $$
declare res bigint := 0;
begin
for i in 1 .. n loop
res := res + i + i + i + i + i;
end loop;
return res;
end
$$ language plpgsql strict stable;
On Sun, Mar 8, 2015 at 11:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Objections? Even better ideas? I object on the grounds that we're three weeks past the deadline for the last CommitFest, and that we should be trying to get committed those patches that were submitted on time, not writing new ones that will further increase the amount of reviewing that needs to be done before we can get to beta, and perhaps the bug count of that eventually when it arrives. In particular, I think that the fact that you haven't made more of an effort to give the GROUPING SETS patch a more detailed review is quite unfair to Andrew Gierth. But regardless of that, this is untimely and should be pushed to 9.6. On the technical side, I do agree that the requirement to allocate and zero an array for every new simple expression is unfortunate, but I'm not convinced that repeatedly invoking the hook-function is a good way to solve that problem. Calling the hook-function figures to be significantly more expensive than an array-fetch, so you can almost think of the ParamListInfo entries themselves as a cache of data retrieved from the hook function. In that view of the world, the problem here is that initializing the cache is too expensive. Your solution to that is to rip the cache out completely, but what would be better is keep the cache and make initializing it cheaper - e.g. by sharing one ParamListInfo across all expressions in the same scope; or by not zeroing the memory and instead tracking the the first N slots are the only ones we've initialized; or $your_idea_here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 03/09/2015 09:11 AM, Robert Haas wrote: > > On Sun, Mar 8, 2015 at 11:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Objections? Even better ideas? > > I object on the grounds that we're three weeks past the deadline for > the last CommitFest, and that we should be trying to get committed > those patches that were submitted on time, not writing new ones that > will further increase the amount of reviewing that needs to be done > before we can get to beta, and perhaps the bug count of that > eventually when it arrives. In particular, I think that the fact that > you haven't made more of an effort to give the GROUPING SETS patch a > more detailed review is quite unfair to Andrew Gierth. But regardless > of that, this is untimely and should be pushed to 9.6. From the reading the original post it seems like the patch was developed on Sales Force's time, not TGLs. I do not think we get to have an opinion on that. Thank you Tom for the efforts you made here, making the largest used PL better, faster, stronger is exactly the type of micro-changes we need to be looking at for further polish within the database. JD -- Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, @cmdpromptinc Now I get it: your service is designed for a customer base that grew up with Facebook, watches Japanese seizure robot anime, and has the attention span of a gnat. I'm not that user., "Tyler Riddle"
On Mon, Mar 9, 2015 at 12:26 PM, Joshua D. Drake <jd@commandprompt.com> wrote: > From the reading the original post it seems like the patch was developed on > Sales Force's time, not TGLs. I do not think we get to have an opinion on > that. Salesforce gets to develop their patches whenever they want, but they don't get to control the community process for getting those patches committed to PostgreSQL. > Thank you Tom for the efforts you made here, making the largest used PL > better, faster, stronger is exactly the type of micro-changes we need to be > looking at for further polish within the database. So, just to be clear, are you proposing that everybody is exempted from the CommitFest deadlines, or just Tom? Thanks, -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
"Joshua D. Drake" <jd@commandprompt.com> writes:
> On 03/09/2015 09:11 AM, Robert Haas wrote:
>> I object on the grounds that we're three weeks past the deadline for
>> the last CommitFest, and that we should be trying to get committed
>> those patches that were submitted on time, not writing new ones that
>> will further increase the amount of reviewing that needs to be done
>> before we can get to beta, and perhaps the bug count of that
>> eventually when it arrives. In particular, I think that the fact that
>> you haven't made more of an effort to give the GROUPING SETS patch a
>> more detailed review is quite unfair to Andrew Gierth. But regardless
>> of that, this is untimely and should be pushed to 9.6.
> From the reading the original post it seems like the patch was
> developed on Sales Force's time, not TGLs. I do not think we get to have
> an opinion on that.
JD sees the situation correctly: this is $dayjob work, and it's going
to get done now not in four months because I have a deadline to meet.
I would like to push it into the community sources to reduce divergence
between our copy and Salesforce's, but if I'm told it has to wait till
9.6, I may or may not remember to try to do something then.
I will admit that I'm been slacking on commitfest work. This is not
unrelated to the fact that we've been in commitfest mode continuously
since last August. I'm afraid whatever enthusiasm I had for reviewing
other peoples' patches burned out some time ago.
regards, tom lane
> JD sees the situation correctly: this is $dayjob work, and it's going > to get done now not in four months because I have a deadline to meet. > I would like to push it into the community sources to reduce divergence > between our copy and Salesforce's, but if I'm told it has to wait till > 9.6, I may or may not remember to try to do something then. I think most of the committers are pretty much in that situation? > I will admit that I'm been slacking on commitfest work. This is not > unrelated to the fact that we've been in commitfest mode continuously > since last August. I'm afraid whatever enthusiasm I had for reviewing > other peoples' patches burned out some time ago. I can certainly relate to that :(. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 03/09/2015 09:29 AM, Robert Haas wrote: > > On Mon, Mar 9, 2015 at 12:26 PM, Joshua D. Drake <jd@commandprompt.com> wrote: >> From the reading the original post it seems like the patch was developed on >> Sales Force's time, not TGLs. I do not think we get to have an opinion on >> that. > > Salesforce gets to develop their patches whenever they want, but they > don't get to control the community process for getting those patches > committed to PostgreSQL. Did I miss the part of the post that said, (speaking as Tom): FYI, I am ignoring the commit fest deadline and committing this? Are we supposed to hold patches when we are post-deadline and not seek feedback? That seems rather hostile. > >> Thank you Tom for the efforts you made here, making the largest used PL >> better, faster, stronger is exactly the type of micro-changes we need to be >> looking at for further polish within the database. > > So, just to be clear, are you proposing that everybody is exempted > from the CommitFest deadlines, or just Tom? I am not suggesting that anyone is exempted from a CommitFest deadline. I am actually rather hard line that we should be keeping them. I think it is ridiculous to post on the bad/good/timing of a patch submission unless there is a case being made that the process isn't actually being followed. I don't see that here. In short, soap boxing does nobody any good. JD -- Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, @cmdpromptinc Now I get it: your service is designed for a customer base that grew up with Facebook, watches Japanese seizure robot anime, and has the attention span of a gnat. I'm not that user., "Tyler Riddle"
* Andres Freund (andres@2ndquadrant.com) wrote:
> > JD sees the situation correctly: this is $dayjob work, and it's going
> > to get done now not in four months because I have a deadline to meet.
> > I would like to push it into the community sources to reduce divergence
> > between our copy and Salesforce's, but if I'm told it has to wait till
> > 9.6, I may or may not remember to try to do something then.
>
> I think most of the committers are pretty much in that situation?
Indeed.. And, further, the commitfest wasn't intended to prevent
committers from working on things, at least as I understood it. Core
sets a feature freeze date which is quite a different thing.
Considering feature freeze is generally after the last commitfest it
also seems counter-intuitive that committers are expected to have
finished all of their work for the next release prior to the start of
the last commitfest.
> > I will admit that I'm been slacking on commitfest work. This is not
> > unrelated to the fact that we've been in commitfest mode continuously
> > since last August. I'm afraid whatever enthusiasm I had for reviewing
> > other peoples' patches burned out some time ago.
>
> I can certainly relate to that :(.
+1.
Thanks,
Stephen
On Mon, Mar 9, 2015 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Joshua D. Drake" <jd@commandprompt.com> writes: >> On 03/09/2015 09:11 AM, Robert Haas wrote: >>> I object on the grounds that we're three weeks past the deadline for >>> the last CommitFest, and that we should be trying to get committed >>> those patches that were submitted on time, not writing new ones that >>> will further increase the amount of reviewing that needs to be done >>> before we can get to beta, and perhaps the bug count of that >>> eventually when it arrives. In particular, I think that the fact that >>> you haven't made more of an effort to give the GROUPING SETS patch a >>> more detailed review is quite unfair to Andrew Gierth. But regardless >>> of that, this is untimely and should be pushed to 9.6. > >> From the reading the original post it seems like the patch was >> developed on Sales Force's time, not TGLs. I do not think we get to have >> an opinion on that. > > JD sees the situation correctly: this is $dayjob work, and it's going > to get done now not in four months because I have a deadline to meet. > I would like to push it into the community sources to reduce divergence > between our copy and Salesforce's, but if I'm told it has to wait till > 9.6, I may or may not remember to try to do something then. Sure, I have things that I've wanted to push into the community sources for the benefit of EnterpriseDB, too. Nobody's offered to let me ignore the community process when it happens to be good for EnterpriseDB. If we're changing that policy for patches submitted by Salesforce employes, I'm afraid I must object unless EnterpriseDB employees will get the same privilege. And I think 2ndQuadrant will want in on it, too. > I will admit that I'm been slacking on commitfest work. This is not > unrelated to the fact that we've been in commitfest mode continuously > since last August. I'm afraid whatever enthusiasm I had for reviewing > other peoples' patches burned out some time ago. Yeah, it would be so much easier to get the release out the door if people didn't keep submitting patches to make the product better. We should really try to discourage that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 9, 2015 at 12:53 PM, Joshua D. Drake <jd@commandprompt.com> wrote: > I think it is ridiculous to post on the bad/good/timing of a patch > submission unless there is a case being made that the process isn't actually > being followed. I don't see that here. The CommitFest deadline was three weeks ago and I think it's abundantly clear that Tom is not submitting this for the June CommitFest. If that wasn't clear from his initial post, his follow-up points have removed any doubt on that point. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-03-09 12:54:44 -0400, Robert Haas wrote: > If we're changing that policy for patches submitted by Salesforce > employes, I'm afraid I must object unless EnterpriseDB employees will > get the same privilege. And I think 2ndQuadrant will want in on it, > too. Right. I'm not really sure how much actual policy there is around this, and how much (differently) perceived policy it is. > > I will admit that I'm been slacking on commitfest work. This is not > > unrelated to the fact that we've been in commitfest mode continuously > > since last August. I'm afraid whatever enthusiasm I had for reviewing > > other peoples' patches burned out some time ago. > > Yeah, it would be so much easier to get the release out the door if > people didn't keep submitting patches to make the product better. We > should really try to discourage that. I don't think that's really the point. There's been awfully little progress on the committing side, and what did happen has been born by few shoulders. Which leaves little time that's officially dedicated to the fun parts. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Mar 9, 2015 at 1:03 PM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2015-03-09 12:54:44 -0400, Robert Haas wrote: >> If we're changing that policy for patches submitted by Salesforce >> employes, I'm afraid I must object unless EnterpriseDB employees will >> get the same privilege. And I think 2ndQuadrant will want in on it, >> too. > > Right. I'm not really sure how much actual policy there is around this, > and how much (differently) perceived policy it is. Yes, that's fair. I do realize that long-time, highly-trusted committers, especially Tom, get more slack than newer committers or, say, first-time patch submitters, and that is fair too. At the same time, at some point we have to cut off 9.5 development. >> > I will admit that I'm been slacking on commitfest work. This is not >> > unrelated to the fact that we've been in commitfest mode continuously >> > since last August. I'm afraid whatever enthusiasm I had for reviewing >> > other peoples' patches burned out some time ago. >> >> Yeah, it would be so much easier to get the release out the door if >> people didn't keep submitting patches to make the product better. We >> should really try to discourage that. > > I don't think that's really the point. There's been awfully little > progress on the committing side, and what did happen has been born by > few shoulders. Which leaves little time that's officially dedicated to > the fun parts. Yeah, sorry, I lost my temper there a bit. Sorry, Tom. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
<p dir="ltr">I don't think Tom, or that matter anyone needs to forgo working on changes at any time. The only effect missinga commitfest deadline means is that other reviewers don't offer any promises to give any feedback on it before thisround of the commitfest is done.<p dir="ltr">We don't have a policy of requiring code reviews before changes are committed-- it's up to the commuters judgement whether the patch is type for committing.<p dir="ltr">There has been the suggestionthat commiters should concentrate on reviews rather than fresh development during commitfests but nobody is goingto do 100% reviewing work for the whole time and nobody could seriously suggest Tom hasn't pulled his weight reviewingpatches.<br />
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Mar 9, 2015 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> JD sees the situation correctly: this is $dayjob work, and it's going
>> to get done now not in four months because I have a deadline to meet.
>> I would like to push it into the community sources to reduce divergence
>> between our copy and Salesforce's, but if I'm told it has to wait till
>> 9.6, I may or may not remember to try to do something then.
> Sure, I have things that I've wanted to push into the community
> sources for the benefit of EnterpriseDB, too. Nobody's offered to let
> me ignore the community process when it happens to be good for
> EnterpriseDB. If we're changing that policy for patches submitted by
> Salesforce employes, I'm afraid I must object unless EnterpriseDB
> employees will get the same privilege. And I think 2ndQuadrant will
> want in on it, too.
As far as that goes, it has never been the case that we expected every
patch to go through the commitfest review process. (If we did, our
response time to bugs would be probably a couple orders of magnitude
longer than it is.) The way I see the policy is that committers have
authority to commit things that they feel are ready and that haven't
been objected to by other developers. Depending on the particular
committer and the particular patch, feeling that a patch is "ready"
might or might not require that it's been through a commitfest review.
There is no process-based restriction on committing "ready" patches
except when we are in alpha/beta/RC states, which is surely months
away for 9.5.
If I were looking for a formal review on this one, I would certainly
not expect that it would get one during the current CF. I wasn't
though.
For context, I have currently got half a dozen irons in the fire:
* "expanded objects" (array performance fixes): needs review, was
submitted timely to current CF
* operator precedence changes: committable IMO, but it's not entirely
clear if we have consensus
* adjust loop count estimates for semijoins: committable IMO, waiting
for objections
* find stats on-the-fly for children of UNION ALL appendrels: WIP, far
from done though
* flatten empty sub-SELECTs and VALUES where feasible: committable IMO,
waiting for objections
* parameter fetch hook rethink: WIP, not as close to done
as I thought last night
I wasn't really planning to use the CF process for any but the first
of these. If we start insisting that committers go through CF for
even rather small patches like these other things, it's going to
destroy our productivity completely.
regards, tom lane
On Mon, Mar 9, 2015 at 2:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > As far as that goes, it has never been the case that we expected every > patch to go through the commitfest review process. (If we did, our > response time to bugs would be probably a couple orders of magnitude > longer than it is.) The way I see the policy is that committers have > authority to commit things that they feel are ready and that haven't > been objected to by other developers. Depending on the particular > committer and the particular patch, feeling that a patch is "ready" > might or might not require that it's been through a commitfest review. > There is no process-based restriction on committing "ready" patches > except when we are in alpha/beta/RC states, which is surely months > away for 9.5. > > If I were looking for a formal review on this one, I would certainly > not expect that it would get one during the current CF. I wasn't > though. Yes, I understand. I don't really have anything more to say about this. Nothing here changes my basic feeling that we need to stop putting new irons in the fire and start working hard on taking irons out of the fire; and I think most if not all other committers are already doing that. Even to the extent that they are focusing on their own patches rather than other people's patches, I think it is mostly patches that they wrote some time ago, not new things that they have only just written. I also will say, in fairness, that I do not really care about this particular patch all that much one way or the other. I am less convinced than you that this will always work out to a win, but when it comes right down to it, you're a pretty smart guy, and if this gets complaints about some scenario you've overlooked, I have confidence you'll get around to repairing it. So whatever. What I do care about is that we as a project have to at some point be willing to begin closing the spigot on new patches and focusing on polishing and shipping the patches we've already got. I think it's perfectly reasonable to worry about where we are on that continuum when it's already several weeks after the start of the last CommitFest, but if I'm in the minority on that, so be it. But it's got to be done at some point, or we will not be able to ship a beta, or a release, on the anticipated timetable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 9, 2015 at 12:06 PM, Robert Haas <robertmhaas@gmail.com> wrote: > Yes, I understand. I don't really have anything more to say about > this. Nothing here changes my basic feeling that we need to stop > putting new irons in the fire and start working hard on taking irons > out of the fire; and I think most if not all other committers are > already doing that. Even to the extent that they are focusing on > their own patches rather than other people's patches, I think it is > mostly patches that they wrote some time ago, not new things that they > have only just written. I must say that I share your concern here. I have no idea what's going to happen with my ON CONFLICT patch, 9.5-wise. I hope that at least the IGNORE variant makes it into 9.5, but I'm not sure that it will. The ON CONFLICT IGNORE/UPDATE patch has existed in essentially the same form since August, although research level work went into that project as long ago as 2012. I'm not blaming anyone, but it's quite discouraging that it has taken so much to get it to where it is now, even though it's something that there is a huge, tangible demand for (few other things are in the same category, and few of those have actually been implemented). An enormous amount of effort went into internals documentation (the Wiki pages), and into stress-testing (Jeff Janes' "double entry bookkeeping" stress test, among others). I hate to say it, but at some point I'll need to cut my loses. To any interested contributor: If there is something that I can do to help to advance the state of the ON CONFLICT UPDATE patch, let me know. Perhaps there can be some useful reciprocal arrangement with review time. That's not ideal, but I see no alternative. -- Peter Geoghegan
On 2015-03-09 13:15:59 -0700, Peter Geoghegan wrote: > I must say that I share your concern here. I have no idea what's going > to happen with my ON CONFLICT patch, 9.5-wise. I hope that at least > the IGNORE variant makes it into 9.5, but I'm not sure that it will. > > The ON CONFLICT IGNORE/UPDATE patch has existed in essentially the > same form since August, although research level work went into that > project as long ago as 2012. I'm not blaming anyone, but it's quite > discouraging that it has taken so much to get it to where it is now, > even though it's something that there is a huge, tangible demand for > (few other things are in the same category, and few of those have > actually been implemented). An enormous amount of effort went into > internals documentation (the Wiki pages), and into stress-testing > (Jeff Janes' "double entry bookkeeping" stress test, among others). I > hate to say it, but at some point I'll need to cut my loses. > > To any interested contributor: If there is something that I can do to > help to advance the state of the ON CONFLICT UPDATE patch, let me > know. Perhaps there can be some useful reciprocal arrangement with > review time. That's not ideal, but I see no alternative. FWIW, I think you actually don't have much reason to complain. This work has probably gotten more attention in total than any other recent patch. Certainly, by far, more than any other in the 9.5 cycle. So far I've not seen a single version that could be considered 'ready for committer'. Even if there's points to be resolved you can make one (presumably yours) solution ready. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Mar 9, 2015 at 4:03 PM, Andres Freund <andres@2ndquadrant.com> wrote: > FWIW, I think you actually don't have much reason to complain. This work > has probably gotten more attention in total than any other recent > patch. Certainly, by far, more than any other in the 9.5 cycle. That has to be true, because the patch has been around in various forms for so long. > So far I've not seen a single version that could be considered 'ready > for committer'. Even if there's points to be resolved you can make one > (presumably yours) solution ready. Of course there isn't, since (for example, hint hint) the logical decoding stuff isn't fully resolved (there is really only one other issue with unique index inference). Those issues are the only two real impediments to at least committing ON CONFLICT IGNORE that I'm aware of, and the former is by far the biggest. My frustration is down to not having anything I can do without feedback on these issues...I've run out of things to do without feedback entirely. It's not for me to decide whether or not I'm justified in complaining about that. I don't think it matters much either way, but as a matter of fact I am feeling very concerned about it. It would be very unfortunate if the UPSERT patch missed the 9.5 release, and for reasons that don't have much to do with me in particular. -- Peter Geoghegan
Robert Haas <robertmhaas@gmail.com> writes:
> On the technical side, I do agree that the requirement to allocate and
> zero an array for every new simple expression is unfortunate, but I'm
> not convinced that repeatedly invoking the hook-function is a good way
> to solve that problem. Calling the hook-function figures to be
> significantly more expensive than an array-fetch, so you can almost
> think of the ParamListInfo entries themselves as a cache of data
> retrieved from the hook function. In that view of the world, the
> problem here is that initializing the cache is too expensive. Your
> solution to that is to rip the cache out completely, but what would be
> better is keep the cache and make initializing it cheaper - e.g. by
> sharing one ParamListInfo across all expressions in the same scope; or
> by not zeroing the memory and instead tracking the the first N slots
> are the only ones we've initialized; or $your_idea_here.
Getting back to the actual merits of this patch --- you're right, it
was not a good idea as proposed. I'd been thinking about the scalar
expression case only, and forgot that the same code is used to pass
parameters to queries, which might evaluate those parameters quite
a large number of times. So any savings from reducing the setup time
is sure to be swamped if the hook-function gets called repeatedly.
Another problem is that for ROW and REC datums, exec_eval_datum()
pallocs memory that won't get freed till the end of the statement,
so repeat evaluation would cause memory leaks. So we really need
evaluate-at-most-once semantics in there.
However, this attempt did confirm that we don't need to create a separate
ParamListInfo struct for each evaluation attempt. So the attached,
greatly stripped-down patch just allocates a ParamListInfo once at
function startup, and then zeroes it each time. This does nothing for
the memset overhead but it does get rid of the palloc traffic, which
seems to be good for a few percent on simple-assignment-type statements.
AFAICS this is an unconditional win compared to HEAD. What's more, it
provides at least a basis for doing better later: if we could think of
a reasonably cheap way of tracking which datums get invalidated by an
assignment, we might be able to reset only selected entries in the array
rather than blindly blowing away all of them.
I propose to apply this and leave it at that for now.
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index f24f55a..0ad32f7 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** exec_stmt_forc(PLpgSQL_execstate *estate
*** 2197,2206 ****
elog(ERROR, "could not open cursor: %s",
SPI_result_code_string(SPI_result));
- /* don't need paramlist any more */
- if (paramLI)
- pfree(paramLI);
-
/*
* If cursor variable was NULL, store the generated portal name in it
*/
--- 2197,2202 ----
*************** plpgsql_estate_setup(PLpgSQL_execstate *
*** 3169,3174 ****
--- 3165,3180 ----
estate->datums = palloc(sizeof(PLpgSQL_datum *) * estate->ndatums);
/* caller is expected to fill the datums array */
+ /* initialize ParamListInfo with one entry per datum, all invalid */
+ estate->paramLI = (ParamListInfo)
+ palloc0(offsetof(ParamListInfoData, params) +
+ estate->ndatums * sizeof(ParamExternData));
+ estate->paramLI->paramFetch = plpgsql_param_fetch;
+ estate->paramLI->paramFetchArg = (void *) estate;
+ estate->paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
+ estate->paramLI->parserSetupArg = NULL; /* filled during use */
+ estate->paramLI->numParams = estate->ndatums;
+
/* set up for use of appropriate simple-expression EState */
if (simple_eval_estate)
estate->simple_eval_estate = simple_eval_estate;
*************** plpgsql_estate_setup(PLpgSQL_execstate *
*** 3179,3185 ****
estate->eval_processed = 0;
estate->eval_lastoid = InvalidOid;
estate->eval_econtext = NULL;
- estate->cur_expr = NULL;
estate->err_stmt = NULL;
estate->err_text = NULL;
--- 3185,3190 ----
*************** exec_stmt_execsql(PLpgSQL_execstate *est
*** 3495,3503 ****
(rc == SPI_OK_SELECT) ? errhint("If you want to discard the results of a SELECT, use PERFORM
instead."): 0));
}
- if (paramLI)
- pfree(paramLI);
-
return PLPGSQL_RC_OK;
}
--- 3500,3505 ----
*************** exec_stmt_open(PLpgSQL_execstate *estate
*** 3864,3871 ****
if (curname)
pfree(curname);
- if (paramLI)
- pfree(paramLI);
return PLPGSQL_RC_OK;
}
--- 3866,3871 ----
*************** exec_assign_c_string(PLpgSQL_execstate *
*** 4050,4056 ****
/* ----------
! * exec_assign_value Put a value into a target field
*
* Note: in some code paths, this will leak memory in the eval_econtext;
* we assume that will be cleaned up later by exec_eval_cleanup. We cannot
--- 4050,4056 ----
/* ----------
! * exec_assign_value Put a value into a target datum
*
* Note: in some code paths, this will leak memory in the eval_econtext;
* we assume that will be cleaned up later by exec_eval_cleanup. We cannot
*************** exec_run_select(PLpgSQL_execstate *estat
*** 4909,4916 ****
if (*portalP == NULL)
elog(ERROR, "could not open implicit cursor for query \"%s\": %s",
expr->query, SPI_result_code_string(SPI_result));
- if (paramLI)
- pfree(paramLI);
return SPI_OK_CURSOR;
}
--- 4909,4914 ----
*************** exec_run_select(PLpgSQL_execstate *estat
*** 4930,4938 ****
estate->eval_processed = SPI_processed;
estate->eval_lastoid = SPI_lastoid;
- if (paramLI)
- pfree(paramLI);
-
return rc;
}
--- 4928,4933 ----
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5140,5146 ****
LocalTransactionId curlxid = MyProc->lxid;
CachedPlan *cplan;
ParamListInfo paramLI;
! PLpgSQL_expr *save_cur_expr;
MemoryContext oldcontext;
/*
--- 5135,5141 ----
LocalTransactionId curlxid = MyProc->lxid;
CachedPlan *cplan;
ParamListInfo paramLI;
! void *save_setup_arg;
MemoryContext oldcontext;
/*
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5216,5229 ****
}
/*
! * Create the param list in econtext's temporary memory context. We won't
! * need to free it explicitly, since it will go away at the next reset of
! * that context.
! *
! * Just for paranoia's sake, save and restore the prior value of
! * estate->cur_expr, which setup_param_list() sets.
*/
! save_cur_expr = estate->cur_expr;
paramLI = setup_param_list(estate, expr);
econtext->ecxt_param_list_info = paramLI;
--- 5211,5220 ----
}
/*
! * Set up param list. For safety, save and restore
! * estate->paramLI->parserSetupArg around our use of the param list.
*/
! save_setup_arg = estate->paramLI->parserSetupArg;
paramLI = setup_param_list(estate, expr);
econtext->ecxt_param_list_info = paramLI;
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5244,5250 ****
/* Assorted cleanup */
expr->expr_simple_in_use = false;
! estate->cur_expr = save_cur_expr;
if (!estate->readonly_func)
PopActiveSnapshot();
--- 5235,5241 ----
/* Assorted cleanup */
expr->expr_simple_in_use = false;
! estate->paramLI->parserSetupArg = save_setup_arg;
if (!estate->readonly_func)
PopActiveSnapshot();
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5268,5273 ****
--- 5259,5273 ----
/*
* Create a ParamListInfo to pass to SPI
*
+ * We share a single ParamListInfo array across all SPI calls made from this
+ * estate. This is generally OK since any given slot in the array would
+ * need to contain the same current datum value no matter which query or
+ * expression we're evaluating. However, paramLI->parserSetupArg points to
+ * the specific PLpgSQL_expr being evaluated. This is not an issue for
+ * statement-level callers, but lower-level callers should save and restore
+ * estate->paramLI->parserSetupArg just in case there's an active evaluation
+ * at an outer call level.
+ *
* We fill in the values for any expression parameters that are plain
* PLpgSQL_var datums; these are cheap and safe to evaluate, and by setting
* them with PARAM_FLAG_CONST flags, we allow the planner to use those values
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5277,5285 ****
* the expression that might never be evaluated at runtime. To handle those
* parameters, we set up a paramFetch hook for the executor to call when it
* wants a not-presupplied value.
- *
- * The result is a locally palloc'd array that should be pfree'd after use;
- * but note it can be NULL.
*/
static ParamListInfo
setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
--- 5277,5282 ----
*************** setup_param_list(PLpgSQL_execstate *esta
*** 5293,5314 ****
Assert(expr->plan != NULL);
/*
! * Could we re-use these arrays instead of palloc'ing a new one each time?
! * However, we'd have to re-fill the array each time anyway, since new
! * values might have been assigned to the variables.
*/
! if (!bms_is_empty(expr->paramnos))
{
int dno;
! paramLI = (ParamListInfo)
! palloc0(offsetof(ParamListInfoData, params) +
! estate->ndatums * sizeof(ParamExternData));
! paramLI->paramFetch = plpgsql_param_fetch;
! paramLI->paramFetchArg = (void *) estate;
! paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
! paramLI->parserSetupArg = (void *) expr;
! paramLI->numParams = estate->ndatums;
/* Instantiate values for "safe" parameters of the expression */
dno = -1;
--- 5290,5317 ----
Assert(expr->plan != NULL);
/*
! * We only need a ParamListInfo if the expression has parameters. In
! * principle we should test with bms_is_empty(), but we use a not-null
! * test because it's faster. In current usage bits are never removed from
! * expr->paramnos, only added, so this test is correct anyway.
*/
! if (expr->paramnos)
{
int dno;
! /* Use the common ParamListInfo for all evals in this estate */
! paramLI = estate->paramLI;
!
! /*
! * Reset all entries to "invalid". It's pretty annoying to have to do
! * this, but we don't currently track enough information to know which
! * old entries might be obsolete. (There are a couple of nontrivial
! * issues that would have to be dealt with in order to do better here.
! * First, ROW and RECFIELD datums depend on other datums, and second,
! * exec_eval_datum() will return short-lived palloc'd values for ROW
! * and REC datums.)
! */
! MemSet(paramLI->params, 0, estate->ndatums * sizeof(ParamExternData));
/* Instantiate values for "safe" parameters of the expression */
dno = -1;
*************** setup_param_list(PLpgSQL_execstate *esta
*** 5330,5339 ****
/*
* Set up link to active expr where the hook functions can find it.
! * Callers must save and restore cur_expr if there is any chance that
! * they are interrupting an active use of parameters.
*/
! estate->cur_expr = expr;
/*
* Also make sure this is set before parser hooks need it. There is
--- 5333,5342 ----
/*
* Set up link to active expr where the hook functions can find it.
! * Callers must save and restore parserSetupArg if there is any chance
! * that they are interrupting an active use of parameters.
*/
! paramLI->parserSetupArg = (void *) expr;
/*
* Also make sure this is set before parser hooks need it. There is
*************** plpgsql_param_fetch(ParamListInfo params
*** 5373,5379 ****
/* fetch back the hook data */
estate = (PLpgSQL_execstate *) params->paramFetchArg;
! expr = estate->cur_expr;
Assert(params->numParams == estate->ndatums);
/*
--- 5376,5382 ----
/* fetch back the hook data */
estate = (PLpgSQL_execstate *) params->paramFetchArg;
! expr = (PLpgSQL_expr *) params->parserSetupArg;
Assert(params->numParams == estate->ndatums);
/*
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 4ec4628..66d4da6 100644
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
*************** typedef struct PLpgSQL_execstate
*** 784,789 ****
--- 784,792 ----
int ndatums;
PLpgSQL_datum **datums;
+ /* we pass datums[i] to the executor, when needed, in paramLI->params[i] */
+ ParamListInfo paramLI;
+
/* EState to use for "simple" expression evaluation */
EState *simple_eval_estate;
*************** typedef struct PLpgSQL_execstate
*** 792,798 ****
uint32 eval_processed;
Oid eval_lastoid;
ExprContext *eval_econtext; /* for executing simple expressions */
- PLpgSQL_expr *cur_expr; /* current query/expr being evaluated */
/* status information for error context reporting */
PLpgSQL_stmt *err_stmt; /* current stmt */
--- 795,800 ----
On Mon, Mar 09, 2015 at 03:06:24PM -0400, Robert Haas wrote: > What I do care about > is that we as a project have to at some point be willing to begin > closing the spigot on new patches and focusing on polishing and > shipping the patches we've already got. I think it's perfectly > reasonable to worry about where we are on that continuum when it's > already several weeks after the start of the last CommitFest, but if > I'm in the minority on that, so be it. I am in your minority on that point.
2015-03-11 0:24 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
+1
Regards
Robert Haas <robertmhaas@gmail.com> writes:
> On the technical side, I do agree that the requirement to allocate and
> zero an array for every new simple expression is unfortunate, but I'm
> not convinced that repeatedly invoking the hook-function is a good way
> to solve that problem. Calling the hook-function figures to be
> significantly more expensive than an array-fetch, so you can almost
> think of the ParamListInfo entries themselves as a cache of data
> retrieved from the hook function. In that view of the world, the
> problem here is that initializing the cache is too expensive. Your
> solution to that is to rip the cache out completely, but what would be
> better is keep the cache and make initializing it cheaper - e.g. by
> sharing one ParamListInfo across all expressions in the same scope; or
> by not zeroing the memory and instead tracking the the first N slots
> are the only ones we've initialized; or $your_idea_here.
Getting back to the actual merits of this patch --- you're right, it
was not a good idea as proposed. I'd been thinking about the scalar
expression case only, and forgot that the same code is used to pass
parameters to queries, which might evaluate those parameters quite
a large number of times. So any savings from reducing the setup time
is sure to be swamped if the hook-function gets called repeatedly.
Another problem is that for ROW and REC datums, exec_eval_datum()
pallocs memory that won't get freed till the end of the statement,
so repeat evaluation would cause memory leaks. So we really need
evaluate-at-most-once semantics in there.
However, this attempt did confirm that we don't need to create a separate
ParamListInfo struct for each evaluation attempt. So the attached,
greatly stripped-down patch just allocates a ParamListInfo once at
function startup, and then zeroes it each time. This does nothing for
the memset overhead but it does get rid of the palloc traffic, which
seems to be good for a few percent on simple-assignment-type statements.
AFAICS this is an unconditional win compared to HEAD. What's more, it
provides at least a basis for doing better later: if we could think of
a reasonably cheap way of tracking which datums get invalidated by an
assignment, we might be able to reset only selected entries in the array
rather than blindly blowing away all of them.
I propose to apply this and leave it at that for now.
Regards
Pave
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Mar 9, 2015 at 03:06:24PM -0400, Robert Haas wrote: > On Mon, Mar 9, 2015 at 2:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > As far as that goes, it has never been the case that we expected every > > patch to go through the commitfest review process. (If we did, our > > response time to bugs would be probably a couple orders of magnitude > > longer than it is.) The way I see the policy is that committers have > > authority to commit things that they feel are ready and that haven't > > been objected to by other developers. Depending on the particular > > committer and the particular patch, feeling that a patch is "ready" > > might or might not require that it's been through a commitfest review. > > There is no process-based restriction on committing "ready" patches > > except when we are in alpha/beta/RC states, which is surely months > > away for 9.5. > > > > If I were looking for a formal review on this one, I would certainly > > not expect that it would get one during the current CF. I wasn't > > though. > > Yes, I understand. I don't really have anything more to say about > this. Nothing here changes my basic feeling that we need to stop > putting new irons in the fire and start working hard on taking irons > out of the fire; and I think most if not all other committers are > already doing that. Even to the extent that they are focusing on > their own patches rather than other people's patches, I think it is > mostly patches that they wrote some time ago, not new things that they > have only just written. Sorry to be coming late to this thread. I don't think the problem is that Tom is working on these patches. Rather, I think since Tom's employer now cares more about his current work, Tom just isn't as available to help with commitfest stuff and patch application. It is hard to see how the community can complain about that --- it is like having an uncle who used to give you $20 every time you saw him, then suddenly stops. You would certainly be disappointed, but it is hard to see how you have a right to complain. Now, if Tom's work was interrupting others' work, then there is a reasonable complaint. As a contrast, as someone who almost never applies things from the commitfest, I would be even more open to criticism. I spend my spare time closing out unaddressed emails, but again, I have decided that is the best use of my time, and I didn't ask anyone if they agreed. My assumption has always been that if activity is positive, it is up to the individual to decide which efforts to pursue. My employer could adjust my activity too. I think the larger issue is that we have to adjust to a new-normal where Tom isn't going to be as helpful in this area. Do we need more committers? Do we need to adjust the process or dates? These are probably the questions we should be addressing. I think one valid criticism is that Tom should transition his commitments to this new-normal, especially for the the Grouping Set patch, rather than allowing things to dangle in an unknown state. Again, communicating expectations goes a long way in avoiding conflict. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> I think one valid criticism is that Tom should transition his
> commitments to this new-normal, especially for the the Grouping Set
> patch, rather than allowing things to dangle in an unknown state.
Well, as far as that goes, I had every intention of getting to the
GROUPING SETS patch before the end of the final commitfest, and I still
will if nobody else picks it up soon.
regards, tom lane
On Tue, Mar 17, 2015 at 01:03:03PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > I think one valid criticism is that Tom should transition his > > commitments to this new-normal, especially for the the Grouping Set > > patch, rather than allowing things to dangle in an unknown state. > > Well, as far as that goes, I had every intention of getting to the > GROUPING SETS patch before the end of the final commitfest, and I still > will if nobody else picks it up soon. OK, very good to know, and state publicly. :-) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Tue, Mar 17, 2015 at 12:47 PM, Bruce Momjian <bruce@momjian.us> wrote: > Sorry to be coming late to this thread. I don't think the problem is > that Tom is working on these patches. Rather, I think since Tom's > employer now cares more about his current work, Tom just isn't as > available to help with commitfest stuff and patch application. > > It is hard to see how the community can complain about that --- it is > like having an uncle who used to give you $20 every time you saw him, > then suddenly stops. You would certainly be disappointed, but it is > hard to see how you have a right to complain. Now, if Tom's work was > interrupting others' work, then there is a reasonable complaint. > > As a contrast, as someone who almost never applies things from the > commitfest, I would be even more open to criticism. I spend my spare > time closing out unaddressed emails, but again, I have decided that is > the best use of my time, and I didn't ask anyone if they agreed. My > assumption has always been that if activity is positive, it is up to the > individual to decide which efforts to pursue. My employer could adjust > my activity too. > > I think the larger issue is that we have to adjust to a new-normal where > Tom isn't going to be as helpful in this area. Do we need more > committers? Do we need to adjust the process or dates? These are > probably the questions we should be addressing. I was NOT complaining about Tom spending less time on PostgreSQL CommitFests. I would like him to spend more, but that's his decision, in conjunction with his employer. I would like to spend more time on PostgreSQL CommitFests myself, but I have trouble finding the time, too. What I was complaining about is new feature patches for 9.5 arriving after the start of the last CF. There has to be some date after which a patch is too late to be considered for a given release, or we will never ship a release. We can argue about what that date is, and it can be different for different people if we so choose, but at the end of the day, you have to cut it off someplace, or you never get the release out. I think these are really two different problems. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Bruce Momjian (bruce@momjian.us) wrote:
> I think the larger issue is that we have to adjust to a new-normal where
> Tom isn't going to be as helpful in this area. Do we need more
> committers? Do we need to adjust the process or dates? These are
> probably the questions we should be addressing.
I'm afraid we are seeing issues beyond this- there don't seem (at least
to me) to be as many reviewers working on reviewing patches to give
feedback on them prior to having committers look either. I suspect
that's largely because there hasn't been the 'push' for that to happen
of late.
Further, as it relates to individuals and the organizations that they
work for, I think we should be very clear that we are happy to accept
the work they are doing even if it isn't furthering the commitfest,
provided the code is at the level of quality we expect, the capability
is one we desire, and the design is sound. Insisting that we punt
patches from Tom, or any other committer, which meet those criteria
because Tom (or whomever) didn't do enough work on the latest commitfest
is doing ourselves a disservice. That these companies are funding Tom
and other committers to write open source code for all of us to benefit
from is great and should be encouraged.
That said, we certainly don't want to abandon the commitfests or, more
generally, patch submission from non-committers. I haven't got any
great solutions to that problem, as I've been having trouble finding
time to support the commitfest too, but cutting off all progress would
surely be worse.
Thanks!
Stephen
Robert Haas <robertmhaas@gmail.com> writes:
> What I was complaining about is new feature patches for 9.5 arriving
> after the start of the last CF. There has to be some date after which
> a patch is too late to be considered for a given release, or we will
> never ship a release. We can argue about what that date is, and it
> can be different for different people if we so choose, but at the end
> of the day, you have to cut it off someplace, or you never get the
> release out.
Well, I'm going to push back on that concept a bit. I do not think the
CF process is, or ever has been, meant to tell committers they can't work
on things at times they find convenient. That would be holding back
progress to little purpose. What the CF process is about is making sure
that things don't slip through the cracks, and in particular that
submissions from non-committers get due consideration on a reasonably
timely basis.
We do have a process in which even committers have to think twice about
whether it's appropriate to push something, but that's feature freeze
during alpha/beta/RC testing, and we are still a long way away from that
stage for 9.5.
Or in short: yes, the rules are different for committers and non
committers. That's one of the reasons we are slow to hand out commit
bits.
regards, tom lane
On Tue, Mar 17, 2015 at 01:28:21PM -0400, Tom Lane wrote: > Or in short: yes, the rules are different for committers and non > committers. That's one of the reasons we are slow to hand out commit > bits. I think one reason the rules are different for committers and non-committers is that committers are responsible for quickly fixing whatever breakage their patch application caused. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Tue, Mar 17, 2015 at 1:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> What I was complaining about is new feature patches for 9.5 arriving >> after the start of the last CF. There has to be some date after which >> a patch is too late to be considered for a given release, or we will >> never ship a release. We can argue about what that date is, and it >> can be different for different people if we so choose, but at the end >> of the day, you have to cut it off someplace, or you never get the >> release out. > > Well, I'm going to push back on that concept a bit. I do not think the > CF process is, or ever has been, meant to tell committers they can't work > on things at times they find convenient. That would be holding back > progress to little purpose. What the CF process is about is making sure > that things don't slip through the cracks, and in particular that > submissions from non-committers get due consideration on a reasonably > timely basis. I agree with all of that. > We do have a process in which even committers have to think twice about > whether it's appropriate to push something, but that's feature freeze > during alpha/beta/RC testing, and we are still a long way away from that > stage for 9.5. My understanding has been that for the last 5 years, the feature freeze deadline, for both committers and non-committers, is the beginning of the last CF, and earlier for "major" patches, most of which tend to come from committers. Now, I understand that we are more free with making exceptions for committers, and there's some justification for that, but, let's be honest, an awful lot of the people submitting major patches at this point *are* committers, and essentially all of those people take care to get their major patches submitted before the last CF starts, and have for years, whether you want to admit that or not. I think we'd making a huge mistake if we choose to back away from that, and I think you'd be arguing the other side of this question in a heartbeat if somebody who happened to have a commit bit submitted something you thought was half-baked. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Mar 17, 2015 at 1:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> We do have a process in which even committers have to think twice about
>> whether it's appropriate to push something, but that's feature freeze
>> during alpha/beta/RC testing, and we are still a long way away from that
>> stage for 9.5.
> My understanding has been that for the last 5 years, the feature
> freeze deadline, for both committers and non-committers, is the
> beginning of the last CF, and earlier for "major" patches, most of
> which tend to come from committers.
Um, I consider that "feature freeze" marks a point at which we stop
*committing* new features, not a point after which new features can
no longer be submitted or considered. Otherwise we'd be spending
four or more months a year in feature freeze, and that just isn't
appropriate IMV.
In any case, I reject the notion that the CF process has anything to
do with that decision. The point of the CF submission deadline is
that we promise to consider every submission made before the deadline.
It is not to forbid committers from taking up items that arrived later,
if they feel motivated to.
regards, tom lane
On Tue, Mar 17, 2015 at 5:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Mar 17, 2015 at 1:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> We do have a process in which even committers have to think twice about >>> whether it's appropriate to push something, but that's feature freeze >>> during alpha/beta/RC testing, and we are still a long way away from that >>> stage for 9.5. > >> My understanding has been that for the last 5 years, the feature >> freeze deadline, for both committers and non-committers, is the >> beginning of the last CF, and earlier for "major" patches, most of >> which tend to come from committers. > > Um, I consider that "feature freeze" marks a point at which we stop > *committing* new features, not a point after which new features can > no longer be submitted or considered. Otherwise we'd be spending > four or more months a year in feature freeze, and that just isn't > appropriate IMV. Fine, call it a feature submission deadline. > In any case, I reject the notion that the CF process has anything to > do with that decision. The point of the CF submission deadline is > that we promise to consider every submission made before the deadline. > It is not to forbid committers from taking up items that arrived later, > if they feel motivated to. So this is really what it boils down to: you evidently think there is no problem with new feature patches showing up a month or two after the last CommitFest has started. I do. It distracts everyone from focusing on the patches that were submitted earlier, so that they don't get dealt with. If the patch comes from a committer, it's actually worse, because patches from non-committers can be safely ignored until a committer expresses interest in them, but if the patch is from a committer who obviously intend to push it along, you'd better drop what you're doing and object pretty fast, or it'll already be in the tree before you get around to it. I think it's perfectly appropriate for the project to have a feature submission deadline, I think having such a deadline is important to both the timeliness and the quality of our releases, and I think most people here understand that deadline to be the start of the last CF. I'm just repeating myself here, so I'm going to shut up now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Mar 17, 2015 at 3:50 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> In any case, I reject the notion that the CF process has anything to >> do with that decision. The point of the CF submission deadline is >> that we promise to consider every submission made before the deadline. >> It is not to forbid committers from taking up items that arrived later, >> if they feel motivated to. > > So this is really what it boils down to: you evidently think there is > no problem with new feature patches showing up a month or two after > the last CommitFest has started. I do. It distracts everyone from > focusing on the patches that were submitted earlier, so that they > don't get dealt with. If the patch comes from a committer, it's > actually worse, because patches from non-committers can be safely > ignored until a committer expresses interest in them, but if the patch > is from a committer who obviously intend to push it along, you'd > better drop what you're doing and object pretty fast, or it'll already > be in the tree before you get around to it. I think it's perfectly > appropriate for the project to have a feature submission deadline, I > think having such a deadline is important to both the timeliness and > the quality of our releases, and I think most people here understand > that deadline to be the start of the last CF. I agree. New feature patches being committed that were originally posted significantly after that deadline are not against the letter of the law. However, that does not make them consistent with its spirit. It's a value judgement as to whether or not any individual case violates this spirit, but in my judgement this work does. I don't think it's outrageous that it was suggested, but that's how I feel about it. I don't see a very compelling cost/benefit ratio for the community as a whole. I, as a non-committer, have proposed that the rules be bent once or twice in the past, and those suggestions were rejected without exception, even though I imagined that there was a compelling cost/benefit ratio. I thought that was fine. I always assumed that I had the same right to suggest something as a committer. The only fundamental difference was that I had to convince a committer that my assessment was correct, rather than simply avoiding having the suggestion be vetoed. I'd need to do both. Clearly my previous understanding of this was questionable, to say the least. -- Peter Geoghegan
On Tue, Mar 17, 2015 at 04:21:16PM -0700, Peter Geoghegan wrote: > I, as a non-committer, have proposed that the rules be bent once or > twice in the past, and those suggestions were rejected without > exception, even though I imagined that there was a compelling > cost/benefit ratio. I thought that was fine. I always assumed that I > had the same right to suggest something as a committer. The only > fundamental difference was that I had to convince a committer that my > assessment was correct, rather than simply avoiding having the > suggestion be vetoed. I'd need to do both. Clearly my previous > understanding of this was questionable, to say the least. Basically, the same rules apply to all commitfests, i.e. a committer can apply anything during that period. I think the only restriction for the last commitfest is that the committer can not apply a new patch that would have been too big to be submitted to the last commitfest. If enough people feel that this committer behavior during the last commitfest is a problem, we can discuss changing that policy. Now, with that right comes the significant responsibility to fix any breakage they cause. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Tue, Mar 17, 2015 at 8:01 PM, Bruce Momjian <bruce@momjian.us> wrote: > On Tue, Mar 17, 2015 at 04:21:16PM -0700, Peter Geoghegan wrote: >> I, as a non-committer, have proposed that the rules be bent once or >> twice in the past, and those suggestions were rejected without >> exception, even though I imagined that there was a compelling >> cost/benefit ratio. I thought that was fine. I always assumed that I >> had the same right to suggest something as a committer. The only >> fundamental difference was that I had to convince a committer that my >> assessment was correct, rather than simply avoiding having the >> suggestion be vetoed. I'd need to do both. Clearly my previous >> understanding of this was questionable, to say the least. > > Basically, the same rules apply to all commitfests, i.e. a committer can > apply anything during that period. I think the only restriction for the > last commitfest is that the committer can not apply a new patch that > would have been too big to be submitted to the last commitfest. If > enough people feel that this committer behavior during the last > commitfest is a problem, we can discuss changing that policy. One thing that's crystal clear here is that we don't all agree on what the policy actually is. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Mar 17, 2015 at 8:01 PM, Bruce Momjian <bruce@momjian.us> wrote:
>> Basically, the same rules apply to all commitfests, i.e. a committer can
>> apply anything during that period. I think the only restriction for the
>> last commitfest is that the committer can not apply a new patch that
>> would have been too big to be submitted to the last commitfest. If
>> enough people feel that this committer behavior during the last
>> commitfest is a problem, we can discuss changing that policy.
> One thing that's crystal clear here is that we don't all agree on what
> the policy actually is.
Indeed. In this case, since the patch in question is one that
improves/simplifies a patch that's already in the current commitfest,
I'm going to go ahead and push it. If you want to call a vote on
revoking my commit bit, go right ahead.
regards, tom lane
On Wed, Mar 18, 2015 at 1:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Mar 17, 2015 at 8:01 PM, Bruce Momjian <bruce@momjian.us> wrote: >>> Basically, the same rules apply to all commitfests, i.e. a committer can >>> apply anything during that period. I think the only restriction for the >>> last commitfest is that the committer can not apply a new patch that >>> would have been too big to be submitted to the last commitfest. If >>> enough people feel that this committer behavior during the last >>> commitfest is a problem, we can discuss changing that policy. > >> One thing that's crystal clear here is that we don't all agree on what >> the policy actually is. > > Indeed. In this case, since the patch in question is one that > improves/simplifies a patch that's already in the current commitfest, > I'm going to go ahead and push it. If you want to call a vote on > revoking my commit bit, go right ahead. One might almost get the impression you don't think we're all on the same team, here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-03-18 13:12:11 -0400, Tom Lane wrote: > Indeed. In this case, since the patch in question is one that > improves/simplifies a patch that's already in the current commitfest, > I'm going to go ahead and push it. If you want to call a vote on > revoking my commit bit, go right ahead. Seriously? In my opinion it has to be possible to doubt whether a patch should be committed in certain release without it being interpreted as a personal attack. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Mar 18, 2015 at 10:26 AM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2015-03-18 13:12:11 -0400, Tom Lane wrote: >> Indeed. In this case, since the patch in question is one that >> improves/simplifies a patch that's already in the current commitfest, >> I'm going to go ahead and push it. If you want to call a vote on >> revoking my commit bit, go right ahead. > > Seriously? In my opinion it has to be possible to doubt whether a patch > should be committed in certain release without it being interpreted as a > personal attack. +1 -- Peter Geoghegan
Andres Freund <andres@2ndquadrant.com> writes:
> Seriously? In my opinion it has to be possible to doubt whether a patch
> should be committed in certain release without it being interpreted as a
> personal attack.
I don't think anyone's said anything in this thread that amounts to a
personal attack. We have a difference of opinion on policy, and what
I'm saying is that the policy ultimately reduces to trusting individual
committers to use their best judgment. If someone's going to tell me
that my judgment about when to push something is not acceptable, then
they probably ought to be calling for removal of my commit privileges.
regards, tom lane
On Wed, Mar 18, 2015 at 2:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andres Freund <andres@2ndquadrant.com> writes: >> Seriously? In my opinion it has to be possible to doubt whether a patch >> should be committed in certain release without it being interpreted as a >> personal attack. > > I don't think anyone's said anything in this thread that amounts to a > personal attack. We have a difference of opinion on policy, and what > I'm saying is that the policy ultimately reduces to trusting individual > committers to use their best judgment. If someone's going to tell me > that my judgment about when to push something is not acceptable, then > they probably ought to be calling for removal of my commit privileges. Neither I nor anyone else is prepared to do that on the basis of what happens to this one patch. But if we adopt a project policy that says a committer can ignore the contrary opinions of other people, even when those other people include multiple other committers, this project will not, in the long term, be successful. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-03-18 14:01:41 -0400, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > Seriously? In my opinion it has to be possible to doubt whether a patch > > should be committed in certain release without it being interpreted as a > > personal attack. > > I don't think anyone's said anything in this thread that amounts to a > personal attack. From where I am sitting you appear somewhat offended. If not, great. > If someone's going to tell me that my judgment about when to push > something is not acceptable, then they probably ought to be calling > for removal of my commit privileges. I think this is making the issue far bigger than it is. I think every committer and most contributors in this project have swallowed more decisions they didn't agree with than they care to count. And that's normal. Let's try to keep it a bit more low key. > We have a difference of opinion on policy, and what I'm saying is that > the policy ultimately reduces to trusting individual committers to use > their best judgment. I think the issue is much less, if at all, about trusting committers, than about a different view about how to hanlde our problems with release management. Quite apparently 9.4 didn't work out greatly, which probably excerbates preexisting (perhaps unknowingly so) differences in opinion about until when new patches should be "allowed". There's also the problem, as you noted, that we're all fucking tired of neverending 2014-06 commitfest. Robert is, apparently at least, of the opinion that that's best handled by stopping to accept new patches and trying reduce the size of the backlog. That doesn't sound entirely unreasonable. It also doesn't sound unreasonable to deal with that backlog by doing the stuff oneself deems important. I personally am *really* worried about the current state; without having a short/mid term solution. I think this makes postgres really unattractive to contribute to. And that has, and will, cost us a lot of help. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services