Thread: SPI bug.
Hi, While trying to determine if SPI_cursor_move can rewind (and receiving a great help from the guys at the irc), we found out that since the count parameter is int and FETCH_ALL is LONG_MAX then setting the count parameter to FETCH_ALL to rewind will not work on 64bit systems. On my pIII 32 bit system it works since int size=long size. I am using 8.0.2 (i.e. the repositioning bug is already fixed here). I think the solution can be either changing the FETCH_ALL to INT_MAX or changing the interface parameter count and subsequent usages to long. (FETCH_ALL at parsenodes.h) Regards,tzahi. WARNING TO SPAMMERS: see at http://members.lycos.co.uk/my2nis/spamwarning.html
Tzahi Fadida wrote: > I think the solution can be either changing the FETCH_ALL to > INT_MAX or changing the interface parameter count and subsequent usages > to long. I think changing SPI_cursor_fetch() and SPI_cursor_move() to take a "long" for the "count" parameter is the right fix for HEAD. It would probably not be wise to backport, though, as it is probably not worth breaking ABI compatibility for SPI during a stable release series. -Neil
Neil Conway wrote:
> I think changing SPI_cursor_fetch() and SPI_cursor_move() to take a
> "long" for the "count" parameter is the right fix for HEAD.
Attached is a patch that implements this. A bunch of functions had to be
updated: SPI_execute(), SPI_execute_snapshot(), SPI_exec(), SPI_execp(),
SPI_execute_plan(), SPI_cursor_fetch(), and SPI_cursor_move().
I also updated PL/Python, which was invoking SPI_execute() with an `int'
parameter. PL/Tcl could be updated as well, but it seems the base Tcl
package doesn't provide a Tcl_GetLong() function. PL/Perl could also be
updated (plperl_spi_exec()), but I don't know XS, so I will leave that
to someone else.
Barring any objections, I'll apply this to HEAD tomorrow.
-Neil
Index: doc/src/sgml/spi.sgml
===================================================================
RCS file: /Users/neilc/local/cvs/pgsql/doc/src/sgml/spi.sgml,v
retrieving revision 1.40
diff -c -r1.40 spi.sgml
*** doc/src/sgml/spi.sgml 29 Mar 2005 02:53:53 -0000 1.40
--- doc/src/sgml/spi.sgml 1 May 2005 06:29:47 -0000
***************
*** 292,298 ****
<refsynopsisdiv>
<synopsis>
! int SPI_execute(const char * <parameter>command</parameter>, bool <parameter>read_only</parameter>, int
<parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
--- 292,298 ----
<refsynopsisdiv>
<synopsis>
! int SPI_execute(const char * <parameter>command</parameter>, bool <parameter>read_only</parameter>, long
<parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
***************
*** 423,429 ****
</varlistentry>
<varlistentry>
! <term><literal>int <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to process or return
--- 423,429 ----
</varlistentry>
<varlistentry>
! <term><literal>long <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to process or return
***************
*** 598,604 ****
<refsynopsisdiv>
<synopsis>
! int SPI_exec(const char * <parameter>command</parameter>, int <parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
--- 598,604 ----
<refsynopsisdiv>
<synopsis>
! int SPI_exec(const char * <parameter>command</parameter>, long <parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
***************
*** 627,633 ****
</varlistentry>
<varlistentry>
! <term><literal>int <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to process or return
--- 627,633 ----
</varlistentry>
<varlistentry>
! <term><literal>long <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to process or return
***************
*** 963,969 ****
<refsynopsisdiv>
<synopsis>
int SPI_execute_plan(void * <parameter>plan</parameter>, Datum * <parameter>values</parameter>, const char *
<parameter>nulls</parameter>,
! bool <parameter>read_only</parameter>, int <parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
--- 963,969 ----
<refsynopsisdiv>
<synopsis>
int SPI_execute_plan(void * <parameter>plan</parameter>, Datum * <parameter>values</parameter>, const char *
<parameter>nulls</parameter>,
! bool <parameter>read_only</parameter>, long <parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
***************
*** 1030,1036 ****
</varlistentry>
<varlistentry>
! <term><literal>int <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to process or return
--- 1030,1036 ----
</varlistentry>
<varlistentry>
! <term><literal>long <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to process or return
***************
*** 1104,1110 ****
<refsynopsisdiv>
<synopsis>
! int SPI_execp(void * <parameter>plan</parameter>, Datum * <parameter>values</parameter>, const char *
<parameter>nulls</parameter>,int <parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
--- 1104,1110 ----
<refsynopsisdiv>
<synopsis>
! int SPI_execp(void * <parameter>plan</parameter>, Datum * <parameter>values</parameter>, const char *
<parameter>nulls</parameter>,long <parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
***************
*** 1162,1168 ****
</varlistentry>
<varlistentry>
! <term><literal>int <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to process or return
--- 1162,1168 ----
</varlistentry>
<varlistentry>
! <term><literal>long <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to process or return
***************
*** 1375,1381 ****
<refsynopsisdiv>
<synopsis>
! void SPI_cursor_fetch(Portal <parameter>portal</parameter>, bool <parameter>forward</parameter>, int
<parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
--- 1375,1381 ----
<refsynopsisdiv>
<synopsis>
! void SPI_cursor_fetch(Portal <parameter>portal</parameter>, bool <parameter>forward</parameter>, long
<parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
***************
*** 1411,1417 ****
</varlistentry>
<varlistentry>
! <term><literal>int <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to fetch
--- 1411,1417 ----
</varlistentry>
<varlistentry>
! <term><literal>long <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to fetch
***************
*** 1448,1454 ****
<refsynopsisdiv>
<synopsis>
! void SPI_cursor_move(Portal <parameter>portal</parameter>, bool <parameter>forward</parameter>, int
<parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
--- 1448,1454 ----
<refsynopsisdiv>
<synopsis>
! void SPI_cursor_move(Portal <parameter>portal</parameter>, bool <parameter>forward</parameter>, long
<parameter>count</parameter>)
</synopsis>
</refsynopsisdiv>
***************
*** 1485,1491 ****
</varlistentry>
<varlistentry>
! <term><literal>int <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to move
--- 1485,1491 ----
</varlistentry>
<varlistentry>
! <term><literal>long <parameter>count</parameter></literal></term>
<listitem>
<para>
maximum number of rows to move
Index: src/backend/executor/spi.c
===================================================================
RCS file: /Users/neilc/local/cvs/pgsql/src/backend/executor/spi.c,v
retrieving revision 1.137
diff -c -r1.137 spi.c
*** src/backend/executor/spi.c 29 Mar 2005 02:53:53 -0000 1.137
--- src/backend/executor/spi.c 1 May 2005 06:27:14 -0000
***************
*** 39,51 ****
static int _SPI_execute_plan(_SPI_plan *plan,
Datum *Values, const char *Nulls,
Snapshot snapshot, Snapshot crosscheck_snapshot,
! bool read_only, int tcount);
! static int _SPI_pquery(QueryDesc *queryDesc, int tcount);
static void _SPI_error_callback(void *arg);
! static void _SPI_cursor_operation(Portal portal, bool forward, int count,
DestReceiver *dest);
static _SPI_plan *_SPI_copy_plan(_SPI_plan *plan, int location);
--- 39,51 ----
static int _SPI_execute_plan(_SPI_plan *plan,
Datum *Values, const char *Nulls,
Snapshot snapshot, Snapshot crosscheck_snapshot,
! bool read_only, long tcount);
! static int _SPI_pquery(QueryDesc *queryDesc, long tcount);
static void _SPI_error_callback(void *arg);
! static void _SPI_cursor_operation(Portal portal, bool forward, long count,
DestReceiver *dest);
static _SPI_plan *_SPI_copy_plan(_SPI_plan *plan, int location);
***************
*** 278,286 ****
_SPI_curid = _SPI_connected - 1;
}
! /* Parse, plan, and execute a querystring */
int
! SPI_execute(const char *src, bool read_only, int tcount)
{
_SPI_plan plan;
int res;
--- 278,286 ----
_SPI_curid = _SPI_connected - 1;
}
! /* Parse, plan, and execute a query string */
int
! SPI_execute(const char *src, bool read_only, long tcount)
{
_SPI_plan plan;
int res;
***************
*** 309,315 ****
/* Obsolete version of SPI_execute */
int
! SPI_exec(const char *src, int tcount)
{
return SPI_execute(src, false, tcount);
}
--- 309,315 ----
/* Obsolete version of SPI_execute */
int
! SPI_exec(const char *src, long tcount)
{
return SPI_execute(src, false, tcount);
}
***************
*** 317,323 ****
/* Execute a previously prepared plan */
int
SPI_execute_plan(void *plan, Datum *Values, const char *Nulls,
! bool read_only, int tcount)
{
int res;
--- 317,323 ----
/* Execute a previously prepared plan */
int
SPI_execute_plan(void *plan, Datum *Values, const char *Nulls,
! bool read_only, long tcount)
{
int res;
***************
*** 342,348 ****
/* Obsolete version of SPI_execute_plan */
int
! SPI_execp(void *plan, Datum *Values, const char *Nulls, int tcount)
{
return SPI_execute_plan(plan, Values, Nulls, false, tcount);
}
--- 342,348 ----
/* Obsolete version of SPI_execute_plan */
int
! SPI_execp(void *plan, Datum *Values, const char *Nulls, long tcount)
{
return SPI_execute_plan(plan, Values, Nulls, false, tcount);
}
***************
*** 360,366 ****
SPI_execute_snapshot(void *plan,
Datum *Values, const char *Nulls,
Snapshot snapshot, Snapshot crosscheck_snapshot,
! bool read_only, int tcount)
{
int res;
--- 360,366 ----
SPI_execute_snapshot(void *plan,
Datum *Values, const char *Nulls,
Snapshot snapshot, Snapshot crosscheck_snapshot,
! bool read_only, long tcount)
{
int res;
***************
*** 989,995 ****
* Fetch rows in a cursor
*/
void
! SPI_cursor_fetch(Portal portal, bool forward, int count)
{
_SPI_cursor_operation(portal, forward, count,
CreateDestReceiver(SPI, NULL));
--- 989,995 ----
* Fetch rows in a cursor
*/
void
! SPI_cursor_fetch(Portal portal, bool forward, long count)
{
_SPI_cursor_operation(portal, forward, count,
CreateDestReceiver(SPI, NULL));
***************
*** 1003,1009 ****
* Move in a cursor
*/
void
! SPI_cursor_move(Portal portal, bool forward, int count)
{
_SPI_cursor_operation(portal, forward, count, None_Receiver);
}
--- 1003,1009 ----
* Move in a cursor
*/
void
! SPI_cursor_move(Portal portal, bool forward, long count)
{
_SPI_cursor_operation(portal, forward, count, None_Receiver);
}
***************
*** 1319,1325 ****
static int
_SPI_execute_plan(_SPI_plan *plan, Datum *Values, const char *Nulls,
Snapshot snapshot, Snapshot crosscheck_snapshot,
! bool read_only, int tcount)
{
volatile int res = 0;
Snapshot saveActiveSnapshot;
--- 1319,1325 ----
static int
_SPI_execute_plan(_SPI_plan *plan, Datum *Values, const char *Nulls,
Snapshot snapshot, Snapshot crosscheck_snapshot,
! bool read_only, long tcount)
{
volatile int res = 0;
Snapshot saveActiveSnapshot;
***************
*** 1503,1509 ****
}
static int
! _SPI_pquery(QueryDesc *queryDesc, int tcount)
{
int operation = queryDesc->operation;
int res;
--- 1503,1509 ----
}
static int
! _SPI_pquery(QueryDesc *queryDesc, long tcount)
{
int operation = queryDesc->operation;
int res;
***************
*** 1541,1547 ****
ExecutorStart(queryDesc, false);
! ExecutorRun(queryDesc, ForwardScanDirection, (long) tcount);
_SPI_current->processed = queryDesc->estate->es_processed;
save_lastoid = queryDesc->estate->es_lastoid;
--- 1541,1547 ----
ExecutorStart(queryDesc, false);
! ExecutorRun(queryDesc, ForwardScanDirection, tcount);
_SPI_current->processed = queryDesc->estate->es_processed;
save_lastoid = queryDesc->estate->es_lastoid;
***************
*** 1609,1615 ****
* Do a FETCH or MOVE in a cursor
*/
static void
! _SPI_cursor_operation(Portal portal, bool forward, int count,
DestReceiver *dest)
{
long nfetched;
--- 1609,1615 ----
* Do a FETCH or MOVE in a cursor
*/
static void
! _SPI_cursor_operation(Portal portal, bool forward, long count,
DestReceiver *dest)
{
long nfetched;
***************
*** 1631,1637 ****
/* Run the cursor */
nfetched = PortalRunFetch(portal,
forward ? FETCH_FORWARD : FETCH_BACKWARD,
! (long) count,
dest);
/*
--- 1631,1637 ----
/* Run the cursor */
nfetched = PortalRunFetch(portal,
forward ? FETCH_FORWARD : FETCH_BACKWARD,
! count,
dest);
/*
Index: src/include/executor/spi.h
===================================================================
RCS file: /Users/neilc/local/cvs/pgsql/src/include/executor/spi.h,v
retrieving revision 1.51
diff -c -r1.51 spi.h
*** src/include/executor/spi.h 29 Mar 2005 02:53:53 -0000 1.51
--- src/include/executor/spi.h 1 May 2005 06:26:30 -0000
***************
*** 82,98 ****
extern void SPI_push(void);
extern void SPI_pop(void);
extern void SPI_restore_connection(void);
! extern int SPI_execute(const char *src, bool read_only, int tcount);
extern int SPI_execute_plan(void *plan, Datum *Values, const char *Nulls,
! bool read_only, int tcount);
! extern int SPI_exec(const char *src, int tcount);
extern int SPI_execp(void *plan, Datum *Values, const char *Nulls,
! int tcount);
extern int SPI_execute_snapshot(void *plan,
Datum *Values, const char *Nulls,
Snapshot snapshot,
Snapshot crosscheck_snapshot,
! bool read_only, int tcount);
extern void *SPI_prepare(const char *src, int nargs, Oid *argtypes);
extern void *SPI_saveplan(void *plan);
extern int SPI_freeplan(void *plan);
--- 82,98 ----
extern void SPI_push(void);
extern void SPI_pop(void);
extern void SPI_restore_connection(void);
! extern int SPI_execute(const char *src, bool read_only, long tcount);
extern int SPI_execute_plan(void *plan, Datum *Values, const char *Nulls,
! bool read_only, long tcount);
! extern int SPI_exec(const char *src, long tcount);
extern int SPI_execp(void *plan, Datum *Values, const char *Nulls,
! long tcount);
extern int SPI_execute_snapshot(void *plan,
Datum *Values, const char *Nulls,
Snapshot snapshot,
Snapshot crosscheck_snapshot,
! bool read_only, long tcount);
extern void *SPI_prepare(const char *src, int nargs, Oid *argtypes);
extern void *SPI_saveplan(void *plan);
extern int SPI_freeplan(void *plan);
***************
*** 123,130 ****
extern Portal SPI_cursor_open(const char *name, void *plan,
Datum *Values, const char *Nulls, bool read_only);
extern Portal SPI_cursor_find(const char *name);
! extern void SPI_cursor_fetch(Portal portal, bool forward, int count);
! extern void SPI_cursor_move(Portal portal, bool forward, int count);
extern void SPI_cursor_close(Portal portal);
extern void AtEOXact_SPI(bool isCommit);
--- 123,130 ----
extern Portal SPI_cursor_open(const char *name, void *plan,
Datum *Values, const char *Nulls, bool read_only);
extern Portal SPI_cursor_find(const char *name);
! extern void SPI_cursor_fetch(Portal portal, bool forward, long count);
! extern void SPI_cursor_move(Portal portal, bool forward, long count);
extern void SPI_cursor_close(Portal portal);
extern void AtEOXact_SPI(bool isCommit);
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /Users/neilc/local/cvs/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.135
diff -c -r1.135 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c 7 Apr 2005 14:53:04 -0000 1.135
--- src/pl/plpgsql/src/pl_exec.c 1 May 2005 06:36:37 -0000
***************
*** 158,164 ****
bool *isNull,
Oid *rettype);
static int exec_run_select(PLpgSQL_execstate *estate,
! PLpgSQL_expr *expr, int maxtuples, Portal *portalP);
static void exec_move_row(PLpgSQL_execstate *estate,
PLpgSQL_rec *rec,
PLpgSQL_row *row,
--- 158,164 ----
bool *isNull,
Oid *rettype);
static int exec_run_select(PLpgSQL_execstate *estate,
! PLpgSQL_expr *expr, long maxtuples, Portal *portalP);
static void exec_move_row(PLpgSQL_execstate *estate,
PLpgSQL_rec *rec,
PLpgSQL_row *row,
***************
*** 3482,3488 ****
*/
static int
exec_run_select(PLpgSQL_execstate *estate,
! PLpgSQL_expr *expr, int maxtuples, Portal *portalP)
{
int i;
Datum *values;
--- 3482,3488 ----
*/
static int
exec_run_select(PLpgSQL_execstate *estate,
! PLpgSQL_expr *expr, long maxtuples, Portal *portalP)
{
int i;
Datum *values;
Index: src/pl/plpython/plpython.c
===================================================================
RCS file: /Users/neilc/local/cvs/pgsql/src/pl/plpython/plpython.c,v
retrieving revision 1.60
diff -c -r1.60 plpython.c
*** src/pl/plpython/plpython.c 29 Mar 2005 00:17:24 -0000 1.60
--- src/pl/plpython/plpython.c 1 May 2005 06:42:09 -0000
***************
*** 1546,1553 ****
static PyObject *PLy_spi_prepare(PyObject *, PyObject *);
static PyObject *PLy_spi_execute(PyObject *, PyObject *);
! static PyObject *PLy_spi_execute_query(char *query, int limit);
! static PyObject *PLy_spi_execute_plan(PyObject *, PyObject *, int);
static PyObject *PLy_spi_execute_fetch_result(SPITupleTable *, int, int);
--- 1546,1553 ----
static PyObject *PLy_spi_prepare(PyObject *, PyObject *);
static PyObject *PLy_spi_execute(PyObject *, PyObject *);
! static PyObject *PLy_spi_execute_query(char *query, long limit);
! static PyObject *PLy_spi_execute_plan(PyObject *, PyObject *, long);
static PyObject *PLy_spi_execute_fetch_result(SPITupleTable *, int, int);
***************
*** 1965,1971 ****
char *query;
PyObject *plan;
PyObject *list = NULL;
! int limit = 0;
/* Can't execute more if we have an unhandled error */
if (PLy_error_in_progress)
--- 1965,1971 ----
char *query;
PyObject *plan;
PyObject *list = NULL;
! long limit = 0;
/* Can't execute more if we have an unhandled error */
if (PLy_error_in_progress)
***************
*** 1974,1985 ****
return NULL;
}
! if (PyArg_ParseTuple(args, "s|i", &query, &limit))
return PLy_spi_execute_query(query, limit);
PyErr_Clear();
! if ((PyArg_ParseTuple(args, "O|Oi", &plan, &list, &limit)) &&
(is_PLyPlanObject(plan)))
return PLy_spi_execute_plan(plan, list, limit);
--- 1974,1985 ----
return NULL;
}
! if (PyArg_ParseTuple(args, "s|l", &query, &limit))
return PLy_spi_execute_query(query, limit);
PyErr_Clear();
! if ((PyArg_ParseTuple(args, "O|Ol", &plan, &list, &limit)) &&
(is_PLyPlanObject(plan)))
return PLy_spi_execute_plan(plan, list, limit);
***************
*** 1988,1994 ****
}
static PyObject *
! PLy_spi_execute_plan(PyObject * ob, PyObject * list, int limit)
{
volatile int nargs;
int i,
--- 1988,1994 ----
}
static PyObject *
! PLy_spi_execute_plan(PyObject * ob, PyObject * list, long limit)
{
volatile int nargs;
int i,
***************
*** 2123,2129 ****
}
static PyObject *
! PLy_spi_execute_query(char *query, int limit)
{
int rv;
MemoryContext oldcontext;
--- 2123,2129 ----
}
static PyObject *
! PLy_spi_execute_query(char *query, long limit)
{
int rv;
MemoryContext oldcontext;
On 2005-05-01, Neil Conway <neilc@samurai.com> wrote: > Tzahi Fadida wrote: >> I think the solution can be either changing the FETCH_ALL to >> INT_MAX or changing the interface parameter count and subsequent usages >> to long. > > I think changing SPI_cursor_fetch() and SPI_cursor_move() to take a > "long" for the "count" parameter is the right fix for HEAD. It would > probably not be wise to backport, though, as it is probably not worth > breaking ABI compatibility for SPI during a stable release series. While you're at it, how about a way to specify WITH SCROLL for a cursor created in SPI? At the moment, SPI_cursor_open hardwires the scroll option according to the result of ExecSupportsBackwardScan. -- Andrew, Supernews http://www.supernews.com - individual and corporate NNTP services
Neil Conway wrote: > Neil Conway wrote: > >> I think changing SPI_cursor_fetch() and SPI_cursor_move() to take a >> "long" for the "count" parameter is the right fix for HEAD. > > > Attached is a patch that implements this. A bunch of functions had to be > updated: SPI_execute(), SPI_execute_snapshot(), SPI_exec(), SPI_execp(), > SPI_execute_plan(), SPI_cursor_fetch(), and SPI_cursor_move(). > > I also updated PL/Python, which was invoking SPI_execute() with an `int' > parameter. PL/Tcl could be updated as well, but it seems the base Tcl > package doesn't provide a Tcl_GetLong() function. PL/Perl could also be > updated (plperl_spi_exec()), but I don't know XS, so I will leave that > to someone else. > > Barring any objections, I'll apply this to HEAD tomorrow. > Since both int and long are types whos size that vary depending on platform, and since the SPI protocol often interfaces with other languages where the sizes are fixed, wouldn't it be better to use something that is fixed in size here too? I.e. int32 or perhaps int64? Regards, Thomas Hallgren
Thomas Hallgren wrote: > Since both int and long are types whos size that vary depending on > platform, and since the SPI protocol often interfaces with other > languages where the sizes are fixed ISTM there are no "languages where the sizes are fixed". In this context, int and long are C and C++ types; types that happen to have the same name but different behavior (e.g. int and long in Java) are not the same type at all. The reason the API types should use "long" is that the underlying executor APIs (e.g. ExecutorRun()) use "long". It might be a good idea to change the executor stuff to use int64s -- then I'd have no issue with making a corresponding change to the SPI APIs. I guess the main objection to doing this is that a 64-bit integral type is not available on all platforms (at least in theory; are there any platforms we care about that don't have one?) -Neil
Neil Conway <neilc@samurai.com> writes:
> The reason the API types should use "long" is that the underlying
> executor APIs (e.g. ExecutorRun()) use "long". It might be a good idea
> to change the executor stuff to use int64s
No, it would not. There is a potential performance cost ("long" should
have at least acceptable performance on all machines, "long long" is
another story) and there is no demonstrated need.
regards, tom lane
Neil Conway wrote: > Thomas Hallgren wrote: > >> Since both int and long are types whos size that vary depending on >> platform, and since the SPI protocol often interfaces with other >> languages where the sizes are fixed > > > ISTM there are no "languages where the sizes are fixed". In this > context, int and long are C and C++ types; types that happen to have > the same name but different behavior (e.g. int and long in Java) are > not the same type at all. I fully agree that an int and long in Java is very different from an int or long in C/C++. Hence my proposal :-) What I meant was that SPI will interface with languages where there is no correspondence to a type who's size varies depending on platform and that it therefore would be better to chose a type who's size will not vary. > > The reason the API types should use "long" is that the underlying > executor APIs (e.g. ExecutorRun()) use "long". An API should ideally hide the internals of the underlying code so I'm not sure this is a valid reason. I would instead say that "An API should remain consistent over the range of platforms where it is supported". Especially if the intention with this API is to make the life easier for PL/<some language> authors. > It might be a good idea to change the executor stuff to use int64s -- > then I'd have no issue with making a corresponding change to the SPI > APIs. I guess the main objection to doing this is that a 64-bit > integral type is not available on all platforms (at least in theory; > are there any platforms we care about that don't have one?) I'm sure there is some obscure platform where this matters. I don't know of one though and in my world there isn't. The Java Native Interface (JNI) uses the jlong type and it's required to be 64 bit. If PostgreSQL could be made to rely the int64, then we could get rid of the integer-datetimes conditional also. That would be nice. For this purpose I wonder if there's a need to use int64's though. An int32 covers extremely huge result-sets. But perhaps I'm not visionary enough. I still remember the days when 640Kb RAM should be enough for all foreseeable future :-) Regards, Thomas Hallgren
Thomas Hallgren wrote: > What I meant was that SPI will interface with languages where there is > no correspondence to a type who's size varies depending on platform and > that it therefore would be better to chose a type who's size will not vary. My point is that since they are different types, the language itself will need to provide some mechanism for doing this type conversion _anyway_. 'int' and 'long' are used throughout the backend APIs, so I don't see the gain in only converting the SPI functions over to using int32/int64. > An API should ideally hide the internals of the underlying code so I'm > not sure this is a valid reason. Well, the executor allows you to specify a 64-bit count on platforms where "long" is 64-bit, and a 32-bit count otherwise. ISTM the most straightforward way to expose this to clients is to just make the parameter a "long". As I said before, we may or may not want to change the executor itself to use a constant-sized type, but as a matter of interface definition, I think using "long" makes the most sense. BTW, patch applied to HEAD. -Neil
Neil Conway wrote: > As I said before, we may or may not want to change > the executor itself to use a constant-sized type, but as a matter of > interface definition, I think using "long" makes the most sense. > One thing that I forgot. If you indeed will do something like that in the future, the implication is yet another change to the SPI interfaces. Why not decide, once and for all and right now, what the size of this integer should be and then *start* with a change of the interface. The change of the underlying implementation can come later. Now you effectively force a second change that will make things incompatible should you decide to change the executor in the future. Regards, Thomas Hallgren
Neil Conway wrote: > My point is that since they are different types, the language itself > will need to provide some mechanism for doing this type conversion > _anyway_. 'int' and 'long' are used throughout the backend APIs, so I > don't see the gain in only converting the SPI functions over to using > int32/int64. Mainly because it's easier to do that mapping knowing that the semantics equipped with the involved types never change. There's also a performance issue. I must map a C/C++ long to a 64bit value at all times and thereby get a suboptimal solution. >> An API should ideally hide the internals of the underlying code so >> I'm not sure this is a valid reason. > > > Well, the executor allows you to specify a 64-bit count on platforms > where "long" is 64-bit, and a 32-bit count otherwise. Exactly. Why should a user of the SPI API be exposed to or even concerned with this at all? As an application programmer you couldn't care less. You want your app to perform equally well on all platforms without surprises. IMHO, PostgreSQL should make a decision whether the SPI functions support 32-bit or the 64-bit sizes for result sets and the API should reflect that choice. Having the maximum number of rows dependent on platform ports is a bad design. Regards, Thomas Hallgren
Thomas Hallgren <thhal@mailblocks.com> writes:
> Exactly. Why should a user of the SPI API be exposed to or even
> concerned with this at all? As an application programmer you couldn't
> care less. You want your app to perform equally well on all platforms
> without surprises. IMHO, PostgreSQL should make a decision whether the
> SPI functions support 32-bit or the 64-bit sizes for result sets and the
> API should reflect that choice. Having the maximum number of rows
> dependent on platform ports is a bad design.
The fact that 64-bit platforms can tackle bigger problems than 32-bit
ones is not a bug to be worked around, and so I don't see any problem
with the use of "long" for tuple counts. Furthermore, we have never
promised ABI-level compatibility across versions inside the backend,
and we are quite unlikely to make such a promise in the foreseeable
future. (Most of the time you are lucky if you get source-level
compatibility ;-).) So I can't get excited about avoiding platform
dependency in this particular tiny aspect of the API.
regards, tom lane
Tom Lane wrote: >Thomas Hallgren <thhal@mailblocks.com> writes: > > >>Exactly. Why should a user of the SPI API be exposed to or even >>concerned with this at all? As an application programmer you couldn't >>care less. You want your app to perform equally well on all platforms >>without surprises. IMHO, PostgreSQL should make a decision whether the >>SPI functions support 32-bit or the 64-bit sizes for result sets and the >>API should reflect that choice. Having the maximum number of rows >>dependent on platform ports is a bad design. >> >> > >The fact that 64-bit platforms can tackle bigger problems than 32-bit >ones is not a bug to be worked around, and so I don't see any problem >with the use of "long" for tuple counts. > I'm not concerned with the use of 32 or 64 bits. I would be equally happy with both. What I am concerned is that the problem that started this "SPI bug" was caused by the differences in how platforms handle the int and long types. Instead of rectifying this problem once and for all, the type was just changed to a long. > Furthermore, we have never >promised ABI-level compatibility across versions inside the backend, >and we are quite unlikely to make such a promise in the foreseeable >future. > I know that no promises has been made but PostgreSQL is improved every day and this would be a very easy promise to make. > (Most of the time you are lucky if you get source-level >compatibility ;-).) So I can't get excited about avoiding platform >dependency in this particular tiny aspect of the API. > > Maybe I've misunderstood the objectives behind the SPI layer altogether but since it's well documented and seems to be the "public interface" of the backend that extensions are supposed to use, I think it would be an excellent idea to make that interface as stable and platform independent as possible. I can't really see the disadvantages. The use of int, long, and long long is often a source of bugs (as with this one) and many recommend that you avoid them when possible. The definition of int is meant to be a datatype used for storing integers where size of that datatype equals natural size of processor. The long is defined as 'at least as big as int' and the 'long long' is 'bigger than long'. I wonder what that makes 'long long' on a platform where the int is 64 bits. 128 bits? Also, the interpretation of the definition vary between compiler vendors. On Windows Itanium, the int is 32 bit. On Unix it's 64. It's a mess... The 1998 revision of C declares the following types for a good reason: int8_t , int16_t, int32_t int64_t, uint8_t, uint16_t, uint32_t, uint64_t. Why not use them unless you have very specific requirements? And why not *always* use them in a public interface like the SPI? Regards, Thomas Hallgren
Thomas Hallgren wrote: > Tom Lane wrote: >> Furthermore, we have never promised ABI-level compatibility across >> versions inside the backend, and we are quite unlikely to make such >> a promise in the foreseeable future. >> > I know that no promises has been made but PostgreSQL is improved every > day and this would be a very easy promise to make. Binary compatibility of backend APIs is by no means a "very easy promise to make," nor would it be a good idea in any case. > Also, the interpretation of the definition vary between compiler > vendors. On Windows Itanium, the int is 32 bit. On Unix it's 64. `int' is 32 bit on most modern platforms I can think of. Perhaps you're thinking of `long', which is indeed 64-bit on many 64-bit Unixen but 32-bit on 64-bit Windows (BTW, this likely means that Postgres is completely broken on 64-bit Windows: sizeof(Datum) == sizeof(unsigned long) == 4, sizeof(void *) == 8). > The 1998 revision of C declares the following types for a good reason: > > int8_t , int16_t, int32_t int64_t, > uint8_t, uint16_t, uint32_t, uint64_t. We don't currently depend on C99, and not all platforms have a 64-bit datatype. In any case, I'm still unconvinced that using `int' and `long' in backend APIs is a problem. -Neil
Neil Conway wrote: > We don't currently depend on C99, and not all platforms have a 64-bit > datatype. In any case, I'm still unconvinced that using `int' and > `long' in backend APIs is a problem. Using long means that you sometimes get a 32-bit value and sometimes a 64-bit value, I think we agree on that. There's no correlation between getting a 64-bit value and the fact that you run on a 64-bit platform since many 64-bit platforms treat a long as 32-bit. I think we agree on that too. If the objective behind using a long is that you get a data-type that followes the CPU register size then that objective is not met. No such data-type exists unless you use C99 intptr_t (an int that can hold a pointer). You could of course explicitly typedef a such in c.h but AFAICS, there is no such definition today. By using a long you will: a) maximize the differences of the SPI interfaces between platforms. b) only enable 64-bit resultset sizes on a limited range of 64-bit platforms. Wouldn't it be better if you: a) Minimized the differences between platforms. b) Made a decision to either use 32- or 64-bit resultset sizes (my preference would be the former) or to conseqently used 32-bit resultset sizes on 32-bit platforms and 64-bit resultset sizes on 64-bit platforms? Regards, Thomas Hallgren