Re: [PATCHES] Re: [BUGS] BUG #4203: perform dblink() in begin/exception returns wrong SQLSTATE code
Joe Conway wrote: Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: [ improved patch ] Still a couple quibbles: [ more good feedback ] All valid complaints, and noticeably improved/simplified code as a result. Third patch attached. Patch applied to HEAD. Joe -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] Re: [BUGS] BUG #4203: perform dblink() in begin/exception returns wrong SQLSTATE code
Tom Lane wrote:
Joe Conway <[EMAIL PROTECTED]> writes:
[ improved patch ]
Still a couple quibbles:
[ more good feedback ]
All valid complaints, and noticeably improved/simplified code as a
result. Third patch attached.
Joe
Index: dblink.c
===
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.73
diff -c -r1.73 dblink.c
*** dblink.c 4 Apr 2008 17:02:56 - 1.73
--- dblink.c 1 Jun 2008 23:52:39 -
***
*** 94,99
--- 94,100
static Oid get_relid_from_relname(text *relname_text);
static char *generate_relation_name(Oid relid);
static void dblink_security_check(PGconn *conn, remoteConn *rconn);
+ static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
/* Global */
static remoteConn *pconn = NULL;
***
*** 125,158
} \
} while (0)
! #define DBLINK_RES_INTERNALERROR(p2) \
! do { \
! msg = pstrdup(PQerrorMessage(conn)); \
! if (res) \
! PQclear(res); \
! elog(ERROR, "%s: %s", p2, msg); \
! } while (0)
!
! #define DBLINK_RES_ERROR(p2) \
do { \
! msg = pstrdup(PQerrorMessage(conn)); \
! if (res) \
! PQclear(res); \
! ereport(ERROR, \
! (errcode(ERRCODE_SYNTAX_ERROR), \
! errmsg("%s", p2), \
! errdetail("%s", msg))); \
} while (0)
! #define DBLINK_RES_ERROR_AS_NOTICE(p2) \
do { \
msg = pstrdup(PQerrorMessage(conn)); \
if (res) \
PQclear(res); \
! ereport(NOTICE, \
! (errcode(ERRCODE_SYNTAX_ERROR), \
! errmsg("%s", p2), \
! errdetail("%s", msg))); \
} while (0)
#define DBLINK_CONN_NOT_AVAIL \
--- 126,145
} \
} while (0)
! #define xpstrdup(var_c, var_) \
do { \
! if (var_ != NULL) \
! var_c = pstrdup(var_); \
! else \
! var_c = NULL; \
} while (0)
! #define DBLINK_RES_INTERNALERROR(p2) \
do { \
msg = pstrdup(PQerrorMessage(conn)); \
if (res) \
PQclear(res); \
! elog(ERROR, "%s: %s", p2, msg); \
} while (0)
#define DBLINK_CONN_NOT_AVAIL \
***
*** 396,408
res = PQexec(conn, buf.data);
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
! if (fail)
! DBLINK_RES_ERROR("sql error");
! else
! {
! DBLINK_RES_ERROR_AS_NOTICE("sql error");
! PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
! }
}
PQclear(res);
--- 383,390
res = PQexec(conn, buf.data);
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
! dblink_res_error(conname, res, "could not open cursor", fail);
! PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
}
PQclear(res);
***
*** 470,482
res = PQexec(conn, buf.data);
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
! if (fail)
! DBLINK_RES_ERROR("sql error");
! else
! {
! DBLINK_RES_ERROR_AS_NOTICE("sql error");
! PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
! }
}
PQclear(res);
--- 452,459
res = PQexec(conn, buf.data);
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
! dblink_res_error(conname, res, "could not close cursor", fail);
! PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
}
PQclear(res);
***
*** 513,519
int call_cntr;
int max_calls;
AttInMetadata *attinmeta;
- char *msg;
PGresult *res = NULL;
MemoryContext oldcontext;
char *conname = NULL;
--- 490,495
***
*** 590,602
(PQresultStatus(res) != PGRES_COMMAND_OK &&
PQresultStatus(res) != PGRES_TUPLES_OK))
{
! if (fail)
! DBLINK_RES_ERROR("sql error");
! else
! {
! DBLINK_RES_ERROR_AS_NOTICE("sql error");
! SRF_RETURN_DONE(funcctx);
! }
}
else if (PQresultStatus(res) == PGRES_COMMAND_OK)
{
--- 566,573
(PQresultStatus(res) != PGRES_COMMAND_OK &&
PQresultStatus(res) != PGRES_TUPLES_OK))
{
! dblink_res_error(conname, res, "could not fetch from cursor", fail);
! SRF_RETURN_DONE(funcctx);
}
else if (PQresultStatus(res) == PGRES_COMMAND_OK)
{
***
*** 846,860
(PQresultStatus(res) != PGRES_COMMAND_OK &&
PQresultStatus(res) != PGRES_TUPLES_OK))
{
! if (fail)
! DBLINK_RES_ERROR("sql error");
! else
! {
! DBLINK_RES_ERROR_AS_NOTICE("sql error");
! if (freeconn)
! PQfinish(conn);
! SRF_RETURN_DONE(funcctx);
! }
}
if (PQresultStatus(res) == PGRES_COMMAND_OK)
--- 817,826
(PQresultStatus(res) != PGRES_COMMAND_OK &&
PQresultStatus(res) != PGRES_TUPLES_OK))
{
! dblink_res_error(conname, res, "could not execute que
[PATCHES] Re: [BUGS] BUG #4203: perform dblink() in begin/exception returns wrong SQLSTATE code
Tom Lane wrote:
Joe Conway <[EMAIL PROTECTED]> writes:
Here is my proposed patch -- as suggested for cvs tip only.
A few comments:
[...great comments as usual...]
Thanks for the excellent feedback! I think the attached addresses it all.
I haven't been around enough lately to be sure I understand the process
these days. Should I be posting this to the wiki and waiting for the
next commit fest, or just apply myself in a day or two assuming no
objections?
No, you can apply it yourself when you feel it's ready. The wiki queue
is just to keep track of stuff that is submitted by non-committers or
that a committer wants extra review of.
Great. Assuming no other objections I'll commit in a day or three.
Joe
Index: dblink.c
===
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.73
diff -c -r1.73 dblink.c
*** dblink.c 4 Apr 2008 17:02:56 - 1.73
--- dblink.c 1 Jun 2008 21:58:46 -
***
*** 94,99
--- 94,100
static Oid get_relid_from_relname(text *relname_text);
static char *generate_relation_name(Oid relid);
static void dblink_security_check(PGconn *conn, remoteConn *rconn);
+ static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
/* Global */
static remoteConn *pconn = NULL;
***
*** 125,158
} \
} while (0)
! #define DBLINK_RES_INTERNALERROR(p2) \
! do { \
! msg = pstrdup(PQerrorMessage(conn)); \
! if (res) \
! PQclear(res); \
! elog(ERROR, "%s: %s", p2, msg); \
! } while (0)
!
! #define DBLINK_RES_ERROR(p2) \
do { \
! msg = pstrdup(PQerrorMessage(conn)); \
! if (res) \
! PQclear(res); \
! ereport(ERROR, \
! (errcode(ERRCODE_SYNTAX_ERROR), \
! errmsg("%s", p2), \
! errdetail("%s", msg))); \
} while (0)
! #define DBLINK_RES_ERROR_AS_NOTICE(p2) \
do { \
msg = pstrdup(PQerrorMessage(conn)); \
if (res) \
PQclear(res); \
! ereport(NOTICE, \
! (errcode(ERRCODE_SYNTAX_ERROR), \
! errmsg("%s", p2), \
! errdetail("%s", msg))); \
} while (0)
#define DBLINK_CONN_NOT_AVAIL \
--- 126,145
} \
} while (0)
! #define xpstrdup(var_c, var_) \
do { \
! if (var_ != NULL) \
! var_c = pstrdup(var_); \
! else \
! var_c = NULL; \
} while (0)
! #define DBLINK_RES_INTERNALERROR(p2) \
do { \
msg = pstrdup(PQerrorMessage(conn)); \
if (res) \
PQclear(res); \
! elog(ERROR, "%s: %s", p2, msg); \
} while (0)
#define DBLINK_CONN_NOT_AVAIL \
***
*** 396,408
res = PQexec(conn, buf.data);
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
! if (fail)
! DBLINK_RES_ERROR("sql error");
! else
! {
! DBLINK_RES_ERROR_AS_NOTICE("sql error");
PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
- }
}
PQclear(res);
--- 383,391
res = PQexec(conn, buf.data);
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
! dblink_res_error(conname, res, "could not open cursor", fail);
! if (!fail)
PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
}
PQclear(res);
***
*** 470,482
res = PQexec(conn, buf.data);
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
! if (fail)
! DBLINK_RES_ERROR("sql error");
! else
! {
! DBLINK_RES_ERROR_AS_NOTICE("sql error");
PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
- }
}
PQclear(res);
--- 453,461
res = PQexec(conn, buf.data);
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
! dblink_res_error(conname, res, "could not close cursor", fail);
! if (!fail)
PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
}
PQclear(res);
***
*** 513,519
int call_cntr;
int max_calls;
AttInMetadata *attinmeta;
- char *msg;
PGresult *res = NULL;
MemoryContext oldcontext;
char *conname = NULL;
--- 492,497
***
*** 590,602
(PQresultStatus(res) != PGRES_COMMAND_OK &&
PQresultStatus(res) != PGRES_TUPLES_OK))
{
! if (fail)
! DBLINK_RES_ERROR("sql error");
! else
! {
! DBLINK_RES_ERROR_AS_NOTICE("sql error");
SRF_RETURN_DONE(funcctx);
- }
}
else if (PQresultStatus(res) == PGRES_COMMAND_OK)
{
--- 568,576
(PQresultStatus(res) != PGRES_COMMAND_OK &&
PQresultStatus(res) != PGRES_TUPLES_OK))
{
! dblink_res_error(conname, res, "could not fetch from cursor", fail);
! if (!fail)
SRF_RETURN_DONE(funcctx);
}
else if (PQresultStatus(res) == PGRES_COMMAND_OK)
{
***
*** 846,856
(PQresultStatus(res) != PGRES_COMMA
[PATCHES] Re: [BUGS] BUG #4203: perform dblink() in begin/exception returns wrong SQLSTATE code
Tom Lane wrote:
Joe Conway <[EMAIL PROTECTED]> writes:
Tom Lane wrote:
Yeah, the dblink code should probably try a bit harder to propagate the
original error fields.
Sounds reasonable. Do you think this is a bug fix or an 8.4 enhancement?
8.4 enhancement I think, since a behavioral change here could pose a
compatibility issue for applications.
Here is my proposed patch -- as suggested for cvs tip only.
I haven't been around enough lately to be sure I understand the process
these days. Should I be posting this to the wiki and waiting for the
next commit fest, or just apply myself in a day or two assuming no
objections?
Thanks,
Joe
Index: dblink.c
===
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.73
diff -c -r1.73 dblink.c
*** dblink.c 4 Apr 2008 17:02:56 - 1.73
--- dblink.c 1 Jun 2008 18:50:04 -
***
*** 135,158
#define DBLINK_RES_ERROR(p2) \
do { \
! msg = pstrdup(PQerrorMessage(conn)); \
if (res) \
PQclear(res); \
! ereport(ERROR, \
! (errcode(ERRCODE_SYNTAX_ERROR), \
! errmsg("%s", p2), \
! errdetail("%s", msg))); \
} while (0)
#define DBLINK_RES_ERROR_AS_NOTICE(p2) \
do { \
! msg = pstrdup(PQerrorMessage(conn)); \
if (res) \
PQclear(res); \
! ereport(NOTICE, \
! (errcode(ERRCODE_SYNTAX_ERROR), \
! errmsg("%s", p2), \
! errdetail("%s", msg))); \
} while (0)
#define DBLINK_CONN_NOT_AVAIL \
--- 135,182
#define DBLINK_RES_ERROR(p2) \
do { \
! char *sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE); \
! char *pg_diag_message_primary = PQresultErrorField(res, PG_DIAG_MESSAGE_PRIMARY); \
! char *pg_diag_message_detail = PQresultErrorField(res, PG_DIAG_MESSAGE_DETAIL); \
! char *pg_diag_message_hint = PQresultErrorField(res, PG_DIAG_MESSAGE_HINT); \
! char *pg_diag_context = PQresultErrorField(res, PG_DIAG_CONTEXT); \
if (res) \
PQclear(res); \
! errstart(ERROR, __FILE__, __LINE__, PG_FUNCNAME_MACRO); \
! errcode(MAKE_SQLSTATE(sqlstate[0],sqlstate[1],sqlstate[2],sqlstate[3],sqlstate[4])); \
! errmsg("%s", pg_diag_message_primary); \
! if (pg_diag_message_detail) \
! errdetail("%s", pg_diag_message_detail); \
! if (pg_diag_message_hint) \
! errhint("%s", pg_diag_message_hint); \
! if (pg_diag_context) \
! errcontext("%s", pg_diag_context); \
! if (p2) \
! errcontext("error on dblink connection: %s", p2); \
! errfinish(0); \
} while (0)
#define DBLINK_RES_ERROR_AS_NOTICE(p2) \
do { \
! char *sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE); \
! char *pg_diag_message_primary = PQresultErrorField(res, PG_DIAG_MESSAGE_PRIMARY); \
! char *pg_diag_message_detail = PQresultErrorField(res, PG_DIAG_MESSAGE_DETAIL); \
! char *pg_diag_message_hint = PQresultErrorField(res, PG_DIAG_MESSAGE_HINT); \
! char *pg_diag_context = PQresultErrorField(res, PG_DIAG_CONTEXT); \
if (res) \
PQclear(res); \
! errstart(NOTICE, __FILE__, __LINE__, PG_FUNCNAME_MACRO); \
! errcode(MAKE_SQLSTATE(sqlstate[0],sqlstate[1],sqlstate[2],sqlstate[3],sqlstate[4])); \
! errmsg("%s", pg_diag_message_primary); \
! if (pg_diag_message_detail) \
! errdetail("%s", pg_diag_message_detail); \
! if (pg_diag_message_hint) \
! errhint("%s", pg_diag_message_hint); \
! if (pg_diag_context) \
! errcontext("%s", pg_diag_context); \
! if (p2) \
! errcontext("error on dblink connection: %s", p2); \
! errfinish(0); \
} while (0)
#define DBLINK_CONN_NOT_AVAIL \
***
*** 397,406
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
if (fail)
! DBLINK_RES_ERROR("sql error");
else
{
! DBLINK_RES_ERROR_AS_NOTICE("sql error");
PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
}
}
--- 421,430
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
if (fail)
! DBLINK_RES_ERROR("unable to open cursor");
else
{
! DBLINK_RES_ERROR_AS_NOTICE("unable to open cursor");
PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
}
}
***
*** 471,480
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
if (fail)
! DBLINK_RES_ERROR("sql error");
else
{
! DBLINK_RES_ERROR_AS_NOTICE("sql error");
PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
}
}
--- 495,504
if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
{
if (fail)
! DBLINK_RES_ERROR("unable to close cursor");
else
{
! DBLINK_RES_ERROR_AS_NOTICE("unable to close cursor");
PG_RETURN_TEXT_P(cstring_to_text("ER
Re: [PATCHES] libpq type system 0.9a
Alvaro Herrera wrote: Merlin Moncure escribió: Yesterday, we notified -hackers of the latest version of the libpq type system. Just to be sure the right people are getting notified, we are posting the latest patch here as well. Would love to get some feedback on this. I had a look at this patch some days ago, and the first question in my mind was: why is it explicitely on libpq? Why not have it as a separate library (say libpqtypes)? That way, applications not using it would not need to link to it. Applications interested in using it would just need to add another -l switch to their link line. +1 Joe -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [GENERAL] Crosstab Problems
Bruce Momjian wrote: Joe, are you nearly ready to apply this? Done (head and backwards to 7.3). Joe ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [GENERAL] Crosstab Problems
Bruce Momjian wrote: Joe, are you nearly ready to apply this? Yeah, sorry for the delay. By the end of the weekend. Joe ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [GENERAL] Crosstab Problems
Joe Conway wrote:
Tom Lane wrote:
A couple of minor thoughts:
* You could reduce the ugliness of many of the tests by introducing a
variant strcmp function that does the "right" things with NULL inputs.
It might also be worth adding a variant pstrdup that takes a NULL.
I had thoughts along those lines -- it would certainly make the code
more readable. I'll go ahead and do that but it won't be in time for a
26 October beta2.
I'm not quite ready to commit this, mostly because I'd like to give the
rest of tablefunc.c the once-over for similar issues related to not
checking for NULL return values from SPI_getvalue(). But it is close
enough if needed for a beta2 tomorrow -- let me know if we plan to
bundle up beta2 and I'll get it in.
Thanks,
Joe
Index: tablefunc.c
===
RCS file: /opt/src/cvs/pgsql/contrib/tablefunc/tablefunc.c,v
retrieving revision 1.47
diff -c -r1.47 tablefunc.c
*** tablefunc.c 3 Mar 2007 19:32:54 - 1.47
--- tablefunc.c 26 Oct 2007 05:35:23 -
***
*** 106,111
--- 106,123
} \
} while (0)
+ #define xpstrdup(tgtvar_, srcvar_) \
+ do { \
+ if (srcvar_) \
+ tgtvar_ = pstrdup(srcvar_); \
+ else \
+ tgtvar_ = NULL; \
+ } while (0)
+
+ #define xstreq(tgtvar_, srcvar_) \
+ (((tgtvar_ == NULL) && (srcvar_ == NULL)) || \
+ ((tgtvar_ != NULL) && (srcvar_ != NULL) && (strcmp(tgtvar_, srcvar_) == 0)))
+
/* sign, 10 digits, '\0' */
#define INT32_STRLEN 12
***
*** 355,360
--- 367,373
crosstab_fctx *fctx;
int i;
int num_categories;
+ bool firstpass = false;
MemoryContext oldcontext;
/* stuff done only on the first call of the function */
***
*** 469,474
--- 482,488
funcctx->max_calls = proc;
MemoryContextSwitchTo(oldcontext);
+ firstpass = true;
}
/* stuff done on every call of the function */
***
*** 500,506
HeapTuple tuple;
Datum result;
char **values;
! bool allnulls = true;
while (true)
{
--- 514,520
HeapTuple tuple;
Datum result;
char **values;
! bool skip_tuple = false;
while (true)
{
***
*** 530,555
rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);
/*
! * If this is the first pass through the values for this rowid
! * set it, otherwise make sure it hasn't changed on us. Also
! * check to see if the rowid is the same as that of the last
! * tuple sent -- if so, skip this tuple entirely
*/
if (i == 0)
- values[0] = pstrdup(rowid);
-
- if ((rowid != NULL) && (strcmp(rowid, values[0]) == 0))
{
! if ((lastrowid != NULL) && (strcmp(rowid, lastrowid) == 0))
break;
! else if (allnulls == true)
! allnulls = false;
/*
! * Get the next category item value, which is alway
* attribute number three.
*
! * Be careful to sssign the value to the array index based
* on which category we are presently processing.
*/
values[1 + i] = SPI_getvalue(spi_tuple, spi_tupdesc, 3);
--- 544,578
rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);
/*
! * If this is the first pass through the values for this
! * rowid, set the first column to rowid
*/
if (i == 0)
{
! xpstrdup(values[0], rowid);
!
! /*
! * Check to see if the rowid is the same as that of the last
! * tuple sent -- if so, skip this tuple entirely
! */
! if (!firstpass && xstreq(lastrowid, rowid))
! {
! skip_tuple = true;
break;
! }
! }
+ /*
+ * If rowid hasn't changed on us, continue building the
+ * ouput tuple.
+ */
+ if (xstreq(rowid, values[0]))
+ {
/*
! * Get the next category item value, which is always
* attribute number three.
*
! * Be careful to assign the value to the array index based
* on which category we are presently processing.
*/
values[1 + i] = SPI_getvalue(spi_tuple, spi_tupdesc, 3);
***
*** 572,597
call_cntr = --funcctx->call_cntr;
break;
}
!
! if (rowid != NULL)
! xpfree(rowid);
}
! xpfree(fctx->lastrowid);
! if (values[0] != NULL)
! {
! /*
! * switch to memory context appropriate for multiple function
! * calls
! */
! oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
! lastrowid = fctx->lastrowid = pstrdup(values[0]);
! MemoryContextSwitchTo(oldcontext);
! }
! if (!allnulls)
{
/* build the tuple */
tuple = BuildTupleFromCStrings(attinmeta, values);
--- 595,616
call_cntr = --funcctx->call_cntr;
break;
Re: [PATCHES] [GENERAL] Crosstab Problems
Tom Lane wrote:
Joe Conway <[EMAIL PROTECTED]> writes:
Well, maybe the attached patches better explain what I mean.
In the case of the 8.2 patch, a very small code change allows new
regression data including NULL rowids to:
1) not crash
2) have no impact otherwise
The much bigger 8.3 patch shows that for the very same new regression
data, there is a significant impact on the output (i.e. NULL rowids get
their own output row as discussed).
I'm still leaning toward applying the 8.2 patch for back branches but
I'll bow to the general consensus.
I'd vote for the bigger patch all the way back. The smaller patch has
nothing to recommend it except being smaller. It replaces the crash
with a behavior that will change in 8.3, thus creating a potential
portability issue for users of (post-repair) back branches. Why not
get it right the first time?
OK, I can live with that.
A couple of minor thoughts:
* You could reduce the ugliness of many of the tests by introducing a
variant strcmp function that does the "right" things with NULL inputs.
It might also be worth adding a variant pstrdup that takes a NULL.
I had thoughts along those lines -- it would certainly make the code
more readable. I'll go ahead and do that but it won't be in time for a
26 October beta2.
* Surely this bit:
xpfree(lastrowid);
! if (rowid)
! lastrowid = pstrdup(rowid);
}
needs to be:
if (rowid)
lastrowid = pstrdup(rowid);
else
lastrowid = NULL;
no? (Again the variant pstrdup would save some notation)
Well I had already defined xpfree like this:
8<--
#define xpfree(var_) \
do { \
if (var_ != NULL) \
{ \
pfree(var_); \
var_ = NULL; \
} \
} while (0)
8<--
so lastrowid is already NULL (I sometimes wish this was the default
behavior for pfree() itself). But the point about pstrdup variant is
well taken, and I guess the xpfree behavior is not obvious, so it
deserves at least a comment.
regards, tom lane
PS: I hear things are pretty crazy out your way -- hope the fire's
not too close to you.
We packed and were ready to evacuate two or three times, but never
actually had to leave our house, thankfully. The closest the fire ever
got was about 4 miles, and at this point I don't think we're in any more
direct danger. But I know many people who were not so fortunate :-(
Joe
---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match
Re: [PATCHES] [GENERAL] Crosstab Problems
Tom Lane wrote:
Joe Conway <[EMAIL PROTECTED]> writes:
Tom Lane wrote:
1. Treat NULL rowid as a category in its own right. This would conform
with the behavior of GROUP BY and DISTINCT, for instance.
In any case, the attached changes the behavior to #1 for both flavors of
crosstab (the original crosstab(text, int) and the usually more useful
crosstab(text, text)).
It is appropriate for 8.3 but not back-patching as it changes behavior
in a non-backward compatible way and is probably too invasive anyway.
Um, if the previous code crashed in this case, why would you worry about
being backward-compatible with it? You're effectively changing the
behavior anyway, so you might as well make it do what you've decided is
the right thing.
Well, maybe the attached patches better explain what I mean.
In the case of the 8.2 patch, a very small code change allows new
regression data including NULL rowids to:
1) not crash
2) have no impact otherwise
The much bigger 8.3 patch shows that for the very same new regression
data, there is a significant impact on the output (i.e. NULL rowids get
their own output row as discussed).
I'm still leaning toward applying the 8.2 patch for back branches but
I'll bow to the general consensus.
Joe
Index: tablefunc.c
===
RCS file: /opt/src/cvs/pgsql/contrib/tablefunc/tablefunc.c,v
retrieving revision 1.47
diff -c -r1.47 tablefunc.c
*** tablefunc.c 3 Mar 2007 19:32:54 - 1.47
--- tablefunc.c 25 Oct 2007 02:11:06 -
***
*** 355,360
--- 355,361
crosstab_fctx *fctx;
int i;
int num_categories;
+ bool firstpass = false;
MemoryContext oldcontext;
/* stuff done only on the first call of the function */
***
*** 469,474
--- 470,476
funcctx->max_calls = proc;
MemoryContextSwitchTo(oldcontext);
+ firstpass = true;
}
/* stuff done on every call of the function */
***
*** 500,506
HeapTuple tuple;
Datum result;
char **values;
! bool allnulls = true;
while (true)
{
--- 502,508
HeapTuple tuple;
Datum result;
char **values;
! bool skip_tuple = false;
while (true)
{
***
*** 530,555
rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);
/*
! * If this is the first pass through the values for this rowid
! * set it, otherwise make sure it hasn't changed on us. Also
! * check to see if the rowid is the same as that of the last
! * tuple sent -- if so, skip this tuple entirely
*/
if (i == 0)
- values[0] = pstrdup(rowid);
-
- if ((rowid != NULL) && (strcmp(rowid, values[0]) == 0))
{
! if ((lastrowid != NULL) && (strcmp(rowid, lastrowid) == 0))
break;
! else if (allnulls == true)
! allnulls = false;
/*
! * Get the next category item value, which is alway
* attribute number three.
*
! * Be careful to sssign the value to the array index based
* on which category we are presently processing.
*/
values[1 + i] = SPI_getvalue(spi_tuple, spi_tupdesc, 3);
--- 532,574
rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);
/*
! * If this is the first pass through the values for this
! * rowid, set the first column to rowid
*/
if (i == 0)
{
! if (rowid)
! values[0] = pstrdup(rowid);
! else
! values[0] = NULL;
!
! /*
! * Check to see if the rowid is the same as that of the last
! * tuple sent -- if so, skip this tuple entirely
! */
! if (!firstpass &&
! (((lastrowid == NULL) && (rowid == NULL)) ||
! ((lastrowid != NULL) &&
! (rowid != NULL) &&
! (strcmp(rowid, lastrowid) == 0
! {
! skip_tuple = true;
break;
! }
! }
+ /*
+ * If rowid hasn't changed on us, continue building the
+ * ouput tuple.
+ */
+ if ((rowid && values[0] && (strcmp(rowid, values[0]) == 0)) ||
+ ((rowid == NULL) && (values[0] == NULL)))
+ {
/*
! * Get the next category item value, which is always
* attribute number three.
*
! * Be careful to assign the value to the array index based
* on which category we are presently processing.
*/
values[1 + i] = SPI_getvalue(spi_tuple, spi_tupdesc, 3);
***
*** 572,584
call_cntr = --funcctx->call_cntr;
break;
}
!
! if (rowid != NULL)
! xpfree(rowid);
}
xpfree(fctx->lastrowid);
-
if (values[0] != NULL)
{
/*
--- 591,600
call_cntr = --funcctx->call_cntr;
break;
}
! xpfree(rowid);
}
xpfree(fctx->lastrow
[PATCHES] [Fwd: Re: [GENERAL] Crosstab Problems]
Oops, just noticed I sent this to the General list instead of Patches --
sorry about that.
Joe
Original Message
Subject: Re: [GENERAL] Crosstab Problems
Date: Wed, 24 Oct 2007 19:26:16 -0700
From: Joe Conway <[EMAIL PROTECTED]>
To: Tom Lane <[EMAIL PROTECTED]>
CC: Jorge Godoy <[EMAIL PROTECTED]>, [EMAIL PROTECTED],
Stefan Schwarzer <[EMAIL PROTECTED]>
References: <[EMAIL PROTECTED]>
<[EMAIL PROTECTED]> <[EMAIL PROTECTED]>
<[EMAIL PROTECTED]> <[EMAIL PROTECTED]>
Tom Lane wrote:
Jorge Godoy <[EMAIL PROTECTED]> writes:
Em Thursday 18 October 2007 16:37:59 Joe Conway escreveu:
The row is pretty useless without a rowid in this context -- it seems
like the best thing to do would be to skip those rows entirely. Of
course you could argue I suppose that it ought to throw an ERROR and
bail out entirely. Maybe a good compromise would be to skip the row but
throw a NOTICE?
If I were using it and having this problem I'd rather have an ERROR.
I can think of four reasonably credible alternatives:
1. Treat NULL rowid as a category in its own right. This would conform
with the behavior of GROUP BY and DISTINCT, for instance.
> 4. Silently ignore rows with NULL rowid.
After looking closer I realized that #4 was my original intention, and
there was even code attempting to implement it, but just not very well ;-(.
In any case, the attached changes the behavior to #1 for both flavors of
crosstab (the original crosstab(text, int) and the usually more useful
crosstab(text, text)).
It is appropriate for 8.3 but not back-patching as it changes behavior
in a non-backward compatible way and is probably too invasive anyway.
I'll do something much simpler just to prevent crashing for 8.2 and earlier.
If there are no objections I'll apply Thursday.
Joe
Index: tablefunc.c
===
RCS file: /opt/src/cvs/pgsql/contrib/tablefunc/tablefunc.c,v
retrieving revision 1.47
diff -c -r1.47 tablefunc.c
*** tablefunc.c 3 Mar 2007 19:32:54 - 1.47
--- tablefunc.c 25 Oct 2007 02:11:06 -
***
*** 355,360
--- 355,361
crosstab_fctx *fctx;
int i;
int num_categories;
+ bool firstpass = false;
MemoryContext oldcontext;
/* stuff done only on the first call of the function */
***
*** 469,474
--- 470,476
funcctx->max_calls = proc;
MemoryContextSwitchTo(oldcontext);
+ firstpass = true;
}
/* stuff done on every call of the function */
***
*** 500,506
HeapTuple tuple;
Datum result;
char **values;
! bool allnulls = true;
while (true)
{
--- 502,508
HeapTuple tuple;
Datum result;
char **values;
! bool skip_tuple = false;
while (true)
{
***
*** 530,555
rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);
/*
! * If this is the first pass through the values for this rowid
! * set it, otherwise make sure it hasn't changed on us. Also
! * check to see if the rowid is the same as that of the last
! * tuple sent -- if so, skip this tuple entirely
*/
if (i == 0)
- values[0] = pstrdup(rowid);
-
- if ((rowid != NULL) && (strcmp(rowid, values[0]) == 0))
{
! if ((lastrowid != NULL) && (strcmp(rowid, lastrowid) == 0))
break;
! else if (allnulls == true)
! allnulls = false;
/*
! * Get the next category item value, which is alway
* attribute number three.
*
! * Be careful to sssign the value to the array index based
* on which category we are presently processing.
*/
values[1 + i] = SPI_getvalue(spi_tuple, spi_tupdesc, 3);
--- 532,574
rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);
/*
! * If this is the first pass through the values for this
! * rowid, set the first column to rowid
*/
if (i == 0)
{
! if (rowid)
! values[0] = pstrdup(rowid);
! else
! values[0] = NULL;
!
! /*
! * Check to see if the rowid is the same as that of the last
! * tuple sent -- if so, skip this tuple entirely
! */
! if (!firstpass &&
! (((lastrowid == NULL) && (rowid == NULL)) ||
! ((lastrowid != NULL) &&
! (rowid != NULL) &&
! (strcmp(rowid, lastrowid) == 0
! {
! skip_tuple = true;
break;
! }
! }
+ /*
+ * If rowid hasn't changed on us, continue building the
+ * ouput tuple.
+ */
+ if ((rowid && values[0] && (strcmp(rowid, values[0]) == 0)) ||
+ ((rowid == NULL) && (values[0] == NULL)))
+ {
/*
! * Get the next category item value, which is always
* attribute number three.
*
!
Re: [PATCHES] dblink connection security
Gregory Stark wrote: "Joe Conway" <[EMAIL PROTECTED]> writes: Stephen Frost wrote: * Joe Conway ([EMAIL PROTECTED]) wrote: There are none installed by default -- that's the point. Uhh... None what? Functions in untrusted languages? That's certainly not the case, there's a whole slew of them, from boolin to generate_series and beyond. They're available to regular users, even! Get serious. Internal functions are specifically designed and maintained to be safe within the confines of the database security model. We are discussing extensions to the core, all of which must be installed by choice, by a superuser. That doesn't mean they shouldn't be concerned with security. Of course they should be concerned with security. Its the job of the DBA/superuser to be concerned with security, and therefore they ought to know what the implications are for what they install, before they install it. Consider dblink as an entirely separate product which depends on Postgres the way Postgres depends on the OS. We discussing how the dblink software should behave when installed with *its* default configuration. And the the security escalation scenario, which is a consequence of the DBA's inadequate understanding of the security setup of the system in the first place, has now been closed for them. Granted, this was an easy enough mistake to make, so I agree that what we've done to close it is a good thing. But if you know of a security risk related to using libpq with a password authenticated connection, let's hear it. Joe ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] dblink connection security
Stephen Frost wrote: It's about as good as saying "Well, an admin had to install PostgreSQL on the system, by choice, and therefore we don't need to worry about PG allowing someone remote shell access to the system". That's a ridiculous assertion -- I said nothing of the sort. Joe ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] dblink connection security
Stephen Frost wrote: * Joe Conway ([EMAIL PROTECTED]) wrote: There are none installed by default -- that's the point. Uhh... None what? Functions in untrusted languages? That's certainly not the case, there's a whole slew of them, from boolin to generate_series and beyond. They're available to regular users, even! Get serious. Internal functions are specifically designed and maintained to be safe within the confines of the database security model. We are discussing extensions to the core, all of which must be installed by choice, by a superuser. Joe ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] dblink connection security
Stephen Frost wrote: * Joe Conway ([EMAIL PROTECTED]) wrote: Sure it matters. A function written in a trusted language is known to be safe, a priori. A function written in an untrusted language has no such guarantees, and therefore has to be assumed unsafe unless carefully proved otherwise. I see.. So all the functions in untrusted languages that come with PG initially should be checked over by every sysadmin when installing PG every time... And the same for PostGIS, and all of the PL's that use untrusted languages? There are none installed by default -- that's the point. On my pretty modest install that's 2,206 functions. For some reason I see something of a difference between 'generate_series' and 'dblink' in terms of security and which one I'm comfortable having enabled by default and which one I'm not. generate_series is a built in function. We aren't discussing those. Joe ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] dblink connection security
Stephen Frost wrote: * Joe Conway ([EMAIL PROTECTED]) wrote: Consider a scenario like "package uses untrusted language z>". Exact same concerns arise. No, it doesn't... Said arbitrary function in y, in untrusted language z, could be perfectly safe for users to call. ^ *Could* be. But we just said that the admin was not interested in reading the documentation, and has no idea if it *is* safe. And, it very well might not be safe. We have no way to know in advance because the language is untrusted. Being written in an untrusted language has got next to nothing to do with the security implications of a particular function. It depends entirely on what the function is *doing*, not what language it's written in. Sure it matters. A function written in a trusted language is known to be safe, a priori. A function written in an untrusted language has no such guarantees, and therefore has to be assumed unsafe unless carefully proved otherwise. Joe ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] dblink connection security
Gregory Stark wrote: Consider a scenario like "package uses dblink". Sysadmin follows instructions for package and installs dblink. Now package 's documentation isn't going to explain the second-order effects and discuss restricting who has access to dblink. The sysadmin has no particular interest in using dblink himself and probably will never read any dblink docs. On the other hand if dblink can't be executed by random users then when package x tells you to install dblink it will also tell you to grant access to the user that package runs as. The sysadmin can consider which users that should be. See my last email... Consider a scenario like "package uses untrusted language z>". Exact same concerns arise. Joe ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] dblink connection security
Tom Lane wrote: Gregory Stark <[EMAIL PROTECTED]> writes: My objection is that I think we should still revoke access for non-superuser by default. The patch makes granting execute reasonable for most users but nonetheless it shouldn't be the default. Being able to connect to a postgres server shouldn't mean being able to open tcp connections *from* that server to arbitrary other host/ports. You forget that dblink isn't even installed by default. I could see having some more verbiage in the documentation explaining these possible security risks, but making it unusable is an overreaction. Agreed. If you are going to argue that we should revoke access for non-superusers by default for dblink, then you are also arguing that we should do the same for every function created with any untrusted language. E.g. as I pointed out to Robert last week, just because an unsafe function is created in plperlu, it doesn't mean that a non-superuser can't run it immediately after it is created. There is no difference. It is incumbent upon the DBA/superuser to be careful _whenever_ they create any function using an untrusted language. Joe ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] dblink connection security
Joe Conway wrote:
I'm working on the back branch solution now.
Attached is for back-patching. I left in the SECURITY DEFINER functions
and dblink doc changes -- even though they may not do existing
installations much good, I think there will still be enough new 8.2.x
installations for a while to make it worthwhile. And even for older
branches, at least it gives us something handy to point to as a
workaround when people complain we broke their apps.
If there are no objections I'll commit this later today.
Thanks,
Joe
Index: contrib/dblink/dblink.c
===
RCS file: /cvsroot/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.60
diff -c -r1.60 dblink.c
*** contrib/dblink/dblink.c 19 Oct 2006 19:53:03 - 1.60
--- contrib/dblink/dblink.c 8 Jul 2007 17:38:00 -
***
*** 37,42
--- 37,43
#include "libpq-fe.h"
#include "fmgr.h"
#include "funcapi.h"
+ #include "miscadmin.h"
#include "access/heapam.h"
#include "access/tupdesc.h"
#include "catalog/namespace.h"
***
*** 89,94
--- 90,96
static HeapTuple get_tuple_of_interest(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals);
static Oid get_relid_from_relname(text *relname_text);
static char *generate_relation_name(Oid relid);
+ static char *connstr_strip_password(const char *connstr);
/* Global */
static remoteConn *pconn = NULL;
***
*** 228,233
--- 230,257
if (connname)
rconn = (remoteConn *) palloc(sizeof(remoteConn));
+
+ /* for non-superusers, check that server requires a password */
+ if (!superuser())
+ {
+ /* this attempt must fail */
+ conn = PQconnectdb(connstr_strip_password(connstr));
+
+ if (PQstatus(conn) == CONNECTION_OK)
+ {
+ PQfinish(conn);
+ if (rconn)
+ pfree(rconn);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("password is required"),
+ errdetail("Non-superuser cannot connect if the server does not request a password."),
+ errhint("Target server's authentication method must be changed.")));
+ }
+ else
+ PQfinish(conn);
+ }
conn = PQconnectdb(connstr);
MemoryContextSwitchTo(oldcontext);
***
*** 2273,2275
--- 2297,2430
errmsg("undefined connection name")));
}
+
+
+ /*
+ * Modified version of conninfo_parse() from fe-connect.c
+ * Used to remove any password from the connection string
+ * in order to test whether the server auth method will
+ * require it.
+ */
+ static char *
+ connstr_strip_password(const char *connstr)
+ {
+ char *pname;
+ char *pval;
+ char *buf;
+ char *cp;
+ char *cp2;
+ StringInfoData result;
+
+ /* initialize return value */
+ initStringInfo(&result);
+
+ /* Need a modifiable copy of the input string */
+ buf = pstrdup(connstr);
+ cp = buf;
+
+ while (*cp)
+ {
+ /* Skip blanks before the parameter name */
+ if (isspace((unsigned char) *cp))
+ {
+ cp++;
+ continue;
+ }
+
+ /* Get the parameter name */
+ pname = cp;
+ while (*cp)
+ {
+ if (*cp == '=')
+ break;
+ if (isspace((unsigned char) *cp))
+ {
+ *cp++ = '\0';
+ while (*cp)
+ {
+ if (!isspace((unsigned char) *cp))
+ break;
+ cp++;
+ }
+ break;
+ }
+ cp++;
+ }
+
+ /* Check that there is a following '=' */
+ if (*cp != '=')
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("missing \"=\" after \"%s\" in connection string", pname)));
+ *cp++ = '\0';
+
+ /* Skip blanks after the '=' */
+ while (*cp)
+ {
+ if (!isspace((unsigned char) *cp))
+ break;
+ cp++;
+ }
+
+ /* Get the parameter value */
+ pval = cp;
+
+ if (*cp != '\'')
+ {
+ cp2 = pval;
+ while (*cp)
+ {
+ if (isspace((unsigned char) *cp))
+ {
+ *cp++ = '\0';
+ break;
+ }
+ if (*cp == '\\')
+ {
+ cp++;
+ if (*cp != '\0')
+ *cp2++ = *cp++;
+ }
+ else
+ *cp2++ = *cp++;
+ }
+ *cp2 = '\0';
+ }
+ else
+ {
+ cp2 = pval;
+ cp++;
+ for (;;)
+ {
+ if (*cp == '\0')
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("unterminated quoted string in connection string")));
+ if (*cp == '\\')
+ {
+ cp++;
+ if (*cp != '\0')
+ *cp2++ = *cp++;
+ continue;
+ }
+ if (*cp == '\'')
+ {
+ *cp2 = '\0';
+ cp++;
+ break;
+ }
+ *cp2++ =
Re: [PATCHES] dblink connection security
Tom Lane wrote:
Joe Conway <[EMAIL PROTECTED]> writes:
Attached patch implements this proposal, including documentation
changes. I'll work separately on the back-branch version.
Any comments/objections?
Looks OK in a fast scan, except that you are not following the message
style guidelines here:
Thanks for the corrections. Final version committed to HEAD attached.
I'm working on the back branch solution now.
Joe
Index: contrib/dblink/dblink.c
===
RCS file: /cvsroot/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.63
diff -c -r1.63 dblink.c
*** contrib/dblink/dblink.c 6 Apr 2007 04:21:41 - 1.63
--- contrib/dblink/dblink.c 8 Jul 2007 16:52:53 -
***
*** 37,42
--- 37,43
#include "libpq-fe.h"
#include "fmgr.h"
#include "funcapi.h"
+ #include "miscadmin.h"
#include "access/heapam.h"
#include "access/tupdesc.h"
#include "catalog/namespace.h"
***
*** 245,250
--- 246,267
errdetail("%s", msg)));
}
+ if (!superuser())
+ {
+ if (!PQconnectionUsedPassword(conn))
+ {
+ PQfinish(conn);
+ if (rconn)
+ pfree(rconn);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("password is required"),
+ errdetail("Non-superuser cannot connect if the server does not request a password."),
+ errhint("Target server's authentication method must be changed.")));
+ }
+ }
+
if (connname)
{
rconn->conn = conn;
Index: contrib/dblink/dblink.sql.in
===
RCS file: /cvsroot/pgsql/contrib/dblink/dblink.sql.in,v
retrieving revision 1.11
diff -c -r1.11 dblink.sql.in
*** contrib/dblink/dblink.sql.in 2 Sep 2006 21:11:15 - 1.11
--- contrib/dblink/dblink.sql.in 8 Jul 2007 16:52:53 -
***
*** 1,3
--- 1,5
+ -- dblink_connect now restricts non-superusers to password
+ -- authenticated connections
CREATE OR REPLACE FUNCTION dblink_connect (text)
RETURNS text
AS 'MODULE_PATHNAME','dblink_connect'
***
*** 8,13
--- 10,31
AS 'MODULE_PATHNAME','dblink_connect'
LANGUAGE C STRICT;
+ -- dblink_connect_u allows non-superusers to use
+ -- non-password authenticated connections, but initially
+ -- privileges are revoked from public
+ CREATE OR REPLACE FUNCTION dblink_connect_u (text)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_connect'
+ LANGUAGE C STRICT SECURITY DEFINER;
+
+ CREATE OR REPLACE FUNCTION dblink_connect_u (text, text)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_connect'
+ LANGUAGE C STRICT SECURITY DEFINER;
+
+ REVOKE ALL ON FUNCTION dblink_connect_u (text) FROM public;
+ REVOKE ALL ON FUNCTION dblink_connect_u (text, text) FROM public;
+
CREATE OR REPLACE FUNCTION dblink_disconnect ()
RETURNS text
AS 'MODULE_PATHNAME','dblink_disconnect'
Index: contrib/dblink/doc/connection
===
RCS file: /cvsroot/pgsql/contrib/dblink/doc/connection,v
retrieving revision 1.4
diff -c -r1.4 connection
*** contrib/dblink/doc/connection 11 Mar 2006 04:38:29 - 1.4
--- contrib/dblink/doc/connection 8 Jul 2007 16:52:53 -
***
*** 27,32
--- 27,38
Returns status = "OK"
+ Notes
+
+ Only superusers may use dblink_connect to create non-password
+ authenticated connections. If non-superusers need this capability,
+ use dblink_connect_u instead.
+
Example usage
select dblink_connect('dbname=postgres');
***
*** 44,49
--- 50,95
==
Name
+ dblink_connect_u -- Opens a persistent connection to a remote database
+
+ Synopsis
+
+ dblink_connect_u(text connstr)
+ dblink_connect_u(text connname, text connstr)
+
+ Inputs
+
+ connname
+ if 2 arguments are given, the first is used as a name for a persistent
+ connection
+
+ connstr
+
+ standard libpq format connection string,
+ e.g. "hostaddr=127.0.0.1 port=5432 dbname=mydb user=postgres password=mypasswd"
+
+ if only one argument is given, the connection is unnamed; only one unnamed
+ connection can exist at a time
+
+ Outputs
+
+ Returns status = "OK"
+
+ Notes
+
+ With dblink_connect_u, a non-superuser may connect to any database server
+ using any authentication method. If the authentication method specified
+ for a particular user does not require a password, impersonation and
+ therefore escalation of privileges may occur. For this reason,
+ dblink_connect_u is initially installed with all privileges revoked from
+ publ
Re: [PATCHES] dblink connection security
Tom Lane wrote:
Here's a straw-man proposal that we could perhaps do for 8.3:
1. Invent a libpq connection-status function
bool PQconnectionUsedPassword(const PGconn *conn);
This returns true if the server had demanded a password during the
authentication phase. Aside from solving the immediate problem, this
can be useful for regular clients such as psql: it could be applied to a
failed connection object to decide whether to prompt for a password
(replacing the current egregious hack of strcmp'ing the error message).
2. Make dblink close the connection and throw error if called by a
non-superuser and PQconnectionUsedPassword returns false.
Attached patch implements this proposal, including documentation
changes. I'll work separately on the back-branch version.
Any comments/objections?
Joe
Index: contrib/dblink/dblink.c
===
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.63
diff -c -r1.63 dblink.c
*** contrib/dblink/dblink.c 6 Apr 2007 04:21:41 - 1.63
--- contrib/dblink/dblink.c 7 Jul 2007 22:49:05 -
***
*** 37,42
--- 37,43
#include "libpq-fe.h"
#include "fmgr.h"
#include "funcapi.h"
+ #include "miscadmin.h"
#include "access/heapam.h"
#include "access/tupdesc.h"
#include "catalog/namespace.h"
***
*** 245,250
--- 246,261
errdetail("%s", msg)));
}
+ if (!superuser())
+ {
+ if (!PQconnectionUsedPassword(conn))
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("connection without password not allowed"),
+ errdetail("non-superuser cannot connect if server does not request password"),
+ errhint("target server authentication method must be changed")));
+ }
+
if (connname)
{
rconn->conn = conn;
Index: contrib/dblink/dblink.sql.in
===
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.sql.in,v
retrieving revision 1.11
diff -c -r1.11 dblink.sql.in
*** contrib/dblink/dblink.sql.in 2 Sep 2006 21:11:15 - 1.11
--- contrib/dblink/dblink.sql.in 8 Jul 2007 01:22:13 -
***
*** 1,3
--- 1,5
+ -- dblink_connect now restricts non-superusers to password
+ -- authenticated connections
CREATE OR REPLACE FUNCTION dblink_connect (text)
RETURNS text
AS 'MODULE_PATHNAME','dblink_connect'
***
*** 8,13
--- 10,31
AS 'MODULE_PATHNAME','dblink_connect'
LANGUAGE C STRICT;
+ -- dblink_connect_u allows non-superusers to use
+ -- non-password authenticated connections, but initially
+ -- privileges are revoked from public
+ CREATE OR REPLACE FUNCTION dblink_connect_u (text)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_connect'
+ LANGUAGE C STRICT SECURITY DEFINER;
+
+ CREATE OR REPLACE FUNCTION dblink_connect_u (text, text)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_connect'
+ LANGUAGE C STRICT SECURITY DEFINER;
+
+ REVOKE ALL ON FUNCTION dblink_connect_u (text) FROM public;
+ REVOKE ALL ON FUNCTION dblink_connect_u (text, text) FROM public;
+
CREATE OR REPLACE FUNCTION dblink_disconnect ()
RETURNS text
AS 'MODULE_PATHNAME','dblink_disconnect'
Index: contrib/dblink/doc/connection
===
RCS file: /opt/src/cvs/pgsql/contrib/dblink/doc/connection,v
retrieving revision 1.4
diff -c -r1.4 connection
*** contrib/dblink/doc/connection 11 Mar 2006 04:38:29 - 1.4
--- contrib/dblink/doc/connection 8 Jul 2007 01:51:07 -
***
*** 27,32
--- 27,38
Returns status = "OK"
+ Notes
+
+ Only superusers may use dblink_connect to create non-password
+ authenticated connections. If non-superusers need this capability,
+ use dblink_connect_u instead.
+
Example usage
select dblink_connect('dbname=postgres');
***
*** 44,49
--- 50,95
==
Name
+ dblink_connect_u -- Opens a persistent connection to a remote database
+
+ Synopsis
+
+ dblink_connect_u(text connstr)
+ dblink_connect_u(text connname, text connstr)
+
+ Inputs
+
+ connname
+ if 2 arguments are given, the first is used as a name for a persistent
+ connection
+
+ connstr
+
+ standard libpq format connection string,
+ e.g. "hostaddr=127.0.0.1 port=5432 dbname=mydb user=postgres password=mypasswd"
+
+ if only one argument is given, the connection is unnamed; only one unnamed
+ connection can exist at a time
+
+ Outputs
+
+ Returns status = "OK"
+
+ Notes
+
+ With dblink_connect_u, a non-superuser may connect to any database server
+ using any authentication method. If the authentication method specified
+ for a particular user does not require a password, impersonation and
+ therefore escalation of privileges may occur. For this reason,
+ dblink_connect_u is
Re: [PATCHES] dblink connection security
Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: What about using the attached for 8.3, as well as earlier? It simply does not allow the local database user to become someone else on the libpq remote connection unless they are a superuser. This assumes that usernames on the remote site are equivalent to those locally. Which is helpful for the sort of local-loop scenarios we've been thinking about, but is hardly watertight even then (consider multiple postmasters on one machine). For remote connections it seems counterproductive; you might as well say "you must be superuser" and keep it simple. I see your point. OK, I'm back to implementing your proposal... One question: should we provide the SECURITY DEFINER functions with revoked privileges or just mention that in the docs? I was thinking something along the lines of the following even for the backpatched version: CREATE OR REPLACE FUNCTION dblink_connect_u (text) RETURNS text AS 'MODULE_PATHNAME','dblink_connect' LANGUAGE C STRICT SECURITY DEFINER; CREATE OR REPLACE FUNCTION dblink_connect_u (text, text) RETURNS text AS 'MODULE_PATHNAME','dblink_connect' LANGUAGE C STRICT SECURITY DEFINER; REVOKE execute ON FUNCTION dblink_connect_u (text) FROM public; REVOKE execute ON FUNCTION dblink_connect_u (text, text) FROM public; Joe ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] dblink connection security
Tom Lane wrote:
Here's a straw-man proposal that we could perhaps do for 8.3:
What about using the attached for 8.3, as well as earlier?
It simply does not allow the local database user to become someone else
on the libpq remote connection unless they are a superuser. As Tom
noted, a simple SECURITY DEFINER function created as a superuser could
allow backward compatible behavior.
CREATE OR REPLACE FUNCTION dblink_connect_u(connstr TEXT)
RETURNS TEXT AS $$
DECLARE passed TEXT;
BEGIN
SELECT dblink_connect(connstr) INTO passed;
RETURN passed;
END;
$$ LANGUAGE plpgsql SECURITY DEFINER;
contrib_regression=# \c - foo
You are now connected to database "contrib_regression" as user "foo".
contrib_regression=> select dblink_connect('dbname=contrib_regression');
ERROR: switching user not allowed
DETAIL: failed to connect local user "foo" as remote user "postgres"
HINT: only superuser may switch user name
contrib_regression=> select dblink_connect_u('dbname=contrib_regression');
dblink_connect_u
--
OK
(1 row)
Comments?
Thanks,
Joe
Index: dblink.c
===
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.63
diff -c -r1.63 dblink.c
*** dblink.c 6 Apr 2007 04:21:41 - 1.63
--- dblink.c 7 Jul 2007 04:39:49 -
***
*** 37,42
--- 37,43
#include "libpq-fe.h"
#include "fmgr.h"
#include "funcapi.h"
+ #include "miscadmin.h"
#include "access/heapam.h"
#include "access/tupdesc.h"
#include "catalog/namespace.h"
***
*** 230,235
--- 231,249
rconn = (remoteConn *) palloc(sizeof(remoteConn));
conn = PQconnectdb(connstr);
+ if (!superuser())
+ {
+ char *luser = PQuser(conn);
+ char *cuser = GetUserNameFromId(GetUserId());
+
+ if (strcmp(luser, cuser) != 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("switching user not allowed"),
+ errdetail("failed to connect local user \"%s\" as remote user \"%s\"", cuser, luser),
+ errhint("only superuser may switch user name")));
+ }
+
MemoryContextSwitchTo(oldcontext);
if (PQstatus(conn) == CONNECTION_BAD)
---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] dblink connection security
Joe Conway wrote:
Robert Treat wrote:
Joe Conway <[EMAIL PROTECTED]> writes:
Well certainly dbi-link has the exact same issue.
dbi-link only works in plperlu, so you've already decided your superuser only.
How so -- it is fundamentally no different than dblink, which is C
language (also untrusted).
I think the issue is that once the superuser creates said functions,
usage of the functions is automatically granted to PUBLIC, no? Being an
untrusted language just means that it takes a superuser to create the
functions using that language, not to use the functions themselves.
In fact, this misconception can prove dangerous in other ways. From the
docs:
CREATE FUNCTION badfunc() RETURNS integer AS $$
my $tmpfile = "/tmp/badfile";
open my $fh, '>', $tmpfile
or elog(ERROR, qq{could not open the file "$tmpfile": $!});
print $fh "Testing writing to a file\n";
close $fh or elog(ERROR, qq{could not close the file "$tmpfile": $!});
return 1;
$$ LANGUAGE plperlu;
select usename, usesuper from pg_shadow;
usename | usesuper
--+--
postgres | t
foo | f
(2 rows)
contrib_regression=# \c - foo
You are now connected to database "contrib_regression" as user "foo".
contrib_regression=> select badfunc();
badfunc
-
1
(1 row)
So anyone thinking that just because a language is untrusted means that
they don't need to be careful, is mistaken.
Joe
---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
Re: [PATCHES] dblink connection security
Robert Treat wrote: Joe Conway <[EMAIL PROTECTED]> writes: Well certainly dbi-link has the exact same issue. dbi-link only works in plperlu, so you've already decided your superuser only. How so -- it is fundamentally no different than dblink, which is C language (also untrusted). I think the issue is that once the superuser creates said functions, usage of the functions is automatically granted to PUBLIC, no? Being an untrusted language just means that it takes a superuser to create the functions using that language, not to use the functions themselves. Joe ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] dblink connection security
Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: Tom Lane wrote: Would it be sensible to change dblink so that unless invoked by a superuser, it fails any connection attempt in which no password is demanded? I am not sure that this is possible without changes to libpq; but ignoring implementation difficulties, is this a sane idea from the standpoint of security and usability? Possibly so. Remember that dblink is simply a libpq client. Doesn't that mean that similar (although likely less severe) issues affect other libpq clients executing locally, such as php or perl-dbi clients? Yeah, in principle this issue applies to any process performing a Postgres connection on behalf of someone else. (Whether there are any programs doing that, other than dblink, is debatable; but someday there may be.) Well certainly dbi-link has the exact same issue. And a local php-apache instance connecting to Postgres would allow Postgres connections as the apache user, no? Not that it is likely to be a problem, but if for some reason there was an apache user in Postgres, and even worse, if that user was given superuser status, you would have the exact same problem. The point about Kerberos delegation is interesting, but given that it doesn't work anyway, I'm not sure we need a solution for it right now. Possibly, when and if we get around to implementing it, we can somehow treat use of a delegated ticket as equivalent to use of a password. The general point is that we'd like to know whether the connection was authorized by means of some data supplied by the client, or on the basis of our own process identity (the latter being the case we wish to reject). Right now the only kind of "data supplied by the client" here is a password. Here's a straw-man proposal that we could perhaps do for 8.3: 1. Invent a libpq connection-status function bool PQconnectionUsedPassword(const PGconn *conn); Maybe PQconnectionUsedAuthToken() to mean "data supplied by the client", including other potential future mechanisms? 2. Make dblink close the connection and throw error if called by a non-superuser and PQconnectionUsedPassword returns false. Sounds good to me. This idea isn't usable as a back-patch, however, because adding functions to existing libpq versions is too chancy. What we could possibly do in back versions is, if dblink_connect is called by a non-superuser, first issue the connection attempt without any password and reject if that doesn't fail. (This'd involve parsing the connect string well enough to remove the password, which is tedious, but certainly doable.) Why not just require the connect string to contain a password for non-superusers? I like this approach better than removing public execute privileges on the functions, for two reasons: * A routine minor version update would install the security fix into existing installations, without need for any DBA intervention. * It does not take away functionality that has perfectly legitimate uses. Agreed. I won't have time to work on this until the end of the coming week -- tomorrow is the last day of my current business trip which tends to be busy. Tuesday I spend all day getting from Germany to San Diego. If it can wait that long, I'll look into it starting on the 5th, unless someone beats me to it. Joe ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] dblink connection security
Tom Lane wrote: Stephen Frost <[EMAIL PROTECTED]> writes: * Magnus Hagander ([EMAIL PROTECTED]) wrote: Kerberos is not affected either, because the server does not get a copy of the ticket. In theory it could be affected if the server requested a delegation enabled ticket, and exported it so it could be used, but none of these are done. That's quite a stretch even there, imv anyway... It'd have to be put somewhere a backend connecting would think to look for it, given that the user can't change the environment variables and whatnot (I don't think) of the backend process... Hmm. I think what you are both saying is that if the remote end wants Kerberos auth then you would expect a dblink connection to always fail. If so, then we still seem to be down to the conclusion that there are only three kinds of dblink connection: * those that require a password; * those that don't work; * those that are insecure. Would it be sensible to change dblink so that unless invoked by a superuser, it fails any connection attempt in which no password is demanded? I am not sure that this is possible without changes to libpq; but ignoring implementation difficulties, is this a sane idea from the standpoint of security and usability? Possibly so. Remember that dblink is simply a libpq client. Doesn't that mean that similar (although likely less severe) issues affect other libpq clients executing locally, such as php or perl-dbi clients? Joe ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] dblink connection security
Tom Lane wrote: Robert Treat <[EMAIL PROTECTED]> writes: Did you mean s/trust/ident/g, otherwise I don't think I understand the above... Both trust and ident local auth are sources of risk for this, although ident is particularly nasty since the DBA probably thinks he's being secure. For that matter, I'm not sure that *any* auth method except password offers much security against the problem; don't LDAP and Kerberos likewise rely mostly on process-level identity? And possibly PAM depending on which PAM plugin you're using? OK, so following that line of thought, how about: As a security precaution, dblink revokes access from PUBLIC role usage for the dblink_connect functions. It is not safe to allow ordinary users to execute dblink from a database in a PostgreSQL installation that allows account access using any authentication method which does not require a password. In that case, ordinary users could gain access to other accounts via dblink as if they had the privileges of the database superuser. If the allowed authentication methods require a password, this is no longer an issue. I'm not sure whether this is something to back-patch, though, since a back-patch will accomplish zero for existing installations. OK. But it might still be worth doing, along with something in the release notes. Joe ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] dblink connection security
Robert Treat wrote: Patch based on recent -hackers discussions, it removes usage from public, and adds a note to the documentation about why this is neccessary. I agree with the fix as the simplest and most sensible approach, and in general with the doc change, but I'm not inclined to reference the security paper. Maybe something like: As a security precaution, dblink revokes access from PUBLIC role usage for the dblink_connect functions. It is not safe to allow remote users to execute dblink from a database in a PostgreSQL installation that allows local account access using the "trust" authentication method. In that case, remote users could gain access to other accounts via dblink. If "trust" authentication is disabled, this is no longer an issue. I suppose this ought to be applied back through the 7.3 branch? Joe ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] resetStringInfo
Tom Lane wrote: Neil Conway <[EMAIL PROTECTED]> writes: Attached is a patch that makes a minor addition to the StringInfo API: resetStringInfo(), which clears the current content of the StringInfo but leaves it valid for future operations. I needed this for an external project, but ISTM this would be worth including in mainline: Sure. I'm pretty sure there are a number of places currently doing this "by hand"; would you mind looking around to see if you can fix 'em up to use this function? I have used pfree(var.data) combined with initStringInfo(&var) in a few places (e.g. in tablefunc.c). Joe ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [Fwd: dblink patch - Asynchronous queries and parallel
Joe Conway wrote: Sorry for the delay. I've done some rework to the original code sent by Kai, mainly to reduce duplication with the existing synchronous case, and to better fit with the existing docs, regression script, etc. I also changed the return type of dblink_get_connections() to text[], and added dblink_error_message(). If it isn't already too late, and there are no objections, I'd like to commit this in the next day or so. Patch applied. Joe ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] [Fwd: dblink patch - Asynchronous queries and parallel
Joe Conway wrote: I just received this (offlist), and have not had a chance to review it myself yet, but figured I should post it now in case others want to have a look and comment or discuss before feature freeze. If there are no major objections to the concept, I'll take responsibility to review and commit once I'm through with the "Values list-of-targetlists" stuff. Sorry for the delay. I've done some rework to the original code sent by Kai, mainly to reduce duplication with the existing synchronous case, and to better fit with the existing docs, regression script, etc. I also changed the return type of dblink_get_connections() to text[], and added dblink_error_message(). If it isn't already too late, and there are no objections, I'd like to commit this in the next day or so. Kai, please verify that this still works in a similar fashion as your original. Thanks, Joe ? .deps ? dblink.sql ? libdblink.so.0.0 ? results Index: README.dblink === RCS file: /cvsroot/pgsql/contrib/dblink/README.dblink,v retrieving revision 1.13 diff -c -r1.13 README.dblink *** README.dblink 3 Jan 2006 23:45:52 - 1.13 --- README.dblink 27 Aug 2006 23:21:15 - *** *** 7,12 --- 7,13 * And contributors: * Darko Prenosil <[EMAIL PROTECTED]> * Shridhar Daithankar <[EMAIL PROTECTED]> + * Kai Londenberg ([EMAIL PROTECTED]) * * Copyright (c) 2001-2006, PostgreSQL Global Development Group * ALL RIGHTS RESERVED; *** *** 31,36 --- 32,40 */ Release Notes: + 27 August 2006 + - Added async query capability. Original patch by + Kai Londenberg ([EMAIL PROTECTED]), modified by Joe Conway Version 0.7 (as of 25 Feb, 2004) - Added new version of dblink, dblink_exec, dblink_open, dblink_close, and, dblink_fetch -- allows ERROR on remote side of connection to *** *** 85,159 psql template1 < dblink.sql ! installs following functions into database template1: ! ! connection ! ! dblink_connect(text) RETURNS text !- opens an unnamed connection that will persist for duration of ! current backend or until it is disconnected ! dblink_connect(text,text) RETURNS text !- opens a named connection that will persist for duration of current ! backend or until it is disconnected ! dblink_disconnect() RETURNS text !- disconnects the unnamed persistent connection ! dblink_disconnect(text) RETURNS text !- disconnects a named persistent connection ! ! cursor ! ! dblink_open(text,text [, bool fail_on_error]) RETURNS text !- opens a cursor using unnamed connection already opened with ! dblink_connect() that will persist for duration of current backend ! or until it is closed ! dblink_open(text,text,text [, bool fail_on_error]) RETURNS text !- opens a cursor using a named connection already opened with ! dblink_connect() that will persist for duration of current backend ! or until it is closed ! dblink_fetch(text, int [, bool fail_on_error]) RETURNS setof record !- fetches data from an already opened cursor on the unnamed connection ! dblink_fetch(text, text, int [, bool fail_on_error]) RETURNS setof record !- fetches data from an already opened cursor on a named connection ! dblink_close(text [, bool fail_on_error]) RETURNS text !- closes a cursor on the unnamed connection ! dblink_close(text,text [, bool fail_on_error]) RETURNS text !- closes a cursor on a named connection ! ! query ! ! dblink(text,text [, bool fail_on_error]) RETURNS setof record !- returns a set of results from remote SELECT query; the first argument ! is either a connection string, or the name of an already opened ! persistant connection ! dblink(text [, bool fail_on_error]) RETURNS setof record !- returns a set of results from remote SELECT query, using the unnamed ! connection already opened with dblink_connect() ! ! execute ! ! dblink_exec(text, text [, bool fail_on_error]) RETURNS text !- executes an INSERT/UPDATE/DELETE query remotely; the first argument ! is either a connection string, or the name of an already opened ! persistant connection ! dblink_exec(text [, bool fail_on_error]) RETURNS text !- executes an INSERT/UPDATE/DELETE query remotely, using connection ! already opened with dblink_connect() ! ! misc ! ! dblink_current_query() RETURNS text !- returns the current query string ! dblink_get_pkey(text) RETURNS setof text !- returns the field names of a relation's primary key fields !
Re: Values list-of-targetlists patch for comments (was Re: [PATCHES]
Tom Lane wrote: Yeah, I've just been doing that and some hand analysis too. What I get (on a 64-bit machine) is that essentially all the space goes into lists of A_Const lists: 32000 lists of Const lists: 32000 transformInsertRow extra lists: 14400 I think we could safely list_free the input list in transformInsertRow as your patch suggests, which would buy back the 144M part. But I don't believe it's safe at all to free the raw_parser output --- the grammar sometimes makes multiple links to the same subtree, eg in BETWEEN. In any case the patch as proposed wouldn't catch all the detritus for any case more complicated than a simple integer constant. :-( The way that the list memory usage works (again, 64-bit machine) is sizeof(List) = 24 sizeof(ListCell) = 16 sizeof(A_Const) = 32 Each of these nodes will have 16 bytes palloc overhead, and the List header will be rounded up to 32 bytes as well, so we have total space for a 3-element integer list of 32+16 + (16+16 + 32+16) * 3 Add in 16+16 for the associated ListCell of the top list-of-lists, and you come to 320 bytes per sublist. Const happens to also be 32 bytes so the transformed lists are the same size. What if we built an array of A_Const nodes instead of a List? Maybe we could use something akin to appendStringInfo()/enlargeStringInfo() to build the array of nodes and enlarge it in chunks. Joe ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: Values list-of-targetlists patch for comments (was Re: [PATCHES]
Tom Lane wrote: I wonder whether there is any reasonable way to determine which data structures are responsible for how much space ... in my test I'm seeing MessageContext: 822075440 total in 104 blocks; 4510280 free (1 chunks); 817565160 used ExecutorState: 8024624 total in 3 blocks; 20592 free (12 chunks); 8004032 used so it seems mostly not the executor's fault, but that's not much to go on. I was doing it by sprinkling MemoryContextStats() in various places. I'll spend some time again later today and see if I can narrow it down to specific data structures using that. It shouldn't be too hard -- the patch I sent last night only pfrees a few structures, and they represent the bulk of what we need to clean up. Joe ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: Values list-of-targetlists patch for comments (was Re: [PATCHES]
Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: This patch retains the memory consumption savings but doesn't break any regression tests... I'm unconvinced that retail pfree's are the way to go. I just did some profiling of this test case: It's slightly depressing that there's not more time being spent in places we can easily tweak, but anyway the salient point to me is that AllocSetFree is already chewing a nontrivial part of the runtime. That's undoubtedly true, and important for the case that isn't memory constrained (but where I'm already seeing us perform relatively well). But once we start the machine swapping, runtime goes in the toilet. And without addressing the memory leak somehow, we will start a machine swapping significantly earlier than mysql. Joe ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: Values list-of-targetlists patch for comments (was Re: [PATCHES]
Joe Conway wrote:
Tom Lane wrote:
Here's what I've got so far. I think there's probably more gold to be
mined in terms of reducing runtime memory consumption (I don't like the
list_free_deep bit, we should use a context), but functionally it seems
complete.
I checked out memory usage, and it had regressed to about 1.4 GB (from
730 MB as reported yesterday) for 2 million inserts of 2 integers (i.e.
with the php script I've been using).
I know you're not too happy with the attached approach to solving this,
but I'm not sure how creating a memory context is going to help. Part of
the problem is that the various transformXXX functions sometimes return
freshly palloc'd memory, and sometimes return the pointer they are given.
Anyway, with the attached diff, the 2 million inserts case is back to
about 730 MB memory use, and speed is pretty much the same as reported
yesterday (i.e both memory use and performance better than mysql with
innodb tables).
Of course it also breaks a bunch of regression tests -- I guess that
just points to the fragility of this approach.
This patch retains the memory consumption savings but doesn't break any
regression tests...
Joe
? src/test/regress/sql/insert.sql.new
Index: src/backend/parser/analyze.c
===
RCS file: /cvsroot/pgsql/src/backend/parser/analyze.c,v
retrieving revision 1.341
diff -c -r1.341 analyze.c
*** src/backend/parser/analyze.c 2 Aug 2006 01:59:46 - 1.341
--- src/backend/parser/analyze.c 2 Aug 2006 05:48:18 -
***
*** 888,893
--- 888,894
icols = lnext(icols);
attnos = lnext(attnos);
}
+ list_free(exprlist);
return result;
}
***
*** 2191,2196
--- 2192,2198
for (i = 0; i < sublist_length; i++)
{
coltypes[i] = select_common_type(coltype_lists[i], "VALUES");
+ list_free(coltype_lists[i]);
}
newExprsLists = NIL;
***
*** 2203,2216
foreach(lc2, sublist)
{
Node *col = (Node *) lfirst(lc2);
- col = coerce_to_common_type(pstate, col, coltypes[i], "VALUES");
- newsublist = lappend(newsublist, col);
i++;
}
newExprsLists = lappend(newExprsLists, newsublist);
}
/*
* Generate the VALUES RTE
--- 2205,2224
foreach(lc2, sublist)
{
Node *col = (Node *) lfirst(lc2);
+ Node *new_col;
+
+ new_col = coerce_to_common_type(pstate, col, coltypes[i], "VALUES");
+ newsublist = lappend(newsublist, new_col);
+ if (new_col != col)
+ pfree(col);
i++;
}
newExprsLists = lappend(newExprsLists, newsublist);
+ list_free(sublist);
}
+ list_free(exprsLists);
/*
* Generate the VALUES RTE
Index: src/backend/parser/parse_target.c
===
RCS file: /cvsroot/pgsql/src/backend/parser/parse_target.c,v
retrieving revision 1.147
diff -c -r1.147 parse_target.c
*** src/backend/parser/parse_target.c 2 Aug 2006 01:59:47 - 1.147
--- src/backend/parser/parse_target.c 2 Aug 2006 05:48:18 -
***
*** 172,177
--- 172,178
foreach(lc, exprlist)
{
Node *e = (Node *) lfirst(lc);
+ Node *p = e;
/*
* Check for "something.*". Depending on the complexity of the
***
*** 188,193
--- 189,195
result = list_concat(result,
ExpandColumnRefStar(pstate, cref,
false));
+ pfree(e);
continue;
}
}
***
*** 203,208
--- 205,211
result = list_concat(result,
ExpandIndirectionStar(pstate, ind,
false));
+ pfree(e);
continue;
}
}
***
*** 210,218
/*
* Not "something.*", so transform as a single expression
*/
! result = lappend(result,
! transformExpr(pstate, e));
}
return result;
}
--- 213,224
/*
* Not "something.*", so transform as a single expression
*/
! p = transformExpr(pstate, e);
! result = lappend(result, p);
! if (e != p)
! pfree(e);
}
+ list_free(exprlist);
return result;
}
---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?
http://www.postgresql.org/docs/faq
Re: Values list-of-targetlists patch for comments (was Re: [PATCHES]
Tom Lane wrote:
Here's what I've got so far. I think there's probably more gold to be
mined in terms of reducing runtime memory consumption (I don't like the
list_free_deep bit, we should use a context), but functionally it seems
complete.
I checked out memory usage, and it had regressed to about 1.4 GB (from
730 MB as reported yesterday) for 2 million inserts of 2 integers (i.e.
with the php script I've been using).
I know you're not too happy with the attached approach to solving this,
but I'm not sure how creating a memory context is going to help. Part of
the problem is that the various transformXXX functions sometimes return
freshly palloc'd memory, and sometimes return the pointer they are given.
Anyway, with the attached diff, the 2 million inserts case is back to
about 730 MB memory use, and speed is pretty much the same as reported
yesterday (i.e both memory use and performance better than mysql with
innodb tables).
Thoughts?
Thanks,
Joe
Index: src/backend/parser/analyze.c
===
RCS file: /cvsroot/pgsql/src/backend/parser/analyze.c,v
retrieving revision 1.341
diff -c -r1.341 analyze.c
*** src/backend/parser/analyze.c 2 Aug 2006 01:59:46 - 1.341
--- src/backend/parser/analyze.c 2 Aug 2006 05:13:20 -
***
*** 872,877
--- 872,878
foreach(lc, exprlist)
{
Expr *expr = (Expr *) lfirst(lc);
+ Expr *p = expr;
ResTarget *col;
col = (ResTarget *) lfirst(icols);
***
*** 885,893
--- 886,898
result = lappend(result, expr);
+ if (expr != p)
+ pfree(p);
+
icols = lnext(icols);
attnos = lnext(attnos);
}
+ list_free(exprlist);
return result;
}
***
*** 2191,2196
--- 2196,2202
for (i = 0; i < sublist_length; i++)
{
coltypes[i] = select_common_type(coltype_lists[i], "VALUES");
+ list_free(coltype_lists[i]);
}
newExprsLists = NIL;
***
*** 2203,2216
foreach(lc2, sublist)
{
Node *col = (Node *) lfirst(lc2);
- col = coerce_to_common_type(pstate, col, coltypes[i], "VALUES");
- newsublist = lappend(newsublist, col);
i++;
}
newExprsLists = lappend(newExprsLists, newsublist);
}
/*
* Generate the VALUES RTE
--- 2209,2228
foreach(lc2, sublist)
{
Node *col = (Node *) lfirst(lc2);
+ Node *new_col;
+
+ new_col = coerce_to_common_type(pstate, col, coltypes[i], "VALUES");
+ newsublist = lappend(newsublist, new_col);
+ if (new_col != col)
+ pfree(col);
i++;
}
newExprsLists = lappend(newExprsLists, newsublist);
+ list_free(sublist);
}
+ list_free(exprsLists);
/*
* Generate the VALUES RTE
Index: src/backend/parser/parse_target.c
===
RCS file: /cvsroot/pgsql/src/backend/parser/parse_target.c,v
retrieving revision 1.147
diff -c -r1.147 parse_target.c
*** src/backend/parser/parse_target.c 2 Aug 2006 01:59:47 - 1.147
--- src/backend/parser/parse_target.c 2 Aug 2006 05:13:21 -
***
*** 172,177
--- 172,178
foreach(lc, exprlist)
{
Node *e = (Node *) lfirst(lc);
+ Node *p = e;
/*
* Check for "something.*". Depending on the complexity of the
***
*** 188,193
--- 189,195
result = list_concat(result,
ExpandColumnRefStar(pstate, cref,
false));
+ pfree(e);
continue;
}
}
***
*** 203,208
--- 205,211
result = list_concat(result,
ExpandIndirectionStar(pstate, ind,
false));
+ pfree(e);
continue;
}
}
***
*** 210,218
/*
* Not "something.*", so transform as a single expression
*/
! result = lappend(result,
! transformExpr(pstate, e));
}
return result;
}
--- 213,224
/*
* Not "something.*", so transform as a single expression
*/
! p = transformExpr(pstate, e);
! result = lappend(result, p);
! if (e != p)
! pfree(e);
}
+ list_free(exprlist);
return result;
}
---(end of broadcast)---
TIP 4: Have you searched our list archives?
http://archives.postgresql.org
Re: Values list-of-targetlists patch for comments (was Re: [PATCHES]
Tom Lane wrote: Here's what I've got so far. I think there's probably more gold to be mined in terms of reducing runtime memory consumption (I don't like the list_free_deep bit, we should use a context), but functionally it seems complete. I'm off to dinner again, it's in your court to look over some more if you want. (PS: if you want to apply, go ahead, don't forget catversion bump.) Committed, with catversion bump. Joe ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: Values list-of-targetlists patch for comments (was Re: [PATCHES]
Gavin Sherry wrote:
Is this intentional:
template1=# values(1), (2);
column1
-
1
2
(2 rows)
This is legal because of:
simple_select:
/* ... */
| values_clause { $$ = $2; }
hmm, not sure about that...
Also, I am working out some docs and regression tests.
Oh, cool. I was going to start working on that myself tonight, but if
you're already working on it, don't let me stand in the way ;-)
Actually, if you want me to finish up whatever you have started, I'm
happy to do that too.
Joe
---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster
Re: Values list-of-targetlists patch for comments (was Re: [PATCHES]
Tom Lane wrote: Here's what I've got so far. I think there's probably more gold to be mined in terms of reducing runtime memory consumption (I don't like the list_free_deep bit, we should use a context), but functionally it seems complete. I'm off to dinner again, it's in your court to look over some more if you want. OK, I'll continue to look at it this week. (PS: if you want to apply, go ahead, don't forget catversion bump.) Sure, I'll commit shortly. Thanks, Joe ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] 8.2 features?
Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: I wanted to post an updated patch even though there are still things not working again after conversion to bare expressions. I've been through the planner part of this and it looks OK (one or two small errors). I'm currently messing with a revised version of the grammar that supports putting VALUES everyplace that the spec allows, and is a bit simpler than the old one to boot: it folds VALUES and SELECT together, so we need fewer cases in the INSERT production. Of course this breaks most of what you did in the parser :-( ... I'm working on fixing that. I'm about to go out to dinner but thought I'd post the gram.y and parsenodes.h files so you could see where I'm headed. These are diffs from CVS tip, not from your patch. Yup, I can see where you're headed. Looks nice! In case you can make use of it, here's my latest. I found that I was being too aggressive at freeing the input nodes to transformExpr() in transformRangeValues() after using them. In many cases the returned node is a new palloc'd node, but in some cases it is not. The other issue I found was that I had neglected to fixup/coerce the raw expressions ala updateTargetListEntry(). I ended up creating a somewhat simpler updateValuesExprListEntry() to use on values expression lists. I have yet to get to the similar/more general issue of coercing values expression lists to common datatypes (i.e. using select_common_type()). FWIW, here's a list of non-working cases at the moment: 8<- create table inserttest (col1 int4, col2 int4 NOT NULL, col3 text default 'testing'); --doesn't work --- --wrong result insert into inserttest (col2, col3) values (23, DEFAULT), (24, DEFAULT), (25, 'hello'), (26, DEFAULT); select * from (values (3,4),(2,3)) as t1(f1,f2) join (values (3,8),(2,6)) as t2(f1,f2) using (f1); select * from (values (3,4),(2,3)) as t1(f1,f2) join (values (3,8),(2,6)) as t2(f1,f2) using (f1) where t2.f2 = 8; select * from (values (3,4),(2,3)) as t1(f1,f2) join (values (3,8),(2,6)) as t2(f1,f2) on t1.f1 = t2.f2 where t1.f1 = 3; --corrupt result but no crash select f1,f2 from (values (11,2),(26,'a'),(6,4)) as t(f1,f2) order by 1 desc; --crash select f1 from (values (1,2),(2,3)) as t(f1,f2) order by 1 desc; select f1,f2 from (values (11,'a'),(26,13),(6,'c')) as t(f1,f2) order by 1 desc; 8<- Joe multi-insert-r19.diff.gz Description: GNU Zip compressed data ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [HACKERS] 8.2 features?
Tom Lane wrote: As far as avoiding overhead goes, here's what I'm thinking: * The Values RTE node should contain a list of lists of bare expressions, without TargetEntry decoration (you probably do not need ResTarget in the raw parse tree for VALUES, either). * The ValuesScan plan node will just reference this list-of-lists (avoiding making a copy). It will need to contain a targetlist because all plan nodes do, but the base version of that will just be a trivial "Var 1", "Var 2", etc. (The planner might replace that with a nontrivial targetlist in cases such as the example above.) I wanted to post an updated patch even though there are still things not working again after conversion to bare expressions. Note that I hacked enough of the executor stuff so I could test my changes on the parser area. The basic "INSERT ... VALUES (...), (...), ..." does work, but without DEFAULT again :-(. The good news is that from a memory and perfomance standpoint, my simple test now shows us outperforming mysql: $loopcount = 100; Postgres: multi-INSERT-at-once Elapsed time is 12 seconds ~420MB MySQL: multi-INSERT-at-once Elapsed time is 17 seconds ~600MB $loopcount = 200; Postgres: multi-INSERT-at-once Elapsed time is 29 seconds ~730MB MySQL: multi-INSERT-at-once Elapsed time is 37 seconds ~1.2GB (this one is from memory -- I didn't write it in my notes) Joe multi-insert-r18.diff.gz Description: GNU Zip compressed data ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [HACKERS] 8.2 features?
Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: I'm afraid though that after 2 or so days heading down the last path you suggested (namely making a new jointree leaf node) I was having trouble, and at the same time came to the conclusion that adding a new RTE was alot cleaner and made more sense to me. So I'm hoping you won't want to send me back to the drawing board again. I believe I have cleaned up the things you objected to: I was just objecting to having both a new RTE type and a new jointree node type --- you only need one or the other. Opting for the new RTE type is fine with me, and it probably is a bit cleaner at the end of the day. Great! I still dislike the way you're doing things in the executor though. I don't see the point of using the execScan.c machinery; most of the time that'll be useless overhead. As I said before, I think the right direction here is to split Result into two single-purpose node types and make the non-filter version capable of taking a list of targetlists. OK. As far as reducing memory use goes, it seems to me that there's no need for the individual "targetlists" to have ResTarget/TargetEntry decoration. For the simple case where the expressions are just Const nodes, this could save something like a third of the space (there's also a List node per item, which we can't do much about). I think we'd have to gin up a fake targetlist to attach to the Plan node, but there'd be only one. OK, I'll take a look at that (actually I was just in that general vicinity anyway). Since the result-node split is my hot button, I'm willing to volunteer to make it happen. Do you want to concentrate on the remaining parser-area issues and leave the executor part to me? Sure, sounds good to me. Joe ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] [HACKERS] 8.2 features?
Joe Conway wrote: Tom Lane wrote: I thought Joe was off in a corner doing a whole new version. (I'm willing to help if he needs help...) Yeah, I was going to post the latest tonight. Sorry for the delay. Ever see the movie "The Money Pit"? This afternoon I started to think I lived in that house :-( Anyway, as mentioned below, I think the attached works well for the "INSERT ... VALUES (...), (...), ..." and related cases. There are still things wrong that I have not even tried to fix with respect to FROM clause VALUES lists. Namely column aliases have no effect, and neither does "ORDER BY" clause (I'm pretty sure addRangeTableEntryForValues needs work among other places). From a memory usage standpoint, I got the following using 1,000,000 values targetlists: sql length = 632 NOTICE: enter transformInsertStmt MessageContext: 478142520 total in 66 blocks; 5750400 free (3 chunks); 472392120 used NOTICE: enter transformRangeValues MessageContext: 478142520 total in 66 blocks; 5749480 free (6 chunks); 472393040 used NOTICE: enter updateTargetListEntry MessageContext: 629137464 total in 84 blocks; 44742464 free (91 chunks); 584395000 used NOTICE: exit transformInsertStmt MessageContext: 629137464 total in 84 blocks; 44742408 free (91 chunks); 584395056 used NOTICE: start ExecInitValuesScan MessageContext: 1015013432 total in 130 blocks; 6614008 free (8 chunks); 1008399424 used NOTICE: end ExecInitValuesScan MessageContext: 1015013432 total in 130 blocks; 6614008 free (8 chunks); 1008399424 used ExecutorState: 8024632 total in 3 blocks; 21256 free (8 chunks); 8003376 used This shows original SQL statement is about 6MB, by the time we get to parse analysis we're at almost 500 MB, and that memory is never recovered. Transforming from ResTarget to TargetEntry chews up about 100MB. Then between exiting transformInsertStmt and entering ExecInitValuesScan we double in memory usage to about 1 GB. It isn't shown here, but we add another 200 MB or so during tuple projection. So we top out at about 1.2 GB. Note that mysql tops out at about 600 MB for this same SQL. I'm not sure what if anything can be done to improve the above -- I'm open to suggestions. Please note that this patch requires an initdb, although I have not yet bothered to bump CATVERSION. Thanks for help, comments, suggestions, etc... Joe I'm afraid though that after 2 or so days heading down the last path you suggested (namely making a new jointree leaf node) I was having trouble, and at the same time came to the conclusion that adding a new RTE was alot cleaner and made more sense to me. So I'm hoping you won't want to send me back to the drawing board again. I believe I have cleaned up the things you objected to: 1. Now I'm not doing both alternative -- the targetlists are only attached to the RTE from the point of parse analysis onward. 2. I've eliminated the tuplestore in favor of runtime evaluation of the targetlists which are in an array (allowing forward or backward scanning -- although I haven't tested the latter yet). I've also solved the INSERT related issues that I had earlier: 1. Fixed the rules regression test -- now all regression tests pass 2. Fixed evaluation of DEFAULT values 3. Improved memory consumption and speed some more -- basically we are approximately equal to mysql as long as we don't swap, and we consume about twice the RAM as mysql instead of several times as much. I have more analysis of memory use I'd also like to share later. 4. I think the INSERT part of this is ready to go basically, but I need a bit more time to test corner cases. I've made some progress on "SELECT ... FROM (VALUES ...) AS ..." 1. No more shift/reduce issues 2. The ValuesScan work and memory improvements mentioned above applies here too. 3. This part still needs the most work though. I'll post a patch in a few hours -- there is some debug code in there currently that I should clean up before I send it to the list. BTW, I'm reserving Saturday, Sunday, and Monday (taking Monday off from my day job) to work on outstanding issues. I can continue to work through the end of next Friday, 4 August. After that I'm heading to Germany on a business trip and my "spare" time will evaporate for a few weeks. multi-insert-r17.diff.gz Description: GNU Zip compressed data ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [HACKERS] 8.2 features?
Tom Lane wrote: Bruce Momjian <[EMAIL PROTECTED]> writes: Are you going to apply this? Seems it is ready. I thought Joe was off in a corner doing a whole new version. (I'm willing to help if he needs help...) Yeah, I was going to post the latest tonight. I'm afraid though that after 2 or so days heading down the last path you suggested (namely making a new jointree leaf node) I was having trouble, and at the same time came to the conclusion that adding a new RTE was alot cleaner and made more sense to me. So I'm hoping you won't want to send me back to the drawing board again. I believe I have cleaned up the things you objected to: 1. Now I'm not doing both alternative -- the targetlists are only attached to the RTE from the point of parse analysis onward. 2. I've eliminated the tuplestore in favor of runtime evaluation of the targetlists which are in an array (allowing forward or backward scanning -- although I haven't tested the latter yet). I've also solved the INSERT related issues that I had earlier: 1. Fixed the rules regression test -- now all regression tests pass 2. Fixed evaluation of DEFAULT values 3. Improved memory consumption and speed some more -- basically we are approximately equal to mysql as long as we don't swap, and we consume about twice the RAM as mysql instead of several times as much. I have more analysis of memory use I'd also like to share later. 4. I think the INSERT part of this is ready to go basically, but I need a bit more time to test corner cases. I've made some progress on "SELECT ... FROM (VALUES ...) AS ..." 1. No more shift/reduce issues 2. The ValuesScan work and memory improvements mentioned above applies here too. 3. This part still needs the most work though. I'll post a patch in a few hours -- there is some debug code in there currently that I should clean up before I send it to the list. BTW, I'm reserving Saturday, Sunday, and Monday (taking Monday off from my day job) to work on outstanding issues. I can continue to work through the end of next Friday, 4 August. After that I'm heading to Germany on a business trip and my "spare" time will evaporate for a few weeks. Joe ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: Values list-of-targetlists patch for comments (was Re: [PATCHES]
Tom Lane wrote: ISTM that this should be represented using an RTE_SUBQUERY node in the outer query; the alias attaches to that node, not to the VALUES itself. So I don't think you need that alias field in the jointree entry either. If we stick with the plan of representing VALUES as if it were SELECT * FROM (valuesnode), then this approach would make the second query above have a structure like Query .rtable -> RTE_SUBQUERY .subquery -> Query .jointree -> Values (leaving out a ton of detail of course, but those are the key nodes). OK, I'll go try to wrap my mind around that this evening and see where it takes me. Thanks, Joe ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
[PATCHES] [Fwd: dblink patch - Asynchronous queries and parallel execution]
I just received this (offlist), and have not had a chance to review it
myself yet, but figured I should post it now in case others want to have
a look and comment or discuss before feature freeze.
If there are no major objections to the concept, I'll take
responsibility to review and commit once I'm through with the "Values
list-of-targetlists" stuff.
(I'm not sure where we finished off with the discussion of PATCHES vs
HACKERS list for this kind of stuff, so I'm going to send another copy
of this to HACKERS without the attachement)
Thanks,
Joe
Original Message
Subject: dblink patch - Asynchronous queries and parallel execution
Date: Mon, 24 Jul 2006 12:47:51 +0200
From: Kai Londenberg <[EMAIL PROTECTED]>
To: [EMAIL PROTECTED]
Hello,
I needed parallel query execution features for a project, and so I
modified the dblink module to add support
for asynchronous query execution.
I thought others might find these features useful as well, therefore I'd
like to contribute this to the
current Postgresql contrib/dblink codebase.
The code is based on the contrib/dblink code included with the current
8.1.4 version of PostgreSQL.
I'm including the entire modified contrib/dblink directory in archived form.
I modified dblink.c and dblink.sql.in, and created the file README.async
Hope you like it, and include it in a possible next version of dblink.
The code still needs some testing and code review. I made it work for
me, but I don't have any
experience writing Postgresql Extensions, and haven't touched C for a while.
The most important thing about this code is that it allows parallel
execution of queries on several
backend databases, and re-joining of their results. This solves a lot of
scalability problems.
This is my corresponding README.async file which describes my additions.
-
dblink-async patch by Kai Londenberg ([EMAIL PROTECTED])
24.7.2006
All code is licensed under the same terms as the rest of the dblink
code.
SQL Function declarations have been added at the bottom of dblink.sql
Added functions:
int dblink_send_query(connstr text, sql text)
Sends a query to a remote server for asynchronous execution.
returns immediately without waiting for results.
returns 1 on success, or 1 on failure.
results *must* be fetched by dblink_get_result(connstr)
a running query may be cancelled by dblink_cancel_query(connstr)
dblink_get_result(connstr text[,bool fail_on_error])
retrieves the result of a query started by dblink_send_query.
Blocks until a result gets available.
This function *must* be called if dblink_send_query returned
a 1, even on cancelled queries - otherwise the connection
can't be used anymore.
dblink_get_connections()
List all open dblink connections by name.
Returns a comma separated string of all connection names.
Takes no params
Example: SELECT string_to_array(dblink_get_connections(), ',');
int dblink_is_busy(connstr)
returns 1 if connection is busy, 0 if it is not busy.
If this function returns 0, it is guaranteed that dblink_get_result
will not block.
text dblink_cancel_query(connstr)
Cancels a running query on a given connection.
returns "OK" on success, or an error message on failure.
Examples:
Example 1 - Union over parallel executed remote queries --
SELECT dblink_connect('dtest1', 'host=server1 port=5432 dbname=dtest_1
user=duser password=pass');
SELECT * from
dblink_send_query('dtest1', 'SELECT country_code, city from
world_cities where city like \'fe%\'') as t1;
SELECT dblink_connect('dtest2', 'host=server2 port=5432 dbname=dtest_2
user=duser password=pass');
SELECT * from
dblink_send_query('dtest2', 'SELECT country_code, city from
world_cities where city like \'fe%\'') as t1;
SELECT dblink_connect('dtest3', 'host=server3 port=5432 dbname=dtest_3
user=duser password=pass');
SELECT * from
dblink_send_query('dtest3', 'SELECT country_code, city from
world_cities where city like \'fe%\'') as t1;
CREATE TEMPORARY TABLE result AS
(SELECT * from dblink_get_result('dtest1') as t1(country_code text, city
text))
UNION
(SELECT * from dblink_get_result('dtest2') as t2(country_code text, city
text))
UNION
(SELECT * from dblink_get_result('dtest3') as t3(country_code text, city
text))
ORDER by city DESC LIMIT 100;
SELECT dblink_disconnect('dtest1');
SELECT dblink_disconnect('dtest2');
SELECT dblink_disconnect('dtest3');
SELECT * from result;
--- End of Example 1 ---
best regards,
Kai Londenberg
dblink.tar.gz
Description: GNU Zip compressed data
---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster
Re: Values list-of-targetlists patch for comments (was Re: [PATCHES]
Tom Lane wrote: There are basically two ways you could go about this: 1. Make a new jointree leaf node type to represent a VALUES construct, and dangle the list of lists of expressions off that. 2. Make a new RangeTblEntry type to represent a VALUES construct, and just put a RangeTblRef to it into the jointree. The expressions dangle off the RangeTblEntry. You seem to have done *both*, which is certainly not what I had in mind. I'd drop the RangeTblEntry changes, I think. Good feedback -- thanks! But without the RTE, how would VALUES in the FROM clause work? Or should I just drop that part and focus on just the InsertStmt case? Joe ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: Values list-of-targetlists patch for comments (was Re: [PATCHES]
Joe Conway wrote: Since the feature freeze is only about a week off, I wanted to post this patch even though it is not yet ready to be applied. Sorry -- I just realized that two new files for ValuesScan didn't make it into the patch posted earlier. Here they are now -- please untar in your postgres sourcetree root in addition to applying the patch. (I thought "cvs diff -cN" should have included the new files, since I had earlier done "cvs add" on them, but it didn't work. I could swear that worked for me in the past...) Thanks, Joe multi-insert-r6a.new.tar.gz Description: GNU Zip compressed data ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Values list-of-targetlists patch for comments (was Re: [PATCHES] [HACKERS] 8.2 features?)
Tom Lane wrote:
Joe Conway <[EMAIL PROTECTED]> writes:
I'm liking this too. But when you say "jointree node", are you saying to
model the new node type after NestLoop/MergeJoin/HashJoin nodes? These
are referred to as "join nodes" in ExecInitNode. Or as you mentioned a
couple of times, should this look more like an Append node?
No, I guess I confused you by talking about the executor representation
at the same time. This is really unrelated to the executor. The join
tree I'm thinking of here is the data structure that dangles off
Query.jointree --- it's a representation of the query's FROM clause,
and (at present) can contain RangeTblRef, FromExpr, and JoinExpr nodes.
See the last hundred or so lines of primnodes.h for some details.
The jointree is used by the planner to compute the plan node tree that
the executor will run, but it's not the same thing.
There are basically two ways you could go about this:
1. Make a new jointree leaf node type to represent a VALUES construct,
and dangle the list of lists of expressions off that.
2. Make a new RangeTblEntry type to represent a VALUES construct, and
just put a RangeTblRef to it into the jointree. The expressions
dangle off the RangeTblEntry.
Offhand I'm not certain which of these would be cleanest. The second
way has some similarities to the way we handle set operation trees
(UNION et al), so it might be worth looking at that stuff. However,
being a RangeTblEntry has a lot of baggage (eg, various routines expect
to find an RTE alias, column names, column types, etc) and maybe we
don't need all that for VALUES.
Since the feature freeze is only about a week off, I wanted to post this
patch even though it is not yet ready to be applied.
Executive summary:
==
1. The patch is now large and invasive based on adding new node
types and associated infrastructure. I modelled the nodes largely
on RangeFunction and FunctionScan.
2. Performance is close enough to mysql to not be a big issue (I think,
more data below) as long as the machine does not get into a memory
swapping regime. Memory usage is now better, but not as good as
mysql.
3. I specifically coded with the intent of preserving current insert
statement behavior and code paths for current functionality. So there
*should* be no performance degradation or subtle semantics changes
for "INSERT DEFAULT VALUES", "INSERT ... VALUES (with one target
list)", "INSERT ... SELECT ...". Even Tom's recently discovered
"insert into foo values (tenk1.*)" still works ;-)
Performance:
On my development machine (dual core amd64, 2GB RAM) I get the following
results using the php script posted earlier:
Postgres:
-
$loopcount = 10;
multi-INSERT-at-once Elapsed time is 1 second
$loopcount = 30;
multi-INSERT-at-once Elapsed time is 5 seconds
$loopcount = 50;
multi-INSERT-at-once Elapsed time is 9 seconds
$loopcount = 80;
multi-INSERT-at-once Elapsed time is 14 seconds
$loopcount = 90;
multi-INSERT-at-once Elapsed time is 17 seconds
$loopcount = 100;
multi-INSERT-at-once Elapsed time is 42 seconds
$loopcount = 200;
killed after 5 minutes due to swapping
MySQL:
--
$loopcount = 10;
multi-INSERT-at-once Elapsed time is 2 seconds
$loopcount = 30;
INSERT failed:Got a packet bigger than 'max_allowed_packet' bytes
changed max_allowed_packet=64M
multi-INSERT-at-once Elapsed time is 5 seconds
$loopcount = 50;
multi-INSERT-at-once Elapsed time is 8 seconds
$loopcount = 80;
multi-INSERT-at-once Elapsed time is 13 seconds
$loopcount = 90;
multi-INSERT-at-once Elapsed time is 15 seconds
$loopcount = 100;
multi-INSERT-at-once Elapsed time is 17 seconds
$loopcount = 200;
multi-INSERT-at-once Elapsed time is 36 seconds
$loopcount = 300;
multi-INSERT-at-once Elapsed time is 54 seconds
$loopcount = 400;
multi-INSERT-at-once Elapsed time is 134 seconds
:
==
Included in this patch is support for in the
FROM clause, e.g.:
regression=# select * from {values (1,array[1,2]),(2,array[3,4])};
?column? | array
--+---
1 | {1,2}
2 | {3,4}
(2 rows)
The strange syntax is a temporary hack to eliminate shift/reduce
conflicts. I'm not entirely sure we want to try to support this (or
something like it) for 8.2, but much of what is needed is now readily
available. More on known issues next.
Known Issues:
=
General:
1. Several comments in the patch are marked "FIXME". These are areas
where I was uncertain what was the "right thing to do". Any advice
on these specific spots would be very much appreciated.
2. I broke the rules regression test -- still need to look at what I
did to mess that up. Somewhere in the reconstruction of "VALUES ..."
Re: [PATCHES] [HACKERS] 8.2 features?
Tom Lane wrote:
Christopher Kings-Lynne <[EMAIL PROTECTED]> writes:
Strange. Last time I checked I thought MySQL dump used 'multivalue
lists in inserts' for dumps, for the same reason that we use COPY
I think Andrew identified the critical point upthread: they don't try
to put an unlimited number of rows into one INSERT, only a megabyte
or so's worth. Typical klugy-but-effective mysql design approach ...
OK, so given that we don't need to be able to do 1 million
multi-targetlist insert statements, here is rev 2 of the patch.
It is just slightly more invasive, but performs *much* better. In fact,
it can handle as many targetlists as you have memory to deal with. It
also deals with DEFAULT values in the targetlist.
I've attached a php script that I used to do crude testing. Basically I
tested 3 cases in this order:
single-INSERT-multi-statement:
--
"INSERT INTO foo2a (f1,f2) VALUES (1,2);"
-- repeat statement $loopcount times
single-INSERT-at-once:
--
"INSERT INTO foo2b (f1,f2) VALUES (1,2);INSERT INTO foo2a (f1,f2)
VALUES (1,2);INSERT INTO foo2a (f1,f2) VALUES (1,2)..."
-- build a single SQL string by looping $loopcount times,
-- and execute it all at once
multi-INSERT-at-once:
-
"INSERT INTO foo2c (f1,f2) VALUES (1,2),(1,2),(1,2)..."
-- build a single SQL string by looping $loopcount times,
-- and execute it all at once
Here are the results:
$loopcount = 10;
single-INSERT-multi-statement Elapsed time is 34 seconds
single-INSERT-at-once Elapsed time is 7 seconds
multi-INSERT-at-once Elapsed time is 4 seconds
about 370MB peak memory usage
$loopcount = 20;
single-INSERT-multi-statement Elapsed time is 67 seconds
single-INSERT-at-once Elapsed time is 12 seconds
multi-INSERT-at-once Elapsed time is 9 seconds
about 750MB peak memory usage
$loopcount = 30;
single-INSERT-multi-statement Elapsed time is 101 seconds
single-INSERT-at-once Elapsed time is 18 seconds
multi-INSERT-at-once Elapsed time is 13 seconds
about 1.1GB peak memory usage
Somewhere beyond this, my machine goes into swap hell, and I didn't have
the patience to wait for it to complete :-)
It would be interesting to see a side-by-side comparison with MySQL
since that seems to be our benchmark on this feature. I'll try to do
that tomorrow if no one beats me to it.
There is only one downside to the current approach that I'm aware of.
The command-result tag is only set by the "original" query, meaning that
even if you insert 300,000 rows using this method, the command-result
tag looks like "INSERT 0 1"; e.g.:
regression=# create table foo2(f1 int default 42,f2 int default 6);
CREATE TABLE
regression=# insert into foo2 (f1,f2) values
(default,12),(default,10),(115,21);
INSERT 0 1
regression=# select * from foo2;
f1 | f2
-+
42 | 12
42 | 10
115 | 21
(3 rows)
Any thoughts on how to fix that?
Thanks,
Joe
Index: src/backend/parser/analyze.c
===
RCS file: /cvsroot/pgsql/src/backend/parser/analyze.c,v
retrieving revision 1.340
diff -c -r1.340 analyze.c
*** src/backend/parser/analyze.c 14 Jul 2006 14:52:21 - 1.340
--- src/backend/parser/analyze.c 19 Jul 2006 03:53:35 -
***
*** 657,667
}
else
{
/*
* For INSERT ... VALUES, transform the given list of values to form a
! * targetlist for the INSERT.
*/
! qry->targetList = transformTargetList(pstate, stmt->targetList);
}
/*
--- 657,699
}
else
{
+ ListCell *tlr;
+
/*
* For INSERT ... VALUES, transform the given list of values to form a
! * targetlist for the INSERT. In a multi-targetlist INSERT, append all
! * but the first targetlist to extras_after to be processed later by
! * do_parse_analyze
*/
! qry->targetList = NIL;
! foreach(tlr, stmt->targetList)
! {
! List *tgtlist = (List *) lfirst(tlr);
!
! if (qry->targetList == NIL)
! {
! /* transform the first targetlist */
! qry->targetList = transformTargetList(pstate, tgtlist);
! }
! else
! {
! /*
! * Create an InsertStmt node for each additional targetlist
! * and append to extras_after
! */
! InsertStmt *insnode = makeNode(InsertStmt);
!
! insnode->cols = NIL;
! insnode->targetList = list_make1(tgtlist);
! insnode->selectStmt = NULL;
! insnode->relation = stmt->relation;
!
! if (*extras_after == NIL)
! *extras_after = list_make1(insnode);
! else
! *extras_after = lappend(*extras_after, insnode);
! }
! }
}
/*
Index: src/backend/parser/gram.y
===
RCS file: /cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.551
diff -c -r2.551 gram.y
*** src/backend/parser/gram.y 3 Jul 2006 22:45:39 - 2.551
--- src/backend/parser/gram.y 19 Jul 2006 03:53:40 -
***
Re: [PATCHES] [HACKERS] 8.2 features?
Andrew Dunstan wrote: Christopher Kings-Lynne wrote: The major downside is that somewhere between 9000 and 1 VALUES-targetlists produces "ERROR: stack depth limit exceeded". Perhaps for the typical use-case this is sufficient though. I'm open to better ideas, comments, objections... If the use case is people running MySQL dumps, then there will be millions of values-targetlists in MySQL dumps. Yeah. The fabricated select hack does feel wrong to me. Taking a quick 2 minute look at the grammar it looks like a better bet would be to make InsertStmt.targetList a list of lists of values rather than just a list of values. Of course, that would make the changes more invasive. Even with that we'd still be reading the whole thing into memory ... is there a sane way to cache the inline data before statement execution? I started down the path of making InsertStmt.targetList a list of targetlists. The problem is finding a reasonable way to make that available to the executor. Back to the drawing board I guess. I have similar concerns with the millions of values-targetlists comment that Chris made. But I don't see how we can cache the data easily short of inventing a List alternative that spills to disk. I guess we can just say that for true bulk load our supported mechanism is still just COPY, but it would be a pity to restrict a feature that is in the standard that way. True Joe ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] 8.2 features?
Joe Conway wrote:
. multiple values clauses for INSERT
The best way might be to fabricate a selectStmt equiv to
"SELECT UNION ALL SELECT ...",
but that still feels like a hack.
Here is a patch pursuant to my earlier post. It has the advantage of
being fairly simple and noninvasive.
The major downside is that somewhere between 9000 and 1
VALUES-targetlists produces "ERROR: stack depth limit exceeded".
Perhaps for the typical use-case this is sufficient though.
I'm open to better ideas, comments, objections...
Thanks,
Joe
Index: src/backend/parser/gram.y
===
RCS file: /cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.551
diff -c -r2.551 gram.y
*** src/backend/parser/gram.y 3 Jul 2006 22:45:39 - 2.551
--- src/backend/parser/gram.y 18 Jul 2006 04:19:45 -
***
*** 238,251
qualified_name_list any_name any_name_list
any_operator expr_list attrs
target_list update_target_list insert_column_list
! insert_target_list def_list indirection opt_indirection
! group_clause TriggerFuncArgs select_limit
! opt_select_limit opclass_item_list
! transaction_mode_list_or_empty
TableFuncElementList
prep_type_clause prep_type_list
execute_param_clause using_clause
%type into_clause OptTempTableName
%type createfunc_opt_item common_func_opt_item
--- 238,253
qualified_name_list any_name any_name_list
any_operator expr_list attrs
target_list update_target_list insert_column_list
! insert_target_els
! def_list indirection opt_indirection group_clause
! TriggerFuncArgs select_limit opt_select_limit
! opclass_item_list transaction_mode_list_or_empty
TableFuncElementList
prep_type_clause prep_type_list
execute_param_clause using_clause
+ %type insert_target_list insert_target_lists
+
%type into_clause OptTempTableName
%type createfunc_opt_item common_func_opt_item
***
*** 5349,5360
;
insert_rest:
! VALUES '(' insert_target_list ')'
{
$$ = makeNode(InsertStmt);
$$->cols = NIL;
! $$->targetList = $3;
! $$->selectStmt = NULL;
}
| DEFAULT VALUES
{
--- 5351,5370
;
insert_rest:
! VALUES insert_target_lists
{
$$ = makeNode(InsertStmt);
$$->cols = NIL;
! if (((SelectStmt *) $2)->op == SETOP_UNION)
! {
! $$->targetList = NIL;
! $$->selectStmt = $2;
! }
! else
! {
! $$->targetList = ((SelectStmt *) $2)->targetList;
! $$->selectStmt = NULL;
! }
}
| DEFAULT VALUES
{
***
*** 5370,5381
$$->targetList = NIL;
$$->selectStmt = $1;
}
! | '(' insert_column_list ')' VALUES '(' insert_target_list ')'
{
$$ = makeNode(InsertStmt);
$$->cols = $2;
! $$->targetList = $6;
! $$->selectStmt = NULL;
}
| '(' insert_column_list ')' SelectStmt
{
--- 5380,5399
$$->targetList = NIL;
$$->selectStmt = $1;
}
! | '(' insert_column_list ')' VALUES insert_target_lists
{
$$ = makeNode(InsertStmt);
$$->cols = $2;
! if (((SelectStmt *) $5)->op == SETOP_UNION)
! {
! $$->targetList = NIL;
! $$->selectStmt = $5;
! }
! else
! {
! $$->targetList = ((SelectStmt *) $5)->targetList;
! $$->selectStmt = NULL;
! }
}
| '(' insert_column_list ')' SelectStmt
{
***
*** 8189,8197
;
insert_target_list:
! insert_target_el { $$ = list_make1($1); }
! | insert_target_list ',' insert_target_el { $$ = lappend($1, $3); }
;
insert_target_el:
--- 8207,8235
;
+ insert_target_lists:
+ insert_target_list
+ {
+ $$ = $1;
+ }
+ | insert_target_lists ',' insert_target_list
+ {
+ $$ = makeSetOp(SETOP_UNION, TRUE, $1, $3);
+ }
+ ;
+
insert_target_list:
! '(' insert_target_els ')'
! {
! SelectStmt *n = makeNode(SelectStmt);
! n->targetList = $2;
! $$ = (Node *) n;
! }
! ;
!
! insert_target_els:
! insert_target_el { $$ = list_make1($1); }
! | insert_target_els ',' insert_target_el { $$ = lappend($1, $3); }
;
insert_target_el:
---(end of broadcast)---
TIP 4: Have you searched our list archives?
http://archives.postgresql.org
Re: [PATCHES] [HACKERS] kerberos related warning
Joe Conway wrote:
I just noticed this warning:
gcc -O -Wall -Wmissing-prototypes -Wpointer-arith -Winline
-Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -g
-pthread -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -fpic
-DFRONTEND -I. -I../../../src/include -D_GNU_SOURCE -I/usr/include/et
-I../../../src/port -c -o fe-auth.o fe-auth.c -MMD
fe-auth.c: In function 'pg_fe_getauthname':
fe-auth.c:573: warning: passing argument 1 of 'free' discards qualifiers
from pointer target type
I think the attached is the appropriate fix. Any objections?
(moved to patches)
Applied.
Joe
Index: src/interfaces/libpq/fe-auth.c
===
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/fe-auth.c,v
retrieving revision 1.115
diff -c -r1.115 fe-auth.c
*** src/interfaces/libpq/fe-auth.c 20 Jun 2006 19:56:52 - 1.115
--- src/interfaces/libpq/fe-auth.c 4 Jul 2006 17:27:15 -
***
*** 188,197
/*
! * pg_krb5_authname -- returns a pointer to static space containing whatever
! * name the user has authenticated to the system
! */
! static const char *
pg_krb5_authname(char *PQerrormsg)
{
char *tmp_name;
--- 188,197
/*
! * pg_krb5_authname -- returns a copy of whatever name the user
! * has authenticated to the system, or NULL
! */
! static char *
pg_krb5_authname(char *PQerrormsg)
{
char *tmp_name;
***
*** 520,526
pg_fe_getauthname(char *PQerrormsg)
{
#ifdef KRB5
! const char *krb5_name = NULL;
#endif
const char *name = NULL;
char *authn;
--- 520,526
pg_fe_getauthname(char *PQerrormsg)
{
#ifdef KRB5
! char *krb5_name = NULL;
#endif
const char *name = NULL;
char *authn;
---(end of broadcast)---
TIP 4: Have you searched our list archives?
http://archives.postgresql.org
Re: [PATCHES] [BUGS] BUG #2129: dblink problem
Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: Should I just accept that the cursor advances on a row type mismatch error, fix using the attached patch and adding SCROLL to dblink_open(), or something else? Any opinions out there? I would go with accepting (and documenting) the cursor advance. Trying to undo it seems too risky, and it's not like this is a situation that would arise in a properly-debugged application anyway. I can't see expending a great amount of effort on it --- protecting against a crash seems sufficient. OK -- applied back to 7.3. Thanks, Joe ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [BUGS] BUG #2129: dblink problem
Joe Conway wrote:
Joe Conway wrote:
However, there is a remaining oddity with dblink_fetch(). Basically,
each time dblink_fetch() is called, the named cursor is advanced, even
though an error is thrown before returning any rows. Is there a simple
way to get the number of columns in the result, without actually
advancing the cursor?
I thought I could work around this issue by obtaining the count returned
for the FETCH using PQcmdTuples(), and then issuing a "MOVE BACWARD
n..." in the case where the return tuple doesn't match. However I get an
empty string:
The attached seems to work OK, but I'm concerned about these passages
from the docs:
"SCROLL specifies that the cursor may be used to retrieve rows in a
nonsequential fashion (e.g., backward). Depending upon the complexity of
the query's execution plan, specifying SCROLL may impose a performance
penalty on the query's execution time. NO SCROLL specifies that the
cursor cannot be used to retrieve rows in a nonsequential fashion."
" The SCROLL option should be specified when defining a cursor that will
be used to fetch backwards. This is required by the SQL standard.
However, for compatibility with earlier versions, PostgreSQL will allow
backward fetches without SCROLL, if the cursor's query plan is simple
enough that no extra overhead is needed to support it. However,
application developers are advised not to rely on using backward fetches
from a cursor that has not been created with SCROLL. If NO SCROLL is
specified, then backward fetches are disallowed in any case."
So it seems, to fix the cursor issue properly I'd have to force the
SCROLL option to be used, thereby imposing a performance penalty.
Should I just accept that the cursor advances on a row type mismatch
error, fix using the attached patch and adding SCROLL to dblink_open(),
or something else? Any opinions out there?
Thanks,
Joe
? .deps
? current.diff
? dblink.sql
? libdblink.so.0.0
? reproduce-bug.sql
? results
Index: README.dblink
===
RCS file: /cvsroot/pgsql/contrib/dblink/README.dblink,v
retrieving revision 1.12
diff -c -r1.12 README.dblink
*** README.dblink 1 Jan 2005 20:44:11 - 1.12
--- README.dblink 3 Jan 2006 18:14:55 -
***
*** 8,14
* Darko Prenosil <[EMAIL PROTECTED]>
* Shridhar Daithankar <[EMAIL PROTECTED]>
*
! * Copyright (c) 2001-2005, PostgreSQL Global Development Group
* ALL RIGHTS RESERVED;
*
* Permission to use, copy, modify, and distribute this software and its
--- 8,14
* Darko Prenosil <[EMAIL PROTECTED]>
* Shridhar Daithankar <[EMAIL PROTECTED]>
*
! * Copyright (c) 2001-2006, PostgreSQL Global Development Group
* ALL RIGHTS RESERVED;
*
* Permission to use, copy, modify, and distribute this software and its
Index: dblink.c
===
RCS file: /cvsroot/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.50
diff -c -r1.50 dblink.c
*** dblink.c 22 Nov 2005 18:17:04 - 1.50
--- dblink.c 3 Jan 2006 18:14:56 -
***
*** 8,14
* Darko Prenosil <[EMAIL PROTECTED]>
* Shridhar Daithankar <[EMAIL PROTECTED]>
*
! * Copyright (c) 2001-2005, PostgreSQL Global Development Group
* ALL RIGHTS RESERVED;
*
* Permission to use, copy, modify, and distribute this software and its
--- 8,14
* Darko Prenosil <[EMAIL PROTECTED]>
* Shridhar Daithankar <[EMAIL PROTECTED]>
*
! * Copyright (c) 2001-2006, PostgreSQL Global Development Group
* ALL RIGHTS RESERVED;
*
* Permission to use, copy, modify, and distribute this software and its
***
*** 579,592
/* got results, keep track of them */
funcctx->user_fctx = res;
- /* fast track when no results */
- if (funcctx->max_calls < 1)
- {
- if (res)
- PQclear(res);
- SRF_RETURN_DONE(funcctx);
- }
-
/* get a tuple descriptor for our result type */
switch (get_call_result_type(fcinfo, NULL, &tupdesc))
{
--- 579,584
***
*** 609,614
--- 601,647
/* make sure we have a persistent copy of the tupdesc */
tupdesc = CreateTupleDescCopy(tupdesc);
+ /* check result and tuple descriptor have the same number of columns */
+ if (PQnfields(res) != tupdesc->natts)
+ {
+ if (funcctx->max_calls > 0)
+ {
+ StringInfo movestr = makeStringInfo();
+ int moveback;
+
+ if (howmany == funcctx->max_calls)
+ moveback = funcctx->max_calls;
+ else
+ moveback = funcctx->max_calls + 1;
+
+ appendStringInfo(movestr, "MOVE BACKWARD %d FROM %s", moveback, curname);
+ res = PQexec(conn, movestr->data);
+ if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
+ {
+
Re: [PATCHES] [BUGS] BUG #2129: dblink problem
Joe Conway wrote: However, there is a remaining oddity with dblink_fetch(). Basically, each time dblink_fetch() is called, the named cursor is advanced, even though an error is thrown before returning any rows. Is there a simple way to get the number of columns in the result, without actually advancing the cursor? I thought I could work around this issue by obtaining the count returned for the FETCH using PQcmdTuples(), and then issuing a "MOVE BACWARD n..." in the case where the return tuple doesn't match. However I get an empty string: (gdb) p str->data $34 = 0x8a4e5a8 "FETCH 2 FROM rmt_foo_cursor" (gdb) p PQcmdStatus(res) $35 = 0x8a447c8 "FETCH" (gdb) p PQcmdTuples(res) $36 = 0x29dada "" Any ideas why this isn't working? Thanks, Joe ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [BUGS] BUG #2129: dblink problem
Akio Iwaasa wrote:
The following bug has been logged online:
Bug reference: 2129
Logged by: Akio Iwaasa
"postgres" process terminated with "signal 11"
because of my wrong SQL statement using "dblink".
--- SQL statement(Select statement a function) ---
select into RET *
from dblink(''select C1,C2,C3 from TABLE01 where ... '') < 3 column
as LINK_TABLE01(LC1 varchar(5),LC2 varchar(5),
LC3 varchar(5),LC4 varchar(5)) ;< 4 column
The attached patch (against cvs HEAD) fixes the reported issue.
However, there is a remaining oddity with dblink_fetch(). Basically,
each time dblink_fetch() is called, the named cursor is advanced, even
though an error is thrown before returning any rows. Is there a simple
way to get the number of columns in the result, without actually
advancing the cursor?
If no one thinks the above is a problem, I'll commit the attached
against HEAD and stable branches back to 7.3.
Joe
Index: dblink.c
===
RCS file: /cvsroot/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.50
diff -c -r1.50 dblink.c
*** dblink.c 22 Nov 2005 18:17:04 - 1.50
--- dblink.c 3 Jan 2006 02:20:43 -
***
*** 579,592
/* got results, keep track of them */
funcctx->user_fctx = res;
- /* fast track when no results */
- if (funcctx->max_calls < 1)
- {
- if (res)
- PQclear(res);
- SRF_RETURN_DONE(funcctx);
- }
-
/* get a tuple descriptor for our result type */
switch (get_call_result_type(fcinfo, NULL, &tupdesc))
{
--- 579,584
***
*** 609,614
--- 601,621
/* make sure we have a persistent copy of the tupdesc */
tupdesc = CreateTupleDescCopy(tupdesc);
+ /* check result and tuple descriptor have the same number of columns */
+ if (PQnfields(res) != tupdesc->natts)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("remote query result rowtype does not match "
+ "the specified FROM clause rowtype")));
+
+ /* fast track when no results */
+ if (funcctx->max_calls < 1)
+ {
+ if (res)
+ PQclear(res);
+ SRF_RETURN_DONE(funcctx);
+ }
+
/* store needed metadata for subsequent calls */
attinmeta = TupleDescGetAttInMetadata(tupdesc);
funcctx->attinmeta = attinmeta;
***
*** 778,791
if (freeconn)
PQfinish(conn);
- /* fast track when no results */
- if (funcctx->max_calls < 1)
- {
- if (res)
- PQclear(res);
- SRF_RETURN_DONE(funcctx);
- }
-
if (!is_sql_cmd)
{
/* get a tuple descriptor for our result type */
--- 785,790
***
*** 811,816
--- 810,830
tupdesc = CreateTupleDescCopy(tupdesc);
}
+ /* check result and tuple descriptor have the same number of columns */
+ if (PQnfields(res) != tupdesc->natts)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("remote query result rowtype does not match "
+ "the specified FROM clause rowtype")));
+
+ /* fast track when no results */
+ if (funcctx->max_calls < 1)
+ {
+ if (res)
+ PQclear(res);
+ SRF_RETURN_DONE(funcctx);
+ }
+
/* store needed metadata for subsequent calls */
attinmeta = TupleDescGetAttInMetadata(tupdesc);
funcctx->attinmeta = attinmeta;
---(end of broadcast)---
TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about
Bruce Momjian wrote: Joe Conway wrote: Thanks for the review Tom -- as usual, great suggestions. The attached (simpler) patch makes use of your advice. If there are no objections, I'll apply this tomorrow evening. Looks good. Thanks. Committed. Joe ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about
Tom Lane wrote:
I think it would be shorter and clearer to write
remoteConn *remconn = NULL;
...
remconn = rconn;
...
remconn->newXactForCursor = TRUE;
Also, you might be able to combine this variable with the existing
rconn local variable and thus simplify the code even more.
Thanks for the review Tom -- as usual, great suggestions. The attached
(simpler) patch makes use of your advice. If there are no objections,
I'll apply this tomorrow evening.
Joe
Index: dblink.c
===
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.47
diff -c -r1.47 dblink.c
*** dblink.c 15 Oct 2005 02:49:04 - 1.47
--- dblink.c 17 Oct 2005 02:11:59 -
***
*** 60,68
typedef struct remoteConn
{
! PGconn *conn; /* Hold the remote connection */
! int autoXactCursors;/* Indicates the number of open cursors,
! * non-zero means we opened the xact ourselves */
} remoteConn;
/*
--- 60,68
typedef struct remoteConn
{
! PGconn *conn;/* Hold the remote connection */
! int openCursorCount; /* The number of open cursors */
! bool newXactForCursor; /* Opened a transaction for a cursor */
} remoteConn;
/*
***
*** 84,93
static char *generate_relation_name(Oid relid);
/* Global */
! List *res_id = NIL;
! int res_id_index = 0;
! PGconn *persistent_conn = NULL;
! static HTAB *remoteConnHash = NULL;
/*
* Following is list that holds multiple remote connections.
--- 84,91
static char *generate_relation_name(Oid relid);
/* Global */
! static remoteConn *pconn = NULL;
! static HTAB *remoteConnHash = NULL;
/*
* Following is list that holds multiple remote connections.
***
*** 184,189
--- 182,197
} \
} while (0)
+ #define DBLINK_INIT \
+ do { \
+ if (!pconn) \
+ { \
+ pconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext, sizeof(remoteConn)); \
+ pconn->conn = NULL; \
+ pconn->openCursorCount = 0; \
+ pconn->newXactForCursor = FALSE; \
+ } \
+ } while (0)
/*
* Create a persistent connection to another database
***
*** 199,204
--- 207,214
PGconn *conn = NULL;
remoteConn *rconn = NULL;
+ DBLINK_INIT;
+
if (PG_NARGS() == 2)
{
connstr = GET_STR(PG_GETARG_TEXT_P(1));
***
*** 234,240
createNewConnection(connname, rconn);
}
else
! persistent_conn = conn;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
--- 244,250
createNewConnection(connname, rconn);
}
else
! pconn->conn = conn;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
***
*** 250,255
--- 260,267
remoteConn *rconn = NULL;
PGconn *conn = NULL;
+ DBLINK_INIT;
+
if (PG_NARGS() == 1)
{
conname = GET_STR(PG_GETARG_TEXT_P(0));
***
*** 258,264
conn = rconn->conn;
}
else
! conn = persistent_conn;
if (!conn)
DBLINK_CONN_NOT_AVAIL;
--- 270,276
conn = rconn->conn;
}
else
! conn = pconn->conn;
if (!conn)
DBLINK_CONN_NOT_AVAIL;
***
*** 270,276
pfree(rconn);
}
else
! persistent_conn = NULL;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
--- 282,288
pfree(rconn);
}
else
! pconn->conn = NULL;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
***
*** 292,303
remoteConn *rconn = NULL;
bool fail = true; /* default to backward compatible behavior */
if (PG_NARGS() == 2)
{
/* text,text */
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
! conn = persistent_conn;
}
else if (PG_NARGS() == 3)
{
--- 304,317
remoteConn *rconn = NULL;
bool fail = true; /* default to backward compatible behavior */
+ DBLINK_INIT;
+
if (PG_NARGS() == 2)
{
/* text,text */
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
! rconn = pconn;
}
else if (PG_NARGS() == 3)
{
***
*** 307,313
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
fail = PG_GETARG_BOOL(2);
! conn = persistent_conn;
}
else
{
--- 321,327
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
fail = PG_GETARG_BOOL(2);
! rconn = pconn;
}
else
{
***
*** 315,322
curname = GET_STR(PG_GETARG_TEXT_P(1));
sql = GET_STR(PG_GETARG_TEXT_P(2));
rconn = getConnectionByName(conname);
- if (rconn)
- conn = rconn->conn;
}
}
else if (PG_NARGS() == 4)
--- 329,334
***
*** 327,344
sql = GET_STR(PG_GETARG_TEXT_P(2));
fail = PG_GETARG_BOOL(3);
rconn = getConnectionByName(conname);
- if (rconn)
- conn = rconn->conn;
}
! if (!conn)
DBLINK_CONN_
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about
Bruce Momjian wrote:
No problem -- thanks. I have slimmed down the patch by applying the
cosmetic parts to CVS. Use the URL above to get the newest versions of
the dblink.c and regression changes.
Here is my counter-proposal to Bruce's dblink patch. Any comments?
Is it too late to apply this for 8.1? I tend to agree with calling this
a bugfix.
Thanks,
Joe
Index: dblink.c
===
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.47
diff -c -r1.47 dblink.c
*** dblink.c 15 Oct 2005 02:49:04 - 1.47
--- dblink.c 16 Oct 2005 02:04:13 -
***
*** 60,68
typedef struct remoteConn
{
! PGconn *conn; /* Hold the remote connection */
! int autoXactCursors;/* Indicates the number of open cursors,
! * non-zero means we opened the xact ourselves */
} remoteConn;
/*
--- 60,68
typedef struct remoteConn
{
! PGconn *conn;/* Hold the remote connection */
! int openCursorCount; /* The number of open cursors */
! bool newXactForCursor; /* Opened a transaction for a cursor */
} remoteConn;
/*
***
*** 84,93
static char *generate_relation_name(Oid relid);
/* Global */
! List *res_id = NIL;
! int res_id_index = 0;
! PGconn *persistent_conn = NULL;
! static HTAB *remoteConnHash = NULL;
/*
* Following is list that holds multiple remote connections.
--- 84,91
static char *generate_relation_name(Oid relid);
/* Global */
! static remoteConn *pconn = NULL;
! static HTAB *remoteConnHash = NULL;
/*
* Following is list that holds multiple remote connections.
***
*** 184,189
--- 182,197
} \
} while (0)
+ #define DBLINK_INIT \
+ do { \
+ if (!pconn) \
+ { \
+ pconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext, sizeof(remoteConn)); \
+ pconn->conn = NULL; \
+ pconn->openCursorCount = 0; \
+ pconn->newXactForCursor = FALSE; \
+ } \
+ } while (0)
/*
* Create a persistent connection to another database
***
*** 199,204
--- 207,214
PGconn *conn = NULL;
remoteConn *rconn = NULL;
+ DBLINK_INIT;
+
if (PG_NARGS() == 2)
{
connstr = GET_STR(PG_GETARG_TEXT_P(1));
***
*** 234,240
createNewConnection(connname, rconn);
}
else
! persistent_conn = conn;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
--- 244,250
createNewConnection(connname, rconn);
}
else
! pconn->conn = conn;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
***
*** 250,255
--- 260,267
remoteConn *rconn = NULL;
PGconn *conn = NULL;
+ DBLINK_INIT;
+
if (PG_NARGS() == 1)
{
conname = GET_STR(PG_GETARG_TEXT_P(0));
***
*** 258,264
conn = rconn->conn;
}
else
! conn = persistent_conn;
if (!conn)
DBLINK_CONN_NOT_AVAIL;
--- 270,276
conn = rconn->conn;
}
else
! conn = pconn->conn;
if (!conn)
DBLINK_CONN_NOT_AVAIL;
***
*** 270,276
pfree(rconn);
}
else
! persistent_conn = NULL;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
--- 282,288
pfree(rconn);
}
else
! pconn->conn = NULL;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
***
*** 285,290
--- 297,304
char *msg;
PGresult *res = NULL;
PGconn *conn = NULL;
+ int *openCursorCount = NULL;
+ bool *newXactForCursor = NULL;
char *curname = NULL;
char *sql = NULL;
char *conname = NULL;
***
*** 292,303
remoteConn *rconn = NULL;
bool fail = true; /* default to backward compatible behavior */
if (PG_NARGS() == 2)
{
/* text,text */
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
! conn = persistent_conn;
}
else if (PG_NARGS() == 3)
{
--- 306,321
remoteConn *rconn = NULL;
bool fail = true; /* default to backward compatible behavior */
+ DBLINK_INIT;
+
if (PG_NARGS() == 2)
{
/* text,text */
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
! conn = pconn->conn;
! openCursorCount = &pconn->openCursorCount;
! newXactForCursor = &pconn->newXactForCursor;
}
else if (PG_NARGS() == 3)
{
***
*** 307,313
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
fail = PG_GETARG_BOOL(2);
! conn = persistent_conn;
}
else
{
--- 325,333
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
fail = PG_GETARG_BOOL(2);
! conn = pconn->conn;
! openCursorCount = &pconn->openCursorCount;
! newXactForCursor = &pconn->newXactForCursor;
}
else
{
***
*** 316,322
--- 336,346
sql = GET_STR(PG_GETARG_TEXT_P(2));
rconn = getConnectionByName(conname);
if (r
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about
Bruce Momjian wrote: There was also a problem in that if two cursors were opened, the first close would close the transaction. I have fixed that code by changing the xact variable in to a counter that keeps track of the number of opened cursors and commits only when they are all closed. Both the dblink.c patch and the regression patch are at: ftp://candle.pha.pa.us/pub/postgresql/mypatches OK, I'll take a look, but I won't have time for a couple of days (I'm not at home -- visiting my dad for his 80th birthday -- and have no broadband access). Joe ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about
Tom Lane wrote: David Fetter <[EMAIL PROTECTED]> writes: On Thu, Oct 06, 2005 at 10:38:54PM -0400, Bruce Momjian wrote: I don't know if people want this for 8.1 or 8.2. 8.1, IMHO. It's a bug fix. :) Not unless Joe Conway signs off on it ... I had asked on the original simple patch, and I'll ask again now -- why is this needed? I don't have a clear understanding of the problem that this (or the earlier) patch is trying to solve. Without that, it is impossible to say whether it is a bug fix or feature. Joe ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [SQL] ARRAY() returning NULL instead of ARRAY[] resp.
Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: + Oid element_type = planstate->ps_ResultTupleSlot->tts_tupleDescriptor->attrs[0]->atttypid; Hmm, that makes me itch ... it seems like unwarranted familiarity with the innards of the subplan; not only as to where it keeps things, but when things have been initialized. Perhaps we have no choice, but isn't the datatype available on the current plan level? I poked around a bit, and that was the best I could come up with. I'll take another look. Joe ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [SQL] ARRAY() returning NULL instead of ARRAY[] resp. {}
Joe Conway wrote:
OK, looks like I'm outnumbered.
But as far as I know, we have never had a way to produce a
one-dimensional empty array. Empty arrays thus far have been dimensionless.
Assuming we really want an empty 1D array, I created the attached patch.
This works fine, but now leaves a few oddities to be dealt with, e.g.:
regression=# select array_dims(array(select 1 where false));
array_dims
[1:0]
(1 row)
Any thoughts on how this should be handled for an empty 1D array?
Any thoughts or objections? If not, I'll commit the attached in a day or so.
Thanks,
Joe
Index: src/backend/executor/nodeSubplan.c
===
RCS file: /cvsroot/pgsql/src/backend/executor/nodeSubplan.c,v
retrieving revision 1.69
diff -c -r1.69 nodeSubplan.c
*** src/backend/executor/nodeSubplan.c 6 May 2005 17:24:54 - 1.69
--- src/backend/executor/nodeSubplan.c 26 May 2005 18:52:16 -
***
*** 215,220
--- 215,221
ListCell *pvar;
ListCell *l;
ArrayBuildState *astate = NULL;
+ Oid element_type =
planstate->ps_ResultTupleSlot->tts_tupleDescriptor->attrs[0]->atttypid;
/*
* We are probably in a short-lived expression-evaluation context.
***
*** 259,268
*
* For EXPR_SUBLINK we require the subplan to produce no more than one
* tuple, else an error is raised. For ARRAY_SUBLINK we allow the
!* subplan to produce more than one tuple. In either case, if zero
!* tuples are produced, we return NULL. Assuming we get a tuple, we
!* just use its first column (there can be only one non-junk column in
!* this case).
*/
result = BoolGetDatum(subLinkType == ALL_SUBLINK);
*isNull = false;
--- 260,269
*
* For EXPR_SUBLINK we require the subplan to produce no more than one
* tuple, else an error is raised. For ARRAY_SUBLINK we allow the
!* subplan to produce more than one tuple. In the former case, if zero
!* tuples are produced, we return NULL. In the latter, we return an
!* empty array. Assuming we get a tuple, we just use its first column
!* (there can be only one non-junk column in this case).
*/
result = BoolGetDatum(subLinkType == ALL_SUBLINK);
*isNull = false;
***
*** 432,458
}
}
! if (!found)
{
/*
* deal with empty subplan result. result/isNull were
previously
!* initialized correctly for all sublink types except EXPR,
ARRAY,
* and MULTIEXPR; for those, return NULL.
*/
if (subLinkType == EXPR_SUBLINK ||
- subLinkType == ARRAY_SUBLINK ||
subLinkType == MULTIEXPR_SUBLINK)
{
result = (Datum) 0;
*isNull = true;
}
}
- else if (subLinkType == ARRAY_SUBLINK)
- {
- Assert(astate != NULL);
- /* We return the result in the caller's context */
- result = makeArrayResult(astate, oldcontext);
- }
MemoryContextSwitchTo(oldcontext);
--- 433,459
}
}
! if (subLinkType == ARRAY_SUBLINK)
! {
! if (!astate)
! astate = initArrayResult(element_type, oldcontext);
! /* We return the result in the caller's context */
! result = makeArrayResult(astate, oldcontext);
! }
! else if (!found)
{
/*
* deal with empty subplan result. result/isNull were
previously
!* initialized correctly for all sublink types except EXPR
* and MULTIEXPR; for those, return NULL.
*/
if (subLinkType == EXPR_SUBLINK ||
subLinkType == MULTIEXPR_SUBLINK)
{
result = (Datum) 0;
*isNull = true;
}
}
MemoryContextSwitchTo(oldcontext);
***
*** 925,930
--- 926,932
ListCell *l;
boolfound = false;
ArrayBuildState *astate = NULL;
+ Oid element_type =
planstate->ps_ResultTupleSlot->tts_tupleDescriptor->attrs[0]->atttypid;
/*
* Must switch to child query's per-query memory context.
***
*** 1010,1016
}
}
! if (!found)
{
if (subLinkType == EXISTS_SUBLINK)
{
--- 1012,1033
}
}
! if (subLinkType == A
Re: [PATCHES] plpython win32
Magnus Hagander wrote: The distutils module has a get_python_inc() function which returns the include directory. If this one was used, we wouldn't have to hack up the include path as I do now. Is there any reason this is not used on Unix, instead of the hardcoded subdirectory-of-"python_prefix" way it is now? (in _PGAC_CHECK_PYTHON_DIRS) Probably because until about 2 weeks ago, we didn't check for, or use, distutils at all ;-). Now we probably should. Joe ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] x86_64 configure problem
Peter Eisentraut wrote:
Joe Conway wrote:
One procedural issue did occur to me regarding this kind of change.
It requires someone to run autoconf after applying -- how is that
normally handled?
You run autoconf before you commit and then check it in. Please use
version 2.53.
Thanks. Attached patch applied.
BTW, if anyone is interested in an autoconf253 source RPM that will
build on Fedora Core 2, let me know.
Joe
Index: configure
===
RCS file: /cvsroot/pgsql-server/configure,v
retrieving revision 1.393
diff -c -r1.393 configure
*** configure 11 Sep 2004 02:12:14 - 1.393
--- configure 16 Sep 2004 23:28:44 -
***
*** 4221,4232
fi
echo "$as_me:$LINENO: checking Python installation directories" >&5
echo $ECHO_N "checking Python installation directories... $ECHO_C" >&6
python_version=`${PYTHON} -c "import sys; print sys.version[:3]"`
python_prefix=`${PYTHON} -c "import sys; print sys.prefix"`
python_execprefix=`${PYTHON} -c "import sys; print sys.exec_prefix"`
! python_configdir="${python_execprefix}/lib/python${python_version}/config"
python_includespec="-I${python_prefix}/include/python${python_version}"
if test "$python_prefix" != "$python_execprefix"; then
python_includespec="-I${python_execprefix}/include/python${python_version} $python_includespec"
--- 4221,4245
fi
+ echo "$as_me:$LINENO: checking for Python distutils module" >&5
+ echo $ECHO_N "checking for Python distutils module... $ECHO_C" >&6
+ if "${PYTHON}" 2>&- -c 'import distutils'
+ then
+ echo "$as_me:$LINENO: result: yes" >&5
+ echo "${ECHO_T}yes" >&6
+ else
+ echo "$as_me:$LINENO: result: no" >&5
+ echo "${ECHO_T}no" >&6
+ { { echo "$as_me:$LINENO: error: distutils module not found" >&5
+ echo "$as_me: error: distutils module not found" >&2;}
+{ (exit 1); exit 1; }; }
+ fi
echo "$as_me:$LINENO: checking Python installation directories" >&5
echo $ECHO_N "checking Python installation directories... $ECHO_C" >&6
python_version=`${PYTHON} -c "import sys; print sys.version[:3]"`
python_prefix=`${PYTHON} -c "import sys; print sys.prefix"`
python_execprefix=`${PYTHON} -c "import sys; print sys.exec_prefix"`
! python_configdir=`${PYTHON} -c "from distutils.sysconfig import get_python_lib as f; import os; print os.path.join(f(plat_specific=1,standard_lib=1),'config')"`
python_includespec="-I${python_prefix}/include/python${python_version}"
if test "$python_prefix" != "$python_execprefix"; then
python_includespec="-I${python_execprefix}/include/python${python_version} $python_includespec"
Index: config/python.m4
===
RCS file: /cvsroot/pgsql-server/config/python.m4,v
retrieving revision 1.7
diff -c -r1.7 python.m4
*** config/python.m4 29 Nov 2003 19:51:17 - 1.7
--- config/python.m4 16 Sep 2004 23:28:44 -
***
*** 21,31
# Determine the name of various directory of a given Python installation.
AC_DEFUN([_PGAC_CHECK_PYTHON_DIRS],
[AC_REQUIRE([PGAC_PATH_PYTHON])
AC_MSG_CHECKING([Python installation directories])
python_version=`${PYTHON} -c "import sys; print sys.version[[:3]]"`
python_prefix=`${PYTHON} -c "import sys; print sys.prefix"`
python_execprefix=`${PYTHON} -c "import sys; print sys.exec_prefix"`
! python_configdir="${python_execprefix}/lib/python${python_version}/config"
python_includespec="-I${python_prefix}/include/python${python_version}"
if test "$python_prefix" != "$python_execprefix"; then
python_includespec="-I${python_execprefix}/include/python${python_version} $python_includespec"
--- 21,39
# Determine the name of various directory of a given Python installation.
AC_DEFUN([_PGAC_CHECK_PYTHON_DIRS],
[AC_REQUIRE([PGAC_PATH_PYTHON])
+ AC_MSG_CHECKING([for Python distutils module])
+ if "${PYTHON}" 2>&- -c 'import distutils'
+ then
+ AC_MSG_RESULT(yes)
+ else
+ AC_MSG_RESULT(no)
+ AC_MSG_ERROR([distutils module not found])
+ fi
AC_MSG_CHECKING([Python installation directories])
python_version=`${PYTHON} -c "import sys; print sys.version[[:3]]"`
python_prefix=`${PYTHON} -c "import sys; print sys.prefix"`
python_execprefix=`${PYTHON} -c "import sys; print sys.exec_prefix"`
! python_configdir=`${PYTHON} -c "from distutils.sysconfig import get_python_lib as f; import os; print os.path.join(f(plat_specific=1,standard_lib=1),'config')"`
python_includespec="-I${python_prefix}/include/python${python_version}"
if test "$python_prefix" != "$python_execprefix"; then
python_includespec="-I${python_execprefix}/include/python${python_version} $python_includespec"
---(end of broadcast)---
TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] arrayfuncs: fix read of uninitialized mem
Neil Conway wrote: Barring any objections, I'll apply the patch within 24 hours. *** *** 965,978 * (including any overhead such as escaping backslashes), and detect * whether each item needs double quotes. */ ! values = (char **) palloc(nitems * sizeof(char *)); ! needquotes = (bool *) palloc(nitems * sizeof(bool)); --- 965,978 * (including any overhead such as escaping backslashes), and detect * whether each item needs double quotes. */ ! values = (char **) palloc(nitems * sizeof(*values)); ! needquotes = (bool *) palloc(nitems * sizeof(*needquotes)); Personally I prefer the original style here. And I agree with Tom's nearby comments. But otherwise looks good to me. Joe ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] [SQL] array_in: '{}}'::text[]
Markus Bertheau wrote:
Without looking at the code in a whole, you accept '{} ' as an empty
array literal, so why is the special case for '{}' needed here?
It's a fast path for a common special case. Why spend any cycles parsing
if we can immediately recognize it? However, anything other than a
simple '{}' does require parsing.
Joe
---(end of broadcast)---
TIP 8: explain analyze is your friend
Re: [PATCHES] [SQL] array_in: '{}}'::text[]
Tom Lane wrote:
actually, why isn't this just a pstrdup?
Why not just if (strcmp(str, "{}") == 0)
Good points. Changes made, and attached committed.
Joe
Index: src/backend/utils/adt/arrayfuncs.c
===
RCS file: /cvsroot/pgsql-server/src/backend/utils/adt/arrayfuncs.c,v
retrieving revision 1.107
diff -c -r1.107 arrayfuncs.c
*** src/backend/utils/adt/arrayfuncs.c 8 Aug 2004 05:01:55 - 1.107
--- src/backend/utils/adt/arrayfuncs.c 28 Aug 2004 19:19:22 -
***
*** 183,191
typioparam = my_extra->typioparam;
/* Make a modifiable copy of the input */
! /* XXX why are we allocating an extra 2 bytes here? */
! string_save = (char *) palloc(strlen(string) + 3);
! strcpy(string_save, string);
/*
* If the input string starts with dimension info, read and use that.
--- 183,189
typioparam = my_extra->typioparam;
/* Make a modifiable copy of the input */
! string_save = pstrdup(string);
/*
* If the input string starts with dimension info, read and use that.
***
*** 375,380
--- 373,379
nelems_last[MAXDIM];
bool scanning_string = false;
bool eoArray = false;
+ bool empty_array = true;
char *ptr;
ArrayParseState parse_state = ARRAY_NO_LEVEL;
***
*** 385,391
}
/* special case for an empty array */
! if (strncmp(str, "{}", 2) == 0)
return 0;
ptr = str;
--- 384,390
}
/* special case for an empty array */
! if (strcmp(str, "{}") == 0)
return 0;
ptr = str;
***
*** 395,400
--- 394,403
while (!itemdone)
{
+ if (parse_state == ARRAY_ELEM_STARTED ||
+ parse_state == ARRAY_QUOTED_ELEM_STARTED)
+ empty_array = false;
+
switch (*ptr)
{
case '\0':
***
*** 481,487
if (parse_state != ARRAY_ELEM_STARTED &&
parse_state != ARRAY_ELEM_COMPLETED &&
parse_state != ARRAY_QUOTED_ELEM_COMPLETED &&
! parse_state != ARRAY_LEVEL_COMPLETED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("malformed array literal: \"%s\"", str)));
--- 484,491
if (parse_state != ARRAY_ELEM_STARTED &&
parse_state != ARRAY_ELEM_COMPLETED &&
parse_state != ARRAY_QUOTED_ELEM_COMPLETED &&
! parse_state != ARRAY_LEVEL_COMPLETED &&
! !(nest_level == 1 && parse_state == ARRAY_LEVEL_STARTED))
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("malformed array literal: \"%s\"", str)));
***
*** 562,567
--- 566,585
temp[ndim - 1]++;
ptr++;
}
+
+ /* only whitespace is allowed after the closing brace */
+ while (*ptr)
+ {
+ if (!isspace(*ptr++))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("malformed array literal: \"%s\"", str)));
+ }
+
+ /* special case for an empty array */
+ if (empty_array)
+ return 0;
+
for (i = 0; i < ndim; ++i)
dim[i] = temp[i];
Index: src/test/regress/expected/arrays.out
===
RCS file: /cvsroot/pgsql-server/src/test/regress/expected/arrays.out,v
retrieving revision 1.22
diff -c -r1.22 arrays.out
*** src/test/regress/expected/arrays.out 5 Aug 2004 03:30:03 - 1.22
--- src/test/regress/expected/arrays.out 28 Aug 2004 19:19:22 -
***
*** 425,427
--- 425,485
t
(1 row)
+ --
+ -- General array parser tests
+ --
+ -- none of the following should be accepted
+ select '{{1,{2}},{2,3}}'::text[];
+ ERROR: malformed array literal: "{{1,{2}},{2,3}}"
+ select '{{},{}}'::text[];
+ ERROR: malformed array literal: "{{},{}}"
+ select '{{1,2},\\{2,3}}'::text[];
+ ERROR: malformed array literal: "{{1,2},\{2,3}}"
+ select '{{"1 2" x},{3}}'::text[];
+ ERROR: malformed array literal: "{{"1 2" x},{3}}"
+ select '{}}'::text[];
+ ERROR: malformed array literal: "{}}"
+ select '{ }}'::text[];
+ ERROR: malformed array literal: "{ }}"
+ -- none of the above should be accepted
+ -- all of the following should be accepted
+ select '{}'::text[];
+ text
+ --
+ {}
+ (1 row)
+
+ select '{{{1,2,3,4},{2,3,4,5}},{{3,4,5,6},{4,5,6,7}}}'::text[];
+ text
+ ---
+ {{{1,2,3,4},{2,3,4,5}},{{3,4,5,6},{4,5,6,7}}}
+ (1 row)
+
+ select '{0 second ,0 second}'::interval[];
+interval
+ ---
+ {"@ 0","@ 0"}
+ (1 row)
+
+ select '{ { "," } , { 3 } }'::text[];
+ text
+ -
+ {{","},{3}}
+ (1 row)
+
+ select ' { { " 0 second " , 0 second } }'::text[];
+ text
+ ---
+ {{" 0 second ","0 second"}}
+ (1 row)
+
+ select '{
+0 second,
+@ 1 hour @ 42 minutes @ 20 seconds
+ }'::interv
Re: [PATCHES] [SQL] array_in: '{}}'::text[]
Joe Conway wrote:
Markus Bertheau wrote:
Is there a reason the array_in parser accepts additional closing braces
at the end?
oocms=# SELECT '{}}'::text[];
text
--
{}
(1 ÑÑ)
Hmmm, I was *about* to say that this is fixed in cvs (and indeed, the
array_in parser is significantly tightened up compared to previous
releases), but unfortunately, there is still work to be done :(
The attached patch takes care of the above issue:
regression=# SELECT '{}}'::text[];
ERROR: malformed array literal: "{}}"
If there are no objections, I'll apply in about 24 hours.
Joe
Index: src/backend/utils/adt/arrayfuncs.c
===
RCS file: /cvsroot/pgsql-server/src/backend/utils/adt/arrayfuncs.c,v
retrieving revision 1.107
diff -c -r1.107 arrayfuncs.c
*** src/backend/utils/adt/arrayfuncs.c 8 Aug 2004 05:01:55 - 1.107
--- src/backend/utils/adt/arrayfuncs.c 27 Aug 2004 20:31:46 -
***
*** 183,190
typioparam = my_extra->typioparam;
/* Make a modifiable copy of the input */
! /* XXX why are we allocating an extra 2 bytes here? */
! string_save = (char *) palloc(strlen(string) + 3);
strcpy(string_save, string);
/*
--- 183,189
typioparam = my_extra->typioparam;
/* Make a modifiable copy of the input */
! string_save = (char *) palloc0(strlen(string) + 1);
strcpy(string_save, string);
/*
***
*** 375,380
--- 374,380
nelems_last[MAXDIM];
bool scanning_string = false;
bool eoArray = false;
+ bool empty_array = true;
char *ptr;
ArrayParseState parse_state = ARRAY_NO_LEVEL;
***
*** 385,391
}
/* special case for an empty array */
! if (strncmp(str, "{}", 2) == 0)
return 0;
ptr = str;
--- 385,391
}
/* special case for an empty array */
! if (strlen(str) == 2 && strncmp(str, "{}", 2) == 0)
return 0;
ptr = str;
***
*** 395,400
--- 395,404
while (!itemdone)
{
+ if (parse_state == ARRAY_ELEM_STARTED ||
+ parse_state == ARRAY_QUOTED_ELEM_STARTED)
+ empty_array = false;
+
switch (*ptr)
{
case '\0':
***
*** 481,487
if (parse_state != ARRAY_ELEM_STARTED &&
parse_state != ARRAY_ELEM_COMPLETED &&
parse_state != ARRAY_QUOTED_ELEM_COMPLETED &&
! parse_state != ARRAY_LEVEL_COMPLETED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("malformed array literal: \"%s\"", str)));
--- 485,492
if (parse_state != ARRAY_ELEM_STARTED &&
parse_state != ARRAY_ELEM_COMPLETED &&
parse_state != ARRAY_QUOTED_ELEM_COMPLETED &&
! parse_state != ARRAY_LEVEL_COMPLETED &&
! !(nest_level == 1 && parse_state == ARRAY_LEVEL_STARTED))
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("malformed array literal: \"%s\"", str)));
***
*** 562,567
--- 567,586
temp[ndim - 1]++;
ptr++;
}
+
+ /* only whitespace is allowed after the closing brace */
+ while (*ptr)
+ {
+ if (!isspace(*ptr++))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("malformed array literal: \"%s\"", str)));
+ }
+
+ /* special case for an empty array */
+ if (empty_array)
+ return 0;
+
for (i = 0; i < ndim; ++i)
dim[i] = temp[i];
Index: src/test/regress/expected/arrays.out
===
RCS file: /cvsroot/pgsql-server/src/test/regress/expected/arrays.out,v
retrieving revision 1.22
diff -c -r1.22 arrays.out
*** src/test/regress/expected/arrays.out 5 Aug 2004 03:30:03 - 1.22
--- src/test/regress/expected/arrays.out 27 Aug 2004 20:31:46 -
***
*** 425,427
--- 425,485
t
(1 row)
+ --
+ -- General array parser tests
+ --
+ -- none of the following should be accepted
+ select '{{1,{2}},{2,3}}'::text[];
+ ERROR: malformed array literal: "{{1,{2}},{2,3}}"
+ select '{{},{}}'::text[];
+ ERROR: malformed array literal: "{{},{}}"
+ select '{{1,2},\\{2,3}}'::text[];
+ ERROR: malformed array literal: "{{1,2},\{2,3}}"
+ select '{{"1 2" x},{3}}'::text[];
+ ERROR: malformed array literal: "{{"1 2" x},{3}}"
+ select '{}}'::text[];
+ ERROR: malformed array literal: "{}}"
+ select '{ }}'::text[];
+ ERROR: malformed array literal: "{ }}"
+ -- none of the above should be accepted
+ -- all of the following should be accepted
+ select '{}'::text[];
+ text
+ --
+ {}
+ (1 row)
+
+ select
Re: [PATCHES] Patch for Array min() / max()
Bruce Momjian wrote: May I have a context diff please, diff -c? As this is new functionality, I presume it will be held for 8.1, correct? In any case, you can put my name on it for review. Joe ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] [BUGS] casting strings to multidimensional arrays yields strange
Tom Lane wrote:
Joe Conway <[EMAIL PROTECTED]> writes:
While looking at it the last day or so, I started to think it might be
better to use bison to parse array literals -- or is that a bad idea?
Offhand it doesn't seem like a super-appropriate tool. Once you get
past the lexical details like quoting, the syntax of array literals
is not complicated enough to need a bison parser. Also, the issues
you're facing now like enforcing consistent dimensions are not amenable
to solution by a context-free grammar --- so you'd still need most of
the dimension-checking mechanisms.
I'm hesitant to apply the attached this late before the beta without
review, but it seems to take care of the pathological cases I came up
with, doesn't break anything AFAICS, and passes all regression tests. I
guess it can go into beta 2.
Joe
Index: src/backend/utils/adt/arrayfuncs.c
===
RCS file: /cvsroot/pgsql-server/src/backend/utils/adt/arrayfuncs.c,v
retrieving revision 1.106
diff -c -r1.106 arrayfuncs.c
*** src/backend/utils/adt/arrayfuncs.c 5 Aug 2004 03:29:37 - 1.106
--- src/backend/utils/adt/arrayfuncs.c 5 Aug 2004 05:50:07 -
***
*** 351,368
* The syntax for array input is C-like nested curly braces
*-
*/
static int
ArrayCount(char *str, int *dim, char typdelim)
{
! int nest_level = 0,
! i;
! int ndim = 1,
! temp[MAXDIM],
! nelems[MAXDIM],
! nelems_last[MAXDIM];
! bool scanning_string = false;
! bool eoArray = false;
! char *ptr;
for (i = 0; i < MAXDIM; ++i)
{
--- 351,378
* The syntax for array input is C-like nested curly braces
*-
*/
+ typedef enum
+ {
+ ARRAY_NO_LEVEL,
+ ARRAY_LEVEL_STARTED,
+ ARRAY_ELEM_STARTED,
+ ARRAY_LEVEL_COMPLETED,
+ ARRAY_LEVEL_DELIMITED
+ } ArrayParseState;
+
static int
ArrayCount(char *str, int *dim, char typdelim)
{
! intnest_level = 0,
! i;
! intndim = 1,
! temp[MAXDIM],
! nelems[MAXDIM],
! nelems_last[MAXDIM];
! bool scanning_string = false;
! bool eoArray = false;
! char *ptr;
! ArrayParseState parse_state = ARRAY_NO_LEVEL;
for (i = 0; i < MAXDIM; ++i)
{
***
*** 389,394
--- 399,416
errmsg("malformed array literal: \"%s\"", str)));
break;
case '\\':
+ /*
+ * An escape must be after a level start, within an
+ * element, or after a delimiter. In any case
+ * we now must be past an element start.
+ */
+ if (parse_state != ARRAY_LEVEL_STARTED &&
+ parse_state != ARRAY_ELEM_STARTED &&
+ parse_state != ARRAY_LEVEL_DELIMITED)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("malformed array literal: \"%s\"", str)));
+ parse_state = ARRAY_ELEM_STARTED;
/* skip the escaped character */
if (*(ptr + 1))
ptr++;
***
*** 398,408
--- 420,454
errmsg("malformed array literal: \"%s\"", str)));
break;
case '\"':
+ /*
+ * A quote must be after a level start, within an
+ * element, or after a delimiter. In any case
+ * we now must be past an element start.
+ */
+ if (parse_state != ARRAY_LEVEL_STARTED &&
+ parse_state != ARRAY_ELEM_STARTED &&
+ parse_state != ARRAY_LEVEL_DELIMITED)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("malformed array literal: \"%s\"", str)));
+ parse_state = ARRAY_ELEM_STARTED;
scanning_string = !scanning_string;
break;
case '{':
if (!scanning_string)
{
+ /*
+ * A left brace can occur if no nesting has
+ * occurred yet, after a level start, or
+ * after a delimiter.
+ */
+ if (parse_state != ARRAY_NO_LEVEL &&
+ parse_state != ARRAY_LEVEL_STARTED &&
+ parse_state != ARRAY_LEVEL_DELIMITED)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("malformed array literal: \"%s\"", str)));
+ parse_state = ARRAY_LEVEL_STARTED;
if (nest_level >= MAXDIM)
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
***
*** 417,422
--- 463,480
case '}':
if (!scanning_string)
{
+ /*
+ * A right brace can occur after a level start,
+ * after an element start, or after a level
+ * completion.
+ */
+ if (parse_state != ARRAY_LEVEL_STARTED &&
+ par
Re: [PATCHES] [BUGS] casting strings to multidimensional arrays yields strange
Tom Lane wrote: I had pretty much come to the conclusion that the array_in code should be thrown away and rewritten from the ground up --- whoever wrote it originally had no calling as a programmer :-(. I didn't look at the details of your patch, but I have little faith in half measures in this area. I've already wasted a lot of time trying to patch that code in past releases. Agreed, but I don't think I'll be able to get that done tonight. While looking at it the last day or so, I started to think it might be better to use bison to parse array literals -- or is that a bad idea? In any case, I'm planning to find the time to work on NULL array elements as soon as we branch 8.0 from head. At that time I'll also look at cleaning up array_in (and friends undoubtedly). Joe ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] [HACKERS] compile warnings
Joe Conway wrote: Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: In addition to the ecpg warnings mentioned by Tom, I'm also seeing compile warnings wrt plpython: This is surely not a "must fix tomorrow" issue, but please look into it when you get back from your road trip. I find that simply putting #include prior to #include "postgres.h" in plpython.c eliminates the warnings, and compiles fine, but it isn't clear to me that it is safe. Thoughts? If there are no objections, I plan to commit the attached in a few hours. Thanks, Joe Index: src/pl/plpython/plpython.c === RCS file: /cvsroot/pgsql-server/src/pl/plpython/plpython.c,v retrieving revision 1.51 diff -c -r1.51 plpython.c *** src/pl/plpython/plpython.c 31 Jul 2004 20:55:45 - 1.51 --- src/pl/plpython/plpython.c 4 Aug 2004 22:41:44 - *** *** 34,39 --- 34,40 * */ + #include #include "postgres.h" /* system stuff */ *** *** 54,60 #include "utils/syscache.h" #include "utils/typcache.h" - #include #include #include --- 55,60 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] [BUGS] casting strings to multidimensional arrays yields strange
Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: If there are no objections, I'll adjust the appropriate regression script/expected files and commit tonight. And of course I'll follow up with documentation updates too. BTW, I believe the plan is to wrap 8.0beta1 tomorrow, so please do get this in tonight if you can get the docs updates done too. (Otherwise it might be better to wait till the docs are done.) OK, will do. I ought to be able to finish it up today and commit tonight. Thanks, Joe ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [BUGS] casting strings to multidimensional arrays yields strange
Tom Lane wrote:
[ cc'ing pghackers in case anyone wants to object ]
Joe Conway <[EMAIL PROTECTED]> writes:
I think that even once we support NULL array elements, they should be
explicitly requested -- i.e. throwing an error on non-rectangular input
is still the right thing to do. I haven't suggested that in the past
because of the backward-compatibility issue, but maybe now is the time
to bite the bullet.
Okay with me. Anyone on pghackers not happy?
If you think this qualifies as a bug fix for 7.5, I can take a look at
it next week.
Yeah, we can call it a bug fix.
The attached addresses the above issue as well as the ones mentioned in
my RFC post from yesterday found here:
http://archives.postgresql.org/pgsql-hackers/2004-08/msg00126.php
So whereas you used to get this:
test=# select '{{{1,2},{4,5,6}},{7,8}}'::int[];
int4
--
{}
(1 row)
you now get this:
test=# select '{{{1,2},{4,5,6}},{7,8}}'::int[];
ERROR: multidimensional arrays must have array expressions with
matching dimensions
Negative lower bound indicies now work also, and array_out will always
output explicit dimensions for an array with any dimension having a
lower bound of other than one. This allows the dimensions to be
preserved across pg_dump/reload cycles:
CREATE TABLE foo (
f1 integer[]
);
COPY foo (f1) FROM stdin;
[1:1][3:4][-2:0]={{{1,2,3},{4,5,6}}}
[-42:-41][12:14]={{1,2,3},{4,5,6}}
\.
test=# select f1, array_lower(f1, 1) as lb1, array_lower(f1, 2) as lb2,
array_lower(f1, 3) as lb3 from foo;
f1 | lb1 | lb2 | lb3
--+-+-+-
[1:1][3:4][-2:0]={{{1,2,3},{4,5,6}}} | 1 | 3 | -2
[-42:-41][12:14]={{1,2,3},{4,5,6}} | -42 | 12 |
(2 rows)
If there are no objections, I'll adjust the appropriate regression
script/expected files and commit tonight. And of course I'll follow up
with documentation updates too.
Any thoughts on whether or not to apply this to 7.4?
Thanks,
Joe
Index: src/backend/utils/adt/arrayfuncs.c
===
RCS file: /cvsroot/pgsql-server/src/backend/utils/adt/arrayfuncs.c,v
retrieving revision 1.105
diff -c -r1.105 arrayfuncs.c
*** src/backend/utils/adt/arrayfuncs.c 16 Jun 2004 01:26:47 - 1.105
--- src/backend/utils/adt/arrayfuncs.c 4 Aug 2004 15:37:34 -
***
*** 217,223
errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
ndim, MAXDIM)));
! for (q = p; isdigit((unsigned char) *q); q++);
if (q == p)/* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
--- 217,223
errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
ndim, MAXDIM)));
! for (q = p; isdigit((unsigned char) *q) || (*q == '-') || (*q == '+'); q++);
if (q == p)/* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
***
*** 229,235
*q = '\0';
lBound[ndim] = atoi(p);
p = q + 1;
! for (q = p; isdigit((unsigned char) *q); q++);
if (q == p) /* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
--- 229,235
*q = '\0';
lBound[ndim] = atoi(p);
p = q + 1;
! for (q = p; isdigit((unsigned char) *q) || (*q == '-') || (*q == '+'); q++);
if (q == p) /* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
***
*** 270,275
--- 270,278
}
else
{
+ int ndim_braces,
+ dim_braces[MAXDIM];
+
/* If array dimensions are given, expect '=' operator */
if (strncmp(p, ASSGN, strlen(ASSGN)) != 0)
ereport(ERROR,
***
*** 278,283
--- 281,307
p += strlen(ASSGN);
while (isspace((unsigned char) *p))
p++;
+
+ /*
+ * intuit dimensions from brace structure -- it better match what
+ * we were given
+ */
+ if (*p != '{')
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("array value must start with \"{\" or dimension information")));
+ ndim_braces = ArrayCount(p, dim_braces, typdelim);
+ if (ndim_braces != ndim)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("array dimensions incompatible with array literal")));
+ for (i = 0; i < ndim; ++i)
+ {
+ if (dim[i] != dim_braces[i])
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("array dimensions incompatible with array literal")));
+ }
}
#ifdef ARRAYDEBUG
***
*** 303,309
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("missing left brace")));
-
Re: [PATCHES] Another transation fix
Joe Conway wrote:
Bruce Momjian wrote:
Here is another try at fixing the translation message. Instead of
removing the backslashes in the message, I escaped them. Per discussion
with Joe Conway.
Now I'm getting three errors instead of one:
msgfmt -o po/zh_TW.mo po/zh_TW.po
po/zh_TW.po:199: `msgid' and `msgstr' entries do not both end with '\n'
po/zh_TW.po:260:40: invalid control sequence
po/zh_TW.po:471:11: invalid control sequence
msgfmt: found 3 fatal errors
FWIW, I can compile with the attached patch.
Joe
? src/bin/initdb/po/de.mo
? src/bin/initdb/po/fr.mo
? src/bin/initdb/po/it.mo
? src/bin/initdb/po/pt_BR.mo
? src/bin/initdb/po/ru.mo
? src/bin/initdb/po/sv.mo
? src/bin/initdb/po/zh_TW.mo
Index: src/bin/initdb/po/zh_TW.po
===
RCS file: /cvsroot/pgsql-server/src/bin/initdb/po/zh_TW.po,v
retrieving revision 1.3
diff -c -r1.3 zh_TW.po
*** src/bin/initdb/po/zh_TW.po 31 Jul 2004 20:00:26 - 1.3
--- src/bin/initdb/po/zh_TW.po 31 Jul 2004 22:02:59 -
***
*** 197,203
#: initdb.c:1864
msgid "ok\n"
! msgstr "¦¨¥\\\n"
#: initdb.c:1894
#, c-format
--- 197,203
#: initdb.c:1864
msgid "ok\n"
! msgstr "¦¨¥\\n"
#: initdb.c:1894
#, c-format
***
*** 257,263
#: initdb.c:1973
msgid " --no-locale equivalent to --locale=C\n"
! msgstr " --no-locale ¥\\¯à¦P --locale=C\n"
#: initdb.c:1974
msgid " -U, --username=NAME database superuser name\n"
--- 257,263
#: initdb.c:1973
msgid " --no-locale equivalent to --locale=C\n"
! msgstr " --no-locale ¥\¯à¦P --locale=C\n"
#: initdb.c:1974
msgid " -U, --username=NAME database superuser name\n"
***
*** 468,474
"\n"
msgstr ""
"\n"
! "°õ¦æ¦¨¥\\¡A²{¦b§A¥i¥H¥Î¤U¦C©R¥O±Ò°Ê¸ê®Æ®w¦øªA¾¹:\n"
"\n"
"%s%s%s/postmaster -D %s%s%s\n"
"©Î\n"
--- 468,474
"\n"
msgstr ""
"\n"
! "°õ¦æ¦¨¥\¡A²{¦b§A¥i¥H¥Î¤U¦C©R¥O±Ò°Ê¸ê®Æ®w¦øªA¾¹:\n"
"\n"
"%s%s%s/postmaster -D %s%s%s\n"
"©Î\n"
---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] Another transation fix
Bruce Momjian wrote: Here is another try at fixing the translation message. Instead of removing the backslashes in the message, I escaped them. Per discussion with Joe Conway. Now I'm getting three errors instead of one: msgfmt -o po/zh_TW.mo po/zh_TW.po po/zh_TW.po:199: `msgid' and `msgstr' entries do not both end with '\n' po/zh_TW.po:260:40: invalid control sequence po/zh_TW.po:471:11: invalid control sequence msgfmt: found 3 fatal errors Not sure if it is relevant, but here are my locale related environment variables: LANG=C LANGUAGE=C LC_ALL=C This is on a Fedora core 2 machine. Anyone have any ideas how to properly fix this? Thanks, Joe ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] pg_tablespace_databases
Joe Conway wrote:
Tom Lane wrote:
[ shrug... ] The name is not going to change again. I have never cared
for the practice of writing strlen("foo") as if it were a compile-time
constant. But certainly it would be entirely pointless to define such a
macro and then use it in only one place.
Fair enough. If there are no objections, I'll apply the attached patch
in a few hours.
patch applied.
Joe
---(end of broadcast)---
TIP 6: Have you searched our list archives?
http://archives.postgresql.org
Re: [PATCHES] pg_tablespace_databases
Tom Lane wrote:
[ shrug... ] The name is not going to change again. I have never cared
for the practice of writing strlen("foo") as if it were a compile-time
constant. But certainly it would be entirely pointless to define such a
macro and then use it in only one place.
Fair enough. If there are no objections, I'll apply the attached patch
in a few hours.
Joe
Index: doc/src/sgml/func.sgml
===
RCS file: /cvsroot/pgsql-server/doc/src/sgml/func.sgml,v
retrieving revision 1.211
diff -c -r1.211 func.sgml
*** doc/src/sgml/func.sgml 25 Jun 2004 17:20:21 - 1.211
--- doc/src/sgml/func.sgml 2 Jul 2004 15:54:45 -
***
*** 7232,7237
--- 7232,7241
pg_get_serial_sequence
+
+ pg_tablespace_databases
+
+
lists functions that
extract information from the system catalogs.
***
*** 7325,7330
--- 7329,7339
get name of the sequence that a serial or bigserial column
uses
+
+pg_tablespace_databases(tablespace_oid)
+setof oid
+get set of database oids that have objects in the tablespace
+
***
*** 7360,7365
--- 7369,7384
for passing to the sequence functions (see ).
NULL is returned if the column does not have a sequence attached.
+
+
+
+ pg_tablespace_databases allows usage examination of a
+ tablespace. It will return a set of database oids, that have objects
+ stored in the tablespace. If this function returns any row, the
+ tablespace is assumed not to be empty and cannot be dropped. To
+ display the actual objects populating the tablespace, you will need
+ to connect to the databases returned by
+ pg_tablespace_databases to query pg_class.
Index: src/backend/commands/tablespace.c
===
RCS file: /cvsroot/pgsql-server/src/backend/commands/tablespace.c,v
retrieving revision 1.4
diff -c -r1.4 tablespace.c
*** src/backend/commands/tablespace.c 25 Jun 2004 21:55:53 - 1.4
--- src/backend/commands/tablespace.c 2 Jul 2004 15:54:46 -
***
*** 315,321
/*
* All seems well, create the symlink
*/
! linkloc = (char *) palloc(strlen(DataDir) + 16 + 10 + 1);
sprintf(linkloc, "%s/pg_tblspc/%u", DataDir, tablespaceoid);
if (symlink(location, linkloc) < 0)
--- 315,321
/*
* All seems well, create the symlink
*/
! linkloc = (char *) palloc(strlen(DataDir) + 11 + 10 + 1);
sprintf(linkloc, "%s/pg_tblspc/%u", DataDir, tablespaceoid);
if (symlink(location, linkloc) < 0)
Index: src/backend/utils/adt/misc.c
===
RCS file: /cvsroot/pgsql-server/src/backend/utils/adt/misc.c,v
retrieving revision 1.34
diff -c -r1.34 misc.c
*** src/backend/utils/adt/misc.c 2 Jun 2004 21:29:29 - 1.34
--- src/backend/utils/adt/misc.c 2 Jul 2004 15:54:46 -
***
*** 16,26
--- 16,31
#include
#include
+ #include
#include "commands/dbcommands.h"
#include "miscadmin.h"
#include "storage/sinval.h"
+ #include "storage/fd.h"
#include "utils/builtins.h"
+ #include "funcapi.h"
+ #include "catalog/pg_type.h"
+ #include "catalog/pg_tablespace.h"
/*
***
*** 102,105
--- 107,204
pg_cancel_backend(PG_FUNCTION_ARGS)
{
PG_RETURN_INT32(pg_signal_backend(PG_GETARG_INT32(0),SIGINT));
+ }
+
+
+ typedef struct
+ {
+ char *location;
+ DIR *dirdesc;
+ } ts_db_fctx;
+
+ Datum pg_tablespace_databases(PG_FUNCTION_ARGS)
+ {
+ FuncCallContext *funcctx;
+ struct dirent *de;
+ ts_db_fctx *fctx;
+
+ if (SRF_IS_FIRSTCALL())
+ {
+ MemoryContext oldcontext;
+ Oid tablespaceOid=PG_GETARG_OID(0);
+
+ funcctx=SRF_FIRSTCALL_INIT();
+ oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+
+ fctx = palloc(sizeof(ts_db_fctx));
+
+ /*
+ * size = path length + tablespace dirname length
+ *+ 2 dir sep chars + oid + terminator
+ */
+ fctx->location = (char*) palloc(strlen(DataDir) + 11 + 10 + 1);
+ if (tablespaceOid == GLOBALTABLESPACE_OID)
+ {
+ fctx->dirdesc = NULL;
+ ereport(NOTICE,
+ (errcode(ERRCODE_WARNING),
+ errmsg("global tablespace never has databases.")));
+ }
+ else
+ {
+ if (tablespaceOid == DEFAULTTABLESPACE_OID)
+ sprintf(fctx->location, "%s/base", DataDir);
+ else
+ sprintf(fctx->location, "%s/pg_tblspc/%u", DataDir,
+ tablespaceOid);
+
+ fctx->dirdesc = AllocateDir(fctx->location);
+
+ if (!fctx->dirdesc) /* not a tablespace */
+ ereport(NOTICE,
+ (errcode(ERRCODE_WARNING),
+ errmsg("%d is no tablespace oid.", tablespaceOid)));
+ }
+ funcctx->user_fctx = fctx;
+ MemoryContextSwitchTo(oldcontext);
+ }
+
+ funcctx=SRF_PERCALL_SETUP();
+ fctx = (ts_db_fctx*) funcctx
Re: [PATCHES] pg_tablespace_databases
Andreas Pflug wrote:
From an idea of Bruce, the attached patch implements the function
pg_tablespace_databases(oid) RETURNS SETOF oid
which delivers as set of database oids having objects in the selected
tablespace, enabling an admin to examine only the databases affecting
the tablespace for objects instead of scanning all of them.
Attached is the patch I plan to apply. There are a couple of changes
from what was posted.
1) You must have meant tablespace instead of namespace here:
+
+
pg_tablespace_databases(namespace_oid)
+ setof oid
+ get set of database oids that have objects in the
namespace
+
2) This allocation size was a bit ambigous and I think based on a once
longer tablespace directory name:
+ fctx->location = (char*)palloc(strlen(DataDir)+16+10+1);
I take it that is (path len + '/' + strlen("pg_tablespaces") + '/' + oid
string length + terminator). I did this instead:
+ #define PG_TABLESPACE_DIR "pg_tblspc"
+ /* assumes unsigned, 10 digits */
+ #define OID_AS_STR10
+ /*
+* size = path length + tablespace dirname length
+*+ 2 dir sep chars + oid + terminator
+*/
+ fctx->location = (char*) palloc(strlen(DataDir)
+ + strlen(PG_TABLESPACE_DIR)
+ + 2 + OID_AS_STR + 1);
Usage looks like this:
regression=# select d.datname from pg_tablespace_databases(1663) as
t(oid) join pg_database d on t.oid = d.oid order by 1;
datname
regression
template0
template1
(3 rows)
initdb forced.
Any objections?
Joe
Index: doc/src/sgml/func.sgml
===
RCS file: /cvsroot/pgsql-server/doc/src/sgml/func.sgml,v
retrieving revision 1.211
diff -c -r1.211 func.sgml
*** doc/src/sgml/func.sgml 25 Jun 2004 17:20:21 - 1.211
--- doc/src/sgml/func.sgml 2 Jul 2004 05:12:56 -
***
*** 7232,7237
--- 7232,7241
pg_get_serial_sequence
+
+ pg_tablespace_databases
+
+
lists functions that
extract information from the system catalogs.
***
*** 7325,7330
--- 7329,7339
get name of the sequence that a serial or bigserial column
uses
+
+pg_tablespace_databases(tablespace_oid)
+setof oid
+get set of database oids that have objects in the tablespace
+
***
*** 7360,7365
--- 7369,7384
for passing to the sequence functions (see ).
NULL is returned if the column does not have a sequence attached.
+
+
+
+ pg_tablespace_databases allows usage examination of a
+ tablespace. It will return a set of database oids, that have objects
+ stored in the tablespace. If this function returns any row, the
+ tablespace is assumed not to be empty and cannot be dropped. To
+ display the actual objects populating the tablespace, you will need
+ to connect to the databases returned by
+ pg_tablespace_databases to query pg_class.
Index: src/backend/utils/adt/misc.c
===
RCS file: /cvsroot/pgsql-server/src/backend/utils/adt/misc.c,v
retrieving revision 1.34
diff -c -r1.34 misc.c
*** src/backend/utils/adt/misc.c 2 Jun 2004 21:29:29 - 1.34
--- src/backend/utils/adt/misc.c 2 Jul 2004 05:12:56 -
***
*** 16,26
--- 16,31
#include
#include
+ #include
#include "commands/dbcommands.h"
#include "miscadmin.h"
#include "storage/sinval.h"
+ #include "storage/fd.h"
#include "utils/builtins.h"
+ #include "funcapi.h"
+ #include "catalog/pg_type.h"
+ #include "catalog/pg_tablespace.h"
/*
***
*** 102,105
--- 107,210
pg_cancel_backend(PG_FUNCTION_ARGS)
{
PG_RETURN_INT32(pg_signal_backend(PG_GETARG_INT32(0),SIGINT));
+ }
+
+
+ typedef struct
+ {
+ char *location;
+ DIR *dirdesc;
+ } ts_db_fctx;
+ #define PG_TABLESPACE_DIR "pg_tblspc"
+ /* assumes unsigned, 10 digits */
+ #define OID_AS_STR 10
+
+ Datum pg_tablespace_databases(PG_FUNCTION_ARGS)
+ {
+ FuncCallContext *funcctx;
+ struct dirent *de;
+ ts_db_fctx *fctx;
+
+ if (SRF_IS_FIRSTCALL())
+ {
+ MemoryContext oldcontext;
+ Oid tablespaceOid=PG_GETARG_OID(0);
+
+ funcctx=SRF_FIRSTCALL_INIT();
+ oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+
+ fctx = palloc(sizeof(ts_db_fctx));
+
+ /*
+ * size = path length + tablespace dirname length
+ *+ 2 dir sep chars + oid + terminator
+ */
+ fctx->location = (char*) palloc(strlen(DataDir)
+ + strlen(PG_TABLESPACE_DIR)
+ + 2 + OID_AS_STR + 1);
+ if (tablespaceOid == GLOBALTABLESPACE_OID)
+ {
+ fctx->dirdesc = NULL;
+ ereport
Re: [PATCHES] contrib/dbmirror
[EMAIL PROTECTED] wrote: Attached is a 1 line bug fix for dbmirror that was submitted. It fixes a bug where some transactions could be dropped when writing mirrored SQL statements to files. Patch applied. Joe ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] latest plperl
Andrew Dunstan wrote: Joe Conway said: As a side note, I think it would be *really* helpful if there were a more comprehensive test script, and an expected results file available. Not sure though if it could be included in the standard regression tests on a configure-conditional basis -- anyone know? To the best of my knowledge you cannot. We will provide an analogue for pltcl's test directory shortly, if that is desired - it will probably take a few days, though. I assume that will be acceptable after feature freeze? Yup, I think that falls into Tom's "loose ends" category. At a quick glance, modulo the makefile change, the patch looks good. Great! I'll commit shortly. Thanks, Joe ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] pg_tablespace_databases
Andreas Pflug wrote: From an idea of Bruce, the attached patch implements the function pg_tablespace_databases(oid) RETURNS SETOF oid which delivers as set of database oids having objects in the selected tablespace, enabling an admin to examine only the databases affecting the tablespace for objects instead of scanning all of them. If there are no objections, I'll review and apply this tonight (west coast USA time). Andreas, please provide a corresponding documentation patch. Thanks, Joe ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] latest plperl
Andrew Dunstan wrote:
Doh! Very bogus! sizeof(int)and a malloc to boot ???
I didn't check the trigger code much because it has supposedly been working
for quite a while. I will examine more closely.
Well, essentially 4 bytes (sizeof(int)) were being allocated to print a
two byte interger that can never be greater than two characters ("32"),
so I don't expect it would have ever failed, but only by serendipity.
Shouldn't that look more like this?
+ plval = plperl_get_elem(hvNew, platt);
+ if (plval)
+ {
+ modvalues[j] = FunctionCall3(&finfo,
+ CStringGetDatum(plval),
+ ObjectIdGetDatum(typelem),
+ Int32GetDatum(tupdesc->attrs[atti]->atttypmod)); +
modnulls[j] = ' ';
+ }
+ else
+ {
+ modvalues[i] = (Datum) 0;
+ modnulls[j] = 'n';
+ }
Yes, except that that [i] looks wrong too. Surely it should be [j]. And with
this change decl of src appears redundant.
Hmmm, I missed that -- looks wrong to me too. I'll check it out.
I will do some checking on these changes, but with those caveats they look
good to me.
Do you need a revised patch?
Nah, I'll make the changes and post a revised patch for you to comment
on prior to committing.
Joe
---(end of broadcast)---
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
Re: [PATCHES] latest plperl
Andrew Dunstan wrote:
I will do some checking on these changes, but with those caveats they look
good to me.
Attached is an all inclusive revised patch. Please review and comment.
If there are no objections, I'll commit in a few hours.
As a side note, I think it would be *really* helpful if there were a
more comprehensive test script, and an expected results file available.
Not sure though if it could be included in the standard regression tests
on a configure-conditional basis -- anyone know?
Joe
Index: src/pl/plperl/GNUmakefile
===
RCS file: /cvsroot/pgsql-server/src/pl/plperl/GNUmakefile,v
retrieving revision 1.12
diff -c -r1.12 GNUmakefile
*** src/pl/plperl/GNUmakefile 21 Jan 2004 19:04:11 - 1.12
--- src/pl/plperl/GNUmakefile 1 Jul 2004 16:24:53 -
***
*** 25,32
SO_MAJOR_VERSION = 0
SO_MINOR_VERSION = 0
! OBJS = plperl.o eloglvl.o SPI.o
SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS)
include $(top_srcdir)/src/Makefile.shlib
--- 25,37
SO_MAJOR_VERSION = 0
SO_MINOR_VERSION = 0
! OBJS = plperl.o spi_internal.o SPI.o
!
! ifeq ($(enable_rpath), yes)
SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS)
+ else
+ SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS) -Wl,-rpath,$(perl_archlibexp)/CORE
+ endif
include $(top_srcdir)/src/Makefile.shlib
Index: src/pl/plperl/SPI.xs
===
RCS file: /cvsroot/pgsql-server/src/pl/plperl/SPI.xs,v
retrieving revision 1.5
diff -c -r1.5 SPI.xs
*** src/pl/plperl/SPI.xs 4 Sep 2002 22:49:37 - 1.5
--- src/pl/plperl/SPI.xs 1 Jul 2004 16:24:53 -
***
*** 6,22
#include "perl.h"
#include "XSUB.h"
! #include "eloglvl.h"
! MODULE = SPI PREFIX = elog_
PROTOTYPES: ENABLE
VERSIONCHECK: DISABLE
void
! elog_elog(level, message)
int level
char* message
CODE:
--- 6,22
#include "perl.h"
#include "XSUB.h"
! #include "spi_internal.h"
! MODULE = SPI PREFIX = spi_
PROTOTYPES: ENABLE
VERSIONCHECK: DISABLE
void
! spi_elog(level, message)
int level
char* message
CODE:
***
*** 24,44
int
! elog_DEBUG()
int
! elog_LOG()
int
! elog_INFO()
int
! elog_NOTICE()
int
! elog_WARNING()
int
! elog_ERROR()
!
--- 24,56
int
! spi_DEBUG()
int
! spi_LOG()
int
! spi_INFO()
int
! spi_NOTICE()
int
! spi_WARNING()
int
! spi_ERROR()
+ SV*
+ spi_spi_exec_query(query, ...)
+ char* query;
+ PREINIT:
+ HV *ret_hash;
+ int limit=0;
+ CODE:
+ if (items>2) Perl_croak(aTHX_ "Usage: spi_exec_query(query, limit) or spi_exec_query(query)");
+ if (items == 2) limit = SvIV(ST(1));
+ ret_hash=plperl_spi_exec(query, limit);
+ RETVAL = newRV_noinc((SV*)ret_hash);
+ OUTPUT:
+ RETVAL
Index: src/pl/plperl/eloglvl.c
===
RCS file: src/pl/plperl/eloglvl.c
diff -N src/pl/plperl/eloglvl.c
*** src/pl/plperl/eloglvl.c 25 Jul 2003 23:37:28 - 1.9
--- /dev/null 1 Jan 1970 00:00:00 -
***
*** 1,45
- #include "postgres.h"
-
- /*
- * This kludge is necessary because of the conflicting
- * definitions of 'DEBUG' between postgres and perl.
- * we'll live.
- */
-
- #include "eloglvl.h"
-
- int
- elog_DEBUG(void)
- {
- return DEBUG2;
- }
-
- int
- elog_LOG(void)
- {
- return LOG;
- }
-
- int
- elog_INFO(void)
- {
- return INFO;
- }
-
- int
- elog_NOTICE(void)
- {
- return NOTICE;
- }
-
- int
- elog_WARNING(void)
- {
- return WARNING;
- }
-
- int
- elog_ERROR(void)
- {
- return ERROR;
- }
--- 0
Index: src/pl/plperl/eloglvl.h
===
RCS file: src/pl/plperl/eloglvl.h
diff -N src/pl/plperl/eloglvl.h
*** src/pl/plperl/eloglvl.h 4 Sep 2002 20:31:47 - 1.5
--- /dev/null 1 Jan 1970 00:00:00 -
***
*** 1,12
-
- int elog_DEBUG(void);
-
- int elog_LOG(void);
-
- int elog_INFO(void);
-
- int elog_NOTICE(void);
-
- int elog_WARNING(void);
-
- int elog_ERROR(void);
--- 0
Index: src/pl/plperl/plperl.c
===
RCS file: /cvsroot/pgsql-server/src/pl/plperl/plperl.c,v
retrieving revision 1.44
diff -c -r1.44 plperl.c
*** src/pl/plperl/plperl.c 6 Jun 2004 00:41:28 - 1.44
--- src/pl/plperl/plperl.c 1 Jul 2004 16:24:53 -
***
*** 49,54
--- 49,55
#include "catalog/pg_language.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+ #include "funcapi.h" /* need for SRF support */
#include "commands/trigger.h"
#include "executor/spi.h"
#include "fmgr.h"
***
*** 78,83
--- 79,86
TransactionId fn_xmin;
CommandId fn_cmin;
bool lanpltrusted;
+ bool fn_retistuple; /* true, if function returns tuple */
+ Oid ret_oid; /* Oid of re
Re: [Plperlng-devel] Re: [PATCHES] latest plperl
Andrew Dunstan wrote: I also got the rpath test sense wrong in the make file fix. It should read (assuming this mailer dowsn't break lines badly): ifeq ($(enable_rpath), yes) SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS) -Wl,-rpath,$(perl_archlibexp)/CORE else SHLIB_LINK = $(perl_embed_ldflags) $(BE_DLLLIBS) endif OK -- wasn't in the last patch I posted, but I got it now. Joe ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] latest plperl
Andrew Dunstan wrote:
The attached patch (and 2 new files incorporating previous eloglvl.[ch] as
before) has the following changes over previously sent patch
(fixes all by me):
Some comments below:
In plperl_trigger_build_args(), this looks bogus:
+ char *tmp;
+
+ tmp = (char *) malloc(sizeof(int));
...
+ sprintf(tmp, "%d", tdata->tg_trigger->tgnargs);
+ sv_catpvf(rv, ", argc => %s", tmp);
...
+ free(tmp);
I changed it to:
+ sv_catpvf(rv, ", argc => %d", tdata->tg_trigger->tgnargs);
In this section, it appears that empty strings in the tuple will be
coerced into NULL values:
+ plval = plperl_get_elem(hvNew, platt);
+ if (plval)
+ {
+ src = plval;
+ if (strlen(plval))
+ {
+ modvalues[j] = FunctionCall3(&finfo,
+ CStringGetDatum(src),
+ ObjectIdGetDatum(typelem),
+ Int32GetDatum(tupdesc->attrs[atti]->atttypmod));
+ modnulls[j] = ' ';
+ }
+ else
+ {
+ modvalues[i] = (Datum) 0;
+ modnulls[j] = 'n';
+ }
+ }
+ plval = NULL;
Shouldn't that look more like this?
+ plval = plperl_get_elem(hvNew, platt);
+ if (plval)
+ {
+ modvalues[j] = FunctionCall3(&finfo,
+ CStringGetDatum(plval),
+ ObjectIdGetDatum(typelem),
+ Int32GetDatum(tupdesc->attrs[atti]->atttypmod));
+ modnulls[j] = ' ';
+ }
+ else
+ {
+ modvalues[i] = (Datum) 0;
+ modnulls[j] = 'n';
+ }
Joe
---(end of broadcast)---
TIP 6: Have you searched our list archives?
http://archives.postgresql.org
Re: [PATCHES] latest plperl
Andrew Dunstan wrote: The attached patch (and 2 new files incorporating previous eloglvl.[ch] as before) has the following changes over previously sent patch (fixes all by me): The patch file itself seems to be empty -- please resend. Thanks, Joe ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] contrib/dbmirror
[EMAIL PROTECTED] wrote: Attached is a 1 line bug fix for dbmirror that was submitted. It fixes a bug where some transactions could be dropped when writing mirrored SQL statements to files. I know that there were discussions regarding removing the replication contribs (rserv and dbmirror) prior to 7.5 release, but given that that has not happened yet, any objections to me applying this? Joe ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] latest plperl
Tom Lane wrote: Are there any other pending patches you're interested in taking responsibility for? Yeah, I know you've been especially overloaded lately, and I feel badly that I've not been able to help out in recent months :-( If you have some specific patches in mind, I can try to work on one or more tomorrow and Friday. Unfortunately, on Saturday morning I'm leaving on a 3600 mile roadtrip by car, and while I'm gone my connectivity will be spotty (for a week and a half). Joe ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] latest plperl
Andrew Dunstan wrote: The attached patch (and 2 new files incorporating previous eloglvl.[ch] as before) has the following changes over previously sent patch (fixes all by me): - fix null <-> undef mappings - fix GNUmakefile to honor rpath configuration, and remove ugly compile arnings due to inappropriate use of rpath in CFLAGS - very minor code comment cleanup The feature set is as previously advised. I've been working with Andrew and company on this for a few days. I intend to finish up my code review and commit it tomorrow sometime, unless someone has objections. That said, I'm not particularly strong in perl, so it would be helpful if others would test and report in. Thanks, Joe ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
[PATCHES] contrib/fuzzystrmatch updates
If there are no objections, I intend to commit the attached tonight or
tomorrow. Changes as follows:
- Adds double metaphone code from Andrew Dunstan
- Change metaphone so that an empty input string causes an empty
output string to be returned, instead of throwing an ERROR.
Resolves at least one complaint, and makes behavior consistent
with double metaphone.
- Fixes examples in README.soundex
Joe
Index: contrib/fuzzystrmatch/Makefile
===
RCS file: /cvsroot/pgsql-server/contrib/fuzzystrmatch/Makefile,v
retrieving revision 1.3
diff -c -r1.3 Makefile
*** contrib/fuzzystrmatch/Makefile 29 Nov 2003 19:51:35 - 1.3
--- contrib/fuzzystrmatch/Makefile 30 Jun 2004 18:51:19 -
***
*** 4,10
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
! MODULES = fuzzystrmatch
DATA_built = fuzzystrmatch.sql
DOCS = README.fuzzystrmatch README.soundex
--- 4,12
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
! MODULE_big = fuzzystrmatch
! SRCS += fuzzystrmatch.c dmetaphone.c
! OBJS = $(SRCS:.c=.o)
DATA_built = fuzzystrmatch.sql
DOCS = README.fuzzystrmatch README.soundex
Index: contrib/fuzzystrmatch/README.fuzzystrmatch
===
RCS file: /cvsroot/pgsql-server/contrib/fuzzystrmatch/README.fuzzystrmatch,v
retrieving revision 1.4
diff -c -r1.4 README.fuzzystrmatch
*** contrib/fuzzystrmatch/README.fuzzystrmatch 4 Aug 2003 23:59:37 - 1.4
--- contrib/fuzzystrmatch/README.fuzzystrmatch 30 Jun 2004 18:51:19 -
***
*** 23,28
--- 23,33
* Metaphone was originally created by Lawrence Philips and presented in article
* in "Computer Language" December 1990 issue.
*
+ * dmetaphone() and dmetaphone_alt()
+ * -
+ * A port of the DoubleMetaphone perl module by Andrew Dunstan. See dmetaphone.c
+ * for more detail.
+ *
* soundex()
* ---
* Folded existing soundex contrib into this one. Renamed text_soundex() (C function)
***
*** 48,58
*/
! Version 0.2 (7 August, 2001):
! Functions to calculate the degree to which two strings match in a "fuzzy" way
! Tested under Linux (Red Hat 6.2 and 7.0) and PostgreSQL 7.2devel
Release Notes:
Version 0.2
- folded soundex contrib into this one
--- 53,66
*/
! Version 0.3 (30 June, 2004):
Release Notes:
+ Version 0.3
+- added double metaphone code from Andrew Dunstan
+- change metaphone so that an empty input string causes an empty
+ output string to be returned, instead of throwing an ERROR
+- fixed examples in README.soundex
Version 0.2
- folded soundex contrib into this one
Index: contrib/fuzzystrmatch/README.soundex
===
RCS file: /cvsroot/pgsql-server/contrib/fuzzystrmatch/README.soundex,v
retrieving revision 1.1
diff -c -r1.1 README.soundex
*** contrib/fuzzystrmatch/README.soundex 7 Aug 2001 18:16:01 - 1.1
--- contrib/fuzzystrmatch/README.soundex 30 Jun 2004 18:51:19 -
***
*** 20,26
select * from s
where soundex(nm) = soundex('john')\g
! select nm from s a, s b
where soundex(a.nm) = soundex(b.nm)
and a.oid <> b.oid\g
--- 20,26
select * from s
where soundex(nm) = soundex('john')\g
! select a.nm, b.nm from s a, s b
where soundex(a.nm) = soundex(b.nm)
and a.oid <> b.oid\g
***
*** 51,57
DROP OPERATOR #= (text,text)\g
CREATE OPERATOR #= (leftarg=text, rightarg=text, procedure=text_sx_eq,
! commutator=text_sx_eq)\g
SELECT *
FROM s
--- 51,57
DROP OPERATOR #= (text,text)\g
CREATE OPERATOR #= (leftarg=text, rightarg=text, procedure=text_sx_eq,
! commutator = #=)\g
SELECT *
FROM s
Index: contrib/fuzzystrmatch/dmetaphone.c
===
RCS file: contrib/fuzzystrmatch/dmetaphone.c
diff -N contrib/fuzzystrmatch/dmetaphone.c
*** /dev/null 1 Jan 1970 00:00:00 -
--- contrib/fuzzystrmatch/dmetaphone.c 30 Jun 2004 18:51:19 -
***
*** 0
--- 1,1458
+ /*
+ * This is a port of the Double Metaphone algorithm for use in PostgreSQL.
+ *
+ * Double Metaphone computes 2 "sounds like" strings - a primary and an
+ * alternate. In most cases they are the same, but for foreign names
+ * especially they can be a bit different, depending on pronunciation.
+ *
+ * Information on using Double Metaphone can be found at
+ * http://www.codeproject.com/useritems/dmetaphone1.asp
+ * and the original article describing it can be found at
+ * http://www.cuj.com/documents/s=8038/cuj0006philips/
+ *
+ * For PostgrSQL we provide 2 functions - one for the primary and one for
+ * the alternate. That way the functions are pure text->text mappings that
+ * are useful in fun
Re: [PATCHES] Another pg_autovacuum patch
Matthew T. O'Connor wrote: This patch resolves this new found bug and fixes some of the other debugging output to be more consistent. Please apply to both HEAD and the 7.4 branch. Bruce, if you'd like, I'll apply this one. I plan to test it out tonight or tomorrow. Thanks, Joe ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] [GENERAL] dblink: rollback transaction
Joe Conway wrote: Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: I like the idea in general, but maybe instead there should be a new overloaded version of the existing function names that accepts an additional bool argument. Without the argument, behavior would be as it is now; with it, you could specify the old or new behavior. Um, maybe I'm confused about the context, but aren't we talking about C function names here? No overloading is possible in C ... I was thinking in terms of overloaded SQL function names. For example, in addition to dblink_exec(text) and dblink_exec(text,text) we create dblink_exec(text,bool) and dblink_exec(text,text,bool). Currently both SQL versions of dblink_exec are implemented by a single C level function. But yes, we'd need another C level function to support the new SQL functions because there would be no way to distinguish the 2 two-argument versions otherwise. (Actually, now I'm wondering if we could use a single C function for all four SQL versions -- between PG_NARGS() and get_fn_expr_argtype() we should be able to figure out how we were called, shouldn't we?) The attached implements the new overloaded SQL functions as discussed above (i.e. start with existing argument combinations, add a new bool argument to each). I ended up with a single C function (by making use of number and type of the arguments) for each overloaded SQL function name. I'll commit in a day or two if there are no objections. Thanks, Joe Index: contrib/dblink/README.dblink === RCS file: /cvsroot/pgsql-server/contrib/dblink/README.dblink,v retrieving revision 1.9 diff -c -r1.9 README.dblink *** contrib/dblink/README.dblink4 Aug 2003 23:59:37 - 1.9 --- contrib/dblink/README.dblink5 Mar 2004 05:55:45 - *** *** 30,42 * */ - Version 0.6 (14 June, 2003): - Completely removed previously deprecated functions. Added ability - to create "named" persistent connections in addition to the single global - "unnamed" persistent connection. - Tested under Linux (Red Hat 9) and PostgreSQL 7.4devel. - Release Notes: Version 0.6 - functions deprecated in 0.5 have been removed - added ability to create "named" persistent connections --- 30,40 * */ Release Notes: + Version 0.7 (as of 25 Feb, 2004) + - Added new version of dblink, dblink_exec, dblink_open, dblink_close, + and, dblink_fetch -- allows ERROR on remote side of connection to + throw NOTICE locally instead of ERROR Version 0.6 - functions deprecated in 0.5 have been removed - added ability to create "named" persistent connections *** *** 85,91 You can use dblink.sql to create the functions in your database of choice, e.g. ! psql -U postgres template1 < dblink.sql installs following functions into database template1: --- 83,89 You can use dblink.sql to create the functions in your database of choice, e.g. ! psql template1 < dblink.sql installs following functions into database template1: *** *** 104,143 cursor ! dblink_open(text,text) RETURNS text - opens a cursor using unnamed connection already opened with dblink_connect() that will persist for duration of current backend or until it is closed ! dblink_open(text,text,text) RETURNS text - opens a cursor using a named connection already opened with dblink_connect() that will persist for duration of current backend or until it is closed ! dblink_fetch(text, int) RETURNS setof record - fetches data from an already opened cursor on the unnamed connection ! dblink_fetch(text, text, int) RETURNS setof record - fetches data from an already opened cursor on a named connection ! dblink_close(text) RETURNS text - closes a cursor on the unnamed connection ! dblink_close(text,text) RETURNS text - closes a cursor on a named connection query ! dblink(text,text) RETURNS setof record - returns a set of results from remote SELECT query; the first argument is either a connection string, or the name of an already opened persistant connection ! dblink(text) RETURNS setof record - returns a set of results from remote SELECT query, using the unnamed connection already opened with dblink_connect() execute ! dblink_exec(text, text) RETURNS text - executes an INSERT/UPDATE/DELETE query remotely; the first argument is either a connection string, or the name of an already opened persistant connection ! dblink_exec(text) RETURNS
Re: [PATCHES] [GENERAL] dblink: rollback transaction
Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: I like the idea in general, but maybe instead there should be a new overloaded version of the existing function names that accepts an additional bool argument. Without the argument, behavior would be as it is now; with it, you could specify the old or new behavior. Um, maybe I'm confused about the context, but aren't we talking about C function names here? No overloading is possible in C ... I was thinking in terms of overloaded SQL function names. For example, in addition to dblink_exec(text) and dblink_exec(text,text) we create dblink_exec(text,bool) and dblink_exec(text,text,bool). Currently both SQL versions of dblink_exec are implemented by a single C level function. But yes, we'd need another C level function to support the new SQL functions because there would be no way to distinguish the 2 two-argument versions otherwise. (Actually, now I'm wondering if we could use a single C function for all four SQL versions -- between PG_NARGS() and get_fn_expr_argtype() we should be able to figure out how we were called, shouldn't we?) Joe ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] [GENERAL] dblink: rollback transaction
Tom Lane wrote: Joe Conway <[EMAIL PROTECTED]> writes: One question that I'd like some feedback on is the following: should the same change be applied to other functions that might throw an ERROR based on the remote side of the connection? For example, currently if dblink() is used in an attempt to access a non-existent remote table, an ERROR is thrown locally in response, killing any currently open transaction. Thoughts? What seems like a good idea after a few moments' thought is to leave the behavior of the various dblink_foo() functions the same as now (ie, throw error on remote error) and add new API functions named something like dblink_foo_noerror() that don't throw error but return a recognizable failure code instead. My argument for this approach is that there is no situation in which the programmer shouldn't have to think when he writes a given call whether it will elog or return an error indicator, because if he wants an error indicator then he is going to have to check for it. I like the idea in general, but maybe instead there should be a new overloaded version of the existing function names that accepts an additional bool argument. Without the argument, behavior would be as it is now; with it, you could specify the old or new behavior. Joe ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] dblink - custom datatypes NOW work :)
Tom Lane wrote:
Two nitpicks (each applying in 2 places):
First, testing for null rsinfo isn't sufficient, since the resultinfo
mechanism could be used for other things; you need an IsA test too.
Second, is "syntax error" really the most appropriate classification for
this?
(Also, the errmsg text seems a bit out of line with the wording of
comparable errors, but I can't offer better text offhand.)
Thanks for the feedback, Tom. Here's what I ended up with:
if (!rsinfo || !IsA(rsinfo, ReturnSetInfo))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("function returning record called in context "
"that cannot accept type record")));
Joe
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] [GENERAL] dblink: rollback transaction
Oleg Lebedev wrote:
Your fix is awesome! That's exactly what I need.
What version of postgres do I need to have installed to try this patch?
I am on 7.3 now.
I plan to apply the attached to cvs tip in 24 hours or so. I don't think
it qualifies as a bug fix, and it does represent a change in user facing
behavior, so I do not intend to apply for 7.3.6 or 7.4.2.
The patch changes dblink_exec() such that an ERROR on the remote side of
the connection will generate a NOTICE on the local side, with the
dblink_exec() return value set to 'ERROR'. This allows the remote side
ERROR to be trapped and handled locally.
One question that I'd like some feedback on is the following: should the
same change be applied to other functions that might throw an ERROR
based on the remote side of the connection? For example, currently if
dblink() is used in an attempt to access a non-existent remote table, an
ERROR is thrown locally in response, killing any currently open
transaction. Thoughts?
Thanks,
Joe
Index: contrib/dblink/dblink.c
===
RCS file: /opt/src/cvs/pgsql-server/contrib/dblink/dblink.c,v
retrieving revision 1.29
diff -c -r1.29 dblink.c
*** contrib/dblink/dblink.c 28 Nov 2003 05:03:01 - 1.29
--- contrib/dblink/dblink.c 5 Feb 2004 19:49:00 -
***
*** 135,140
--- 135,150
errmsg("%s", p2), \
errdetail("%s", msg))); \
} while (0)
+ #define DBLINK_RES_ERROR_AS_NOTICE(p2) \
+ do { \
+ msg = pstrdup(PQerrorMessage(conn)); \
+ if (res) \
+ PQclear(res); \
+ ereport(NOTICE, \
+ (errcode(ERRCODE_SYNTAX_ERROR), \
+errmsg("%s", p2), \
+errdetail("%s", msg))); \
+ } while (0)
#define DBLINK_CONN_NOT_AVAIL \
do { \
if(conname) \
***
*** 731,739
if (!res ||
(PQresultStatus(res) != PGRES_COMMAND_OK &&
PQresultStatus(res) != PGRES_TUPLES_OK))
! DBLINK_RES_ERROR("sql error");
! if (PQresultStatus(res) == PGRES_COMMAND_OK)
{
/* need a tuple descriptor representing one TEXT column */
tupdesc = CreateTemplateTupleDesc(1, false);
--- 741,762
if (!res ||
(PQresultStatus(res) != PGRES_COMMAND_OK &&
PQresultStatus(res) != PGRES_TUPLES_OK))
! {
! DBLINK_RES_ERROR_AS_NOTICE("sql error");
!
! /* need a tuple descriptor representing one TEXT column */
! tupdesc = CreateTemplateTupleDesc(1, false);
! TupleDescInitEntry(tupdesc, (AttrNumber) 1, "status",
! TEXTOID, -1, 0, false);
! /*
!* and save a copy of the command status string to return as our
!* result tuple
!*/
! sql_cmd_status = GET_TEXT("ERROR");
!
! }
! else if (PQresultStatus(res) == PGRES_COMMAND_OK)
{
/* need a tuple descriptor representing one TEXT column */
tupdesc = CreateTemplateTupleDesc(1, false);
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
