Thread: explain doesn't work with execute using
Hello
I found following bug - using explain in stored procedures like:
CREATE OR REPLACE FUNCTION test(int)
RETURNS void AS $$
DECLARE s varchar;
BEGIN FOR s IN EXECUTE 'EXPLAIN SELECT * FROM o WHERE a = $1+1' USING $1 LOOP RAISE NOTICE '%', s; END LOOP;
END; $$
LANGUAGE plpgsql;
produce wrong result. Real plan is correct, etc variables are
substituted. Bud this explain show variables. Reason is in difference
in pflags. Planner works with PARAM_FLAG_CONST's variables, but
explain (proc ExplainQuery) get variables from Portal, where flag
PARAM_FLAG_CONST is lost.
Portal
SPI_cursor_open_with_args(const char *name, const char *src,
int nargs, Oid *argtypes, Datum
*Values,const
char *Nulls, bool read_only, int
cursorOptions)
{ ... paramLI = _SPI_convert_params(nargs, argtypes,
Values, Nulls,
PARAM_FLAG_CONST);
// variables are correct
but result = SPI_cursor_open(name, &plan, Values, Nulls, read_only); // result->portalParams lost flags
Portal
SPI_cursor_open(const char *name, SPIPlanPtr plan, Datum *Values, const char *Nulls,
bool read_only)
{ CachedPlanSource *plansource; CachedPlan *cplan; List *stmt_list; char
*query_string; ParamListInfo paramLI;.... if (plan->nargs > 0) { /*
sizeof(ParamListInfoData)includes the first array
element */ paramLI = (ParamListInfo) palloc(sizeof(ParamListInfoData) +
(plan->nargs - 1) *sizeof(ParamExternData)); paramLI->numParams = plan->nargs;
for (k = 0; k < plan->nargs; k++) { ParamExternData *prm =
¶mLI->params[k];
prm->ptype = plan->argtypes[k];
/***************************************************/ prm->pflags = 0; // correct flags is
overwritten
/***************************************************/ prm->isnull = (Nulls && Nulls[k] == 'n');
if (prm->isnull) { /* nulls just copy */
prm->value = Values[k]; }
so this is strange bug - EXECUTE USING use well plan, but isn't
possible verify it.
Regards
Pavel Stehule
"Pavel Stehule" <pavel.stehule@gmail.com> writes:
> I found following bug - using explain in stored procedures like:
> ...
> produce wrong result. Real plan is correct, etc variables are
> substituted. Bud this explain show variables.
This seems to be correctable with a one-line patch: make SPI_cursor_open
set the CONST flag on parameters it puts into the portal (attached).
I'm not entirely sure if it's a good idea or not --- comments?
regards, tom lane
Index: src/backend/executor/spi.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/spi.c,v
retrieving revision 1.195
diff -c -r1.195 spi.c
*** src/backend/executor/spi.c 12 May 2008 20:02:00 -0000 1.195
--- src/backend/executor/spi.c 1 Jun 2008 15:33:13 -0000
***************
*** 997,1003 **** ParamExternData *prm = ¶mLI->params[k]; prm->ptype = plan->argtypes[k];
! prm->pflags = 0; prm->isnull = (Nulls && Nulls[k] == 'n'); if (prm->isnull)
{
--- 997,1010 ---- ParamExternData *prm = ¶mLI->params[k]; prm->ptype = plan->argtypes[k];
! /*
! * We mark the parameters as const. This has no effect for simple
! * execution of a plan, but if more planning happens within the
! * portal (eg via EXPLAIN), the effect will be to treat the
! * parameters as constants. This is good and correct as long as
! * no plan generated inside the portal is used outside it.
! */
! prm->pflags = PARAM_FLAG_CONST; prm->isnull = (Nulls && Nulls[k] == 'n'); if
(prm->isnull) {
hello
2008/6/1 Tom Lane <tgl@sss.pgh.pa.us>:
> "Pavel Stehule" <pavel.stehule@gmail.com> writes:
>> I found following bug - using explain in stored procedures like:
>> ...
>> produce wrong result. Real plan is correct, etc variables are
>> substituted. Bud this explain show variables.
>
> This seems to be correctable with a one-line patch: make SPI_cursor_open
> set the CONST flag on parameters it puts into the portal (attached).
> I'm not entirely sure if it's a good idea or not --- comments?
We can do less invasive patch - it's much more ugly, but don't change
any other behave. I am afraid, so one-line patch can change behave of
explain statements in some cases where using variables is correct.
Regards
Pavel Stehule
>
> regards, tom lane
>
> Index: src/backend/executor/spi.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/backend/executor/spi.c,v
> retrieving revision 1.195
> diff -c -r1.195 spi.c
> *** src/backend/executor/spi.c 12 May 2008 20:02:00 -0000 1.195
> --- src/backend/executor/spi.c 1 Jun 2008 15:33:13 -0000
> ***************
> *** 997,1003 ****
> ParamExternData *prm = ¶mLI->params[k];
>
> prm->ptype = plan->argtypes[k];
> ! prm->pflags = 0;
> prm->isnull = (Nulls && Nulls[k] == 'n');
> if (prm->isnull)
> {
> --- 997,1010 ----
> ParamExternData *prm = ¶mLI->params[k];
>
> prm->ptype = plan->argtypes[k];
> ! /*
> ! * We mark the parameters as const. This has no effect for simple
> ! * execution of a plan, but if more planning happens within the
> ! * portal (eg via EXPLAIN), the effect will be to treat the
> ! * parameters as constants. This is good and correct as long as
> ! * no plan generated inside the portal is used outside it.
> ! */
> ! prm->pflags = PARAM_FLAG_CONST;
> prm->isnull = (Nulls && Nulls[k] == 'n');
> if (prm->isnull)
> {
>
Attachment
"Pavel Stehule" <pavel.stehule@gmail.com> writes:
> 2008/6/1 Tom Lane <tgl@sss.pgh.pa.us>:
>> This seems to be correctable with a one-line patch: make SPI_cursor_open
>> set the CONST flag on parameters it puts into the portal (attached).
>> I'm not entirely sure if it's a good idea or not --- comments?
> We can do less invasive patch - it's much more ugly, but don't change
> any other behave. I am afraid, so one-line patch can change behave of
> explain statements in some cases where using variables is correct.
If you can name a case where that is correct, then I'll worry about
this, but offhand I don't see one.
What do you think a "less invasive" patch would be, anyway? I don't
buy that, say, having SPI_cursor_open_with_args set the flag but
SPI_cursor_open not do so is any safer. There is no difference between
the two as to what might get executed, so if there's a problem then
both would be at risk.
regards, tom lane
2008/6/1 Tom Lane <tgl@sss.pgh.pa.us>: > "Pavel Stehule" <pavel.stehule@gmail.com> writes: >> 2008/6/1 Tom Lane <tgl@sss.pgh.pa.us>: >>> This seems to be correctable with a one-line patch: make SPI_cursor_open >>> set the CONST flag on parameters it puts into the portal (attached). >>> I'm not entirely sure if it's a good idea or not --- comments? > >> We can do less invasive patch - it's much more ugly, but don't change >> any other behave. I am afraid, so one-line patch can change behave of >> explain statements in some cases where using variables is correct. > > If you can name a case where that is correct, then I'll worry about > this, but offhand I don't see one. this case - there variables are correct postgres=# create or replace function foo(_a integer) returns void as $$declare s varchar; begin for s in explain select * from o where a = _a loop raise notice '%', s; end loop; end; $$ language plpgsql; CREATE FUNCTION Time: 43,138 ms postgres=# select foo(20); NOTICE: Index Scan using o_pkey on o (cost=0.00..8.27 rows=1 width=4) NOTICE: Index Cond: (a = 20) -- wrong :(foo ----- (1 row) > > What do you think a "less invasive" patch would be, anyway? I don't > buy that, say, having SPI_cursor_open_with_args set the flag but > SPI_cursor_open not do so is any safer. There is no difference between > the two as to what might get executed, so if there's a problem then > both would be at risk. SPI_cursor_open_with_args is new function, it's used only in FOR EXECUTE statement - and in this context variables are really constants. Pavel > > regards, tom lane >
"Pavel Stehule" <pavel.stehule@gmail.com> writes:
> 2008/6/1 Tom Lane <tgl@sss.pgh.pa.us>:
>> What do you think a "less invasive" patch would be, anyway? I don't
>> buy that, say, having SPI_cursor_open_with_args set the flag but
>> SPI_cursor_open not do so is any safer. There is no difference between
>> the two as to what might get executed, so if there's a problem then
>> both would be at risk.
> SPI_cursor_open_with_args is new function, it's used only in FOR
> EXECUTE statement - and in this context variables are really
> constants.
This argument seems entirely bogus. How are they any more constant
than in the other case? The value isn't going to change for the life
of the portal in either case.
ISTM you're expecting EXPLAIN to behave in some magic way that has
got little to do with "correctness".
regards, tom lane
2008/6/1 Tom Lane <tgl@sss.pgh.pa.us>: > "Pavel Stehule" <pavel.stehule@gmail.com> writes: >> 2008/6/1 Tom Lane <tgl@sss.pgh.pa.us>: >>> What do you think a "less invasive" patch would be, anyway? I don't >>> buy that, say, having SPI_cursor_open_with_args set the flag but >>> SPI_cursor_open not do so is any safer. There is no difference between >>> the two as to what might get executed, so if there's a problem then >>> both would be at risk. > >> SPI_cursor_open_with_args is new function, it's used only in FOR >> EXECUTE statement - and in this context variables are really >> constants. > > This argument seems entirely bogus. How are they any more constant > than in the other case? The value isn't going to change for the life > of the portal in either case. this is true Tom, but problem is in EXPLAIN. I thing, so my and your solution are little bit incorect. We solve result, not reason. We have problem, bacause plan doesn't carry parameter's flags, and with EXPLAIN planner is called two times with different param's flags. > > ISTM you're expecting EXPLAIN to behave in some magic way that has > got little to do with "correctness". > It is first time when I do some with EXPLAIN and I don't understad well, but I would correct EXPLAIN output. When original plan use variables I would to see variables in plan and when plan use constant I would to see constant. > regards, tom lane >
"Pavel Stehule" <pavel.stehule@gmail.com> writes:
> 2008/6/1 Tom Lane <tgl@sss.pgh.pa.us>:
>> This argument seems entirely bogus. How are they any more constant
>> than in the other case? The value isn't going to change for the life
>> of the portal in either case.
> this is true Tom, but problem is in EXPLAIN. I thing, so my and your
> solution are little bit incorect. We solve result, not reason. We have
> problem, bacause plan doesn't carry parameter's flags, and with
> EXPLAIN planner is called two times with different param's flags.
[ shrug... ] Well, I'm willing to change the code as you suggest,
but if you're thinking that this will make EXPLAIN exactly reproduce
the plan that would be generated for a plain SELECT invoked in the
same context, you're still mistaken. It doesn't account for the
effects of the fast-start-cursor option. And for what you seem to
want EXPLAIN to do here, it probably shouldn't. The whole thing
seems pretty unprincipled to me ...
regards, tom lane
2008/6/1 Tom Lane <tgl@sss.pgh.pa.us>: > "Pavel Stehule" <pavel.stehule@gmail.com> writes: >> 2008/6/1 Tom Lane <tgl@sss.pgh.pa.us>: >>> This argument seems entirely bogus. How are they any more constant >>> than in the other case? The value isn't going to change for the life >>> of the portal in either case. > >> this is true Tom, but problem is in EXPLAIN. I thing, so my and your >> solution are little bit incorect. We solve result, not reason. We have >> problem, bacause plan doesn't carry parameter's flags, and with >> EXPLAIN planner is called two times with different param's flags. > > [ shrug... ] Well, I'm willing to change the code as you suggest, > but if you're thinking that this will make EXPLAIN exactly reproduce > the plan that would be generated for a plain SELECT invoked in the > same context, you're still mistaken. It doesn't account for the > effects of the fast-start-cursor option. And for what you seem to > want EXPLAIN to do here, it probably shouldn't. The whole thing > seems pretty unprincipled to me ... > It's not best, and it's surprise for me, so EXPLAIN can be different then real plan. It's basic tool for identification of plpgsql procedure's performance problems. So this can be short fix and point for ToDo? Regards Pavel Stehule > regards, tom lane >