Thread: Out-of-memory error reports in libpq
While cleaning out dead branches in my git repo, I came across an
early draft of what eventually became commit ffa2e4670 ("In libpq,
always append new error messages to conn->errorMessage"). I realized
that it contained a good idea that had gotten lost on the way to that
commit. Namely, let's reduce all of the 60-or-so "out of memory"
reports in libpq to calls to a common subroutine, and then let's teach
the common subroutine a recovery strategy for the not-unlikely
possibility that it fails to append the "out of memory" string to
conn->errorMessage. That recovery strategy of course is to reset the
errorMessage buffer to empty, hopefully regaining some space. We lose
whatever we'd had in the buffer before, but we have a better chance of
the "out of memory" message making its way to the user.
The first half of that just saves a few hundred bytes of repetitive
coding. However, I think that the addition of recovery logic is
important for robustness, because as things stand libpq may be
worse off than before for OOM handling. Before ffa2e4670, almost
all of these call sites did printfPQExpBuffer(..., "out of memory").
That would automatically clear the message buffer to empty, and
thereby be sure to report the out-of-memory failure if at all
possible. Now we might fail to report the thing that the user
really needs to know to make sense of what happened.
Therefore, I feel like this was an oversight in ffa2e4670,
and we ought to back-patch the attached into v14.
cc'ing the RMT in case they wish to object.
regards, tom lane
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 4337e89ce9..48565343eb 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -378,8 +378,7 @@ build_client_first_message(fe_scram_state *state)
state->client_nonce = malloc(encoded_len + 1);
if (state->client_nonce == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return NULL;
}
encoded_len = pg_b64_encode(raw_nonce, SCRAM_RAW_NONCE_LEN,
@@ -453,8 +452,7 @@ build_client_first_message(fe_scram_state *state)
oom_error:
termPQExpBuffer(&buf);
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return NULL;
}
@@ -607,8 +605,7 @@ build_client_final_message(fe_scram_state *state)
oom_error:
termPQExpBuffer(&buf);
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return NULL;
}
@@ -628,8 +625,7 @@ read_server_first_message(fe_scram_state *state, char *input)
state->server_first_message = strdup(input);
if (state->server_first_message == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return false;
}
@@ -654,8 +650,7 @@ read_server_first_message(fe_scram_state *state, char *input)
state->nonce = strdup(nonce);
if (state->nonce == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return false;
}
@@ -669,8 +664,7 @@ read_server_first_message(fe_scram_state *state, char *input)
state->salt = malloc(decoded_salt_len);
if (state->salt == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return false;
}
state->saltlen = pg_b64_decode(encoded_salt,
@@ -719,8 +713,7 @@ read_server_final_message(fe_scram_state *state, char *input)
state->server_final_message = strdup(input);
if (!state->server_final_message)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return false;
}
@@ -758,8 +751,7 @@ read_server_final_message(fe_scram_state *state, char *input)
decoded_server_signature = malloc(server_signature_len);
if (!decoded_server_signature)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return false;
}
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 3421ed4685..c190f5ced9 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -73,9 +73,7 @@ pg_GSS_continue(PGconn *conn, int payloadlen)
ginbuf.value = malloc(payloadlen);
if (!ginbuf.value)
{
- appendPQExpBuffer(&conn->errorMessage,
- libpq_gettext("out of memory allocating GSSAPI buffer (%d)\n"),
- payloadlen);
+ pqReportOOM(conn);
return STATUS_ERROR;
}
if (pqGetnchar(ginbuf.value, payloadlen, conn))
@@ -227,9 +225,7 @@ pg_SSPI_continue(PGconn *conn, int payloadlen)
inputbuf = malloc(payloadlen);
if (!inputbuf)
{
- appendPQExpBuffer(&conn->errorMessage,
- libpq_gettext("out of memory allocating SSPI buffer (%d)\n"),
- payloadlen);
+ pqReportOOM(conn);
return STATUS_ERROR;
}
if (pqGetnchar(inputbuf, payloadlen, conn))
@@ -287,8 +283,7 @@ pg_SSPI_continue(PGconn *conn, int payloadlen)
conn->sspictx = malloc(sizeof(CtxtHandle));
if (conn->sspictx == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return STATUS_ERROR;
}
memcpy(conn->sspictx, &newContext, sizeof(CtxtHandle));
@@ -359,8 +354,7 @@ pg_SSPI_startup(PGconn *conn, int use_negotiate, int payloadlen)
conn->sspicred = malloc(sizeof(CredHandle));
if (conn->sspicred == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return STATUS_ERROR;
}
@@ -395,8 +389,7 @@ pg_SSPI_startup(PGconn *conn, int use_negotiate, int payloadlen)
conn->sspitarget = malloc(strlen(conn->krbsrvname) + strlen(host) + 2);
if (!conn->sspitarget)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return STATUS_ERROR;
}
sprintf(conn->sspitarget, "%s/%s", conn->krbsrvname, host);
@@ -620,8 +613,7 @@ oom_error:
termPQExpBuffer(&mechanism_buf);
if (initialresponse)
free(initialresponse);
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return STATUS_ERROR;
}
@@ -644,9 +636,7 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final)
challenge = malloc(payloadlen + 1);
if (!challenge)
{
- appendPQExpBuffer(&conn->errorMessage,
- libpq_gettext("out of memory allocating SASL buffer (%d)\n"),
- payloadlen);
+ pqReportOOM(conn);
return STATUS_ERROR;
}
@@ -795,8 +785,7 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
crypt_pwd = malloc(2 * (MD5_PASSWD_LEN + 1));
if (!crypt_pwd)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return STATUS_ERROR;
}
@@ -1153,8 +1142,7 @@ pg_fe_getauthname(PQExpBuffer errorMessage)
{
result = strdup(name);
if (result == NULL && errorMessage)
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
}
pgunlock_thread();
@@ -1303,8 +1291,7 @@ PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user,
}
if (!crypt_pwd)
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return crypt_pwd;
}
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index e950b41374..071e87b71f 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -903,8 +903,7 @@ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
*connmember = strdup(tmp);
if (*connmember == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return false;
}
}
@@ -1447,8 +1446,7 @@ connectOptions2(PGconn *conn)
oom_error:
conn->status = CONNECTION_BAD;
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return false;
}
@@ -1616,8 +1614,7 @@ PQsetdbLogin(const char *pghost, const char *pgport, const char *pgoptions,
oom_error:
conn->status = CONNECTION_BAD;
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return conn;
}
@@ -2968,8 +2965,7 @@ keep_going: /* We will come back to here until there is
EnvironmentOptions);
if (!startpacket)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
goto error_return;
}
@@ -4650,7 +4646,7 @@ ldapServiceLookup(const char *purl, PQconninfoOption *options,
if ((url = strdup(purl)) == NULL)
{
- appendPQExpBufferStr(errorMessage, libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
return 3;
}
@@ -4910,8 +4906,7 @@ ldapServiceLookup(const char *purl, PQconninfoOption *options,
size += values[i]->bv_len + 1;
if ((result = malloc(size)) == NULL)
{
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
ldap_value_free_len(values);
ldap_unbind(ld);
return 3;
@@ -5029,8 +5024,7 @@ ldapServiceLookup(const char *purl, PQconninfoOption *options,
options[i].val = strdup(optval);
if (!options[i].val)
{
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
free(result);
return 3;
}
@@ -5281,8 +5275,7 @@ parseServiceFile(const char *serviceFile,
options[i].val = strdup(val);
if (!options[i].val)
{
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
result = 3;
goto exit;
}
@@ -5362,8 +5355,7 @@ conninfo_init(PQExpBuffer errorMessage)
options = (PQconninfoOption *) malloc(sizeof(PQconninfoOption) * sizeof(PQconninfoOptions) /
sizeof(PQconninfoOptions[0]));
if (options == NULL)
{
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
return NULL;
}
opt_dest = options;
@@ -5461,8 +5453,7 @@ conninfo_parse(const char *conninfo, PQExpBuffer errorMessage,
/* Need a modifiable copy of the input string */
if ((buf = strdup(conninfo)) == NULL)
{
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
PQconninfoFree(options);
return NULL;
}
@@ -5717,8 +5708,7 @@ conninfo_array_parse(const char *const *keywords, const char *const *values,
options[k].val = strdup(str_option->val);
if (!options[k].val)
{
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
PQconninfoFree(options);
PQconninfoFree(dbname_options);
return NULL;
@@ -5746,8 +5736,7 @@ conninfo_array_parse(const char *const *keywords, const char *const *values,
option->val = strdup(pvalue);
if (!option->val)
{
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
PQconninfoFree(options);
PQconninfoFree(dbname_options);
return NULL;
@@ -5818,8 +5807,7 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
if (!option->val)
{
if (errorMessage)
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
return false;
}
continue;
@@ -5842,8 +5830,7 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
if (!option->val)
{
if (errorMessage)
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
return false;
}
continue;
@@ -5860,8 +5847,7 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
if (!option->val)
{
if (errorMessage)
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
return false;
}
continue;
@@ -5961,8 +5947,7 @@ conninfo_uri_parse_options(PQconninfoOption *options, const char *uri,
initPQExpBuffer(&portbuf);
if (PQExpBufferDataBroken(hostbuf) || PQExpBufferDataBroken(portbuf))
{
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
goto cleanup;
}
@@ -5970,8 +5955,7 @@ conninfo_uri_parse_options(PQconninfoOption *options, const char *uri,
buf = strdup(uri);
if (buf == NULL)
{
- appendPQExpBufferStr(errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
goto cleanup;
}
start = buf;
@@ -6329,7 +6313,7 @@ conninfo_uri_decode(const char *str, PQExpBuffer errorMessage)
buf = malloc(strlen(str) + 1);
if (buf == NULL)
{
- appendPQExpBufferStr(errorMessage, libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
return NULL;
}
p = buf;
@@ -6479,7 +6463,7 @@ conninfo_storeval(PQconninfoOption *connOptions,
value_copy = strdup(value);
if (value_copy == NULL)
{
- appendPQExpBufferStr(errorMessage, libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(errorMessage);
return NULL;
}
}
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index aca81890bb..dc00448292 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1205,8 +1205,7 @@ pqAllocCmdQueueEntry(PGconn *conn)
entry = (PGcmdQueueEntry *) malloc(sizeof(PGcmdQueueEntry));
if (entry == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return NULL;
}
}
@@ -3025,8 +3024,7 @@ pqPipelineProcessQueue(PGconn *conn)
conn->result = PQmakeEmptyPGresult(conn, PGRES_PIPELINE_ABORTED);
if (!conn->result)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
pqSaveErrorResult(conn);
return;
}
@@ -3981,8 +3979,7 @@ PQescapeInternal(PGconn *conn, const char *str, size_t len, bool as_ident)
result = rp = (char *) malloc(result_size);
if (rp == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return NULL;
}
@@ -4146,8 +4143,7 @@ PQescapeByteaInternal(PGconn *conn,
if (rp == NULL)
{
if (conn)
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return NULL;
}
diff --git a/src/interfaces/libpq/fe-gssapi-common.c b/src/interfaces/libpq/fe-gssapi-common.c
index 9e8aeae119..cd5a0b6c70 100644
--- a/src/interfaces/libpq/fe-gssapi-common.c
+++ b/src/interfaces/libpq/fe-gssapi-common.c
@@ -107,8 +107,7 @@ pg_GSS_load_servicename(PGconn *conn)
temp_gbuf.value = (char *) malloc(maxlen);
if (!temp_gbuf.value)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return STATUS_ERROR;
}
snprintf(temp_gbuf.value, maxlen, "%s@%s",
diff --git a/src/interfaces/libpq/fe-lobj.c b/src/interfaces/libpq/fe-lobj.c
index ffd9926dc4..29ce74af8f 100644
--- a/src/interfaces/libpq/fe-lobj.c
+++ b/src/interfaces/libpq/fe-lobj.c
@@ -877,8 +877,7 @@ lo_initialize(PGconn *conn)
lobjfuncs = (PGlobjfuncs *) malloc(sizeof(PGlobjfuncs));
if (lobjfuncs == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return -1;
}
MemSet((char *) lobjfuncs, 0, sizeof(PGlobjfuncs));
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 9a2a970293..92590a9d80 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -339,8 +339,7 @@ pqCheckOutBufferSpace(size_t bytes_needed, PGconn *conn)
}
/* realloc failed. Probably out of memory */
- appendPQExpBufferStr(&conn->errorMessage,
- "cannot allocate memory for output buffer\n");
+ pqReportOOM(conn);
return EOF;
}
@@ -433,8 +432,7 @@ pqCheckInBufferSpace(size_t bytes_needed, PGconn *conn)
}
/* realloc failed. Probably out of memory */
- appendPQExpBufferStr(&conn->errorMessage,
- "cannot allocate memory for input buffer\n");
+ pqReportOOM(conn);
return EOF;
}
@@ -1233,6 +1231,45 @@ PQenv2encoding(void)
return encoding;
}
+/*
+ * Report an out-of-memory error by appending a message to conn->errorMessage.
+ *
+ * This task is just common enough, and nontrivial enough, to deserve its own
+ * subroutine.
+ */
+void
+pqReportOOM(PGconn *conn)
+{
+ pqReportOOMBuffer(&conn->errorMessage);
+}
+
+/*
+ * As above, but work with a bare error-message-buffer pointer.
+ */
+void
+pqReportOOMBuffer(PQExpBuffer errorMessage)
+{
+ const char *msg = libpq_gettext("out of memory\n");
+
+ /*
+ * First just try to append the message. Even though we're up against
+ * OOM, this is likely to succeed because of unused space within
+ * errorMessage's buffer.
+ */
+ appendPQExpBufferStr(errorMessage, msg);
+
+ /*
+ * If that didn't succeed, it'll have marked the buffer broken. Our
+ * fallback plan is to reset the buffer (hopefully regaining some space)
+ * and try again. If still no luck, not much we can do.
+ */
+ if (PQExpBufferBroken(errorMessage))
+ {
+ resetPQExpBuffer(errorMessage);
+ appendPQExpBufferStr(errorMessage, msg);
+ }
+}
+
#ifdef ENABLE_NLS
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 2e83305348..08b9b5d97e 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -215,8 +215,7 @@ pqParseInput3(PGconn *conn)
PGRES_COMMAND_OK);
if (!conn->result)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory"));
+ pqReportOOM(conn);
pqSaveErrorResult(conn);
}
}
@@ -240,8 +239,7 @@ pqParseInput3(PGconn *conn)
PGRES_PIPELINE_SYNC);
if (!conn->result)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory"));
+ pqReportOOM(conn);
pqSaveErrorResult(conn);
}
else
@@ -269,8 +267,7 @@ pqParseInput3(PGconn *conn)
PGRES_EMPTY_QUERY);
if (!conn->result)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory"));
+ pqReportOOM(conn);
pqSaveErrorResult(conn);
}
}
@@ -287,8 +284,7 @@ pqParseInput3(PGconn *conn)
PGRES_COMMAND_OK);
if (!conn->result)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory"));
+ pqReportOOM(conn);
pqSaveErrorResult(conn);
}
}
@@ -367,8 +363,7 @@ pqParseInput3(PGconn *conn)
PGRES_COMMAND_OK);
if (!conn->result)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory"));
+ pqReportOOM(conn);
pqSaveErrorResult(conn);
}
}
@@ -650,10 +645,10 @@ advance_and_error:
* freeing the old result first greatly improves the odds that gettext()
* will succeed in providing a translation.
*/
- if (!errmsg)
- errmsg = libpq_gettext("out of memory for query result");
-
- appendPQExpBuffer(&conn->errorMessage, "%s\n", errmsg);
+ if (errmsg)
+ appendPQExpBuffer(&conn->errorMessage, "%s\n", errmsg);
+ else
+ pqReportOOM(conn);
pqSaveErrorResult(conn);
/*
@@ -739,9 +734,10 @@ advance_and_error:
* freeing the old result first greatly improves the odds that gettext()
* will succeed in providing a translation.
*/
- if (!errmsg)
- errmsg = libpq_gettext("out of memory");
- appendPQExpBuffer(&conn->errorMessage, "%s\n", errmsg);
+ if (errmsg)
+ appendPQExpBuffer(&conn->errorMessage, "%s\n", errmsg);
+ else
+ pqReportOOM(conn);
pqSaveErrorResult(conn);
/*
@@ -856,10 +852,10 @@ advance_and_error:
* freeing the old result first greatly improves the odds that gettext()
* will succeed in providing a translation.
*/
- if (!errmsg)
- errmsg = libpq_gettext("out of memory for query result");
-
- appendPQExpBuffer(&conn->errorMessage, "%s\n", errmsg);
+ if (errmsg)
+ appendPQExpBuffer(&conn->errorMessage, "%s\n", errmsg);
+ else
+ pqReportOOM(conn);
pqSaveErrorResult(conn);
/*
@@ -971,8 +967,7 @@ pqGetErrorNotice3(PGconn *conn, bool isError)
pqClearAsyncResult(conn); /* redundant, but be safe */
conn->result = res;
if (PQExpBufferDataBroken(workBuf))
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory"));
+ pqReportOOM(conn);
else
appendPQExpBufferStr(&conn->errorMessage, workBuf.data);
}
@@ -1013,7 +1008,7 @@ pqBuildErrorMessage3(PQExpBuffer msg, const PGresult *res,
/* If we couldn't allocate a PGresult, just say "out of memory" */
if (res == NULL)
{
- appendPQExpBufferStr(msg, libpq_gettext("out of memory\n"));
+ pqReportOOMBuffer(msg);
return;
}
@@ -1720,8 +1715,7 @@ pqGetCopyData3(PGconn *conn, char **buffer, int async)
*buffer = (char *) malloc(msgLength + 1);
if (*buffer == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return -2;
}
memcpy(*buffer, &conn->inBuffer[conn->inCursor], msgLength);
diff --git a/src/interfaces/libpq/fe-secure-common.c b/src/interfaces/libpq/fe-secure-common.c
index afa5d133e1..67e10d8200 100644
--- a/src/interfaces/libpq/fe-secure-common.c
+++ b/src/interfaces/libpq/fe-secure-common.c
@@ -106,8 +106,7 @@ pq_verify_peer_name_matches_certificate_name(PGconn *conn,
name = malloc(namelen + 1);
if (name == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return -1;
}
memcpy(name, namedata, namelen);
diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c
index c783a53734..d28cd6bdd7 100644
--- a/src/interfaces/libpq/fe-secure-gssapi.c
+++ b/src/interfaces/libpq/fe-secure-gssapi.c
@@ -500,8 +500,7 @@ pqsecure_open_gss(PGconn *conn)
PqGSSResultBuffer = malloc(PQ_GSS_RECV_BUFFER_SIZE);
if (!PqGSSSendBuffer || !PqGSSRecvBuffer || !PqGSSResultBuffer)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return PGRES_POLLING_FAILED;
}
PqGSSSendLength = PqGSSSendNext = PqGSSSendConsumed = 0;
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 2ee5a0a40a..d530ea9939 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -436,8 +436,7 @@ pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
cert_hash = malloc(hash_size);
if (cert_hash == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return NULL;
}
memcpy(cert_hash, hash, hash_size);
@@ -1134,8 +1133,7 @@ initialize_SSL(PGconn *conn)
if (engine_str == NULL)
{
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("out of memory\n"));
+ pqReportOOM(conn);
return -1;
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index e9f214b61b..3e7bf57b5f 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -697,6 +697,8 @@ extern int pqWaitTimed(int forRead, int forWrite, PGconn *conn,
time_t finish_time);
extern int pqReadReady(PGconn *conn);
extern int pqWriteReady(PGconn *conn);
+extern void pqReportOOM(PGconn *conn);
+extern void pqReportOOMBuffer(PQExpBuffer errorMessage);
/* === in fe-secure.c === */
On 7/27/21, 3:41 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> The first half of that just saves a few hundred bytes of repetitive
> coding. However, I think that the addition of recovery logic is
> important for robustness, because as things stand libpq may be
> worse off than before for OOM handling. Before ffa2e4670, almost
> all of these call sites did printfPQExpBuffer(..., "out of memory").
> That would automatically clear the message buffer to empty, and
> thereby be sure to report the out-of-memory failure if at all
> possible. Now we might fail to report the thing that the user
> really needs to know to make sense of what happened.
IIUC, before ffa2e4670, callers mainly used printfPQExpBuffer(), which
always cleared the buffer before attempting to append the OOM message.
With ffa2e4670 applied, callers always attempt to append the OOM
message without resetting the buffer first. With this new change,
callers will attempt to append the OOM message without resetting the
buffer first, but if that fails, we fall back to the original behavior
before ffa2e4670.
+ if (PQExpBufferBroken(errorMessage))
+ {
+ resetPQExpBuffer(errorMessage);
+ appendPQExpBufferStr(errorMessage, msg);
+ }
I see that appendPQExpBufferStr() checks whether the buffer is broken
by way of enlargePQExpBuffer(), so the fallback steps roughly match
the calls to printfPQExpBuffer() before ffa2e4670.
- appendPQExpBuffer(&conn->errorMessage,
- libpq_gettext("out of memory allocating GSSAPI buffer (%d)\n"),
- payloadlen);
+ pqReportOOM(conn);
I see that some context is lost in a few places (e.g., the one above
points to a GSSAPI buffer). Perhaps this extra context could be
useful to identify problematic areas, but it might be unlikely to help
much in these parts of libpq. In any case, the vast majority of
existing callers don't provide any extra context.
Overall, the patch looks good to me.
> Therefore, I feel like this was an oversight in ffa2e4670,
> and we ought to back-patch the attached into v14.
Back-patching to v14 seems reasonable to me.
Nathan
"Bossart, Nathan" <bossartn@amazon.com> writes:
> - appendPQExpBuffer(&conn->errorMessage,
> - libpq_gettext("out of memory allocating GSSAPI buffer (%d)\n"),
> - payloadlen);
> + pqReportOOM(conn);
> I see that some context is lost in a few places (e.g., the one above
> points to a GSSAPI buffer). Perhaps this extra context could be
> useful to identify problematic areas, but it might be unlikely to help
> much in these parts of libpq. In any case, the vast majority of
> existing callers don't provide any extra context.
Yeah, there are half a dozen places that currently print something
more specific than "out of memory". I judged that the value of this
was not worth the complexity it'd add to support it in this scheme.
Different opinions welcome of course.
regards, tom lane
On Tue, Jul 27, 2021 at 10:31:25PM -0400, Tom Lane wrote:
> Yeah, there are half a dozen places that currently print something
> more specific than "out of memory". I judged that the value of this
> was not worth the complexity it'd add to support it in this scheme.
> Different opinions welcome of course.
I don't mind either that this removes a bit of context. For
unlikely-going-to-happen errors that's not worth the extra translation
cost. No objections from me for an integration into 14 as that's
straight-forward, and that would minimize conflicts between HEAD and
14 in the event of a back-patch
+pqReportOOM(PGconn *conn)
+{
+ pqReportOOMBuffer(&conn->errorMessage);
+}
+
+/*
+ * As above, but work with a bare error-message-buffer pointer.
+ */
+void
+pqReportOOMBuffer(PQExpBuffer errorMessage)
+{
Not much a fan of having two routines to do this job though. I would
vote for keeping the one named pqReportOOM() with PQExpBuffer as
argument.
--
Michael
Attachment
On 7/27/21 6:40 PM, Tom Lane wrote:
> While cleaning out dead branches in my git repo, I came across an
> early draft of what eventually became commit ffa2e4670 ("In libpq,
> always append new error messages to conn->errorMessage"). I realized
> that it contained a good idea that had gotten lost on the way to that
> commit. Namely, let's reduce all of the 60-or-so "out of memory"
> reports in libpq to calls to a common subroutine, and then let's teach
> the common subroutine a recovery strategy for the not-unlikely
> possibility that it fails to append the "out of memory" string to
> conn->errorMessage. That recovery strategy of course is to reset the
> errorMessage buffer to empty, hopefully regaining some space. We lose
> whatever we'd had in the buffer before, but we have a better chance of
> the "out of memory" message making its way to the user.
>
> The first half of that just saves a few hundred bytes of repetitive
> coding. However, I think that the addition of recovery logic is
> important for robustness, because as things stand libpq may be
> worse off than before for OOM handling. Before ffa2e4670, almost
> all of these call sites did printfPQExpBuffer(..., "out of memory").
> That would automatically clear the message buffer to empty, and
> thereby be sure to report the out-of-memory failure if at all
> possible. Now we might fail to report the thing that the user
> really needs to know to make sense of what happened.
>
> Therefore, I feel like this was an oversight in ffa2e4670,
> and we ought to back-patch the attached into v14.
>
> cc'ing the RMT in case they wish to object.
>
>
I'm honored you've confused me with Alvaro :-)
This seems sensible, and we certainly shouldn't be worse off than
before, so let's do it.
I'm fine with having two functions for call simplicity, but I don't feel
strongly about it.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Jul 27, 2021 at 10:31:25PM -0400, Tom Lane wrote:
>> Yeah, there are half a dozen places that currently print something
>> more specific than "out of memory". I judged that the value of this
>> was not worth the complexity it'd add to support it in this scheme.
>> Different opinions welcome of course.
> I don't mind either that this removes a bit of context. For
> unlikely-going-to-happen errors that's not worth the extra translation
> cost.
Yeah, the extra translatable strings are the main concrete cost of
keeping this behavior. But I'm dubious that labeling a small number
of the possible OOM points is worth anything, especially if they're
not providing the failed allocation request size. You can't tell if
that request was unreasonable or if it was just an unlucky victim
of bloat elsewhere. Unifying the reports into a common function
could be a starting point for more consistent/detailed OOM reports,
if anyone cared to work on that. (I hasten to add that I don't.)
> + pqReportOOMBuffer(&conn->errorMessage);
> Not much a fan of having two routines to do this job though. I would
> vote for keeping the one named pqReportOOM() with PQExpBuffer as
> argument.
Here I've got to disagree. We do need the form with a PQExpBuffer
argument, because there are some places where that isn't a pointer
to a PGconn's errorMessage. But the large majority of the calls
are "pqReportOOM(conn)", and I think having to write that as
"pqReportOOM(&conn->errorMessage)" is fairly ugly and perhaps
error-prone.
I'm not wedded to the name "pqReportOOMBuffer" though --- maybe
there's some better name for that one?
regards, tom lane
On 7/28/21 11:02 AM, Tom Lane wrote: > > Here I've got to disagree. We do need the form with a PQExpBuffer > argument, because there are some places where that isn't a pointer > to a PGconn's errorMessage. But the large majority of the calls > are "pqReportOOM(conn)", and I think having to write that as > "pqReportOOM(&conn->errorMessage)" is fairly ugly and perhaps > error-prone. > > I'm not wedded to the name "pqReportOOMBuffer" though --- maybe > there's some better name for that one? > > Is it worth making the first one a macro? cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> Is it worth making the first one a macro?
It'd be the same from a source-code perspective, but probably a
shade bulkier in terms of object code.
regards, tom lane
Hi,
On 2021-07-27 18:40:48 -0400, Tom Lane wrote:
> The first half of that just saves a few hundred bytes of repetitive
> coding. However, I think that the addition of recovery logic is
> important for robustness, because as things stand libpq may be
> worse off than before for OOM handling.
Agreed.
> Before ffa2e4670, almost all of these call sites did
> printfPQExpBuffer(..., "out of memory"). That would automatically
> clear the message buffer to empty, and thereby be sure to report the
> out-of-memory failure if at all possible. Now we might fail to report
> the thing that the user really needs to know to make sense of what
> happened.
Hm. It seems we should be able to guarantee that the recovery path can print
something, at least in the PGconn case. Is it perhaps worth pre-sizing
PGConn->errorMessage so it'd fit an error like this?
But perhaps that's more effort than it's worth.
> +void
> +pqReportOOMBuffer(PQExpBuffer errorMessage)
> +{
> + const char *msg = libpq_gettext("out of memory\n");
I should probably know this, but I don't. Nor did I quickly find an answer. I
assume gettext() reliably and reasonably deals with OOM?
Looking in the gettext code I'm again scared by the fact that it takes locks
during gettext (because of stuff like erroring out of signal handlers, not
OOMs).
It does look like it tries to always return the original string in case of
OOM. Although the code is quite maze-like, so it's not easy to tell..
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
> I should probably know this, but I don't. Nor did I quickly find an answer. I
> assume gettext() reliably and reasonably deals with OOM?
I've always assumed that their fallback in cases of OOM, can't read
the message file, yadda yadda is to return the original string.
I admit I haven't gone and checked their code, but it'd be
unbelievably stupid to do otherwise.
> Looking in the gettext code I'm again scared by the fact that it takes locks
> during gettext (because of stuff like erroring out of signal handlers, not
> OOMs).
Hm.
regards, tom lane
Andres Freund <andres@anarazel.de> writes:
> Hm. It seems we should be able to guarantee that the recovery path can print
> something, at least in the PGconn case. Is it perhaps worth pre-sizing
> PGConn->errorMessage so it'd fit an error like this?
Forgot to address this. Right now, the normal situation is that
PGConn->errorMessage is "pre sized" to 256 bytes, because that's
what pqexpbuffer.c does for all PQExpBuffers. So unless you've
overrun that, the question is moot. If you have, and you got
an OOM in trying to expand the PQExpBuffer, then pqexpbuffer.c
will release what it has and substitute the "oom_buffer" empty
string. If you're really unlucky you might then not be able
to allocate another 256-byte buffer, in which case we end up
with an empty-string result. I don't think it's probable,
but in a multithread program it could happen.
> But perhaps that's more effort than it's worth.
Yeah. I considered changing things so that oom_buffer contains
"out of memory\n" rather than an empty string, but I'm afraid
that that's making unsupportable assumptions about what PQExpBuffers
are used for.
For now, I'm content if it's not worse than v13. We've not
heard a lot of complaints in this area.
regards, tom lane
I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> Hm. It seems we should be able to guarantee that the recovery path can print
>> something, at least in the PGconn case. Is it perhaps worth pre-sizing
>> PGConn->errorMessage so it'd fit an error like this?
>> But perhaps that's more effort than it's worth.
> Yeah. I considered changing things so that oom_buffer contains
> "out of memory\n" rather than an empty string, but I'm afraid
> that that's making unsupportable assumptions about what PQExpBuffers
> are used for.
Actually, wait a minute. There are only a couple of places that ever
read out the value of conn->errorMessage, so let's make those places
responsible for dealing with OOM scenarios. That leads to a nicely
small patch, as attached, and it looks to me like it makes us quite
bulletproof against such scenarios.
It might still be worth doing the "pqReportOOM" changes to save a
few bytes of code space, but I'm less excited about that now.
regards, tom lane
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index e950b41374..49eec3e835 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -6739,6 +6739,14 @@ PQerrorMessage(const PGconn *conn)
if (!conn)
return libpq_gettext("connection pointer is NULL\n");
+ /*
+ * The errorMessage buffer might be marked "broken" due to having
+ * previously failed to allocate enough memory for the message. In that
+ * case, tell the application we ran out of memory.
+ */
+ if (PQExpBufferBroken(&conn->errorMessage))
+ return libpq_gettext("out of memory\n");
+
return conn->errorMessage.data;
}
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index aca81890bb..87f348f3dc 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -191,7 +191,7 @@ PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status)
/* non-error cases */
break;
default:
- pqSetResultError(result, conn->errorMessage.data);
+ pqSetResultError(result, &conn->errorMessage);
break;
}
@@ -662,14 +662,28 @@ pqResultStrdup(PGresult *res, const char *str)
* assign a new error message to a PGresult
*/
void
-pqSetResultError(PGresult *res, const char *msg)
+pqSetResultError(PGresult *res, PQExpBuffer errorMessage)
{
+ char *msg;
+
if (!res)
return;
- if (msg && *msg)
- res->errMsg = pqResultStrdup(res, msg);
+
+ /*
+ * We handle two OOM scenarios here. The errorMessage buffer might be
+ * marked "broken" due to having previously failed to allocate enough
+ * memory for the message, or it might be fine but pqResultStrdup fails
+ * and returns NULL. In either case, just make res->errMsg point directly
+ * at a constant "out of memory" string.
+ */
+ if (!PQExpBufferBroken(errorMessage))
+ msg = pqResultStrdup(res, errorMessage->data);
+ else
+ msg = NULL;
+ if (msg)
+ res->errMsg = msg;
else
- res->errMsg = NULL;
+ res->errMsg = libpq_gettext("out of memory\n");
}
/*
@@ -2122,7 +2136,7 @@ PQgetResult(PGconn *conn)
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("PGEventProc \"%s\" failed during PGEVT_RESULTCREATE event\n"),
res->events[i].name);
- pqSetResultError(res, conn->errorMessage.data);
+ pqSetResultError(res, &conn->errorMessage);
res->resultStatus = PGRES_FATAL_ERROR;
break;
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index e9f214b61b..490458adef 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -637,7 +637,7 @@ extern pgthreadlock_t pg_g_threadlock;
/* === in fe-exec.c === */
-extern void pqSetResultError(PGresult *res, const char *msg);
+extern void pqSetResultError(PGresult *res, PQExpBuffer errorMessage);
extern void *pqResultAlloc(PGresult *res, size_t nBytes, bool isBinary);
extern char *pqResultStrdup(PGresult *res, const char *str);
extern void pqClearAsyncResult(PGconn *conn);
Em qua., 28 de jul. de 2021 às 21:25, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> Hm. It seems we should be able to guarantee that the recovery path can print
>> something, at least in the PGconn case. Is it perhaps worth pre-sizing
>> PGConn->errorMessage so it'd fit an error like this?
>> But perhaps that's more effort than it's worth.
> Yeah. I considered changing things so that oom_buffer contains
> "out of memory\n" rather than an empty string, but I'm afraid
> that that's making unsupportable assumptions about what PQExpBuffers
> are used for.
Actually, wait a minute. There are only a couple of places that ever
read out the value of conn->errorMessage, so let's make those places
responsible for dealing with OOM scenarios. That leads to a nicely
small patch, as attached, and it looks to me like it makes us quite
bulletproof against such scenarios.
It might still be worth doing the "pqReportOOM" changes to save a
few bytes of code space, but I'm less excited about that now.
IMO, I think that "char *msg" is unnecessary, isn't it?
+ if (!PQExpBufferBroken(errorMessage))
+ res->errMsg = pqResultStrdup(res, errorMessage->data);
else
- res->errMsg = NULL;
+ res->errMsg = libpq_gettext("out of memory\n");
+ res->errMsg = pqResultStrdup(res, errorMessage->data);
else
- res->errMsg = NULL;
+ res->errMsg = libpq_gettext("out of memory\n");
regards, tom lane
Ranier Vilela <ranier.vf@gmail.com> writes:
> IMO, I think that "char *msg" is unnecessary, isn't it?
> + if (!PQExpBufferBroken(errorMessage))
> + res->errMsg = pqResultStrdup(res, errorMessage->data);
> else
> - res->errMsg = NULL;
> + res->errMsg = libpq_gettext("out of memory\n");
Please read the comment.
regards, tom lane
(This is not a code review - this is just to satisfy my curiosity)
I've seen lots of code like this where I may have been tempted to use
a ternary operator for readability, so I was wondering is there a PG
convention to avoid such ternary operator assignments, or is it simply
a personal taste thing, or is there some other reason?
For example:
if (msg)
res->errMsg = msg;
else
res->errMsg = libpq_gettext("out of memory\n");
VERSUS:
res->errMsg = msg ? msg : libpq_gettext("out of memory\n");
------
Kind Regards,
Peter Smith.
Fujitsu Australia
On 7/29/21 3:01 AM, Peter Smith wrote:
> (This is not a code review - this is just to satisfy my curiosity)
>
> I've seen lots of code like this where I may have been tempted to use
> a ternary operator for readability, so I was wondering is there a PG
> convention to avoid such ternary operator assignments, or is it simply
> a personal taste thing, or is there some other reason?
>
> For example:
>
> if (msg)
> res->errMsg = msg;
> else
> res->errMsg = libpq_gettext("out of memory\n");
>
> VERSUS:
>
> res->errMsg = msg ? msg : libpq_gettext("out of memory\n");
>
A simple grep on the sources should disabuse you of any idea that there
is such a convention. The code is littered with examples of the ?: operator.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Em qui., 29 de jul. de 2021 às 00:40, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes:
> IMO, I think that "char *msg" is unnecessary, isn't it?
> + if (!PQExpBufferBroken(errorMessage))
> + res->errMsg = pqResultStrdup(res, errorMessage->data);
> else
> - res->errMsg = NULL;
> + res->errMsg = libpq_gettext("out of memory\n");
Please read the comment.
You're right, I missed pqResultStrdup fail.
+1
regards,
Ranier Vilela
Em qui., 29 de jul. de 2021 às 04:02, Peter Smith <smithpb2250@gmail.com> escreveu:
(This is not a code review - this is just to satisfy my curiosity)
I've seen lots of code like this where I may have been tempted to use
a ternary operator for readability, so I was wondering is there a PG
convention to avoid such ternary operator assignments, or is it simply
a personal taste thing, or is there some other reason?
For example:
if (msg)
res->errMsg = msg;
else
res->errMsg = libpq_gettext("out of memory\n");
The C compiler will expand:
res->errMsg = msg ? msg : libpq_gettext("out of memory\n");
to
if (msg)
res->errMsg = msg;
else
res->errMsg = libpq_gettext("out of memory\n");
What IMHO is much more readable.
regards,
Ranier Vilela
Andrew Dunstan <andrew@dunslane.net> writes:
> On 7/29/21 3:01 AM, Peter Smith wrote:
>> I've seen lots of code like this where I may have been tempted to use
>> a ternary operator for readability, so I was wondering is there a PG
>> convention to avoid such ternary operator assignments, or is it simply
>> a personal taste thing, or is there some other reason?
> A simple grep on the sources should disabuse you of any idea that there
> is such a convention. The code is littered with examples of the ?: operator.
Yeah. I happened not to write it that way here, but if I'd been reviewing
someone else's code and they'd done it that way, I'd not have objected.
In the case at hand, I'd personally avoid a ternary op for the first
assignment because then the line would run over 80 characters, and
you'd have to make decisions about where to break it. (We don't have
a standardized convention about that, and none of the alternatives
look very good to my eye.) Then it seemed to make sense to also
write the second step as an "if" not a ternary op.
regards, tom lane
On Thu, Jul 29, 2021 at 9:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > In the case at hand, I'd personally avoid a ternary op for the first > assignment because then the line would run over 80 characters, and > you'd have to make decisions about where to break it. (We don't have > a standardized convention about that, and none of the alternatives > look very good to my eye.) This is exactly why I rarely use ?: -- Robert Haas EDB: http://www.enterprisedb.com