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
Attachments:
spi_count_long-2.patchtext/x-patch; name=spi_count_long-2.patch; x-mac-creator=0; x-mac-type=0Download
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
Import Notes
Reply to msg id not found: thhal-043ZQA8s1yicdVlURpJvKDpDAgdPLNt@mailblocks.com
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 fixedISTM 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
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
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:
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
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
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
Import Notes
Reply to msg id not found: thhal-0o+FRAwcyicbD2E2gwGJZ0ukeolGX@mailblocks.com
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