SPI and libpq problems

Hi,

I'm working on porting an application from Sybase to Postgres. One of
the things I really need is the group privilege support. Currently, the
server appears to ignore the create user .... in group clause. So I
thought I would write a create_user function to handle it. This led me to
find two bugs. One simple and the other not so simple. The first is in the
execq() SPI demo function. If you want to see it, try this:

Build the execq() example and insert it into your database. Then do
this:

template1=> create user fred;
template1=> select execq('select * from pg_user', 2);

You should see that the backend crashes! This is because the execq
function isn't checking for NULL columns in the result set. I fixed it like
this:

int execq(text *sql, int cnt)
{
int ret;
int proc = 0;

SPI_connect();

ret = SPI_exec(textout(sql), cnt);

proc = SPI_processed;

if ( (ret == SPI_OK_SELECT) && (SPI_processed > 0) ) {

TupleDesc tupdesc = SPI_tuptable->tupdesc;
SPITupleTable *tuptable = SPI_tuptable;
char buf[8192];
int i;

for (ret = 0; ret < proc; ret++) {

HeapTuple tuple = tuptable->vals[ret];

for (i = 1, buf[0] = 0; i <= tupdesc->natts; i++) {

char *cp = SPI_getvalue(tuple, tupdesc, i);

if (cp != NULL) {
sprintf(buf + strlen(buf), " %s%s", cp, (i ==
tupdesc->natts) ? " " : "|");
} else {
sprintf(buf + strlen(buf), " %s", (i == tupdesc->natts)
? " " : "|");
}

}

elog(NOTICE, "EXECQ: %s", buf);
}
}

SPI_finish();

return proc;
}

I just added a check for NULL in the inner for loop. OK, now try this:

template1=> select execq('drop user fred', 1);

You should get:

Backend sent D message without prior T

And then psql will hang! I need to send "utility" queries inside SPI
because create_user first must actually create the user, then insert his
sysid into pg_group. So when I tried to do this, I got the same error as
the one above. I watched the conversation between the backend and libpq and
saw that the server was sending a 'T' message, then one or more 'C'
messages, then finally the 'D' message. libpq throws the row description
info (T) away when it gets the 'C' message. Then when it gets the 'D', it
is unprepared. I patched postgresql-6.4.2/src/interfaces/libpq/fe-exec.c so
that a 'T' message is remembered until either a 'D' message is received, a
'C' message of type SELECT, or some error is seen. Here is my patch:

==============================================================
--- fe-exec.c.orig Mon Feb 22 05:03:04 1999
+++ fe-exec.c Mon Feb 22 05:12:28 1999
@@ -338,6 +338,7 @@
parseInput(PGconn *conn)
{
char id;
+ static int pendingT = 0;

/*
* Loop to parse successive complete messages available in the buffer.
@@ -406,7 +407,15 @@
PGRES_COMMAND_OK);
if (pqGets(conn->result->cmdStatus, CMDSTATUS_LEN, conn))
return;
- conn->asyncStatus = PGASYNC_READY;
+ if (pendingT) {
+ /* Check the returned message
*/
+ /* if it's a SELECT in a
pendingT case, */
+ /* then it probably means no
rows returned. */
+ /* We clear pendingT in that
case. */
+ if
(strncmp(conn->result->cmdStatus, "SELECT", 6) == 0)
+ pendingT = 0;
+ }
+ if (!pendingT) conn->asyncStatus = PGASYNC_READY;
break;
case 'E': /* error return */
if (pqGets(conn->errorMessage, ERROR_MSG_LENGTH, conn))
@@ -416,10 +425,11 @@
/* and build an error result holding the error message */
conn->result = PQmakeEmptyPGresult(conn,
PGRES_FATAL_ERROR);
- conn->asyncStatus = PGASYNC_READY;
+ if (!pendingT) conn->asyncStatus = PGASYNC_READY;
break;
case 'Z': /* backend is ready for new query */
conn->asyncStatus = PGASYNC_IDLE;
+ pendingT = 0;
break;
case 'I': /* empty query */
/* read and throw away the closing '\0' */
@@ -434,7 +444,7 @@
if (conn->result == NULL)
conn->result = PQmakeEmptyPGresult(conn,
PGRES_EMPTY_QUERY);
- conn->asyncStatus = PGASYNC_READY;
+ if (!pendingT) conn->asyncStatus = PGASYNC_READY;
break;
case 'K': /* secret key data from the backend */

@@ -455,11 +465,15 @@
break;
case 'T': /* row descriptions (start of query
* results) */
+ if (pendingT) {
+ DONOTICE(conn, "Got second 'T'
message!\n");
+ }
if (conn->result == NULL)
{
/* First 'T' in a query sequence */
if (getRowDescriptions(conn))
return;
+ pendingT = 1;
}
else
{
@@ -471,11 +485,13 @@
* We stop parsing until the application accepts
* the current result.
*/
+ pendingT = 0;
conn->asyncStatus = PGASYNC_READY;
return;
}
break;
case 'D': /* ASCII data tuple */
+ pendingT = 0;
if (conn->result != NULL)
{
/* Read another tuple of a normal query response */
@@ -493,6 +509,7 @@
}
break;
case 'B': /* Binary data tuple */
+ pendingT = 0;
if (conn->result != NULL)
{
/* Read another tuple of a normal query response */
@@ -510,12 +527,15 @@
}
break;
case 'G': /* Start Copy In */
+ pendingT = 0;
conn->asyncStatus = PGASYNC_COPY_IN;
break;
case 'H': /* Start Copy Out */
+ pendingT = 0;
conn->asyncStatus = PGASYNC_COPY_OUT;
break;
default:
+ pendingT = 0;
sprintf(conn->errorMessage,
"unknown protocol character '%c' read from backend. "
"(The protocol character is the first character the "
=========================================================

So I guess my question is: Is this the right way to fix this? Is it
reasonable to expect to be able to do things like "create user" inside of a
SPI function? Should the server be doing something different with the order
of messages sent? For now, I'm going to use my patched libpq so my work can
continue. I would like to get opinions on this and eventually "do the right
thing."

By the way, I'm using Solaris 2.6 with egcs-1.1.1.

Jerry