Thread: SPI_prepare() doesn't work well ?

SPI_prepare() doesn't work well ?

From
"Hiroshi Inoue"
Date:
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;



Re: [HACKERS] SPI_prepare() doesn't work well ?

From
jwieck@debis.com (Jan Wieck)
Date:
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) #

RE: [HACKERS] SPI_prepare() doesn't work well ?

From
"Hiroshi Inoue"
Date:
> -----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



Re: [HACKERS] SPI_prepare() doesn't work well ?

From
Bruce Momjian
Date:
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
 


Re: [HACKERS] SPI_prepare() doesn't work well ?

From
Bruce Momjian
Date:
> -----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
 


RE: [HACKERS] SPI_prepare() doesn't work well ?

From
"Hiroshi Inoue"
Date:

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



Re: [HACKERS] SPI_prepare() doesn't work well ?

From
Bruce Momjian
Date:
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;