Re: [PATCHES] refactor CreateTupleDescCopy()
This patch was withdrawn by the author. --- Neil Conway wrote: > Tom Lane <[EMAIL PROTECTED]> writes: > > That would be okay with me. It might be a good idea to change the > > name completely (perhaps CopyTupleDesc() ?) as a means of catching > > places that aren't correctly updated. > > Done, and done -- a revised patch is attached. > > -Neil > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 5: Have you checked our extensive FAQ? > >http://www.postgresql.org/docs/faqs/FAQ.html -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] refactor CreateTupleDescCopy()
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches I will try to apply it within the next 48 hours. --- Neil Conway wrote: > Tom Lane <[EMAIL PROTECTED]> writes: > > That would be okay with me. It might be a good idea to change the > > name completely (perhaps CopyTupleDesc() ?) as a means of catching > > places that aren't correctly updated. > > Done, and done -- a revised patch is attached. > > -Neil > [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 5: Have you checked our extensive FAQ? > >http://www.postgresql.org/docs/faqs/FAQ.html -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] refactor CreateTupleDescCopy()
Tom Lane <[EMAIL PROTECTED]> writes:
> That would be okay with me. It might be a good idea to change the
> name completely (perhaps CopyTupleDesc() ?) as a means of catching
> places that aren't correctly updated.
Done, and done -- a revised patch is attached.
-Neil
Index: src/backend/access/common/tupdesc.c
===
RCS file: /var/lib/cvs/pgsql-server/src/backend/access/common/tupdesc.c,v
retrieving revision 1.100
diff -c -r1.100 tupdesc.c
*** src/backend/access/common/tupdesc.c 25 Sep 2003 06:57:56 - 1.100
--- src/backend/access/common/tupdesc.c 20 Nov 2003 21:43:47 -
***
*** 96,204
}
/*
! * CreateTupleDescCopy
*
! * This function creates a new TupleDesc by copying from an existing
! * TupleDesc
! *
! * !!! Constraints are not copied !!!
*
*/
TupleDesc
! CreateTupleDescCopy(TupleDesc tupdesc)
{
! TupleDesc desc;
! int i,
! size;
! desc = (TupleDesc) palloc(sizeof(struct tupleDesc));
! desc->natts = tupdesc->natts;
! if (desc->natts > 0)
! {
! size = desc->natts * sizeof(Form_pg_attribute);
! desc->attrs = (Form_pg_attribute *) palloc(size);
! for (i = 0; i < desc->natts; i++)
! {
! desc->attrs[i] = (Form_pg_attribute) palloc(ATTRIBUTE_TUPLE_SIZE);
! memcpy(desc->attrs[i], tupdesc->attrs[i], ATTRIBUTE_TUPLE_SIZE);
! desc->attrs[i]->attnotnull = false;
! desc->attrs[i]->atthasdef = false;
! }
! }
! else
! desc->attrs = NULL;
! desc->constr = NULL;
! desc->tdhasoid = tupdesc->tdhasoid;
! return desc;
! }
!
! /*
! * CreateTupleDescCopyConstr
! *
! * This function creates a new TupleDesc by copying from an existing
! * TupleDesc (with Constraints)
! *
! */
! TupleDesc
! CreateTupleDescCopyConstr(TupleDesc tupdesc)
! {
! TupleDesc desc;
! TupleConstr *constr = tupdesc->constr;
! int i,
! size;
! desc = (TupleDesc) palloc(sizeof(struct tupleDesc));
! desc->natts = tupdesc->natts;
! if (desc->natts > 0)
! {
! size = desc->natts * sizeof(Form_pg_attribute);
! desc->attrs = (Form_pg_attribute *) palloc(size);
! for (i = 0; i < desc->natts; i++)
{
! desc->attrs[i] = (Form_pg_attribute) palloc(ATTRIBUTE_TUPLE_SIZE);
! memcpy(desc->attrs[i], tupdesc->attrs[i], ATTRIBUTE_TUPLE_SIZE);
}
}
else
! desc->attrs = NULL;
! if (constr)
{
! TupleConstr *cpy = (TupleConstr *) palloc(sizeof(TupleConstr));
! cpy->has_not_null = constr->has_not_null;
! if ((cpy->num_defval = constr->num_defval) > 0)
{
! cpy->defval = (AttrDefault *) palloc(cpy->num_defval * sizeof(AttrDefault));
! memcpy(cpy->defval, constr->defval, cpy->num_defval * sizeof(AttrDefault));
! for (i = cpy->num_defval - 1; i >= 0; i--)
{
if (constr->defval[i].adbin)
! cpy->defval[i].adbin = pstrdup(constr->defval[i].adbin);
}
}
! if ((cpy->num_check = constr->num_check) > 0)
{
! cpy->check = (ConstrCheck *) palloc(cpy->num_check * sizeof(ConstrCheck));
! memcpy(cpy->check, constr->check, cpy->num_check * sizeof(ConstrCheck));
! for (i = cpy->num_check - 1; i >= 0; i--)
{
if (constr->check[i].ccname)
! cpy->check[i].ccname = pstrdup(constr->check[i].ccname);
if (constr->check[i].ccbin)
! cpy->check[i].ccbin = pstrdup(constr->check[i].ccbin);
}
}
! desc->constr = cpy;
}
- else
- desc->constr = NULL;
! desc->tdhasoid = tupdesc->tdhasoid;
! return desc;
}
void
--- 96,183
}
/*
! * CopyTupleDesc
*
! * This function creates a new TupleDesc by copying from an
! * existing TupleDesc. Iff 'copyConstr' is true, the constraints
! * on the input TupleDesc are also copied.
*
*/
TupleDesc
! CopyTupleDesc(TupleDesc tupDesc, bool copyConstr)
{
! TupleDesc result;
! int i;
! result = (TupleDesc) palloc(sizeof(*result));
! result->natts = tupDesc->natts;
! result->tdhasoid = tupDesc->tdhasoid;
! result->constr = NULL;
! if (result->natts > 0)
! {
! int size;
! size = result->natts * sizeof(Form_pg_attribute);
! result->attrs = (Form_pg_attribute *) palloc(size);
! for (i = 0; i < result->natts; i++)
{
! result->attrs[i] = (Form_pg_attribute) palloc(ATTRIBUTE_TUPLE_SIZE);
! memcpy(result->attrs[i], tupDesc->attrs[i], ATTRIBUTE_TUPLE_SIZE);
!
! /*
! * If we're not copying constraints, fix up the attributes
! * to remove NOT NULL or DEFAULT indicators.
! */
! if (!copyConstr)
! {
! result->attrs[i]->attnotnull = false;
! result->attrs[i]->atthasdef = false;
! }
Re: [PATCHES] refactor CreateTupleDescCopy()
Neil Conway <[EMAIL PROTECTED]> writes: > Now that I think about > it, we could also just change the API to remove > CreateTupleDescCopyConstr(), and replace it with an additional bool > parameter to CreateTupleDescCopy(). That would be okay with me. It might be a good idea to change the name completely (perhaps CopyTupleDesc() ?) as a means of catching places that aren't correctly updated. (I worry about add-on modules that might not get recompiled between versions; they'd still link, but then crash, if the routine name is the same.) regards, tom lane ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] refactor CreateTupleDescCopy()
Tom Lane <[EMAIL PROTECTED]> writes: > I think this is taking the "avoid duplicated code" mantra a little > far. You've defined a subroutine that returns a TupleDesc that is > internally inconsistent and cannot usefully be used for anything > until it's fixed by the parent routines. Fair enough. Another way to refactor this would be to implement both CreateTupleDescCopy() and CreateTupleDescCopyConstr() in terms of an internal function that takes a bool indicating whether or not to include constraints in the returned TupleDesc. Now that I think about it, we could also just change the API to remove CreateTupleDescCopyConstr(), and replace it with an additional bool parameter to CreateTupleDescCopy(). I'm leaning toward doing the latter, and updating the 30 odd call sites this would affect. Does that sound good? -Neil ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] refactor CreateTupleDescCopy()
Neil Conway <[EMAIL PROTECTED]> writes: > This patch refactors CreateTupleDescCopy() and > CreateTupleDescCopyConstr() to remove some duplicated code, and clean > things up a little bit. I think this is taking the "avoid duplicated code" mantra a little far. You've defined a subroutine that returns a TupleDesc that is internally inconsistent and cannot usefully be used for anything until it's fixed by the parent routines. That seems to me to make the module more complex and confusing rather than less so. The amount of code saved is IMHO not worth that price. regards, tom lane ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
