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.

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,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;

-- 
  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

-----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.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.

-- 
  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

-----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