copyObject() ? again

I'm still not real happy about Hiroshi's proposed reimplementation of
copyObject. It would make copyObject much slower and more memory-
hungry, which I think is a bad thing -- especially since we may use
copyObject more than now if we redesign memory allocation as was
discussed a few weeks ago.

I looked at the specific problem he's reporting (coredump involving
a SELECT inside a plpgsql function), and I believe I understand it;
it's a pretty straightforward order-of-operations bug in copyfuncs.c.
Specifically, CopyPlanFields() tries to construct the new plan node's
list of subplans, but you can't do that until the node-type-specific
fields have all been set, since they may contain references to subplans.
As it stands, a copied plan will have a subPlan list that is missing
some of its subplans.

A simple localized fix would be to rearrange operations inside
copyfuncs.c so that the SS_pull_subplan operation is not done until
the end of the node-type-specific routines for copying plan nodes.

More generally, however, I think that the very existence of the subPlan
list is a bug. It violates the notion that "pointer equality shouldn't
matter" because the subPlan list *must* contain pointers to the actual
member objects of a plan node's other substructure.

Moreover the subPlan list isn't even buying us much --- it wouldn't be
that expensive to scan the substructure directly for plan-type nodes
when necessary, exactly as SS_pull_subplan does. Arguably this would
take less time than is expended on memory allocation to build the
explicit subPlan list. So I suggest getting rid of the subPlan list
entirely, rather than hacking on copyfuncs.

Plans also have an "initPlan" list that seems to have the same kind of
problem. I don't really understand the difference between initPlans
and subPlans --- plannodes.h says
List *initPlan; /* Init Plan nodes (un-correlated expr
* subselects) */
List *subPlan; /* Other SubPlan nodes */
which doesn't convey a whole lot to my mind. Does anyone understand
why this distinction is made? Is it really necessary?

regards, tom lane

Tom Lane wrote:

More generally, however, I think that the very existence of the subPlan
list is a bug. It violates the notion that "pointer equality shouldn't
matter" because the subPlan list *must* contain pointers to the actual
member objects of a plan node's other substructure.

Moreover the subPlan list isn't even buying us much --- it wouldn't be
that expensive to scan the substructure directly for plan-type nodes
when necessary, exactly as SS_pull_subplan does. Arguably this would
take less time than is expended on memory allocation to build the
explicit subPlan list. So I suggest getting rid of the subPlan list
entirely, rather than hacking on copyfuncs.

First, I apologize for any inconveniences in my implementation
of subselect. I agreed that current handling of subPlan lists
is bad, but parent plan need in this list for some initializations
and SUBPLAN_EXPR expressions inside parent plan also need in
corresponding subPlan nodes (to execute them).

Well, this could be good solution:

1. Implement TopPlan node (upmost plan, we told about this and
seems that Jan wanted implement it).
2. Construct list of _all_ subPlan inside planner and
put this list into TopPlan node.
3. Put into Plan->subPlan list of _indices_ of subPlans used
in this Plan (indices in TopPlan->subPlan list).
4. Use indices in SUBPLAN_EXPR, istead of subPlan pointers.
5. Add to EState and ExprContext pointer to TopPlan->subPlan
list (to enable index --> node conversion in
ExecSubPlan() & ExecInitSubPlan()).

Unfortunately, I missed this solution year ago -:(
And more of that, I have no time to implement this now, sorry.

Plans also have an "initPlan" list that seems to have the same kind of
problem. I don't really understand the difference between initPlans

No problems here. As you see this list is just copied in copyfuncs
and this is ok.

and subPlans --- plannodes.h says
List *initPlan; /* Init Plan nodes (un-correlated expr
* subselects) */
List *subPlan; /* Other SubPlan nodes */
which doesn't convey a whole lot to my mind. Does anyone understand
why this distinction is made? Is it really necessary?

Uncorrelated expression subqueries are processed differently
from all other subqueries.

Vadim

Hello all,

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Sent: Wednesday, March 03, 1999 1:52 AM
To: Hiroshi Inoue
Cc: pgsql-hackers
Subject: Re: [HACKERS] copyObject() ? again

I'm still not real happy about Hiroshi's proposed reimplementation of
copyObject. It would make copyObject much slower and more memory-
hungry, which I think is a bad thing --

Yes,it's a big defect of my implementation.
But isn't it simple and reliable ?

Currently,limited persons could implement copyObject() or
could rearrange structures which cause copyObject() bugs.
And it seems that we should use copyObject() carefully.
I don't know whether we are allowed to use copyObject() in
various phase (parser,optimizer etc) without limitation.

OK,there's a possibility to reduce the number of (old,new) pairs
maintained during copyObject() operations.
In fact,if only SubPlan and Aggref type nodes are maintained,
the cases I reported return proper results.
It may be risky but would improve the performance of my
implementation.

Comments ?

As to the 2nd case I reported,it was probably introduced by my
patch. The patch was made to solve other Aggregate bugs.
get_agg_tlist_references() is used to reconstruct aggs member
node of Agg type nodes as SS_pull_subPlan() does in CopyPlan
Fields(). (The name of function was set_agg_tlist_references()
originally. Probably Bruce changed it).
We may be able to patch by following related code closely.
But it seems strange that copying objects requires such
a complicated procedure.

Thanks in advance.

Hiroshi Inoue
Inoue@tpf.co.jp

Hello all,

As to the 2nd case I reported,it was probably introduced by my
patch. The patch was made to solve other Aggregate bugs.
get_agg_tlist_references() is used to reconstruct aggs member
node of Agg type nodes as SS_pull_subPlan() does in CopyPlan
Fields(). (The name of function was set_agg_tlist_references()
originally. Probably Bruce changed it).
We may be able to patch by following related code closely.
But it seems strange that copying objects requires such
a complicated procedure.

I made a patch for the 2nd case I reported.
I applied my trial implementation of copyObject() partially to
the relation between Agg and Aggref type nodes. This patch
solves both this bug and an old bug solved by my old patch
[Subject:SPI_prepare() doesn't work well ? ].

Thanks in advance.

Hiroshi Inoue
Inoue@tpf.co.jp

*** backend/nodes/copyfuncs.c.orig	Tue Feb 23 17:00:26 1999
--- backend/nodes/copyfuncs.c	Thu Mar  4 16:32:21 1999
***************
*** 31,37 ****
--- 31,78 ----
  #include "storage/lmgr.h"
  #include "optimizer/planmain.h"

+ #include "access/xact.h"
+
  /*
+  *	Local structures for maintaining the list of
+  *		(old -> new) pairs
+  *		during copyObject operations
+  */
+
+ typedef
+ struct
+ {
+ 	const void	*old;
+ 	void		*new;
+ } old_new_pair;
+
+ typedef
+ struct
+ {
+ 	int	maxcnt;
+ 	int	count;
+ 	old_new_pair old_new[1];
+ } old_new_Hash;
+
+ typedef
+ struct
+ {
+ 	bool		inuse;
+ 	TransactionId	execXID;
+ 	unsigned long	alloconce;
+ 	List		*first;
+ 	List		*last;
+ } copyContext;
+
+ static	copyContext common_context = {false, 0, 0, NIL, NIL};
+
+ static bool appendToCopyContext(const void *, void *, copyContext *);
+ static void initializeCopyContext(copyContext *, int);
+ static void releaseCopyContext(copyContext *);
+ static void *alreadyCopiedTo(const void *, const copyContext *);
+ static bool isContextStillAlive(const copyContext *);
+
+ /*
   * listCopy
   *	  this copy function only copies the "lcons-cells" of the list but not
   *	  its contents. (good for list of pointers as well as list of
integers).
***************
*** 472,483 ****
  static Agg *
  _copyAgg(Agg *from)
  {
  	Agg		   *newnode = makeNode(Agg);

CopyPlanFields((Plan *) from, (Plan *) newnode);

! newnode->aggs = get_agg_tlist_references(newnode);

return newnode;
}

--- 513,535 ----
  static Agg *
  _copyAgg(Agg *from)
  {
+ 	bool		init_here = false;
+ 	copyContext	*copycontext = &common_context;
  	Agg		   *newnode = makeNode(Agg);

+ 	if (!isContextStillAlive(copycontext))
+ 	{
+ 		initializeCopyContext(copycontext,0);
+ 		init_here = true;
+ 	}
+
  	CopyPlanFields((Plan *) from, (Plan *) newnode);

! Node_Copy(from, newnode, aggs);

+ 	if (init_here)
+ 		releaseCopyContext(copycontext);
+
  	return newnode;
  }

***************
*** 871,878 ****
static Aggref *
_copyAggref(Aggref *from)
{
! Aggref *newnode = makeNode(Aggref);

  	/* ----------------
  	 *	copy remainder of node
  	 * ----------------
--- 923,941 ----
  static Aggref *
  _copyAggref(Aggref *from)
  {
! 	copyContext	*copycontext = &common_context;
! 	Aggref	   	*newnode;

+ 	if (copycontext->inuse)
+ 	{
+ 		newnode = (Aggref *)alreadyCopiedTo(from, copycontext);
+ 		if (newnode)	return newnode;
+ 	}
+
+ 	newnode = makeNode(Aggref);
+ 	if (copycontext->inuse)
+ 		appendToCopyContext(from, newnode, copycontext);
+
  	/* ----------------
  	 *	copy remainder of node
  	 * ----------------
***************
*** 1868,1871 ****
--- 1931,2032 ----
  			break;
  	}
  	return retval;
+ }
+
+ /*
+  *	static functions for maintaining the list of
+  *		(old -> new) pairs
+  *		during copyObject operations
+  */
+ #define	_Count_Per_Elem_	127
+ static void initializeCopyContext(copyContext *copycontext, int alloconce)
+ {
+ 	copycontext->inuse   = true;
+ 	copycontext->execXID = GetCurrentTransactionId();
+ 	if (alloconce>0)
+ 		copycontext->alloconce = alloconce;
+ 	else
+ 		copycontext->alloconce = _Count_Per_Elem_;
+ 	copycontext->first = copycontext->last = NIL;
+ }
+
+ static void releaseCopyContext(copyContext *copycontext)
+ {
+ 	List		*l;
+
+ 	if (!copycontext)	return;
+ 	foreach(l, copycontext->first)
+ 		pfree(lfirst(l));
+ 	freeList(copycontext->first);
+ 	copycontext->inuse = false;
+ 	copycontext->alloconce = 0;
+ 	StoreInvalidTransactionId(&copycontext->execXID);
+ 	copycontext->first = copycontext->last = NIL;
+ }
+
+ static bool appendToCopyContext(const void *old, void *new, copyContext
*copycontext)
+ {
+ 	old_new_Hash	*copyHash;
+ 	List		*newl;
+ 	bool		newList = false;
+
+ 	if (!copycontext)		return false;
+ 	if ((!old) || (!new))		return false;
+ 	if (!copycontext->last)
+ 		newList = true;
+ 	else
+ 	{
+ 		copyHash = (old_new_Hash *)lfirst(copycontext->last);
+ 		if (copyHash->count >= copyHash->maxcnt)
+ 			newList = true;
+ 	}
+ 	if (newList)
+ 	{
+ 		copyHash = (old_new_Hash *) palloc(
+ 			sizeof(old_new_Hash) + sizeof(old_new_pair) *
+ 				(copycontext->alloconce - 1) );
+ 		if (!copyHash)		return false;
+ 		copyHash->maxcnt = copycontext->alloconce;
+ 		copyHash->count  = 0;
+ 		if (copycontext->last)
+ 		{
+ 			newl = lcons(copyHash, NIL);
+ 			lnext(copycontext->last) = newl;
+ 			copycontext->last = newl;
+ 		}
+ 		else
+ 		{
+ 			newl = lcons(copyHash, NIL);
+ 			copycontext->first = copycontext->last = newl;
+ 		}
+ 	}
+
+ 	copyHash->old_new[copyHash->count].old = old;
+ 	copyHash->old_new[copyHash->count].new = new;
+ 	copyHash->count++;
+ 	return true;
+ }
+ static void *alreadyCopiedTo(const void *obj, const copyContext
*copycontext)
+ {
+ 	List			*l;
+ 	int			i;
+ 	const old_new_Hash	*copyHash;
+
+ 	if (!copycontext)	return NULL;
+ 	foreach(l, copycontext->first)
+ 	{
+ 		copyHash = (const old_new_Hash *)lfirst(l);
+ 		for (i=0; i<copyHash->count; i++)
+ 		{
+ 			if ( copyHash->old_new[i].old == obj )
+ 				return copyHash->old_new[i].new;
+ 		}
+ 	}
+ 	return NULL;
+ }
+
+ static bool isContextStillAlive(const copyContext *copycontext)
+ {
+ 	return ( copycontext->inuse &&
+ 	TransactionIdIsCurrentTransactionId(copycontext->execXID) ) ;
  }

Just sending this over to you in case it is helpful in the future.

Tom Lane wrote:

More generally, however, I think that the very existence of the subPlan
list is a bug. It violates the notion that "pointer equality shouldn't
matter" because the subPlan list *must* contain pointers to the actual
member objects of a plan node's other substructure.

Moreover the subPlan list isn't even buying us much --- it wouldn't be
that expensive to scan the substructure directly for plan-type nodes
when necessary, exactly as SS_pull_subplan does. Arguably this would
take less time than is expended on memory allocation to build the
explicit subPlan list. So I suggest getting rid of the subPlan list
entirely, rather than hacking on copyfuncs.

First, I apologize for any inconveniences in my implementation
of subselect. I agreed that current handling of subPlan lists
is bad, but parent plan need in this list for some initializations
and SUBPLAN_EXPR expressions inside parent plan also need in
corresponding subPlan nodes (to execute them).

Well, this could be good solution:

1. Implement TopPlan node (upmost plan, we told about this and
seems that Jan wanted implement it).
2. Construct list of _all_ subPlan inside planner and
put this list into TopPlan node.
3. Put into Plan->subPlan list of _indices_ of subPlans used
in this Plan (indices in TopPlan->subPlan list).
4. Use indices in SUBPLAN_EXPR, istead of subPlan pointers.
5. Add to EState and ExprContext pointer to TopPlan->subPlan
list (to enable index --> node conversion in
ExecSubPlan() & ExecInitSubPlan()).

Unfortunately, I missed this solution year ago -:(
And more of that, I have no time to implement this now, sorry.

Plans also have an "initPlan" list that seems to have the same kind of
problem. I don't really understand the difference between initPlans

No problems here. As you see this list is just copied in copyfuncs
and this is ok.

and subPlans --- plannodes.h says
List *initPlan; /* Init Plan nodes (un-correlated expr
* subselects) */
List *subPlan; /* Other SubPlan nodes */
which doesn't convey a whole lot to my mind. Does anyone understand
why this distinction is made? Is it really necessary?

Uncorrelated expression subqueries are processed differently
from all other subqueries.

Vadim

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