Letting plpgsql in on the fun with the new expression eval stuff

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

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

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

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

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 = &paramInfo->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

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 = &paramInfo->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

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

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