Thread
Commits
-
Rearrange execution of PARAM_EXTERN Params for plpgsql's benefit.
- 6719b238e8f0 11.0 landed
-
Further reduce overhead for passing plpgsql variables to the executor.
- 6c82d8d1fdb1 9.6.0 cited
-
Letting plpgsql in on the fun with the new expression eval stuff
Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-19T18:00:41Z
I'm looking at ways to get plpgsql expression evaluation to go faster, and one thing I'm noticing is the rather large overhead of going through ExecEvalParamExtern and plpgsql_param_fetch to get to the useful work (exec_eval_datum). We've ameliorated that for DTYPE_VAR variables by keeping a pre-set-up copy of their values in a ParamListInfo struct, but that's pretty ugly and carries a bunch of costs of its own. What I'm wondering about, given the v10 redesign of expression evaluation, is whether we couldn't be smarter about this by allowing plpgsql (or other external users) to skip the ParamListInfo representation altogether, and instead compile Param references into calls to evaluation functions that are better tailored to the problem of fetching the desired value. In the existing execution infrastructure, what seems to make sense is to have an ExprEvalStep type that has functionality like EEOP_PARAM_EXTERN, but includes a function pointer to a plpgsql-supplied function having the same signature as ExecEvalParamExtern. So the execution would look more or less like EEO_CASE(EEOP_PARAM_CALLBACK) { op->eval_param(state, op, econtext); EEO_NEXT(); } and there'd need to be some extra fields (at least a void*) in the op struct where plpgsql could keep private data. The JIT stuff you're working on could just compile an equivalent of the above, although in the very long term maybe there would be some advantage to letting add-on modules compile specialized code for such steps. The immediate problem is how can ExecInitExpr generate such a step? It can't itself know what to put into the function ptr or the additional fields. There has to be a way for it to call a plpgsql-supplied support routine that can construct the eval step. (And we have to export ExprEvalPushStep, though that doesn't seem like a problem.) For compiling full-fledged query trees, what I think we could do is add a method (function pointer) to ParamListInfo and have ExecInitExpr invoke plan->state->es_param_list_info->compile_param if that's set. However, that solution doesn't immediately work for compiling simple expressions because we pass a null "parent" pointer when building those. I thought about instantiating a dummy PlanState and EState to use just for carrying this info, but that seems pretty ugly. Another way we could do it is to invent ExecInitExprWithParams() that takes an additional ParamListInfo pointer, and use that. Rather than adding yet one more parameter that has to be passed down through ExecInitExprRec, I suggest that we could waste a bit of space in struct ExprState and store that value there. Maybe do the same with the parent pointer so as to reduce the number of recursive parameters. I've not written any code around this idea yet, and am not sure if it conflicts with what you're trying to do for JIT or further out. Comments, better ideas? regards, tom lane -
Re: Letting plpgsql in on the fun with the new expression eval stuff
Andres Freund <andres@anarazel.de> — 2017-12-20T13:56:02Z
Hi, Cool to see you looking at that, I think there's quite some optimization potential around. I've to reread a bunch of plpgsql code, it's not exactly an area of the code I'm intimately familiar with. On 2017-12-19 13:00:41 -0500, Tom Lane wrote: > I'm looking at ways to get plpgsql expression evaluation to go faster, > and one thing I'm noticing is the rather large overhead of going through > ExecEvalParamExtern and plpgsql_param_fetch to get to the useful work > (exec_eval_datum). What's the workload you're testing? I'm mildly surprised to see ExecEvalParamExtern() show up, rather than just plpgsql_param_fetch() & exec_eval_datum(). Or were you just listing that to specify the callpath? > We've ameliorated that for DTYPE_VAR variables > by keeping a pre-set-up copy of their values in a ParamListInfo struct, > but that's pretty ugly and carries a bunch of costs of its own. Just to make sure I understand correctly, you're talking about setup_unshared_param_list() / setup_param_list()? > What I'm wondering about, given the v10 redesign of expression evaluation, > is whether we couldn't be smarter about this by allowing plpgsql (or other > external users) to skip the ParamListInfo representation altogether, and > instead compile Param references into calls to evaluation functions that > are better tailored to the problem of fetching the desired value. Yea, that seems to make sense. > In the existing execution infrastructure, what seems to make sense is to > have an ExprEvalStep type that has functionality like EEOP_PARAM_EXTERN, > but includes a function pointer to a plpgsql-supplied function having the > same signature as ExecEvalParamExtern. So the execution would look more > or less like > > EEO_CASE(EEOP_PARAM_CALLBACK) > { > op->eval_param(state, op, econtext); > EEO_NEXT(); > } > > and there'd need to be some extra fields (at least a void*) in the op > struct where plpgsql could keep private data. I think I'd redo the parameters to the callback slightly, but generally that sounds sane. Was thinking of something more like One question I have is how we will re-initialize the relevant state between exec_simple_expr() calls. I guess the most realistic one is that the op will have a pointer into an array managed by exec_simple_expr() that can get reset? > The JIT stuff you're working on could just compile an equivalent of the > above, although in the very long term maybe there would be some advantage > to letting add-on modules compile specialized code for such steps. Yea, that's reasonable too. I'm not yet 100% sure whether it'll not be more reasonable to add a few more generic opcodes to the central JITing that external code can emit and core can handle, or a hook like you describe is better. > The immediate problem is how can ExecInitExpr generate such a step? > It can't itself know what to put into the function ptr or the additional > fields. There has to be a way for it to call a plpgsql-supplied > support routine that can construct the eval step. (And we have to > export ExprEvalPushStep, though that doesn't seem like a problem.) Hm. We could have a version of ExecInitExpr() / ExecInitExprRec that first gives a callback chance to handle an operation. That'd make overwriting parts like this quite easy, because we know we want to handle Param nodes anywhere differently. So we'd not have a Param specific routine, but the ability to intercept everything, and defer to ExecInitExprRec() if undesired. > For compiling full-fledged query trees, what I think we could do is > add a method (function pointer) to ParamListInfo and have ExecInitExpr > invoke plan->state->es_param_list_info->compile_param if that's set. > However, that solution doesn't immediately work for compiling simple > expressions because we pass a null "parent" pointer when building > those. > > I thought about instantiating a dummy PlanState and EState to use > just for carrying this info, but that seems pretty ugly. Yea, not a fan. > Another way we could do it is to invent ExecInitExprWithParams() that > takes an additional ParamListInfo pointer, and use that. Rather than > adding yet one more parameter that has to be passed down through > ExecInitExprRec, I suggest that we could waste a bit of space in > struct ExprState and store that value there. Maybe do the same with > the parent pointer so as to reduce the number of recursive parameters. I was thinking something slightly different. Namely that we should have two structs ExprState and ExprBuildState. We can stuff random additions to ExprBuildState without concerns about increasing ExprState's state. > I've not written any code around this idea yet, and am not sure > if it conflicts with what you're trying to do for JIT or further out. > Comments, better ideas? I so far see little reason for concern WRT JIT. Implementing support for a expression step like you describe above is a few minutes worth of work. There might be some annoying rebasing if the patches conflict, but even that ought to be manageable. There *are* some longer term implications that could theoretically become relevant, although I'm not sure problematic. In a good number of workloads the initialization of expression steps and executor trees is the bottleneck. The biggest culprit is tupledesc computations, but also the expression building. With the latter the problem obviously becomes a lot "bigger" with JITing - we don't want to recompile functions all the time. The point where JIT becomes beneficial is a lot lower if you've to do it only once per prepared statement, rather than once per query execution... So I'm basically saying that in my opinion more information has to be built at plan time, long term. Greetings, Andres Freund -
Re: Letting plpgsql in on the fun with the new expression eval stuff
Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-20T17:12:48Z
Andres Freund <andres@anarazel.de> writes: > On 2017-12-19 13:00:41 -0500, Tom Lane wrote: >> I'm looking at ways to get plpgsql expression evaluation to go faster, >> and one thing I'm noticing is the rather large overhead of going through >> ExecEvalParamExtern and plpgsql_param_fetch to get to the useful work >> (exec_eval_datum). > What's the workload you're testing? I'm mildly surprised to see > ExecEvalParamExtern() show up, rather than just plpgsql_param_fetch() & > exec_eval_datum(). Or were you just listing that to specify the > callpath? I'm using several different test cases, but one that shows up the problem is create table foo(a int, b int, c int); insert into foo select 1,2,3 from generate_series(1,1000000); vacuum foo; create or replace function ptest4_rec() returns void as $$ declare r record; t int; begin for r in select * from foo loop t := r.a + r.b + r.c; end loop; end$$ language plpgsql stable; -- then trace this: do $$ begin for i in 1..200 loop perform ptest4_rec(); end loop; end; $$ language plpgsql; The outer do-block is just to get a run long enough for reliable perf statistics. I find these relevant entries in the report: Children Self Samples Shared Object Symbol + 98.29% 4.44% 10827 postgres [.] ExecInterpExpr + 61.82% 3.23% 7878 plpgsql.so [.] exec_eval_expr + 34.64% 3.91% 9521 postgres [.] ExecEvalParamExtern + 29.78% 4.88% 11892 plpgsql.so [.] plpgsql_param_fetch + 23.06% 6.90% 16831 plpgsql.so [.] exec_eval_datum + 6.28% 2.89% 7049 plpgsql.so [.] setup_param_list + 4.02% 4.01% 9774 postgres [.] bms_next_member I think the ridiculous "children" percentage for ExecInterpExpr can be discounted, because that's a result of essentially everything happening inside the outer call of ptest4_rec. But the rest of it can be taken at face value, and it says that ExecEvalParamExtern and plpgsql_param_fetch are each pretty expensive; and both of them are just interface routines doing little useful work. >> We've ameliorated that for DTYPE_VAR variables >> by keeping a pre-set-up copy of their values in a ParamListInfo struct, >> but that's pretty ugly and carries a bunch of costs of its own. > Just to make sure I understand correctly, you're talking about > setup_unshared_param_list() / setup_param_list()? Right, and the stuff in e.g. assign_simple_var() to keep the paramlist up to date. >> So the execution would look more or less like >> op->eval_param(state, op, econtext); >> and there'd need to be some extra fields (at least a void*) in the op >> struct where plpgsql could keep private data. > I think I'd redo the parameters to the callback slightly, but generally > that sounds sane. Was thinking of something more like Um, you left out something here? I don't mind changing the callback signature, but it seems like it generally ought to look the same as the other out-of-line eval functions. > One question I have is how we will re-initialize the relevant state > between exec_simple_expr() calls. I guess the most realistic one is that > the op will have a pointer into an array managed by exec_simple_expr() > that can get reset? I'm not seeing anything that needs to be reset? > Hm. We could have a version of ExecInitExpr() / ExecInitExprRec that > first gives a callback chance to handle an operation. That'd make > overwriting parts like this quite easy, because we know we want to > handle Param nodes anywhere differently. So we'd not have a Param > specific routine, but the ability to intercept everything, and defer to > ExecInitExprRec() if undesired. Yeah, I thought of that idea too, but at least for what I'm trying to do here, it doesn't seem all that helpful. The problem is that plpgsql needs to get its hooks into not only its own direct calls of ExecInitExpr for "simple" expressions, but also calls that occur within arbitrary plan trees that are parsed/planned/executed through SPI. And then things need to happen entirely differently in parallel child workers, which will have "flat" copies of the ParamListInfo. So I really want a hook that's tied to ParamListInfo, and that only makes much sense for PARAM_EXTERN Params. There may be use-cases for the more general hook you're talking about, but I'm not very clear where that would be set up. >> Another way we could do it is to invent ExecInitExprWithParams() that >> takes an additional ParamListInfo pointer, and use that. Rather than >> adding yet one more parameter that has to be passed down through >> ExecInitExprRec, I suggest that we could waste a bit of space in >> struct ExprState and store that value there. Maybe do the same with >> the parent pointer so as to reduce the number of recursive parameters. > I was thinking something slightly different. Namely that we should have > two structs ExprState and ExprBuildState. We can stuff random additions > to ExprBuildState without concerns about increasing ExprState's state. Yeah, perhaps --- there's already some existing fields that don't need to be kept around past the build phase. I haven't done it like that in the patch I'm working on, but no objections if you want to separate things into two structs. On the other hand, I doubt it would save anything, what with palloc's habit of rounding up request sizes. I have a patch nearly ready to submit, but please clarify your comment about what you think the callback function signature should be? regards, tom lane -
Re: Letting plpgsql in on the fun with the new expression eval stuff
Andres Freund <andres@anarazel.de> — 2017-12-20T17:42:43Z
Hi, On 2017-12-20 12:12:48 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > What's the workload you're testing? I'm mildly surprised to see > > ExecEvalParamExtern() show up, rather than just plpgsql_param_fetch() & > > exec_eval_datum(). Or were you just listing that to specify the > > callpath? > > I'm using several different test cases, but one that shows up the problem > is [...] Thanks. > The outer do-block is just to get a run long enough for reliable perf > statistics. I find these relevant entries in the report: > > Children Self Samples Shared Object Symbol > + 98.29% 4.44% 10827 postgres [.] ExecInterpExpr > + 61.82% 3.23% 7878 plpgsql.so [.] exec_eval_expr > + 34.64% 3.91% 9521 postgres [.] ExecEvalParamExtern > + 29.78% 4.88% 11892 plpgsql.so [.] plpgsql_param_fetch > + 23.06% 6.90% 16831 plpgsql.so [.] exec_eval_datum > + 6.28% 2.89% 7049 plpgsql.so [.] setup_param_list > + 4.02% 4.01% 9774 postgres [.] bms_next_member > I think the ridiculous "children" percentage for ExecInterpExpr can be > discounted, because that's a result of essentially everything happening > inside the outer call of ptest4_rec. Right. Unfolding this gives: - 79.73% 5.66% postgres postgres [.] ExecInterpExpr - 92.90% ExecInterpExpr plpgsql_call_handler plpgsql_exec_function exec_stmt_block - exec_stmts - 100.00% exec_for_query - 68.11% exec_stmts - exec_assign_expr - 60.96% ExecInterpExpr - 99.10% ExecEvalParamExtern - plpgsql_param_fetch + 61.82% SPI_fnumber 15.87% SPI_getbinval 14.29% nocachegetattr 4.18% bms_is_member 3.84% SPI_gettypeid 0.90% int4pl + 18.95% SPI_plan_get_cached_plan 11.48% bms_next_member + 5.13% exec_assign_value + 3.13% ReleaseCachedPlan + 16.70% SPI_cursor_fetch + 7.19% CreateTupleDescCopy + 5.49% heap_copytuple 1.26% AllocSetFree + 6.13% 0xffffffffffffffff + 0.71% 0x5624318c8d6f Which certainly seems interesting. The outer ExecInterpExpr() indeed doesn't do that much, it's the inner call that's the most relevant piece. That so much time is spent in SPI_fnumber() and the functions it calls, namely strcmp(), certainly doesn't seem right. I suspect that without addressing that cost, a lot of the other potential improvements aren't going to mean much. Looking at the function cost excluding children: + 7.79% postgres plpgsql.so [.] exec_assign_expr + 7.39% postgres plpgsql.so [.] plpgsql_param_fetch - 6.71% postgres libc-2.25.so [.] __strncmp_sse42 - __strncmp_sse42 + 99.97% SPI_fnumber + 5.66% postgres postgres [.] ExecInterpExpr - 4.60% postgres postgres [.] bms_next_member - bms_next_member - 99.98% exec_assign_expr - 4.59% postgres postgres [.] CreateTupleDescCopy - CreateTupleDescCopy - 92.93% exec_for_query exec_stmts exec_stmt_block plpgsql_exec_function plpgsql_call_handler + 4.40% postgres postgres [.] AllocSetAlloc - 3.77% postgres postgres [.] SPI_fnumber + SPI_fnumber + 3.49% postgres plpgsql.so [.] exec_for_query + 2.93% postgres postgres [.] GetCachedPlan + 2.90% postgres postgres [.] nocachegetattr + 2.85% postgres postgres [.] ExecEvalParamExtern + 2.68% postgres postgres [.] heap_getnext + 2.64% postgres postgres [.] SPI_getbinval + 2.39% postgres plpgsql.so [.] exec_assign_value + 2.22% postgres postgres [.] heap_copytuple + 2.21% postgres plpgsql.so [.] exec_stmts The time in exec_assign_expr() is largely spent doing bms_next_member() via the inlined setup_param_list(). Certainly shows that there's some expression eval related overhead. But it seems the lowest hanging fruits aren't quite there, and wouldn't necessarily be addressed by the type of change we're discussing here. > >> So the execution would look more or less like > >> op->eval_param(state, op, econtext); > >> and there'd need to be some extra fields (at least a void*) in the op > >> struct where plpgsql could keep private data. > > > I think I'd redo the parameters to the callback slightly, but generally > > that sounds sane. Was thinking of something more like > > Um, you left out something here? I don't mind changing the callback > signature, but it seems like it generally ought to look the same as > the other out-of-line eval functions. Yes, I did. Intercontinental travel does wonders. I was thinking that it might be better not to expose the exact details of the expression evaluation step struct to plpgsql etc. I'm kinda forseeing a bit of further churn there that I don't think other parts of the code necessarily need to be affected by. We could have the callback have a signature roughly like Datum callback(void *private_data, ExprContext econtext, bool *isnull); > > One question I have is how we will re-initialize the relevant state > > between exec_simple_expr() calls. I guess the most realistic one is that > > the op will have a pointer into an array managed by exec_simple_expr() > > that can get reset? > > I'm not seeing anything that needs to be reset? I was kind of thinking of the params_dirty business in plpgsql_param_fetch(), setup_param_list() etc. But I'm not quite sure how you're thinking of representing parameters on the plpgsql side after changing the callbacks style. > > Hm. We could have a version of ExecInitExpr() / ExecInitExprRec that > > first gives a callback chance to handle an operation. [ ... ] > > Yeah, I thought of that idea too, but at least for what I'm trying to > do here, it doesn't seem all that helpful. [ ... ] Ah, makes sense. Greetings, Andres Freund -
Re: Letting plpgsql in on the fun with the new expression eval stuff
Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-20T18:13:21Z
Andres Freund <andres@anarazel.de> writes: > On 2017-12-20 12:12:48 -0500, Tom Lane wrote: >> I'm using several different test cases, but one that shows up the problem >> is [...] > Which certainly seems interesting. The outer ExecInterpExpr() indeed > doesn't do that much, it's the inner call that's the most relevant > piece. That so much time is spent in SPI_fnumber() and the functions it > calls, namely strcmp(), certainly doesn't seem right. I suspect that > without addressing that cost, a lot of the other potential improvements > aren't going to mean much. Right, that's mostly coming from the fact that exec_eval_datum on a RECFIELD does SPI_fnumber() every time. I have a patch in the pipeline to address that, but this business with the expression eval API is a separable issue (and it stands out a lot more with that patch in place than it does in HEAD ;-)). >> Um, you left out something here? I don't mind changing the callback >> signature, but it seems like it generally ought to look the same as >> the other out-of-line eval functions. > Yes, I did. Intercontinental travel does wonders. > I was thinking that it might be better not to expose the exact details > of the expression evaluation step struct to plpgsql etc. I'm kinda > forseeing a bit of further churn there that I don't think other parts of > the code necessarily need to be affected by. We could have the callback > have a signature roughly like > Datum callback(void *private_data, ExprContext econtext, bool *isnull); I don't especially like that, because it forces the callback to use a separately allocated private_data area even when the available space in the op step struct would serve perfectly well. In my draft patch I have --- 338,352 ---- Oid paramtype; /* OID of parameter's datatype */ } param; + /* for EEOP_PARAM_CALLBACK */ + struct + { + ExecEvalSubroutine paramfunc; /* add-on evaluation subroutine */ + void *paramarg; /* private data for same */ + int paramid; /* numeric ID for parameter */ + Oid paramtype; /* OID of parameter's datatype */ + } cparam; + /* for EEOP_CASE_TESTVAL/DOMAIN_TESTVAL */ struct { and it turns out that plpgsql isn't bothering with paramarg because paramid and paramtype are all it needs. I doubt that whatever you have in mind to do to struct ExprEvalStep is likely to be so major that it changes what we can keep in these fields. >> I'm not seeing anything that needs to be reset? > I was kind of thinking of the params_dirty business in > plpgsql_param_fetch(), setup_param_list() etc. But I'm not quite sure > how you're thinking of representing parameters on the plpgsql side after > changing the callbacks style. Turns out we can just get rid of that junk altogether. I've redesigned the ParamListInfo API a bit in service of this, and there no longer seems to be a need for plpgsql to use a mutable ParamListInfo at all. Will send a patch in a bit. I need to write an explanation of what all I changed. regards, tom lane -
Re: Letting plpgsql in on the fun with the new expression eval stuff
Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-20T20:44:18Z
I wrote: > Will send a patch in a bit. I need to write an explanation of what all > I changed. OK then. What the attached patch does is: * Create a new step type EEOP_PARAM_CALLBACK (if anyone has a better naming idea, I'm receptive) and add the infrastructure needed for add-on modules to generate that. As discussed, the best control mechanism for that, at least for the immediate use, seems to be to add another hook function to ParamListInfo, which will be called by ExecInitExpr if it's supplied and a PARAM_EXTERN Param is found. For stand-alone expressions, we add a new entry point to allow the ParamListInfo to be specified directly rather than found in the parent plan node's EState. * In passing, remove the "parent" pointer from the arguments to ExecInitExprRec and related functions, instead storing that pointer in a transient field in ExprState. There are now several transient fields there. We discussed splitting them out to a different struct, but that seems like material for a different patch. I had to do something here, though, since otherwise I'd have had to pass down the stand-alone ParamListInfo as another parameter :-(. * Redesign the API for the ParamListInfo paramFetch hook so that the ParamExternData array can be entirely virtual. Typical access to the info about a PARAM_EXTERN Param now looks like if (paramInfo->paramFetch != NULL) prm = paramInfo->paramFetch(paramInfo, paramId, ...); else prm = ¶mInfo->params[paramId - 1]; so that if paramFetch is defined, the core code no longer touches the params[] array at all, and it doesn't have to be there. (It still can be there, if paramFetch wants to use it; but the sole existing user of the hook, plpgsql, doesn't want to.) * This also lets us get rid of ParamListInfo.paramMask, instead leaving it to the paramFetch hook to decide which param IDs should be accessible or not. plpgsql_param_fetch was already doing the identical masking check, so having callers do it too seemed quite redundant and overcomplex. * While I was at it, I added a "speculative" flag to paramFetch that the planner can specify as TRUE to avoid unwanted failures. This solves an ancient problem for plpgsql that it couldn't provide values of non-DTYPE_VAR variables to the planner for fear of triggering premature "record not assigned yet" or "field not found" errors during planning. * Rework plpgsql to get rid of the need for "unshared" parameter lists, by dint of turning the single ParamListInfo per estate into a nearly read-only data structure that doesn't instantiate any per-variable data. Instead, the paramFetch hook controls access to per-variable data and can make the right decisions on the fly, replacing the cases that we used to need multiple ParamListInfos for. This might perhaps have been a performance loss on its own, but by using a paramCompile hook we can bypass plpgsql_param_fetch entirely during normal query execution. (It's now only called when, eg, we copy the ParamListInfo into a cursor portal. copyParamList() or SerializeParamList() effectively instantiate the virtual parameter array as a simple physical array without a paramFetch hook, which is what we want in those cases.) This allows reverting most of commit 6c82d8d1f, though I kept the cosmetic code-consolidation aspects of that (eg the assign_simple_var function). * During compilation of a PARAM_EXTERN expression node, predetermine as much as we can to select one of several execution routines. I've done light performance testing on this, mainly comparing the runtimes for the test functions shown in the second attachment. I see overall gains that are modest but reproducible (in the range of a couple percent) for the "row" (named composite type) cases, and more significant -- around 10% -- for the "record" cases. I attribute the difference to the fact that the "row" cases use DTYPE_VAR variables for the fields, which were already pretty well optimized, whereas the "record" cases use DTYPE_RECFIELD variables which invoked all that overhead we discussed. The fact that this isn't losing on DTYPE_VAR convinces me that it should be a win in all cases. It'd theoretically be possible to split this into three patches, one to change the stuff around ExecInitExpr, one to rejigger the ParamListInfo API, and one to get rid of unshared param lists in plpgsql. However, that would require writing some throwaway code in plpgsql, so I don't especially want to bother. I have another patch in the pipeline that interacts with this, so I'd kind of like to get this committed sooner rather than later. regards, tom lane -
Re: Letting plpgsql in on the fun with the new expression eval stuff
Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-20T23:06:15Z
I wrote: > * Redesign the API for the ParamListInfo paramFetch hook so that the > ParamExternData array can be entirely virtual. Typical access to > the info about a PARAM_EXTERN Param now looks like > > if (paramInfo->paramFetch != NULL) > prm = paramInfo->paramFetch(paramInfo, paramId, ...); > else > prm = ¶mInfo->params[paramId - 1]; > > so that if paramFetch is defined, the core code no longer touches > the params[] array at all, and it doesn't have to be there. I forgot to mention that I dithered about changing the params field from a variable-length array to a pointer, ie, - ParamExternData params[FLEXIBLE_ARRAY_MEMBER]; + ParamExternData *params; Then we could set the pointer to NULL in cases where no physical array is provided, which would be a good thing in terms of helping to catch code that hasn't been updated to the new convention. However, this would force less-than-trivial changes in every place that creates a ParamListInfo. For instance, copyParamList might change from size = offsetof(ParamListInfoData, params) + from->numParams * sizeof(ParamExternData); retval = (ParamListInfo) palloc(size); ... fill retval ... to size = MAXALIGN(sizeof(ParamListInfoData)) + from->numParams * sizeof(ParamExternData); retval = (ParamListInfo) palloc(size); retval->params = (ParamExternData *) ((char *) retval + MAXALIGN(sizeof(ParamListInfoData))); ... fill rest of retval ... That seemed like a pain in the rear, and easy to get wrong (although it could be a lot simpler if you didn't mind doing two pallocs instead of one). Now there aren't that many places in the core code that do this, so it wouldn't be very hard to fix them, but I'm concerned about the possible impact on extension modules. Creating param lists seems like something that a lot of things would have code for. Anyway, I left it as-is, but I'm willing to make the change if people feel the other way is better. regards, tom lane
-
Re: Letting plpgsql in on the fun with the new expression eval stuff
Robert Haas <robertmhaas@gmail.com> — 2017-12-21T03:58:26Z
On Wed, Dec 20, 2017 at 6:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Anyway, I left it as-is, but I'm willing to make the change if > people feel the other way is better. I feel the other way -- let's not add more pointer indirections if it isn't really necessary. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: Letting plpgsql in on the fun with the new expression eval stuff
Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-21T18:00:12Z
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, Dec 20, 2017 at 6:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Anyway, I left it as-is, but I'm willing to make the change if >> people feel the other way is better. > I feel the other way -- let's not add more pointer indirections if it > isn't really necessary. OK --- with any luck, the changes in the paramFetch API will force people to update relevant code anyway. It would only be an issue for code that is not calling paramFetch at all, and such code is broken anyway for dynamic param lists. Pushed without touching that issue. regards, tom lane