Thread: Re: [HACKERS] libpq and SPI
Hi, Just in case you'd like to see what I was talking about, I am attaching my patch to src/interfaces/libpq/fe-exec.c to prevent utility functions called from SPI from locking up the client. Jerry
Applied. [Charset iso-8859-1 unsupported, filtering to ASCII...] > Hi, > > Just in case you'd like to see what I was talking about, I am attaching > my patch to src/interfaces/libpq/fe-exec.c to prevent utility functions > called from SPI from locking up the client. > > Jerry > [Attachment, skipping...] -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <maillist@candle.pha.pa.us> writes:
> Applied.
>
>> Just in case you'd like to see what I was talking about, I am attaching
>> my patch to src/interfaces/libpq/fe-exec.c to prevent utility functions
>> called from SPI from locking up the client.
Uh, I didn't actually believe that that patch was a good idea. Hacking
libpq to survive a protocol violation committed by the backend is *not*
a solution; the correct answer is to fix the backend. Otherwise we will
have to discover similar workarounds for other clients that do not
use libpq (ODBC, for example).
Please reverse out that patch until someone can find some time to look
at the issue. (I will, if no one else does, but it would probably be
more efficient for someone who already knows something about SPI to
fix it...)
regards, tom lane
Tom Lane wrote: > Please reverse out that patch until someone can find some time to look > at the issue. (I will, if no one else does, but it would probably be > more efficient for someone who already knows something about SPI to > fix it...) What is the problem? I'll research a SPI patch. :) Clark
Reversed out. Attached is the patch. > Bruce Momjian <maillist@candle.pha.pa.us> writes: > > Applied. > > > >> Just in case you'd like to see what I was talking about, I am attaching > >> my patch to src/interfaces/libpq/fe-exec.c to prevent utility functions > >> called from SPI from locking up the client. > > Uh, I didn't actually believe that that patch was a good idea. Hacking > libpq to survive a protocol violation committed by the backend is *not* > a solution; the correct answer is to fix the backend. Otherwise we will > have to discover similar workarounds for other clients that do not > use libpq (ODBC, for example). > > Please reverse out that patch until someone can find some time to look > at the issue. (I will, if no one else does, but it would probably be > more efficient for someone who already knows something about SPI to > fix it...) > > regards, tom lane > -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026 --- fe-exec.c.orig Mon Feb 22 05:03:04 1999 +++ fe-exec.c Fri Mar 5 06:03:43 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,16 @@ 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 or FETCH 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) || + (strncmp(conn->result->cmdStatus, "FETCH", 5) == 0)) + pendingT = 0; + } + if (!pendingT) conn->asyncStatus = PGASYNC_READY; break; case 'E': /* error return */ if (pqGets(conn->errorMessage, ERROR_MSG_LENGTH, conn)) @@ -416,10 +426,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': /* emptyquery */ /* read and throw away the closing '\0' */ @@ -434,7 +445,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 +466,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 +486,13 @@ * We stop parsing until the application accepts * thecurrent 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 +510,7 @@ } break; case 'B': /* Binary data tuple */ + pendingT = 0; if (conn->result != NULL) { /* Read another tuple of a normal query response */ @@ -510,12 +528,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 characterthe "
> What is the problem? I'll research a SPI patch.
Check the hackers thread (last month I think) titled "libpq and SPI";
the problem occurs when one submits a utility statement rather than
a plannable query via SPI. Apparently what is happening is that the
backend emits a 'T' message before it invokes the called statement
and then emits a 'D' message afterwards --- so if the called statement
causes a 'C' message to come out, libpq gets unhappy. This seems
to be clearly a violation of the FE/BE protocol to me, so I don't think
it's libpq's fault.
Reasonable fixes might be to postpone the sending of 'T' till after
the invoked statement is executed, or to modify the traffic cop so
that a utility statement invoked from SPI doesn't send 'C'.
I know a little bit about the parts of the backend that communicate with
the frontend, but nothing about SPI, so I'm not well prepared to solve
the problem by myself.
regards, tom lane
> Tom Lane wrote: > > Please reverse out that patch until someone can find some time to look > > at the issue. (I will, if no one else does, but it would probably be > > more efficient for someone who already knows something about SPI to > > fix it...) > > What is the problem? I'll research a SPI patch. > > :) Clark > I think someone already supplied such a patch. Update via cvs and try it. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Hello all, > -----Original Message----- > From: owner-pgsql-hackers@postgreSQL.org > [mailto:owner-pgsql-hackers@postgreSQL.org]On Behalf Of Tom Lane > Sent: Monday, March 15, 1999 3:21 AM > To: Clark Evans > Cc: pgsql-hackers@postgreSQL.org > Subject: Re: [HACKERS] libpq and SPI > > > > What is the problem? I'll research a SPI patch. > > Check the hackers thread (last month I think) titled "libpq and SPI"; > the problem occurs when one submits a utility statement rather than > a plannable query via SPI. Apparently what is happening is that the > backend emits a 'T' message before it invokes the called statement > and then emits a 'D' message afterwards --- so if the called statement > causes a 'C' message to come out, libpq gets unhappy. This seems > to be clearly a violation of the FE/BE protocol to me, so I don't think > it's libpq's fault. > > Reasonable fixes might be to postpone the sending of 'T' till after > the invoked statement is executed, or to modify the traffic cop so > that a utility statement invoked from SPI doesn't send 'C'. > > I know a little bit about the parts of the backend that communicate with > the frontend, but nothing about SPI, so I'm not well prepared to solve > the problem by myself. > Probably it's not the problem of SPI. Specific utility commands(CREATE USER/ALTER USER/DROP USER /CREATE DATABASE/DROP DATABASE) break FE/BE protocol when they are executed inside the PostgreSQL function call. They call pg_exec_query() (tcop/postgres.c) in their implementation. In many cases the destination of pg_exec_query() is "Remote" which means that results are sent to frontend process. But the results shouldn't be sent to the frontend directly inside the execution of PostgreSQL function. ProcessUtility() (tcop/utility.c) function processes utility commands and takes the destination of command results as its second parameter. So should we call pg_exec_query_dest(query_string, the destination parameter of ProcessUtility(), FALSE) instead of pg_exec_query(), shouldn't we ? Thanks. Hiroshi Inoue Inoue@tpf.co.jp
>
> > What is the problem? I'll research a SPI patch.
>
> Check the hackers thread (last month I think) titled "libpq and SPI";
> the problem occurs when one submits a utility statement rather than
> a plannable query via SPI. Apparently what is happening is that the
> backend emits a 'T' message before it invokes the called statement
> and then emits a 'D' message afterwards --- so if the called statement
> causes a 'C' message to come out, libpq gets unhappy. This seems
> to be clearly a violation of the FE/BE protocol to me, so I don't think
> it's libpq's fault.
>
> Reasonable fixes might be to postpone the sending of 'T' till after
> the invoked statement is executed, or to modify the traffic cop so
> that a utility statement invoked from SPI doesn't send 'C'.
>
> I know a little bit about the parts of the backend that communicate with
> the frontend, but nothing about SPI, so I'm not well prepared to solve
> the problem by myself.
Another major problem of SPI on utilities must get fixed
prior.
SPI cannot save a prepared plan for utilities because
copyObject() doesn't handle the utility part of Query. So it
get's NULL and invoking such a saved plan will cause the
backend to crash.
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#======================================== jwieck@debis.com (Jan Wieck) #
Are you ready to install the blinking map in the main site? -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
>
>
> Are you ready to install the blinking map in the main site?
Not yet.
I still have problems with the incompatibilities between NS4
and IE4. It's terrible.
I have the whole map inside a centered table entry.
If I don't put the main image (the map itself) into a
diversion, I can get the x,y location of the image after
loading in NS via the undocumented non-standard properties x
and y. But then I'm unable to get the position in IE4.
OTOH, if I put the image into a diversion with a style, then
I can get the position in both browsers. But this time, NS4
does very strange things if I resize the browser window (the
image jumps out of table into the upper left corner of the
window :-}).
I'm still playing around with it locally. Hope to have a
solution later today.
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#======================================== jwieck@debis.com (Jan Wieck) #
Hello all,
> -----Original Message-----
> From: owner-pgsql-hackers@postgreSQL.org
> [mailto:owner-pgsql-hackers@postgreSQL.org]On Behalf Of Hiroshi Inoue
> Sent: Monday, March 15, 1999 7:00 PM
> To: Tom Lane; Clark Evans
> Cc: pgsql-hackers@postgreSQL.org
> Subject: RE: [HACKERS] libpq and SPI
>
> >
> >
> > > What is the problem? I'll research a SPI patch.
> >
> > Check the hackers thread (last month I think) titled "libpq and SPI";
> > the problem occurs when one submits a utility statement rather than
> > a plannable query via SPI. Apparently what is happening is that the
> > backend emits a 'T' message before it invokes the called statement
> > and then emits a 'D' message afterwards --- so if the called statement
> > causes a 'C' message to come out, libpq gets unhappy. This seems
> > to be clearly a violation of the FE/BE protocol to me, so I don't think
> > it's libpq's fault.
> >
> > Reasonable fixes might be to postpone the sending of 'T' till after
> > the invoked statement is executed, or to modify the traffic cop so
> > that a utility statement invoked from SPI doesn't send 'C'.
> >
> > I know a little bit about the parts of the backend that communicate with
> > the frontend, but nothing about SPI, so I'm not well prepared to solve
> > the problem by myself.
> >
>
> Probably it's not the problem of SPI.
> Specific utility commands(CREATE USER/ALTER USER/DROP USER
> /CREATE DATABASE/DROP DATABASE) break FE/BE protocol
> when they are executed inside the PostgreSQL function call.
>
Here is a patch.
I have changed to call pg_exec_query_dest() instead of pg_exec_query().
Thanks.
Hiroshi Inoue
Inoue@tpf.co.jp
*** backend/tcop/utility.c.orig Thu Feb 18 17:01:27 1999
--- backend/tcop/utility.c Tue Mar 16 09:25:07 1999
***************
*** 57,65 **** #include "utils/syscache.h" #endif
! void DefineUser(CreateUserStmt *stmt);
! void AlterUser(AlterUserStmt *stmt);
! void RemoveUser(char *username);
/* ---------------- * CHECK_IF_ABORTED() is used to avoid doing unnecessary
--- 57,65 ---- #include "utils/syscache.h" #endif
! void DefineUser(CreateUserStmt *stmt, CommandDest);
! void AlterUser(AlterUserStmt *stmt, CommandDest);
! void RemoveUser(char *username, CommandDest);
/* ---------------- * CHECK_IF_ABORTED() is used to avoid doing unnecessary
***************
*** 558,564 ****
PS_SET_STATUS(commandTag = "CREATEDB"); CHECK_IF_ABORTED();
! createdb(stmt->dbname, stmt->dbpath, stmt->encoding); } break;
--- 558,564 ----
PS_SET_STATUS(commandTag = "CREATEDB"); CHECK_IF_ABORTED();
! createdb(stmt->dbname, stmt->dbpath, stmt->encoding, dest); } break;
***************
*** 568,574 ****
PS_SET_STATUS(commandTag = "DESTROYDB"); CHECK_IF_ABORTED();
! destroydb(stmt->dbname); } break;
--- 568,574 ----
PS_SET_STATUS(commandTag = "DESTROYDB"); CHECK_IF_ABORTED();
! destroydb(stmt->dbname, dest); } break;
***************
*** 748,754 **** PS_SET_STATUS(commandTag = "CREATE USER"); CHECK_IF_ABORTED();
! DefineUser((CreateUserStmt *) parsetree); break;
case T_AlterUserStmt:
--- 748,754 ---- PS_SET_STATUS(commandTag = "CREATE USER"); CHECK_IF_ABORTED();
! DefineUser((CreateUserStmt *) parsetree, dest); break;
case T_AlterUserStmt:
***************
*** 755,761 **** PS_SET_STATUS(commandTag = "ALTER USER"); CHECK_IF_ABORTED();
! AlterUser((AlterUserStmt *) parsetree); break;
case T_DropUserStmt:
--- 755,761 ---- PS_SET_STATUS(commandTag = "ALTER USER"); CHECK_IF_ABORTED();
! AlterUser((AlterUserStmt *) parsetree, dest); break;
case T_DropUserStmt:
***************
*** 762,768 **** PS_SET_STATUS(commandTag = "DROP USER"); CHECK_IF_ABORTED();
! RemoveUser(((DropUserStmt *) parsetree)->user); break;
case T_LockStmt:
--- 762,768 ---- PS_SET_STATUS(commandTag = "DROP USER"); CHECK_IF_ABORTED();
! RemoveUser(((DropUserStmt *) parsetree)->user, dest); break;
case T_LockStmt:
*** backend/commands/user.c.orig Thu Feb 18 17:00:38 1999
--- backend/commands/user.c Tue Mar 16 09:50:09 1999
***************
*** 46,52 **** */ static void
! UpdatePgPwdFile(char *sql) {
char *filename,
--- 46,52 ---- */ static void
! UpdatePgPwdFile(char *sql, CommandDest dest) {
char *filename,
***************
*** 71,77 **** snprintf(sql, SQL_LENGTH, "copy %s to '%s' using delimiters %s",
ShadowRelationName,tempname, CRYPT_PWD_FILE_SEPCHAR);
! pg_exec_query(sql); rename(tempname, filename); pfree((void *) tempname);
--- 71,77 ---- snprintf(sql, SQL_LENGTH, "copy %s to '%s' using delimiters %s",
ShadowRelationName,tempname, CRYPT_PWD_FILE_SEPCHAR);
! pg_exec_query_dest(sql, dest, false); rename(tempname, filename); pfree((void *) tempname);
***************
*** 92,98 **** *--------------------------------------------------------------------- */ void
! DefineUser(CreateUserStmt *stmt) {
char *pg_shadow,
--- 92,98 ---- *--------------------------------------------------------------------- */ void
! DefineUser(CreateUserStmt *stmt, CommandDest dest) {
char *pg_shadow,
***************
*** 175,187 **** stmt->password ? stmt->password : "''", stmt->validUntil ? stmt->validUntil :
"");
! pg_exec_query(sql);
/* * Add the stuff here for groups. */
! UpdatePgPwdFile(sql);
/* * This goes after the UpdatePgPwdFile to be certain that two backends
--- 175,187 ---- stmt->password ? stmt->password : "''", stmt->validUntil ? stmt->validUntil :
"");
! pg_exec_query_dest(sql, dest, false);
/* * Add the stuff here for groups. */
! UpdatePgPwdFile(sql, dest);
/* * This goes after the UpdatePgPwdFile to be certain that two backends
***************
*** 196,202 ****
extern void
! AlterUser(AlterUserStmt *stmt) {
char *pg_shadow,
--- 196,202 ----
extern void
! AlterUser(AlterUserStmt *stmt, CommandDest dest) {
char *pg_shadow,
***************
*** 282,292 ****
snprintf(sql, SQL_LENGTH, "%s where usename = '%s'", sql, stmt->user);
! pg_exec_query(sql);
/* do the pg_group stuff here */
! UpdatePgPwdFile(sql);
UnlockRelation(pg_shadow_rel, AccessExclusiveLock); heap_close(pg_shadow_rel);
--- 282,292 ----
snprintf(sql, SQL_LENGTH, "%s where usename = '%s'", sql, stmt->user);
! pg_exec_query_dest(sql, dest, false);
/* do the pg_group stuff here */
! UpdatePgPwdFile(sql, dest);
UnlockRelation(pg_shadow_rel, AccessExclusiveLock); heap_close(pg_shadow_rel);
***************
*** 297,303 ****
extern void
! RemoveUser(char *user) {
char *pg_shadow;
--- 297,303 ----
extern void
! RemoveUser(char *user, CommandDest dest) {
char *pg_shadow;
***************
*** 390,396 **** elog(NOTICE, "Dropping database %s", dbase[ndbase]); snprintf(sql, SQL_LENGTH, "drop
database%s", dbase[ndbase]); pfree((void *) dbase[ndbase]);
! pg_exec_query(sql); } if (dbase) pfree((void *) dbase);
--- 390,396 ---- elog(NOTICE, "Dropping database %s", dbase[ndbase]); snprintf(sql, SQL_LENGTH, "drop
database%s", dbase[ndbase]); pfree((void *) dbase[ndbase]);
! pg_exec_query_dest(sql, dest, false); } if (dbase) pfree((void *) dbase);
***************
*** 418,426 **** */ snprintf(sql, SQL_LENGTH, "delete from %s where usename = '%s'",
ShadowRelationName,user);
! pg_exec_query(sql);
! UpdatePgPwdFile(sql);
UnlockRelation(pg_shadow_rel, AccessExclusiveLock); heap_close(pg_shadow_rel);
--- 418,426 ---- */ snprintf(sql, SQL_LENGTH, "delete from %s where usename = '%s'",
ShadowRelationName,user);
! pg_exec_query_dest(sql, dest, false);
! UpdatePgPwdFile(sql, dest);
UnlockRelation(pg_shadow_rel, AccessExclusiveLock); heap_close(pg_shadow_rel);
*** backend/commands/dbcommands.c.orig Thu Feb 18 17:00:36 1999
--- backend/commands/dbcommands.c Tue Mar 16 09:36:33 1999
***************
*** 24,30 **** #include "catalog/catname.h" #include "catalog/pg_database.h" #include "catalog/pg_shadow.h"
- #include "commands/dbcommands.h" #include "fmgr.h" #include "miscadmin.h" /* for DataDir */ #include
"storage/bufmgr.h"
--- 24,29 ----
***************
*** 31,36 ****
--- 30,36 ---- #include "storage/fd.h" #include "storage/lmgr.h" #include "tcop/tcopprot.h"
+ #include "commands/dbcommands.h" #include "utils/rel.h" #include "utils/syscache.h"
***************
*** 42,48 **** static void stop_vacuum(char *dbpath, char *dbname);
void
! createdb(char *dbname, char *dbpath, int encoding) { Oid db_id; int4 user_id;
--- 42,48 ---- static void stop_vacuum(char *dbpath, char *dbname);
void
! createdb(char *dbname, char *dbpath, int encoding, CommandDest dest) { Oid db_id; int4
user_id;
***************
*** 87,97 **** "insert into pg_database (datname, datdba, encoding, datpath)" " values ('%s',
'%d','%d', '%s');", dbname, user_id, encoding,
loc);
! pg_exec_query(buf); }
void
! destroydb(char *dbname) { int4 user_id; Oid db_id;
--- 87,97 ---- "insert into pg_database (datname, datdba, encoding, datpath)" " values ('%s',
'%d','%d', '%s');", dbname, user_id, encoding,
loc);
! pg_exec_query_dest(buf, dest, false); }
void
! destroydb(char *dbname, CommandDest dest) { int4 user_id; Oid db_id;
***************
*** 123,129 **** */ snprintf(buf, 512, "delete from pg_database where pg_database.oid =
\'%d\'::oid",
db_id);
! pg_exec_query(buf);
/* * remove the data directory. If the DELETE above failed, this will
--- 123,129 ---- */ snprintf(buf, 512, "delete from pg_database where pg_database.oid =
\'%d\'::oid",
db_id);
! pg_exec_query_dest(buf ,dest, false);
/* * remove the data directory. If the DELETE above failed, this will
*** include/commands/user.h.orig Thu Feb 18 17:01:49 1999
--- include/commands/user.h Tue Mar 16 09:23:01 1999
***************
*** 10,17 **** #ifndef USER_H #define USER_H
! extern void DefineUser(CreateUserStmt *stmt);
! extern void AlterUser(AlterUserStmt *stmt);
! extern void RemoveUser(char *user);
#endif /* USER_H */
--- 10,17 ---- #ifndef USER_H #define USER_H
! extern void DefineUser(CreateUserStmt *stmt, CommandDest);
! extern void AlterUser(AlterUserStmt *stmt, CommandDest);
! extern void RemoveUser(char *user, CommandDest);
#endif /* USER_H */
*** include/commands/dbcommands.h.orig Thu Feb 18 17:01:48 1999
--- include/commands/dbcommands.h Tue Mar 16 09:23:52 1999
***************
*** 19,25 **** */ #define SIGKILLDAEMON1 SIGTERM
! extern void createdb(char *dbname, char *dbpath, int encoding);
! extern void destroydb(char *dbname);
#endif /* DBCOMMANDS_H */
--- 19,25 ---- */ #define SIGKILLDAEMON1 SIGTERM
! extern void createdb(char *dbname, char *dbpath, int encoding,
CommandDest);
! extern void destroydb(char *dbname, CommandDest);
#endif /* DBCOMMANDS_H */
Applied.
---------------------------------------------------------------------------
Hello all,
> -----Original Message-----
> From: owner-pgsql-hackers@postgreSQL.org
> [mailto:owner-pgsql-hackers@postgreSQL.org]On Behalf Of Hiroshi Inoue
> Sent: Monday, March 15, 1999 7:00 PM
> To: Tom Lane; Clark Evans
> Cc: pgsql-hackers@postgreSQL.org
> Subject: RE: [HACKERS] libpq and SPI
>
> >
> >
> > > What is the problem? I'll research a SPI patch.
> >
> > Check the hackers thread (last month I think) titled "libpq and SPI";
> > the problem occurs when one submits a utility statement rather than
> > a plannable query via SPI. Apparently what is happening is that the
> > backend emits a 'T' message before it invokes the called statement
> > and then emits a 'D' message afterwards --- so if the called statement
> > causes a 'C' message to come out, libpq gets unhappy. This seems
> > to be clearly a violation of the FE/BE protocol to me, so I don't think
> > it's libpq's fault.
> >
> > Reasonable fixes might be to postpone the sending of 'T' till after
> > the invoked statement is executed, or to modify the traffic cop so
> > that a utility statement invoked from SPI doesn't send 'C'.
> >
> > I know a little bit about the parts of the backend that communicate with
> > the frontend, but nothing about SPI, so I'm not well prepared to solve
> > the problem by myself.
> >
>
> Probably it's not the problem of SPI.
> Specific utility commands(CREATE USER/ALTER USER/DROP USER
> /CREATE DATABASE/DROP DATABASE) break FE/BE protocol
> when they are executed inside the PostgreSQL function call.
>
Here is a patch.
I have changed to call pg_exec_query_dest() instead of pg_exec_query().
Thanks.
Hiroshi Inoue
Inoue@tpf.co.jp
*** backend/tcop/utility.c.orig Thu Feb 18 17:01:27 1999
--- backend/tcop/utility.c Tue Mar 16 09:25:07 1999
***************
*** 57,65 **** #include "utils/syscache.h" #endif
! void DefineUser(CreateUserStmt *stmt);
! void AlterUser(AlterUserStmt *stmt);
! void RemoveUser(char *username);
/* ---------------- * CHECK_IF_ABORTED() is used to avoid doing unnecessary
--- 57,65 ---- #include "utils/syscache.h" #endif
! void DefineUser(CreateUserStmt *stmt, CommandDest);
! void AlterUser(AlterUserStmt *stmt, CommandDest);
! void RemoveUser(char *username, CommandDest);
/* ---------------- * CHECK_IF_ABORTED() is used to avoid doing unnecessary
***************
*** 558,564 ****
PS_SET_STATUS(commandTag = "CREATEDB"); CHECK_IF_ABORTED();
! createdb(stmt->dbname, stmt->dbpath, stmt->encoding); } break;
--- 558,564 ----
PS_SET_STATUS(commandTag = "CREATEDB"); CHECK_IF_ABORTED();
! createdb(stmt->dbname, stmt->dbpath, stmt->encoding, dest); } break;
***************
*** 568,574 ****
PS_SET_STATUS(commandTag = "DESTROYDB"); CHECK_IF_ABORTED();
! destroydb(stmt->dbname); } break;
--- 568,574 ----
PS_SET_STATUS(commandTag = "DESTROYDB"); CHECK_IF_ABORTED();
! destroydb(stmt->dbname, dest); } break;
***************
*** 748,754 **** PS_SET_STATUS(commandTag = "CREATE USER"); CHECK_IF_ABORTED();
! DefineUser((CreateUserStmt *) parsetree); break;
case T_AlterUserStmt:
--- 748,754 ---- PS_SET_STATUS(commandTag = "CREATE USER"); CHECK_IF_ABORTED();
! DefineUser((CreateUserStmt *) parsetree, dest); break;
case T_AlterUserStmt:
***************
*** 755,761 **** PS_SET_STATUS(commandTag = "ALTER USER"); CHECK_IF_ABORTED();
! AlterUser((AlterUserStmt *) parsetree); break;
case T_DropUserStmt:
--- 755,761 ---- PS_SET_STATUS(commandTag = "ALTER USER"); CHECK_IF_ABORTED();
! AlterUser((AlterUserStmt *) parsetree, dest); break;
case T_DropUserStmt:
***************
*** 762,768 **** PS_SET_STATUS(commandTag = "DROP USER"); CHECK_IF_ABORTED();
! RemoveUser(((DropUserStmt *) parsetree)->user); break;
case T_LockStmt:
--- 762,768 ---- PS_SET_STATUS(commandTag = "DROP USER"); CHECK_IF_ABORTED();
! RemoveUser(((DropUserStmt *) parsetree)->user, dest); break;
case T_LockStmt:
*** backend/commands/user.c.orig Thu Feb 18 17:00:38 1999
--- backend/commands/user.c Tue Mar 16 09:50:09 1999
***************
*** 46,52 **** */ static void
! UpdatePgPwdFile(char *sql) {
char *filename,
--- 46,52 ---- */ static void
! UpdatePgPwdFile(char *sql, CommandDest dest) {
char *filename,
***************
*** 71,77 **** snprintf(sql, SQL_LENGTH, "copy %s to '%s' using delimiters %s",
ShadowRelationName,tempname, CRYPT_PWD_FILE_SEPCHAR);
! pg_exec_query(sql); rename(tempname, filename); pfree((void *) tempname);
--- 71,77 ---- snprintf(sql, SQL_LENGTH, "copy %s to '%s' using delimiters %s",
ShadowRelationName,tempname, CRYPT_PWD_FILE_SEPCHAR);
! pg_exec_query_dest(sql, dest, false); rename(tempname, filename); pfree((void *) tempname);
***************
*** 92,98 **** *--------------------------------------------------------------------- */ void
! DefineUser(CreateUserStmt *stmt) {
char *pg_shadow,
--- 92,98 ---- *--------------------------------------------------------------------- */ void
! DefineUser(CreateUserStmt *stmt, CommandDest dest) {
char *pg_shadow,
***************
*** 175,187 **** stmt->password ? stmt->password : "''", stmt->validUntil ? stmt->validUntil :
"");
! pg_exec_query(sql);
/* * Add the stuff here for groups. */
! UpdatePgPwdFile(sql);
/* * This goes after the UpdatePgPwdFile to be certain that two backends
--- 175,187 ---- stmt->password ? stmt->password : "''", stmt->validUntil ? stmt->validUntil :
"");
! pg_exec_query_dest(sql, dest, false);
/* * Add the stuff here for groups. */
! UpdatePgPwdFile(sql, dest);
/* * This goes after the UpdatePgPwdFile to be certain that two backends
***************
*** 196,202 ****
extern void
! AlterUser(AlterUserStmt *stmt) {
char *pg_shadow,
--- 196,202 ----
extern void
! AlterUser(AlterUserStmt *stmt, CommandDest dest) {
char *pg_shadow,
***************
*** 282,292 ****
snprintf(sql, SQL_LENGTH, "%s where usename = '%s'", sql, stmt->user);
! pg_exec_query(sql);
/* do the pg_group stuff here */
! UpdatePgPwdFile(sql);
UnlockRelation(pg_shadow_rel, AccessExclusiveLock); heap_close(pg_shadow_rel);
--- 282,292 ----
snprintf(sql, SQL_LENGTH, "%s where usename = '%s'", sql, stmt->user);
! pg_exec_query_dest(sql, dest, false);
/* do the pg_group stuff here */
! UpdatePgPwdFile(sql, dest);
UnlockRelation(pg_shadow_rel, AccessExclusiveLock); heap_close(pg_shadow_rel);
***************
*** 297,303 ****
extern void
! RemoveUser(char *user) {
char *pg_shadow;
--- 297,303 ----
extern void
! RemoveUser(char *user, CommandDest dest) {
char *pg_shadow;
***************
*** 390,396 **** elog(NOTICE, "Dropping database %s", dbase[ndbase]); snprintf(sql, SQL_LENGTH, "drop
database%s", dbase[ndbase]); pfree((void *) dbase[ndbase]);
! pg_exec_query(sql); } if (dbase) pfree((void *) dbase);
--- 390,396 ---- elog(NOTICE, "Dropping database %s", dbase[ndbase]); snprintf(sql, SQL_LENGTH, "drop
database%s", dbase[ndbase]); pfree((void *) dbase[ndbase]);
! pg_exec_query_dest(sql, dest, false); } if (dbase) pfree((void *) dbase);
***************
*** 418,426 **** */ snprintf(sql, SQL_LENGTH, "delete from %s where usename = '%s'",
ShadowRelationName,user);
! pg_exec_query(sql);
! UpdatePgPwdFile(sql);
UnlockRelation(pg_shadow_rel, AccessExclusiveLock); heap_close(pg_shadow_rel);
--- 418,426 ---- */ snprintf(sql, SQL_LENGTH, "delete from %s where usename = '%s'",
ShadowRelationName,user);
! pg_exec_query_dest(sql, dest, false);
! UpdatePgPwdFile(sql, dest);
UnlockRelation(pg_shadow_rel, AccessExclusiveLock); heap_close(pg_shadow_rel);
*** backend/commands/dbcommands.c.orig Thu Feb 18 17:00:36 1999
--- backend/commands/dbcommands.c Tue Mar 16 09:36:33 1999
***************
*** 24,30 **** #include "catalog/catname.h" #include "catalog/pg_database.h" #include "catalog/pg_shadow.h"
- #include "commands/dbcommands.h" #include "fmgr.h" #include "miscadmin.h" /* for DataDir */ #include
"storage/bufmgr.h"
--- 24,29 ----
***************
*** 31,36 ****
--- 30,36 ---- #include "storage/fd.h" #include "storage/lmgr.h" #include "tcop/tcopprot.h"
+ #include "commands/dbcommands.h" #include "utils/rel.h" #include "utils/syscache.h"
***************
*** 42,48 **** static void stop_vacuum(char *dbpath, char *dbname);
void
! createdb(char *dbname, char *dbpath, int encoding) { Oid db_id; int4 user_id;
--- 42,48 ---- static void stop_vacuum(char *dbpath, char *dbname);
void
! createdb(char *dbname, char *dbpath, int encoding, CommandDest dest) { Oid db_id; int4
user_id;
***************
*** 87,97 **** "insert into pg_database (datname, datdba, encoding, datpath)" " values ('%s',
'%d','%d', '%s');", dbname, user_id, encoding,
loc);
! pg_exec_query(buf); }
void
! destroydb(char *dbname) { int4 user_id; Oid db_id;
--- 87,97 ---- "insert into pg_database (datname, datdba, encoding, datpath)" " values ('%s',
'%d','%d', '%s');", dbname, user_id, encoding,
loc);
! pg_exec_query_dest(buf, dest, false); }
void
! destroydb(char *dbname, CommandDest dest) { int4 user_id; Oid db_id;
***************
*** 123,129 **** */ snprintf(buf, 512, "delete from pg_database where pg_database.oid =
\'%d\'::oid",
db_id);
! pg_exec_query(buf);
/* * remove the data directory. If the DELETE above failed, this will
--- 123,129 ---- */ snprintf(buf, 512, "delete from pg_database where pg_database.oid =
\'%d\'::oid",
db_id);
! pg_exec_query_dest(buf ,dest, false);
/* * remove the data directory. If the DELETE above failed, this will
*** include/commands/user.h.orig Thu Feb 18 17:01:49 1999
--- include/commands/user.h Tue Mar 16 09:23:01 1999
***************
*** 10,17 **** #ifndef USER_H #define USER_H
! extern void DefineUser(CreateUserStmt *stmt);
! extern void AlterUser(AlterUserStmt *stmt);
! extern void RemoveUser(char *user);
#endif /* USER_H */
--- 10,17 ---- #ifndef USER_H #define USER_H
! extern void DefineUser(CreateUserStmt *stmt, CommandDest);
! extern void AlterUser(AlterUserStmt *stmt, CommandDest);
! extern void RemoveUser(char *user, CommandDest);
#endif /* USER_H */
*** include/commands/dbcommands.h.orig Thu Feb 18 17:01:48 1999
--- include/commands/dbcommands.h Tue Mar 16 09:23:52 1999
***************
*** 19,25 **** */ #define SIGKILLDAEMON1 SIGTERM
! extern void createdb(char *dbname, char *dbpath, int encoding);
! extern void destroydb(char *dbname);
#endif /* DBCOMMANDS_H */
--- 19,25 ---- */ #define SIGKILLDAEMON1 SIGTERM
! extern void createdb(char *dbname, char *dbpath, int encoding,
CommandDest);
! extern void destroydb(char *dbname, CommandDest);
#endif /* DBCOMMANDS_H */
-- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610)
853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill,
Pennsylvania19026
Is this fixed? > > What is the problem? I'll research a SPI patch. > > Check the hackers thread (last month I think) titled "libpq and SPI"; > the problem occurs when one submits a utility statement rather than > a plannable query via SPI. Apparently what is happening is that the > backend emits a 'T' message before it invokes the called statement > and then emits a 'D' message afterwards --- so if the called statement > causes a 'C' message to come out, libpq gets unhappy. This seems > to be clearly a violation of the FE/BE protocol to me, so I don't think > it's libpq's fault. > > Reasonable fixes might be to postpone the sending of 'T' till after > the invoked statement is executed, or to modify the traffic cop so > that a utility statement invoked from SPI doesn't send 'C'. > > I know a little bit about the parts of the backend that communicate with > the frontend, but nothing about SPI, so I'm not well prepared to solve > the problem by myself. > > regards, tom lane > > -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026