Thread: Re: Tuple-valued datums on Alpha (was Re: 7.1 on DEC/Alpha)
I wrote:
> ... What I'm thinking about doing is setting typmod of
> an entire-tuple function argument to sizeof(Pointer), rather than
> the default -1, to indicate that a pointer representation is being
> used. Comments, hackers?
Here is a patch to current sources along this line. I have not
committed it, since I'm not sure it does the job. It doesn't break
the regress tests on my machine, but does it fix them on Alphas?
Please apply it locally and let me know what you find.
regards, tom lane
*** src/backend/parser/parse_func.c.orig Fri Dec 15 14:22:03 2000
--- src/backend/parser/parse_func.c Tue Dec 26 14:17:09 2000
***************
*** 442,451 ****
/*
* for func(relname), the param to the function is the tuple
! * under consideration. we build a special VarNode to reflect
* this -- it has varno set to the correct range table entry,
* but has varattno == 0 to signal that the whole tuple is the
! * argument.
*/
if (rte->relname == NULL)
elog(ERROR,
--- 442,453 ----
/*
* for func(relname), the param to the function is the tuple
! * under consideration. We build a special VarNode to reflect
* this -- it has varno set to the correct range table entry,
* but has varattno == 0 to signal that the whole tuple is the
! * argument. Also, it has typmod set to sizeof(Pointer) to
! * signal that the runtime representation will be a pointer
! * not an Oid.
*/
if (rte->relname == NULL)
elog(ERROR,
***************
*** 453,459 ****
toid = typenameTypeId(rte->relname);
/* replace it in the arg list */
! lfirst(i) = makeVar(vnum, 0, toid, -1, sublevels_up);
}
else if (!attisset)
toid = exprType(arg);
--- 455,465 ----
toid = typenameTypeId(rte->relname);
/* replace it in the arg list */
! lfirst(i) = makeVar(vnum,
! InvalidAttrNumber,
! toid,
! sizeof(Pointer),
! sublevels_up);
}
else if (!attisset)
toid = exprType(arg);
*** src/backend/access/common/tupdesc.c.orig Thu Nov 16 17:30:15 2000
--- src/backend/access/common/tupdesc.c Tue Dec 26 14:16:06 2000
***************
*** 352,358 ****
AssertArg(!PointerIsValid(desc->attrs[attributeNumber - 1]));
-
/* ----------------
* allocate storage for this attribute
* ----------------
--- 352,357 ----
***************
*** 362,368 ****
desc->attrs[attributeNumber - 1] = att;
/* ----------------
! * initialize some of the attribute fields
* ----------------
*/
att->attrelid = 0; /* dummy value */
--- 361,367 ----
desc->attrs[attributeNumber - 1] = att;
/* ----------------
! * initialize the attribute fields
* ----------------
*/
att->attrelid = 0; /* dummy value */
***************
*** 372,378 ****
else
MemSet(NameStr(att->attname), 0, NAMEDATALEN);
-
att->attdispersion = 0; /* dummy value */
att->attcacheoff = -1;
att->atttypmod = typmod;
--- 371,376 ----
***************
*** 414,421 ****
att->atttypid = InvalidOid;
att->attlen = (int16) 0;
att->attbyval = (bool) 0;
- att->attstorage = 'p';
att->attalign = 'i';
return false;
}
--- 412,419 ----
att->atttypid = InvalidOid;
att->attlen = (int16) 0;
att->attbyval = (bool) 0;
att->attalign = 'i';
+ att->attstorage = 'p';
return false;
}
***************
*** 427,468 ****
typeForm = (Form_pg_type) GETSTRUCT(tuple);
att->atttypid = tuple->t_data->t_oid;
- att->attalign = typeForm->typalign;
! /* ------------------------
! If this attribute is a set, what is really stored in the
! attribute is the OID of a tuple in the pg_proc catalog.
! The pg_proc tuple contains the query string which defines
! this set - i.e., the query to run to get the set.
! So the atttypid (just assigned above) refers to the type returned
! by this query, but the actual length of this attribute is the
! length (size) of an OID.
!
! Why not just make the atttypid point to the OID type, instead
! of the type the query returns? Because the executor uses the atttypid
! to tell the front end what type will be returned (in BeginCommand),
! and in the end the type returned will be the result of the query, not
! an OID.
!
! Why not wait until the return type of the set is known (i.e., the
! recursive call to the executor to execute the set has returned)
! before telling the front end what the return type will be? Because
! the executor is a delicate thing, and making sure that the correct
! order of front-end commands is maintained is messy, especially
! considering that target lists may change as inherited attributes
! are considered, etc. Ugh.
! -----------------------------------------
! */
if (attisset)
{
att->attlen = sizeof(Oid);
att->attbyval = true;
att->attstorage = 'p';
}
else
{
att->attlen = typeForm->typlen;
att->attbyval = typeForm->typbyval;
att->attstorage = typeForm->typstorage;
}
--- 425,487 ----
typeForm = (Form_pg_type) GETSTRUCT(tuple);
att->atttypid = tuple->t_data->t_oid;
! /*------------------------
! * There are a couple of cases where we must override the information
! * stored in pg_type.
! *
! * First: if this attribute is a set, what is really stored in the
! * attribute is the OID of a tuple in the pg_proc catalog.
! * The pg_proc tuple contains the query string which defines
! * this set - i.e., the query to run to get the set.
! * So the atttypid (just assigned above) refers to the type returned
! * by this query, but the actual length of this attribute is the
! * length (size) of an OID.
! *
! * (Why not just make the atttypid point to the OID type, instead
! * of the type the query returns? Because the executor uses the atttypid
! * to tell the front end what type will be returned (in BeginCommand),
! * and in the end the type returned will be the result of the query, not
! * an OID.)
! *
! * (Why not wait until the return type of the set is known (i.e., the
! * recursive call to the executor to execute the set has returned)
! * before telling the front end what the return type will be? Because
! * the executor is a delicate thing, and making sure that the correct
! * order of front-end commands is maintained is messy, especially
! * considering that target lists may change as inherited attributes
! * are considered, etc. Ugh.)
! *
! * Second: if we are dealing with a complex type (a tuple type), then
! * pg_type will say that the representation is the same as Oid. But
! * if typmod is sizeof(Pointer) then the internal representation is
! * actually a pointer to a TupleTableSlot, and we have to substitute
! * that information.
! *
! * A set of complex type is first and foremost a set, so its
! * representation is Oid not pointer. So, test that case first.
! *-----------------------------------------
! */
if (attisset)
{
att->attlen = sizeof(Oid);
att->attbyval = true;
+ att->attalign = 'i';
+ att->attstorage = 'p';
+ }
+ else if (typeForm->typtype == 'c' && typmod == sizeof(Pointer))
+ {
+ att->attlen = sizeof(Pointer);
+ att->attbyval = true;
+ att->attalign = 'd'; /* kluge to work with 8-byte pointers */
+ /* XXX ought to have a separate attalign value for pointers ... */
att->attstorage = 'p';
}
else
{
att->attlen = typeForm->typlen;
att->attbyval = typeForm->typbyval;
+ att->attalign = typeForm->typalign;
att->attstorage = typeForm->typstorage;
}
*** src/backend/executor/execTuples.c.orig Sat Nov 11 19:36:57 2000
--- src/backend/executor/execTuples.c Tue Dec 26 14:17:30 2000
***************
*** 835,841 ****
return tupType;
}
! /*
TupleDesc
ExecCopyTupType(TupleDesc td, int natts)
{
--- 835,841 ----
return tupType;
}
! #ifdef NOT_USED
TupleDesc
ExecCopyTupType(TupleDesc td, int natts)
{
***************
*** 852,881 ****
}
return newTd;
}
! */
/* ----------------------------------------------------------------
* ExecTypeFromTL
*
* Currently there are about 4 different places where we create
* TupleDescriptors. They should all be merged, or perhaps
* be rewritten to call BuildDesc().
- *
- * old comments
- * Forms attribute type info from the target list in the node.
- * It assumes all domains are individually specified in the target list.
- * It fails if the target list contains something like Emp.all
- * which represents all the attributes from EMP relation.
- *
- * Conditions:
- * The inner and outer subtrees should be initialized because it
- * might be necessary to know the type infos of the subtrees.
* ----------------------------------------------------------------
*/
TupleDesc
ExecTypeFromTL(List *targetList)
{
! List *tlcdr;
TupleDesc typeInfo;
Resdom *resdom;
Oid restype;
--- 852,874 ----
}
return newTd;
}
! #endif
/* ----------------------------------------------------------------
* ExecTypeFromTL
*
+ * Generate a tuple descriptor for the result tuple of a targetlist.
+ * Note that resjunk columns, if any, are included in the result.
+ *
* Currently there are about 4 different places where we create
* TupleDescriptors. They should all be merged, or perhaps
* be rewritten to call BuildDesc().
* ----------------------------------------------------------------
*/
TupleDesc
ExecTypeFromTL(List *targetList)
{
! List *tlitem;
TupleDesc typeInfo;
Resdom *resdom;
Oid restype;
***************
*** 897,910 ****
typeInfo = CreateTemplateTupleDesc(len);
/* ----------------
! * notes: get resdom from (resdom expr)
! * get_typbyval comes from src/lib/l-lisp/lsyscache.c
* ----------------
*/
! tlcdr = targetList;
! while (tlcdr != NIL)
{
! TargetEntry *tle = lfirst(tlcdr);
if (tle->resdom != NULL)
{
--- 890,901 ----
typeInfo = CreateTemplateTupleDesc(len);
/* ----------------
! * scan list, generate type info for each entry
* ----------------
*/
! foreach(tlitem, targetList)
{
! TargetEntry *tle = lfirst(tlitem);
if (tle->resdom != NULL)
{
***************
*** 920,926 ****
0,
false);
! /*
ExecSetTypeInfo(resdom->resno - 1,
typeInfo,
(Oid) restype,
--- 911,917 ----
0,
false);
! #ifdef NOT_USED
ExecSetTypeInfo(resdom->resno - 1,
typeInfo,
(Oid) restype,
***************
*** 929,941 ****
NameStr(*resdom->resname),
get_typbyval(restype),
get_typalign(restype));
! */
}
else
{
Resdom *fjRes;
List *fjTlistP;
! List *fjList = lfirst(tlcdr);
#ifdef SETS_FIXED
TargetEntry *tle;
--- 920,933 ----
NameStr(*resdom->resname),
get_typbyval(restype),
get_typalign(restype));
! #endif
}
else
{
+ /* XXX this branch looks fairly broken ... tgl 12/2000 */
Resdom *fjRes;
List *fjTlistP;
! List *fjList = lfirst(tlitem);
#ifdef SETS_FIXED
TargetEntry *tle;
***************
*** 953,959 ****
fjRes->restypmod,
0,
false);
! /*
ExecSetTypeInfo(fjRes->resno - 1,
typeInfo,
(Oid) restype,
--- 945,951 ----
fjRes->restypmod,
0,
false);
! #ifdef NOT_USED
ExecSetTypeInfo(fjRes->resno - 1,
typeInfo,
(Oid) restype,
***************
*** 962,968 ****
(char *) fjRes->resname,
get_typbyval(restype),
get_typalign(restype));
! */
foreach(fjTlistP, lnext(fjList))
{
--- 954,960 ----
(char *) fjRes->resname,
get_typbyval(restype),
get_typalign(restype));
! #endif
foreach(fjTlistP, lnext(fjList))
{
***************
*** 978,984 ****
0,
false);
! /*
ExecSetTypeInfo(fjRes->resno - 1,
typeInfo,
(Oid) fjRes->restype,
--- 970,976 ----
0,
false);
! #ifdef NOT_USED
ExecSetTypeInfo(fjRes->resno - 1,
typeInfo,
(Oid) fjRes->restype,
***************
*** 987,997 ****
(char *) fjRes->resname,
get_typbyval(fjRes->restype),
get_typalign(fjRes->restype));
! */
}
}
-
- tlcdr = lnext(tlcdr);
}
return typeInfo;
--- 979,987 ----
(char *) fjRes->resname,
get_typbyval(fjRes->restype),
get_typalign(fjRes->restype));
! #endif
}
}
}
return typeInfo;
*** src/backend/executor/execUtils.c.orig Thu Nov 16 17:30:20 2000
--- src/backend/executor/execUtils.c Tue Dec 26 14:17:26 2000
***************
*** 274,289 ****
{
List *targetList;
TupleDesc tupDesc;
- int len;
targetList = node->targetlist;
tupDesc = ExecTypeFromTL(targetList);
! len = ExecTargetListLength(targetList);
!
! if (len > 0)
! ExecAssignResultType(commonstate, tupDesc);
! else
! ExecAssignResultType(commonstate, (TupleDesc) NULL);
}
/* ----------------
--- 274,283 ----
{
List *targetList;
TupleDesc tupDesc;
targetList = node->targetlist;
tupDesc = ExecTypeFromTL(targetList);
! ExecAssignResultType(commonstate, tupDesc);
}
/* ----------------
***************
*** 582,589 ****
}
/* ----------------
! * ExecFreeTypeInfo frees the array of attrbutes
! * created by ExecMakeTypeInfo and returned by ExecTypeFromTL...
* ----------------
*/
void
--- 576,583 ----
}
/* ----------------
! * ExecFreeTypeInfo frees the array of attributes
! * created by ExecMakeTypeInfo and returned by ExecTypeFromTL
* ----------------
*/
void
On 26 Dec 2000 at 14:41 (-0500), Tom Lane wrote: | I wrote: | > ... What I'm thinking about doing is setting typmod of | > an entire-tuple function argument to sizeof(Pointer), rather than | > the default -1, to indicate that a pointer representation is being | > used. Comments, hackers? | | Here is a patch to current sources along this line. I have not | committed it, since I'm not sure it does the job. It doesn't break | the regress tests on my machine, but does it fix them on Alphas? | Please apply it locally and let me know what you find. results _look_ the same from 'make check'. I'm gonna get back into the debugger on this (I've learned a few tricks that I didn't know when last I gdb'd on the Alpha). brent
On 26 Dec 2000 at 14:41 (-0500), Tom Lane wrote: | I wrote: | > ... What I'm thinking about doing is setting typmod of | > an entire-tuple function argument to sizeof(Pointer), rather than | > the default -1, to indicate that a pointer representation is being | > used. Comments, hackers? | | Here is a patch to current sources along this line. I have not | committed it, since I'm not sure it does the job. It doesn't break | the regress tests on my machine, but does it fix them on Alphas? | Please apply it locally and let me know what you find. what I'm seeing now is much the same. FWIW, it looks like we're picking up the cruft around functions.c:354 paramLI->value = fcinfo->arg[paramLI->id - 1]; (both of which are type Datum) i've been in circles trying to figure out where fcinfo->arg is filled. can you point me toward that? thanks for your help. brent
Brent Verner <brent@rcfile.org> writes:
> | Please apply it locally and let me know what you find.
> what I'm seeing now is much the same.
Drat. More to do, then.
> i've been in circles trying to figure out where fcinfo->arg is filled.
> can you point me toward that?
See src/backend/utils/fmgr/README and src/backend/utils/fmgr/fmgr.c.
But fmgr is probably only the carrier of disease, not the source...
regards, tom lane
On 26 Dec 2000 at 23:41 (-0500), Tom Lane wrote:
| Brent Verner <brent@rcfile.org> writes:
| > | Please apply it locally and let me know what you find.
|
| > what I'm seeing now is much the same.
|
| Drat. More to do, then.
after hours in the gdb-hole, I see this... maybe a clue? :)
src/include/access/common/heaptuple.c:
450 {
451
452 /*
453 * Fix me when going to a machine with more than a four-byte
454 * word!
455 */
456 off = att_align(off, att[j]->attlen, att[j]->attalign);
457
458 att[j]->attcacheoff = off;
459
460 off = att_addlength(off, att[j]->attlen, tp + off);
461 }
I'm pretty sure I don't know best how to fix this, but I've got some
randomly entered code compiling now :) If it passes the regression
tests I'll send it along.
brent 'glad the coffee shop in the backyard is open now :)'
Brent Verner <brent@rcfile.org> writes:
> after hours in the gdb-hole, I see this... maybe a clue? :)
I don't think that comment means anything. Possibly it's a leftover
from a time when there was something unportable there. But if att_align
were broken on Alphas, you'd have a lot worse problems than what you're
seeing.
regards, tom lane
After further study, I realized that fetchatt() and a number of other
places were not prepared to cope with 8-byte pass-by-value datatypes.
Most of them weren't checking for cases they couldn't handle, either.
Here is a revised patch for you to try (this includes yesterday's patch
plus more changes, so you'll need to reverse out the prior patch before
applying this one). NOTE you will need to do a full reconfigure and
rebuild to make this fly --- I'd suggest "make distclean" to start.
regards, tom lane
Attachment
On 27 Dec 2000 at 16:50 (-0500), Tom Lane wrote: | After further study, I realized that fetchatt() and a number of other | places were not prepared to cope with 8-byte pass-by-value datatypes. | Most of them weren't checking for cases they couldn't handle, either. | | Here is a revised patch for you to try (this includes yesterday's patch | plus more changes, so you'll need to reverse out the prior patch before | applying this one). NOTE you will need to do a full reconfigure and | rebuild to make this fly --- I'd suggest "make distclean" to start. excellent! this patch fixes the SEGV problem in the regression tests. the only remaining failures, which are not due to SEGV, are: oid ... FAILED float8 ... FAILED geometry ... FAILED initial comments WRT failures: float8 fails only when building with gcc. oid recall seeing one-liner change to correct this. will try. many thanks, brent
Brent Verner <brent@rcfile.org> writes:
> this patch fixes the SEGV problem in the regression tests. the only
> remaining failures, which are not due to SEGV, are:
> oid ... FAILED
> float8 ... FAILED
> geometry ... FAILED
What are the regression diffs, exactly?
regards, tom lane
On 27 Dec 2000 at 18:10 (-0500), Tom Lane wrote: | Brent Verner <brent@rcfile.org> writes: | > this patch fixes the SEGV problem in the regression tests. the only | > remaining failures, which are not due to SEGV, are: | > oid ... FAILED | > float8 ... FAILED | > geometry ... FAILED | | What are the regression diffs, exactly? see attachment. brent
Attachment
Brent Verner <brent@rcfile.org> writes:
> INSERT INTO OID_TBL(f1) VALUES ('-1040');
> + ERROR: oidin: error reading "-1040": Error 0 occurred.
Hm. I thought I'd fixed that. Are you up to date on
src/backend/utils/adt/oid.c ? Current CVS has rev 1.42.
> *** ./expected/float8-fp-exception.out Thu Mar 30 02:46:00 2000
> --- ./results/float8.out Wed Dec 27 18:27:15 2000
> ***************
> *** 214,220 ****
> SET f1 = FLOAT8_TBL.f1 * '-1'
> WHERE FLOAT8_TBL.f1 > '0.0';
> SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f;
> ! ERROR: floating point exception! The last floating point operation either exceeded legal ranges or was a divide by
zero
> SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f;
> ERROR: pow() result is out of range
> SELECT '' AS bad, ln(f.f1) from FLOAT8_TBL f where f.f1 = '0.0' ;
> --- 214,220 ----
> SET f1 = FLOAT8_TBL.f1 * '-1'
> WHERE FLOAT8_TBL.f1 > '0.0';
> SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f;
> ! ERROR: Bad float8 input format -- overflow
> SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f;
> ERROR: pow() result is out of range
> SELECT '' AS bad, ln(f.f1) from FLOAT8_TBL f where f.f1 = '0.0' ;
It would appear that Alpha no longer needs the special
float8-fp-exception.out comparison file. Try removing the line
float8/alpha.*-dec-osf=float8-fp-exception
from src/test/regress/resultmap.
The geometry diffs also look like Alpha may be more nearly in sync with
the rest of the world than it used to be. Do any of the other geometry
comparison files match what you are getting as results/geometry.out?
regards, tom lane
On 27 Dec 2000 at 18:44 (-0500), Tom Lane wrote:
| Brent Verner <brent@rcfile.org> writes:
| > INSERT INTO OID_TBL(f1) VALUES ('-1040');
| > + ERROR: oidin: error reading "-1040": Error 0 occurred.
|
| Hm. I thought I'd fixed that. Are you up to date on
| src/backend/utils/adt/oid.c ? Current CVS has rev 1.42.
yup. got that version -- 1.42 2000/12/22 21:36:09 tgl
| It would appear that Alpha no longer needs the special
| float8-fp-exception.out comparison file. Try removing the line
|
| float8/alpha.*-dec-osf=float8-fp-exception
cc w/o line above: FAIL
cc w/ line above: ok
gcc w/o line above: ??? (will retest later)
gcc w/ line above: FAIL
| The geometry diffs also look like Alpha may be more nearly in sync with
| the rest of the world than it used to be. Do any of the other geometry
| comparison files match what you are getting as results/geometry.out?
none match.
| regards, tom lane
Brent Verner <brent@rcfile.org> writes:
> | float8-fp-exception.out comparison file. Try removing the line
> |
> | float8/alpha.*-dec-osf=float8-fp-exception
> cc w/o line above: FAIL
> cc w/ line above: ok
> gcc w/o line above: ??? (will retest later)
> gcc w/ line above: FAIL
OK, then it should work for both cases if you do
float8/alpha.*-dec-osf.*:cc=float8-fp-exception
regards, tom lane
Re: [HACKERS] Re: Tuple-valued datums on Alpha (was Re: 7.1 on DEC/Alpha)
From
Ryan Kirkpatrick
Date:
On Wed, 27 Dec 2000, Tom Lane wrote:
> After further study, I realized that fetchatt() and a number of other
> places were not prepared to cope with 8-byte pass-by-value datatypes.
> Most of them weren't checking for cases they couldn't handle, either.
>
> Here is a revised patch for you to try (this includes yesterday's patch
> plus more changes, so you'll need to reverse out the prior patch before
> applying this one). NOTE you will need to do a full reconfigure and
> rebuild to make this fly --- I'd suggest "make distclean" to start.
Good news is that it solves the 'misc' regression test failure. It
now passes with flying colors! The bad news is that the 'oid' regression
test is still broken (with the exact same problem as before). I think
Brent hit the same problem... I guess, verify that your oid fix actually
hit the CVS tree, and if it did, rethink the solution. :(
For testing I used the snapshot from ftp.postgresql.org:/pub/dev/
dated yesterday on my Alpha XLT366 running Debian GNU/Linux 2.2r0, kernel
2.2.17. Though I found the 'configure' file actually a copy of 'config.in'
and had to run the latter file through autoconf to get the correct version
of the former file. Weird.
Also, I tested a patches source tree on an Linux/x86 box, and it
passed all regression tests w/o problems. I can test the patched source
tree on a Linux/Sparc machine if you want (bit more effort required to do
so).
Overall, it looks like we are making progress! Thanks to both you
and Brent for looking deeper into these problems. TTYL.
---------------------------------------------------------------------------
| "For to me to live is Christ, and to die is gain." |
| --- Philippians 1:21 (KJV) |
---------------------------------------------------------------------------
| Ryan Kirkpatrick | Boulder, Colorado | http://www.rkirkpat.net/ |
---------------------------------------------------------------------------