Re: Update doc changes needed

Started by Bruce Momjianalmost 26 years ago2 messageshackersdocs
Jump to latest
#1Bruce Momjian
bruce@momjian.us
hackersdocs

Can someone comment on this?

Tom Lane <tgl@sss.pgh.pa.us> writes in pgsql-hackers@postgresql.org:

SL Baur pointed out a few days ago that PQsetenv* are too fragile to
risk exporting in their current state. I plan to make them non-exported
and remove 'em from the docs, unless someone comes up with a working
redesign PDQ.

I made a test patch last weekend if you want it. It's been stress
tested in an XEmacs Lisp-calling-libpq environment.

This patch adds a PQsetenvClear function that is analogous to the
clear function for PQresult's and changes the PQsetenvPoll call to
accept a PQsetenvHandle. The PQsetenvHandle is freed automatically
when called during a database connect. When the asynchronous setenv
calls are called by application code, it is now the responsibility of
the application code to free the setenvHandle with PQsetenvClear.

Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.123
diff -u -r1.123 fe-connect.c
--- src/interfaces/libpq/fe-connect.c	2000/03/11 03:08:36	1.123
+++ src/interfaces/libpq/fe-connect.c	2000/03/24 03:38:25
@@ -1314,6 +1314,8 @@
* variables to server.
*/
+			if (conn->setenv_handle)
+				PQsetenvClear(conn->setenv_handle);
if ((conn->setenv_handle = PQsetenvStart(conn)) == NULL)
goto error_return;

@@ -1327,10 +1329,12 @@
these queries. */
conn->status = CONNECTION_OK;

-			switch (PQsetenvPoll(conn))
+			switch (PQsetenvPoll(conn->setenv_handle))
{
case PGRES_POLLING_OK: /* Success */
conn->status = CONNECTION_OK;
+					free(conn->setenv_handle);
+					conn->setenv_handle = (PGsetenvHandle)NULL;
return PGRES_POLLING_OK;

case PGRES_POLLING_READING: /* Still going */
@@ -1343,6 +1347,8 @@

default:
conn->status = CONNECTION_SETENV;
+					free(conn->setenv_handle);
+					conn->setenv_handle = (PGsetenvHandle)NULL;
goto error_return;
}
/* Unreachable */
@@ -1385,6 +1391,11 @@
if (conn == NULL || conn->status == CONNECTION_BAD)
return NULL;
+	if (conn->setenv_handle) {
+		PQsetenvClear(conn->setenv_handle);
+		conn->setenv_handle = (PGsetenvHandle)NULL;
+	}
+
if ((handle = malloc(sizeof(struct pg_setenv_state))) == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
@@ -1394,6 +1405,7 @@
}

handle->conn = conn;
+ conn->setenv_handle = handle;
handle->res = NULL;
handle->eo = EnvironmentOptions;

@@ -1416,9 +1428,10 @@
* ----------------
*/
PostgresPollingStatusType
-PQsetenvPoll(PGconn *conn)
+PQsetenvPoll(PGsetenvHandle handle)
{
-	PGsetenvHandle handle = conn->setenv_handle;
+/*	PGsetenvHandle handle = conn->setenv_handle; */
+
#ifdef MULTIBYTE
static const char envname[] = "PGCLIENTENCODING";
#endif
@@ -1503,10 +1516,10 @@
encoding = PQgetvalue(handle->res, 0, 0);
if (!encoding)			/* this should not happen */
-					conn->client_encoding = SQL_ASCII;
+					handle->conn->client_encoding = SQL_ASCII;
else
/* set client encoding to pg_conn struct */
-					conn->client_encoding = pg_char_to_encoding(encoding);
+					handle->conn->client_encoding = pg_char_to_encoding(encoding);
PQclear(handle->res);
/* We have to keep going in order to clear up the query */
goto keep_going;
@@ -1590,7 +1603,9 @@
case SETENV_STATE_OK:
/* Tidy up */
-			free(handle);
+			/* This is error prone and requires error conditions to be */
+			/*  treated specially */
+			/* free(handle); */
return PGRES_POLLING_OK;

default:
@@ -1606,7 +1621,7 @@
handle->state = SETENV_STATE_FAILED; /* This may protect us even if we
* are called after the handle
* has been freed. */
- free(handle);
+ /* free(handle); */
return PGRES_POLLING_FAILED;
}

@@ -1627,10 +1642,24 @@
if (handle->state != SETENV_STATE_FAILED)
{
handle->state = SETENV_STATE_FAILED;
- free(handle);
+ /* free(handle); */
}
}

+/* ----------------
+ *		PQsetenvClear
+ *
+ * Explicitly release a PGsetenvHandle
+ *
+ * ----------------
+ */
+void
+PQsetenvClear(PGsetenvHandle handle)
+{
+	if (!handle) return;
+	handle->conn->setenv_handle = (PGsetenvHandle)NULL;
+	free(handle);
+}

/* ----------------
* PQsetenv
@@ -1655,6 +1684,10 @@
if ((handle = PQsetenvStart(conn)) == NULL)
return 0;

+ if (conn->setenv_handle)
+ free(conn->setenv_handle);
+ conn->setenv_handle = handle;
+
for (;;) {
/*
* Wait, if necessary. Note that the initial state (just after
@@ -1692,7 +1725,7 @@
/*
* Now try to advance the state machine.
*/
- flag = PQsetenvPoll(conn);
+ flag = PQsetenvPoll(handle);
}
}

@@ -1716,6 +1749,7 @@
conn->asyncStatus = PGASYNC_IDLE;
conn->notifyList = DLNewList();
conn->sock = -1;
+	conn->setenv_handle = (PGsetenvHandle)NULL;
#ifdef USE_SSL
conn->allow_ssl_try = TRUE;
#endif
@@ -1868,6 +1902,9 @@
{
if (conn)
{
+		/* It is safe to do this now */
+		if (conn->setenv_handle)
+			PQsetenvClear(conn->setenv_handle);
closePGconn(conn);
freePGconn(conn);
}
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.61
diff -u -r1.61 libpq-fe.h
--- src/interfaces/libpq/libpq-fe.h	2000/03/11 03:08:37	1.61
+++ src/interfaces/libpq/libpq-fe.h	2000/03/24 03:38:26
@@ -234,8 +234,9 @@
/* Passing of environment variables */
/* Asynchronous (non-blocking) */
extern PGsetenvHandle PQsetenvStart(PGconn *conn);
-	extern PostgresPollingStatusType PQsetenvPoll(PGconn *conn);
+	extern PostgresPollingStatusType PQsetenvPoll(PGsetenvHandle handle);
extern void PQsetenvAbort(PGsetenvHandle handle);
+	extern void PQsetenvClear(PGsetenvHandle handle);

/* Synchronous (blocking) */
extern int PQsetenv(PGconn *conn);

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
#2Bruce Momjian
bruce@momjian.us
In reply to: Bruce Momjian (#1)
hackersdocs
Re: [HACKERS] Re: Update doc changes needed

OK, seems the issue is closed.

SL Baur <steve@beopen.com> writes:

I made a test patch last weekend if you want it. It's been stress
tested in an XEmacs Lisp-calling-libpq environment.

This patch adds a PQsetenvClear function that is analogous to the
clear function for PQresult's and changes the PQsetenvPoll call to
accept a PQsetenvHandle. The PQsetenvHandle is freed automatically
when called during a database connect. When the asynchronous setenv
calls are called by application code, it is now the responsibility of
the application code to free the setenvHandle with PQsetenvClear.

I think this is just throwing good work after bad. The entire exercise
can be eliminated by merging the two useful fields of the 'handle'
object into PGconn (at a net cost of 4 bytes); there's no reason to do
all the bookkeeping needed to keep track of a separate handle object.

I already did that, and de-exported the PQsetenv routines, earlier
today. Since I still haven't heard anyone make an argument why any
app would want to call PQsetenv, I don't see a reason to do more work
on these routines.

regards, tom lane

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026