Thread: SPI_prepare() doesn't work well ?
Hello all,
It seems that SPI_prepare() doesn't work well in some cases.
Pawel Pierscionek [pawel@astercity.net] reported about the
following case 1([SQL] drop table in pgsql).
Michael Contzen [mcontzen@dohle.com] reported about the
following case 2(PL/PGSQL bug using aggregates).
You can find it from pgsql-hackers archive.
1. PL/pgSQL can't execute UTILITY commands. SPI_prepare() doesn't copy(save) the utilityStmt member of Query type
nodes,becausecopyObject() is not implemented for nodes of (Create/Destroy etc)Stmt type.
2. Aggregates in PL/pgSQL cause wrong results.
create table t1 (i int, a int, b int); create table t2 (i int, x int, y int);
insert into t1 values(1, 1,10); insert into t1 values(1, 2,10); insert into t1 values(2, 3,10); insert into t1
values(2, 4,10);
create function func1() returns int as ' declare begin insert into t2 select i, sum(a) as x,
sum(b) as y from t1 group by i; return 1; end; ' language 'plpgsql';
select func1(); select * from t2;
The result must be the following.
i| x| y - -+--+-- 1| 3|20 2| 7|20 (2 rows)
But the result is as follows. i| x| y - -+--+-- 1|20|20 2|20|20 (2 rows)
The result of x's are overwritten by y's.
There is a patch for this case at the end of this mail. After I applied it,I got a correct result. But I'm not
surethis patch is right for all aggregate cases.
SPI_prepare() doesn't copy(save) nodes of Agg type correctly. The node of Agg type has a member named aggs. It's a
listincluding Aggreg type nodes which exist in TargetList(i.e Aggreg type nodes are common to aggs member list and
TargetList). AFAIC the common pointer is not copied to the same pointer by copyObject() function. In my patch I
reconstructaggs member node from new(copied) Agg type node. Is it proper to use set_agg_tlist_references() function
to reconstruct aggs member node for Agg type nodes ?
Thanks.
Hiroshi Inoue
Inoue@tpf.co.jp
*** backend/nodes/copyfuncs.c.orig Tue Jan 19 09:07:48 1999
--- backend/nodes/copyfuncs.c Wed Jan 20 14:42:37 1999
***************
*** 506,512 **** CopyPlanFields((Plan *) from, (Plan *) newnode);
! Node_Copy(from, newnode, aggs); Node_Copy(from, newnode, aggstate); return newnode;
--- 506,512 ---- CopyPlanFields((Plan *) from, (Plan *) newnode);
! newnode->aggs = set_agg_tlist_references(newnode); Node_Copy(from, newnode, aggstate); return newnode;
Hiroshi Inoue wrote:
>
> Hello all,
>
> It seems that SPI_prepare() doesn't work well in some cases.
>
> Pawel Pierscionek [pawel@astercity.net] reported about the
> following case 1([SQL] drop table in pgsql).
> Michael Contzen [mcontzen@dohle.com] reported about the
> following case 2(PL/PGSQL bug using aggregates).
> You can find it from pgsql-hackers archive.
>
> 1. PL/pgSQL can't execute UTILITY commands.
> SPI_prepare() doesn't copy(save) the utilityStmt member of
> Query type nodes,because copyObject() is not implemented
> for nodes of (Create/Destroy etc)Stmt type.
Thank's for that. I wondered why PL/pgSQL wasn't able to
execute utility statements. Unfortunately I wasn't able to
track it down the last days, because I had trouble with my
shared libraries (glibc6 and libstdc++ aren't easy-going on
Linux :-).
Knowing where the problem is located saves me a lot of time.
>
> 2. Aggregates in PL/pgSQL cause wrong results.
>
> Is it proper to use set_agg_tlist_references() function to
> reconstruct aggs member node for Agg type nodes ?
Don't know. It is important, that the copy of the tree has
absolutely NO references to anything outside itself. The
parser/rewrite/planner combo creates all plans in the actual
transactions memory context. So they will get destroyed at
transaction end.
SPI_saveplan() simply copies it into another memory context
that lives until the backend dies. It uses the node copy
function for this, so the result of that should be a totally
independed, self referencing tree that can stand alone.
I think that copyObject() should produce an ERROR if there
are nodes it cannot handle correctly (like queries for
utilities). This would prevent the backend crashes from
trying to invoke utilities inside procedural languages.
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#======================================== jwieck@debis.com (Jan Wieck) #
> -----Original Message-----
> From: owner-pgsql-hackers@postgreSQL.org
> [mailto:owner-pgsql-hackers@postgreSQL.org]On Behalf Of Jan Wieck
> Sent: Thursday, January 21, 1999 12:57 AM
> To: Hiroshi Inoue
> Cc: pgsql-hackers@postgreSQL.org; mcontzen@dohle.com
> Subject: Re: [HACKERS] SPI_prepare() doesn't work well ?
>
>
> Hiroshi Inoue wrote:
>
> >
> > Hello all,
> >
> > It seems that SPI_prepare() doesn't work well in some cases.
> >
> > Pawel Pierscionek [pawel@astercity.net] reported about the
> > following case 1([SQL] drop table in pgsql).
> > Michael Contzen [mcontzen@dohle.com] reported about the
> > following case 2(PL/PGSQL bug using aggregates).
> > You can find it from pgsql-hackers archive.
> >
[snip]
> >
> > 2. Aggregates in PL/pgSQL cause wrong results.
> >
> > Is it proper to use set_agg_tlist_references() function to
> > reconstruct aggs member node for Agg type nodes ?
>
> Don't know. It is important, that the copy of the tree has
> absolutely NO references to anything outside itself. The
> parser/rewrite/planner combo creates all plans in the actual
> transactions memory context. So they will get destroyed at
> transaction end.
>
In my patch set_agg_tlist_refereces() function is appiled for
copied(new) Agg type nodes,not for original(old) nodes.
This case is a special case of
How does copyObject() copy multiply referenced objects ?
As to this case,PostgreSQL Executor sets aggno's of Aggreg Nodes
using aggs member node of Agg type nodes.
As a result,aggno's of the Aggreg nodes contained in TargetList
are set because they have common pointers.
So multiply referenced Aggreg type nodes must be copied to new
plan as multiply referenced nodes.
Thanks.
Hiroshi Inoue
Inoue@tpf.co.jp
Applied.
---------------------------------------------------------------------------
Hello all,
It seems that SPI_prepare() doesn't work well in some cases.
Pawel Pierscionek [pawel@astercity.net] reported about the
following case 1([SQL] drop table in pgsql).
Michael Contzen [mcontzen@dohle.com] reported about the
following case 2(PL/PGSQL bug using aggregates).
You can find it from pgsql-hackers archive.
1. PL/pgSQL can't execute UTILITY commands. SPI_prepare() doesn't copy(save) the utilityStmt member of Query type
nodes,becausecopyObject() is not implemented for nodes of (Create/Destroy etc)Stmt type.
2. Aggregates in PL/pgSQL cause wrong results.
create table t1 (i int, a int, b int); create table t2 (i int, x int, y int);
insert into t1 values(1, 1,10); insert into t1 values(1, 2,10); insert into t1 values(2, 3,10); insert into t1
values(2, 4,10);
create function func1() returns int as ' declare begin insert into t2 select i, sum(a) as x,
sum(b) as y from t1 group by i; return 1; end; ' language 'plpgsql';
select func1(); select * from t2;
The result must be the following.
i| x| y - -+--+-- 1| 3|20 2| 7|20 (2 rows)
But the result is as follows. i| x| y - -+--+-- 1|20|20 2|20|20 (2 rows)
The result of x's are overwritten by y's.
There is a patch for this case at the end of this mail. After I applied it,I got a correct result. But I'm not
surethis patch is right for all aggregate cases.
SPI_prepare() doesn't copy(save) nodes of Agg type correctly. The node of Agg type has a member named aggs. It's a
listincluding Aggreg type nodes which exist in TargetList(i.e Aggreg type nodes are common to aggs member list and
TargetList). AFAIC the common pointer is not copied to the same pointer by copyObject() function. In my patch I
reconstructaggs member node from new(copied) Agg type node. Is it proper to use set_agg_tlist_references() function
to reconstruct aggs member node for Agg type nodes ?
Thanks.
Hiroshi Inoue
Inoue@tpf.co.jp
*** backend/nodes/copyfuncs.c.orig Tue Jan 19 09:07:48 1999
--- backend/nodes/copyfuncs.c Wed Jan 20 14:42:37 1999
***************
*** 506,512 **** CopyPlanFields((Plan *) from, (Plan *) newnode);
! Node_Copy(from, newnode, aggs); Node_Copy(from, newnode, aggstate); return newnode;
--- 506,512 ---- CopyPlanFields((Plan *) from, (Plan *) newnode);
! newnode->aggs = set_agg_tlist_references(newnode); Node_Copy(from, newnode, aggstate); return newnode;
-- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610)
853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill,
Pennsylvania19026
> -----Original Message----- > From: owner-pgsql-hackers@postgreSQL.org > [mailto:owner-pgsql-hackers@postgreSQL.org]On Behalf Of Jan Wieck > Sent: Thursday, January 21, 1999 12:57 AM > To: Hiroshi Inoue > Cc: pgsql-hackers@postgreSQL.org; mcontzen@dohle.com > Subject: Re: [HACKERS] SPI_prepare() doesn't work well ? > > > Hiroshi Inoue wrote: > > > > > Hello all, > > > > It seems that SPI_prepare() doesn't work well in some cases. > > > > Pawel Pierscionek [pawel@astercity.net] reported about the > > following case 1([SQL] drop table in pgsql). > > Michael Contzen [mcontzen@dohle.com] reported about the > > following case 2(PL/PGSQL bug using aggregates). > > > You can find it from pgsql-hackers archive. > > > > > [snip] > > > > > > > 2. Aggregates in PL/pgSQL cause wrong results. > > > > > > Is it proper to use set_agg_tlist_references() function to > > > reconstruct aggs member node for Agg type nodes ? > > > > Don't know. It is important, that the copy of the tree has > > absolutely NO references to anything outside itself. The > > parser/rewrite/planner combo creates all plans in the actual > > transactions memory context. So they will get destroyed at > > transaction end. > > > > In my patch set_agg_tlist_refereces() function is appiled for > copied(new) Agg type nodes,not for original(old) nodes. > > This case is a special case of > > How does copyObject() copy multiply referenced objects ? > > As to this case,PostgreSQL Executor sets aggno's of Aggreg Nodes > using aggs member node of Agg type nodes. > As a result,aggno's of the Aggreg nodes contained in TargetList > are set because they have common pointers. > So multiply referenced Aggreg type nodes must be copied to new > plan as multiply referenced nodes. > It is my understanding that this has always been a problem. I have never 100% been confident I understand it. As I remember, there used to be a: Aggreg **qry_agg that was a member of the Query structure, and cause all sorts of problems because the Agg was in the target list _and_ in the qry_agg. That was removed. Looks like I did it, but I don't remember doing it:revision 1.44date: 1998/01/15 19:00:11; author: momjian; state: Exp; lines: +2 -4Remove Query->qry_aggs and qry_numaggs and replace with Query->hasAggs.Pass List* of Aggregsinto executor, and create needed array there.No longer need to double-processs Aggregs with second copy in Query. The removal of that fixed many problems. Can you explain a little more on how multiple Agg references happen. Perhaps we can get a good clean fix for this. Maybe redesign is needed, as I did for the removal of qry_aggs. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> -----Original Message-----
> From: Bruce Momjian [mailto:maillist@candle.pha.pa.us]
> Sent: Friday, January 22, 1999 1:38 PM
> To: Hiroshi Inoue
> Cc: jwieck@debis.com; pgsql-hackers@postgreSQL.org
> Subject: Re: [HACKERS] SPI_prepare() doesn't work well ?
>
>
[snip]
> > > >
> > > > 2. Aggregates in PL/pgSQL cause wrong results.
> > > >
[snip]
>
> It is my understanding that this has always been a problem. I have
> never 100% been confident I understand it.
>
> As I remember, there used to be a:
>
> Aggreg **qry_agg
>
> that was a member of the Query structure, and cause all sorts of
> problems because the Agg was in the target list _and_ in the qry_agg.
> That was removed. Looks like I did it, but I don't remember doing it:
>
> revision 1.44
> date: 1998/01/15 19:00:11; author: momjian; state: Exp;
> lines: +2 -4
> Remove Query->qry_aggs and qry_numaggs and replace with
> Query->hasAggs.
>
> Pass List* of Aggregs into executor, and create needed array there.
> No longer need to double-processs Aggregs with second copy in Query.
>
> The removal of that fixed many problems. Can you explain a little more
> on how multiple Agg references happen. Perhaps we can get a good clean
> fix for this. Maybe redesign is needed, as I did for the removal of
> qry_aggs.
>
Sorry,I don't know details.
The source tree I can trace is as follows..
1.Multiple references to Aggreg nodes [ Fucntion union_planner() in src/backend/optimizer/plan/planner.c ]
if (parse->hasAggs) { result_plan = (Plan *) make_agg(tlist, result_plan);
/* * set the varno/attno entries to the appropriate references
to * the result tuple of the subplans. */ ((Agg *) result_plan)->aggs =
set_agg_tlist_references((Agg *) result_plan);
[ Function set_agg_tlist_references() in optimzer/opt/setrefs.c ]
aggreg_list = nconc( replace_agg_clause(tle->expr, subplanTargetList),
aggreg_list)
; } return aggreg_list;
[ Function replace_agg_clause() in optimizer/opt/setrefs.c ]
else if (IsA(clause, Aggreg)) { return lcons(clause, ^^^^^^^
replace_agg_clause(((Aggreg *) clause)->target,
subplanTargetList));
clause is contained in the return of replace_agg_clause() and so contained in the return of
set_agg_tlist_references().
2.aggno's of aggs list members are set [ Function ExecAgg() in executor/nodeAgg.c ]
alist = node->aggs; for (i = 0; i < nagg; i++) {
aggregates[i]= lfirst(alist); aggregates[i]->aggno = i; alist =
lnext(alist); }
3.aggno's are used [ Function ExecEvalAggreg() in executor/execQual.c called from ExecEvalExpr() ]
static Datum
ExecEvalAggreg(Aggreg *agg, ExprContext *econtext, bool *isNull)
{ *isNull = econtext->ecxt_nulls[agg->aggno]; return econtext->ecxt_values[agg->aggno];
}
Thanks.
Hiroshi Inoue
Inoue@tpf.co.jp
Hiroshi, I believe I have fixed this problem. Let me know if it still doesn't work. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 Hello all, It seems that SPI_prepare() doesn't work well in some cases. Pawel Pierscionek [pawel@astercity.net] reported about the following case 1([SQL] drop table in pgsql). Michael Contzen [mcontzen@dohle.com] reported about the following case 2(PL/PGSQL bug using aggregates). You can find it from pgsql-hackers archive. 1. PL/pgSQL can't execute UTILITY commands. SPI_prepare() doesn't copy(save) the utilityStmt member of Query type nodes,because copyObject() is not implemented for nodes of (Create/Destroy etc)Stmt type. 2. Aggregates in PL/pgSQL cause wrong results. create table t1 (i int, a int, b int); create table t2 (i int, x int, y int); insert into t1 values(1, 1,10); insert into t1 values(1, 2,10); insert into t1 values(2, 3,10); insert into t1 values(2, 4,10); create function func1() returns int as ' declare begin insert into t2 select i, sum(a) as x, sum(b) as y from t1 group by i; return 1; end; ' language 'plpgsql'; select func1(); select * from t2; The result must be the following. i| x| y - -+--+-- 1| 3|20 2| 7|20 (2 rows) But the result is as follows. i| x| y - -+--+-- 1|20|20 2|20|20 (2 rows) The result of x's are overwritten by y's. There is a patch for this case at the end of this mail. After I applied it,I got a correct result. But I'm not sure this patch is right for all aggregate cases. SPI_prepare() doesn't copy(save) nodes of Agg type correctly. The node of Agg type has a member named aggs. It's a list including Aggreg type nodes which exist in TargetList(i.e Aggreg type nodes are common to aggs member list and TargetList). AFAIC the common pointer is not copied to the same pointer by copyObject() function. In my patch I reconstruct aggs member node from new(copied) Agg type node. Is it proper to use set_agg_tlist_references() function to reconstruct aggs member node for Agg type nodes ? Thanks. Hiroshi Inoue Inoue@tpf.co.jp *** backend/nodes/copyfuncs.c.orig Tue Jan 19 09:07:48 1999 --- backend/nodes/copyfuncs.c Wed Jan 20 14:42:37 1999 *************** *** 506,512 **** CopyPlanFields((Plan *) from, (Plan *) newnode); ! Node_Copy(from, newnode, aggs); Node_Copy(from, newnode, aggstate); return newnode; --- 506,512 ---- CopyPlanFields((Plan *) from, (Plan *) newnode); ! newnode->aggs = set_agg_tlist_references(newnode); Node_Copy(from, newnode, aggstate); return newnode;