IntArray in c.h
I found the struct IntArray defined in c.h is actually used only in
execQual.c. ISTM the definition should be at least moved to the right
place.
Attached is a trivial fix. Addition to the explanation above, I
replaced IntArray by simple int array bounded with MAXDIM and remove
local variable lIndex in ExecEvalArrayRef because the usage of the
variable doesn't seem good to me.
Regression passed and various manual tests like "UPDATE t SET
a[1:2][1] = 1" didn't fail.
Regards,
--
Hitoshi Harada
Attachments:
IntArray.patchapplication/octet-stream; name=IntArray.patchDownload
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
***************
*** 263,271 **** ExecEvalArrayRef(ArrayRefExprState *astate,
ListCell *l;
int i = 0,
j = 0;
! IntArray upper,
! lower;
! int *lIndex;
array_source = (ArrayType *)
DatumGetPointer(ExecEvalExpr(astate->refexpr,
--- 263,270 ----
ListCell *l;
int i = 0,
j = 0;
! int upper[MAXDIM],
! lower[MAXDIM];
array_source = (ArrayType *)
DatumGetPointer(ExecEvalExpr(astate->refexpr,
***************
*** 295,301 **** ExecEvalArrayRef(ArrayRefExprState *astate,
errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
i, MAXDIM)));
! upper.indx[i++] = DatumGetInt32(ExecEvalExpr(eltstate,
econtext,
&eisnull,
NULL));
--- 294,300 ----
errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
i, MAXDIM)));
! upper[i++] = DatumGetInt32(ExecEvalExpr(eltstate,
econtext,
&eisnull,
NULL));
***************
*** 323,329 **** ExecEvalArrayRef(ArrayRefExprState *astate,
errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
i, MAXDIM)));
! lower.indx[j++] = DatumGetInt32(ExecEvalExpr(eltstate,
econtext,
&eisnull,
NULL));
--- 322,328 ----
errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
i, MAXDIM)));
! lower[j++] = DatumGetInt32(ExecEvalExpr(eltstate,
econtext,
&eisnull,
NULL));
***************
*** 341,350 **** ExecEvalArrayRef(ArrayRefExprState *astate,
/* this can't happen unless parser messed up */
if (i != j)
elog(ERROR, "upper and lower index lists are not same length");
- lIndex = lower.indx;
}
- else
- lIndex = NULL;
if (isAssignment)
{
--- 340,346 ----
***************
*** 386,394 **** ExecEvalArrayRef(ArrayRefExprState *astate,
*isNull = false;
}
! if (lIndex == NULL)
resultArray = array_set(array_source, i,
! upper.indx,
sourceData,
eisnull,
astate->refattrlength,
--- 382,390 ----
*isNull = false;
}
! if (j == 0)
resultArray = array_set(array_source, i,
! upper,
sourceData,
eisnull,
astate->refattrlength,
***************
*** 397,403 **** ExecEvalArrayRef(ArrayRefExprState *astate,
astate->refelemalign);
else
resultArray = array_set_slice(array_source, i,
! upper.indx, lower.indx,
(ArrayType *) DatumGetPointer(sourceData),
eisnull,
astate->refattrlength,
--- 393,399 ----
astate->refelemalign);
else
resultArray = array_set_slice(array_source, i,
! upper, lower,
(ArrayType *) DatumGetPointer(sourceData),
eisnull,
astate->refattrlength,
***************
*** 407,414 **** ExecEvalArrayRef(ArrayRefExprState *astate,
return PointerGetDatum(resultArray);
}
! if (lIndex == NULL)
! return array_ref(array_source, i, upper.indx,
astate->refattrlength,
astate->refelemlength,
astate->refelembyval,
--- 403,410 ----
return PointerGetDatum(resultArray);
}
! if (j == 0)
! return array_ref(array_source, i, upper,
astate->refattrlength,
astate->refelemlength,
astate->refelembyval,
***************
*** 417,423 **** ExecEvalArrayRef(ArrayRefExprState *astate,
else
{
resultArray = array_get_slice(array_source, i,
! upper.indx, lower.indx,
astate->refattrlength,
astate->refelemlength,
astate->refelembyval,
--- 413,419 ----
else
{
resultArray = array_get_slice(array_source, i,
! upper, lower,
astate->refattrlength,
astate->refelemlength,
astate->refelembyval,
*** a/src/include/c.h
--- b/src/include/c.h
***************
*** 376,389 **** typedef uint32 CommandId;
#define FirstCommandId ((CommandId) 0)
- /*
- * Array indexing support
- */
#define MAXDIM 6
- typedef struct
- {
- int indx[MAXDIM];
- } IntArray;
/* ----------------
* Variable-length datatypes all share the 'struct varlena' header.
--- 376,382 ----
Hitoshi Harada <umi.tanuki@gmail.com> writes:
I found the struct IntArray defined in c.h is actually used only in
execQual.c. ISTM the definition should be at least moved to the right
place.
It's a general-purpose datatype that might be used anywhere that array
indexing happens. I think the fact that it's currently used only in
execQual is mere happenstance, and should not be "enforced" by moving
or removing the declaration.
regards, tom lane
2009/12/30 Tom Lane <tgl@sss.pgh.pa.us>:
Hitoshi Harada <umi.tanuki@gmail.com> writes:
I found the struct IntArray defined in c.h is actually used only in
execQual.c. ISTM the definition should be at least moved to the right
place.It's a general-purpose datatype that might be used anywhere that array
indexing happens. I think the fact that it's currently used only in
execQual is mere happenstance, and should not be "enforced" by moving
or removing the declaration.
I would be convinced if the struct or the logic was complex, but
actually it is so simple that it can be replaced by primitive int
array. Also, it seems to me that c.h is too general place to declare
it for such purpose. Does nobody else think so?
Regards,
--
Hitoshi Harada
On ons, 2009-12-30 at 11:46 +0900, Hitoshi Harada wrote:
2009/12/30 Tom Lane <tgl@sss.pgh.pa.us>:
Hitoshi Harada <umi.tanuki@gmail.com> writes:
I found the struct IntArray defined in c.h is actually used only in
execQual.c. ISTM the definition should be at least moved to the right
place.It's a general-purpose datatype that might be used anywhere that array
indexing happens. I think the fact that it's currently used only in
execQual is mere happenstance, and should not be "enforced" by moving
or removing the declaration.I would be convinced if the struct or the logic was complex, but
actually it is so simple that it can be replaced by primitive int
array. Also, it seems to me that c.h is too general place to declare
it for such purpose. Does nobody else think so?
The definition of c.h is bogus anyway. You might think it contains
includes and defines to set up a portable C environment, which is what
the first half indeed does.
But then things like regproc, transaction ID types, IntArray, varlena,
bytea, oidvector, NameData, etc. do not belong there and should be moved
to postgres.h.
Peter Eisentraut <peter_e@gmx.net> writes:
The definition of c.h is bogus anyway. You might think it contains
includes and defines to set up a portable C environment, which is what
the first half indeed does.
But then things like regproc, transaction ID types, IntArray, varlena,
bytea, oidvector, NameData, etc. do not belong there and should be moved
to postgres.h.
Actually, what c.h does is to provide definitions that are needed in
both frontend and backend code. And we do NOT want to start including
postgres.h in frontend code. It might be that some of the declarations
there are useless to frontend code and could be moved, but trying to be
as strict as you suggest is only going to create problems.
regards, tom lane
On tor, 2009-12-31 at 11:28 -0500, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
The definition of c.h is bogus anyway. You might think it contains
includes and defines to set up a portable C environment, which is what
the first half indeed does.But then things like regproc, transaction ID types, IntArray, varlena,
bytea, oidvector, NameData, etc. do not belong there and should be moved
to postgres.h.Actually, what c.h does is to provide definitions that are needed in
both frontend and backend code. And we do NOT want to start including
postgres.h in frontend code. It might be that some of the declarations
there are useless to frontend code and could be moved, but trying to be
as strict as you suggest is only going to create problems.
I think the list above is a pretty good list of things that client code
doesn't need, plus or minus a few things maybe.
2010/1/2 Peter Eisentraut <peter_e@gmx.net>:
On tor, 2009-12-31 at 11:28 -0500, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
The definition of c.h is bogus anyway. You might think it contains
includes and defines to set up a portable C environment, which is what
the first half indeed does.But then things like regproc, transaction ID types, IntArray, varlena,
bytea, oidvector, NameData, etc. do not belong there and should be moved
to postgres.h.Actually, what c.h does is to provide definitions that are needed in
both frontend and backend code. And we do NOT want to start including
postgres.h in frontend code. It might be that some of the declarations
there are useless to frontend code and could be moved, but trying to be
as strict as you suggest is only going to create problems.I think the list above is a pretty good list of things that client code
doesn't need, plus or minus a few things maybe.
Looking closer in c.h, there are several things to move or remove (and
it gets slightly more efficient if we do), but it seems we don't have
such motivation...
Regards,
--
Hitoshi Harada