From 40301c1c8bcbe92a9ba9bf017da03e83476ae0e5 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 13 Feb 2018 19:10:43 -0500 Subject: [PATCH] Speed up plpgsql function startup by doing fewer pallocs. Previously, copy_plpgsql_datum did a separate palloc for each variable needing instance-local storage. In simple benchmarks this made for a noticeable fraction of the total runtime. Improve it by precalculating the space needed for all of a function's variables and doing just one palloc for all of them. In passing, remove PLPGSQL_DTYPE_EXPR from the list of plpgsql "datum" types, since in fact it has nothing in common with the others, and there is noplace that needs to discriminate on the basis of dtype between an expression and any type of datum. And add comments clarifying which datum struct fields are generic and which aren't. Tom Lane, reviewed by Pavel Stehule Discussion: https://postgr.es/m/11986.1514407114@sss.pgh.pa.us --- src/pl/plpgsql/src/pl_comp.c | 17 ++++++ src/pl/plpgsql/src/pl_exec.c | 113 +++++++++++++++++++---------------- src/pl/plpgsql/src/pl_gram.y | 4 -- src/pl/plpgsql/src/plpgsql.h | 84 ++++++++++++++++---------- 4 files changed, 131 insertions(+), 87 deletions(-) diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c index 09ecaec635..97cb763641 100644 --- a/src/pl/plpgsql/src/pl_comp.c +++ b/src/pl/plpgsql/src/pl_comp.c @@ -2183,12 +2183,29 @@ plpgsql_adddatum(PLpgSQL_datum *new) static void plpgsql_finish_datums(PLpgSQL_function *function) { + Size copiable_size = 0; int i; function->ndatums = plpgsql_nDatums; function->datums = palloc(sizeof(PLpgSQL_datum *) * plpgsql_nDatums); for (i = 0; i < plpgsql_nDatums; i++) + { function->datums[i] = plpgsql_Datums[i]; + + /* This must agree with copy_plpgsql_datums on what is copiable */ + switch (function->datums[i]->dtype) + { + case PLPGSQL_DTYPE_VAR: + copiable_size += MAXALIGN(sizeof(PLpgSQL_var)); + break; + case PLPGSQL_DTYPE_REC: + copiable_size += MAXALIGN(sizeof(PLpgSQL_rec)); + break; + default: + break; + } + } + function->copiable_size = copiable_size; } diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 7612902e8f..c90024064a 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -235,7 +235,8 @@ static HTAB *shared_cast_hash = NULL; static void coerce_function_result_tuple(PLpgSQL_execstate *estate, TupleDesc tupdesc); static void plpgsql_exec_error_callback(void *arg); -static PLpgSQL_datum *copy_plpgsql_datum(PLpgSQL_datum *datum); +static void copy_plpgsql_datums(PLpgSQL_execstate *estate, + PLpgSQL_function *func); static MemoryContext get_stmt_mcontext(PLpgSQL_execstate *estate); static void push_stmt_mcontext(PLpgSQL_execstate *estate); static void pop_stmt_mcontext(PLpgSQL_execstate *estate); @@ -458,8 +459,7 @@ plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo fcinfo, * Make local execution copies of all the datums */ estate.err_text = gettext_noop("during initialization of execution state"); - for (i = 0; i < estate.ndatums; i++) - estate.datums[i] = copy_plpgsql_datum(func->datums[i]); + copy_plpgsql_datums(&estate, func); /* * Store the actual call argument values into the appropriate variables @@ -859,8 +859,7 @@ plpgsql_exec_trigger(PLpgSQL_function *func, * Make local execution copies of all the datums */ estate.err_text = gettext_noop("during initialization of execution state"); - for (i = 0; i < estate.ndatums; i++) - estate.datums[i] = copy_plpgsql_datum(func->datums[i]); + copy_plpgsql_datums(&estate, func); /* * Put the OLD and NEW tuples into record variables @@ -1153,7 +1152,6 @@ plpgsql_exec_event_trigger(PLpgSQL_function *func, EventTriggerData *trigdata) { PLpgSQL_execstate estate; ErrorContextCallback plerrcontext; - int i; int rc; PLpgSQL_var *var; @@ -1174,8 +1172,7 @@ plpgsql_exec_event_trigger(PLpgSQL_function *func, EventTriggerData *trigdata) * Make local execution copies of all the datums */ estate.err_text = gettext_noop("during initialization of execution state"); - for (i = 0; i < estate.ndatums; i++) - estate.datums[i] = copy_plpgsql_datum(func->datums[i]); + copy_plpgsql_datums(&estate, func); /* * Assign the special tg_ variables @@ -1290,57 +1287,73 @@ plpgsql_exec_error_callback(void *arg) * Support function for initializing local execution variables * ---------- */ -static PLpgSQL_datum * -copy_plpgsql_datum(PLpgSQL_datum *datum) +static void +copy_plpgsql_datums(PLpgSQL_execstate *estate, + PLpgSQL_function *func) { - PLpgSQL_datum *result; + int ndatums = estate->ndatums; + PLpgSQL_datum **indatums; + PLpgSQL_datum **outdatums; + char *workspace; + char *ws_next; + int i; - switch (datum->dtype) + /* Allocate local datum-pointer array */ + estate->datums = (PLpgSQL_datum **) + palloc(sizeof(PLpgSQL_datum *) * ndatums); + + /* + * To reduce palloc overhead, we make a single palloc request for all the + * space needed for locally-instantiated datums. + */ + workspace = palloc(func->copiable_size); + ws_next = workspace; + + /* Fill datum-pointer array, copying datums into workspace as needed */ + indatums = func->datums; + outdatums = estate->datums; + for (i = 0; i < ndatums; i++) { - case PLPGSQL_DTYPE_VAR: - { - PLpgSQL_var *new = palloc(sizeof(PLpgSQL_var)); + PLpgSQL_datum *indatum = indatums[i]; + PLpgSQL_datum *outdatum; - memcpy(new, datum, sizeof(PLpgSQL_var)); - /* should be preset to null/non-freeable */ - Assert(new->isnull); - Assert(!new->freeval); + /* This must agree with plpgsql_finish_datums on what is copiable */ + switch (indatum->dtype) + { + case PLPGSQL_DTYPE_VAR: + outdatum = (PLpgSQL_datum *) ws_next; + memcpy(outdatum, indatum, sizeof(PLpgSQL_var)); + ws_next += MAXALIGN(sizeof(PLpgSQL_var)); + break; - result = (PLpgSQL_datum *) new; - } - break; + case PLPGSQL_DTYPE_REC: + outdatum = (PLpgSQL_datum *) ws_next; + memcpy(outdatum, indatum, sizeof(PLpgSQL_rec)); + ws_next += MAXALIGN(sizeof(PLpgSQL_rec)); + break; - case PLPGSQL_DTYPE_REC: - { - PLpgSQL_rec *new = palloc(sizeof(PLpgSQL_rec)); + case PLPGSQL_DTYPE_ROW: + case PLPGSQL_DTYPE_RECFIELD: + case PLPGSQL_DTYPE_ARRAYELEM: - memcpy(new, datum, sizeof(PLpgSQL_rec)); - /* should be preset to empty */ - Assert(new->erh == NULL); + /* + * These datum records are read-only at runtime, so no need to + * copy them (well, RECFIELD and ARRAYELEM contain cached + * data, but we'd just as soon centralize the caching anyway). + */ + outdatum = indatum; + break; - result = (PLpgSQL_datum *) new; - } - break; + default: + elog(ERROR, "unrecognized dtype: %d", indatum->dtype); + outdatum = NULL; /* keep compiler quiet */ + break; + } - case PLPGSQL_DTYPE_ROW: - case PLPGSQL_DTYPE_RECFIELD: - case PLPGSQL_DTYPE_ARRAYELEM: - - /* - * These datum records are read-only at runtime, so no need to - * copy them (well, RECFIELD and ARRAYELEM contain cached data, - * but we'd just as soon centralize the caching anyway) - */ - result = datum; - break; - - default: - elog(ERROR, "unrecognized dtype: %d", datum->dtype); - result = NULL; /* keep compiler quiet */ - break; + outdatums[i] = outdatum; } - return result; + Assert(ws_next == workspace + func->copiable_size); } /* @@ -3504,8 +3517,8 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate, estate->found_varno = func->found_varno; estate->ndatums = func->ndatums; - estate->datums = palloc(sizeof(PLpgSQL_datum *) * estate->ndatums); - /* caller is expected to fill the datums array */ + estate->datums = NULL; + /* the datums array will be filled by copy_plpgsql_datums() */ estate->datum_context = CurrentMemoryContext; /* initialize our ParamListInfo with appropriate hook functions */ diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y index 97c0d4f98a..ee943eed93 100644 --- a/src/pl/plpgsql/src/pl_gram.y +++ b/src/pl/plpgsql/src/pl_gram.y @@ -564,7 +564,6 @@ decl_statement : decl_varname decl_const decl_datatype decl_collate decl_notnull curname_def = palloc0(sizeof(PLpgSQL_expr)); - curname_def->dtype = PLPGSQL_DTYPE_EXPR; strcpy(buf, "SELECT "); cp1 = new->refname; cp2 = buf + strlen(buf); @@ -2697,7 +2696,6 @@ read_sql_construct(int until, } expr = palloc0(sizeof(PLpgSQL_expr)); - expr->dtype = PLPGSQL_DTYPE_EXPR; expr->query = pstrdup(ds.data); expr->plan = NULL; expr->paramnos = NULL; @@ -2944,7 +2942,6 @@ make_execsql_stmt(int firsttoken, int location) ds.data[--ds.len] = '\0'; expr = palloc0(sizeof(PLpgSQL_expr)); - expr->dtype = PLPGSQL_DTYPE_EXPR; expr->query = pstrdup(ds.data); expr->plan = NULL; expr->paramnos = NULL; @@ -3816,7 +3813,6 @@ read_cursor_args(PLpgSQL_var *cursor, int until, const char *expected) appendStringInfoChar(&ds, ';'); expr = palloc0(sizeof(PLpgSQL_expr)); - expr->dtype = PLPGSQL_DTYPE_EXPR; expr->query = pstrdup(ds.data); expr->plan = NULL; expr->paramnos = NULL; diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index d4eb67b738..dadbfb569c 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -63,8 +63,7 @@ typedef enum PLpgSQL_datum_type PLPGSQL_DTYPE_ROW, PLPGSQL_DTYPE_REC, PLPGSQL_DTYPE_RECFIELD, - PLPGSQL_DTYPE_ARRAYELEM, - PLPGSQL_DTYPE_EXPR + PLPGSQL_DTYPE_ARRAYELEM } PLpgSQL_datum_type; /* @@ -188,39 +187,11 @@ typedef struct PLpgSQL_type int32 atttypmod; /* typmod (taken from someplace else) */ } PLpgSQL_type; -/* - * Generic datum array item - * - * PLpgSQL_datum is the common supertype for PLpgSQL_expr, PLpgSQL_var, - * PLpgSQL_row, PLpgSQL_rec, PLpgSQL_recfield, and PLpgSQL_arrayelem - */ -typedef struct PLpgSQL_datum -{ - PLpgSQL_datum_type dtype; - int dno; -} PLpgSQL_datum; - -/* - * Scalar or composite variable - * - * The variants PLpgSQL_var, PLpgSQL_row, and PLpgSQL_rec share these - * fields - */ -typedef struct PLpgSQL_variable -{ - PLpgSQL_datum_type dtype; - int dno; - char *refname; - int lineno; -} PLpgSQL_variable; - /* * SQL Query to plan and execute */ typedef struct PLpgSQL_expr { - PLpgSQL_datum_type dtype; - int dno; char *query; SPIPlanPtr plan; Bitmapset *paramnos; /* all dnos referenced by this query */ @@ -249,6 +220,32 @@ typedef struct PLpgSQL_expr LocalTransactionId expr_simple_lxid; } PLpgSQL_expr; +/* + * Generic datum array item + * + * PLpgSQL_datum is the common supertype for PLpgSQL_var, PLpgSQL_row, + * PLpgSQL_rec, PLpgSQL_recfield, and PLpgSQL_arrayelem. + */ +typedef struct PLpgSQL_datum +{ + PLpgSQL_datum_type dtype; + int dno; +} PLpgSQL_datum; + +/* + * Scalar or composite variable + * + * The variants PLpgSQL_var, PLpgSQL_row, and PLpgSQL_rec share these + * fields. + */ +typedef struct PLpgSQL_variable +{ + PLpgSQL_datum_type dtype; + int dno; + char *refname; + int lineno; +} PLpgSQL_variable; + /* * Scalar variable */ @@ -258,11 +255,18 @@ typedef struct PLpgSQL_var int dno; char *refname; int lineno; + /* end of PLpgSQL_variable fields */ + bool isconst; + bool notnull; PLpgSQL_type *datatype; - int isconst; - int notnull; PLpgSQL_expr *default_val; + + /* + * Variables declared as CURSOR FOR are mostly like ordinary + * scalar variables of type refcursor, but they have these additional + * properties: + */ PLpgSQL_expr *cursor_explicit_expr; int cursor_explicit_argrow; int cursor_options; @@ -286,6 +290,7 @@ typedef struct PLpgSQL_row int dno; char *refname; int lineno; + /* end of PLpgSQL_variable fields */ /* * rowtupdesc is only set up if we might need to convert the row into a @@ -308,6 +313,8 @@ typedef struct PLpgSQL_rec int dno; char *refname; int lineno; + /* end of PLpgSQL_variable fields */ + Oid rectypeid; /* declared type of variable */ /* RECFIELDs for this record are chained together for easy access */ int firstfield; /* dno of first RECFIELD, or -1 if none */ @@ -322,6 +329,8 @@ typedef struct PLpgSQL_recfield { PLpgSQL_datum_type dtype; int dno; + /* end of PLpgSQL_datum fields */ + char *fieldname; /* name of field */ int recparentno; /* dno of parent record */ int nextfield; /* dno of next child, or -1 if none */ @@ -337,6 +346,8 @@ typedef struct PLpgSQL_arrayelem { PLpgSQL_datum_type dtype; int dno; + /* end of PLpgSQL_datum fields */ + PLpgSQL_expr *subscript; int arrayparentno; /* dno of parent array variable */ @@ -884,6 +895,7 @@ typedef struct PLpgSQL_function /* the datums representing the function's local variables */ int ndatums; PLpgSQL_datum **datums; + Size copiable_size; /* space for locally instantiated datums */ /* function body parsetree */ PLpgSQL_stmt_block *action; @@ -920,8 +932,14 @@ typedef struct PLpgSQL_execstate ResourceOwner tuple_store_owner; ReturnSetInfo *rsi; - /* the datums representing the function's local variables */ int found_varno; + + /* + * The datums representing the function's local variables. Some of these + * are local storage in this execstate, but some just point to the shared + * copy belonging to the PLpgSQL_function, depending on whether or not we + * need any per-execution state for the datum's dtype. + */ int ndatums; PLpgSQL_datum **datums; /* context containing variable values (same as func's SPI_proc context) */