assertion failure w/extended query protocol
Rushabh Lathia of the EnterpriseDB development team and I have been
doing some testing of the extended query protocol and have found a
case where it causes an assertion failure. Here's how to reproduce:
1. Apply the attached patch to teach psql how to use the extended
query protocol. Compile, install.
2. Start the modified psql and do this:
\set PROTOCOL extended
PREPARE stmt as select 1;
CREATE TEMPORARY TABLE tmptbl AS EXECUTE stmt;
The result is:
TRAP: FailedAssertion("!(qry->commandType != CMD_UTILITY)", File:
"utility.c", Line: 1516)
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
psql-protocol.patchapplication/octet-stream; name=psql-protocol.patchDownload
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 179c162..04277ae 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -928,7 +928,10 @@ SendQuery(const char *query)
if (pset.timing)
INSTR_TIME_SET_CURRENT(before);
- results = PQexec(pset.db, query);
+ if (pset.use_extended_protocol)
+ results = PQexecParams(pset.db, query, 0, NULL, NULL, NULL, NULL, 0);
+ else
+ results = PQexec(pset.db, query);
/* these operations are included in the timing result: */
ResetCancelConn();
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index c907fa0..c811734 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -111,6 +111,7 @@ typedef struct _psqlSettings
const char *prompt1;
const char *prompt2;
const char *prompt3;
+ bool use_extended_protocol;
PGVerbosity verbosity; /* current error verbosity level */
} PsqlSettings;
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 1fcc47f..bd9384a 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -143,6 +143,7 @@ main(int argc, char *argv[])
/* Default values for variables */
SetVariableBool(pset.vars, "AUTOCOMMIT");
+ SetVariable(pset.vars, "PROTOCOL", "simple");
SetVariable(pset.vars, "VERBOSITY", "default");
SetVariable(pset.vars, "PROMPT1", DEFAULT_PROMPT1);
SetVariable(pset.vars, "PROMPT2", DEFAULT_PROMPT2);
@@ -790,6 +791,19 @@ prompt3_hook(const char *newval)
}
static void
+protocol_hook(const char *newval)
+{
+ if (newval == NULL)
+ pset.use_extended_protocol = false;
+ else if (strcmp(newval, "simple") == 0)
+ pset.use_extended_protocol = false;
+ else if (strcmp(newval, "extended") == 0)
+ pset.use_extended_protocol = true;
+ else
+ pset.use_extended_protocol = false;
+}
+
+static void
verbosity_hook(const char *newval)
{
if (newval == NULL)
@@ -826,5 +840,6 @@ EstablishVariableSpace(void)
SetVariableAssignHook(pset.vars, "PROMPT1", prompt1_hook);
SetVariableAssignHook(pset.vars, "PROMPT2", prompt2_hook);
SetVariableAssignHook(pset.vars, "PROMPT3", prompt3_hook);
+ SetVariableAssignHook(pset.vars, "PROTOCOL", protocol_hook);
SetVariableAssignHook(pset.vars, "VERBOSITY", verbosity_hook);
}
On 19 October 2012 17:19, Robert Haas <robertmhaas@gmail.com> wrote:
Rushabh Lathia of the EnterpriseDB development team and I have been
doing some testing of the extended query protocol and have found a
case where it causes an assertion failure. Here's how to reproduce:1. Apply the attached patch to teach psql how to use the extended
query protocol. Compile, install.2. Start the modified psql and do this:
\set PROTOCOL extended
PREPARE stmt as select 1;
CREATE TEMPORARY TABLE tmptbl AS EXECUTE stmt;The result is:
TRAP: FailedAssertion("!(qry->commandType != CMD_UTILITY)", File:
"utility.c", Line: 1516)
I'm reasonably confident that commit
9dbf2b7d75de5af38d087cbe2b1147dd0fd10f0a caused this breakage.
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
On Friday, October 19, 2012 06:41:27 PM Peter Geoghegan wrote:
On 19 October 2012 17:19, Robert Haas <robertmhaas@gmail.com> wrote:
Rushabh Lathia of the EnterpriseDB development team and I have been
doing some testing of the extended query protocol and have found a
case where it causes an assertion failure. Here's how to reproduce:1. Apply the attached patch to teach psql how to use the extended
query protocol. Compile, install.2. Start the modified psql and do this:
\set PROTOCOL extended
PREPARE stmt as select 1;
CREATE TEMPORARY TABLE tmptbl AS EXECUTE stmt;The result is:
TRAP: FailedAssertion("!(qry->commandType != CMD_UTILITY)", File:
"utility.c", Line: 1516)I'm reasonably confident that commit
9dbf2b7d75de5af38d087cbe2b1147dd0fd10f0a caused this breakage.
Not a bad guess, looking.
Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Friday, October 19, 2012 06:41:27 PM Peter Geoghegan wrote:
On 19 October 2012 17:19, Robert Haas <robertmhaas@gmail.com> wrote:
Rushabh Lathia of the EnterpriseDB development team and I have been
doing some testing of the extended query protocol and have found a
case where it causes an assertion failure. Here's how to reproduce:1. Apply the attached patch to teach psql how to use the extended
query protocol. Compile, install.2. Start the modified psql and do this:
\set PROTOCOL extended
PREPARE stmt as select 1;
CREATE TEMPORARY TABLE tmptbl AS EXECUTE stmt;The result is:
TRAP: FailedAssertion("!(qry->commandType != CMD_UTILITY)", File:
"utility.c", Line: 1516)fI'm reasonably confident that commit
9dbf2b7d75de5af38d087cbe2b1147dd0fd10f0a caused this breakage.
Simple fix attached.
Btw, do you plan to submit that psql patch at some point? I repeatedly wished
to be able to use the extended protocol without writing code or misusing
pgbench exactly to test stuff like this.
Greetings,
Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
0001-Fix-simple-oversight-causing-UtilityContainsQuery-no.patchtext/x-patch; charset=utf-8; name=0001-Fix-simple-oversight-causing-UtilityContainsQuery-no.patchDownload
From 434ce09ecae207a1f128eec5593f667b8507f220 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 19 Oct 2012 19:40:49 +0200
Subject: [PATCH] Fix simple oversight causing UtilityContainsQuery not to
support CREATE TABLE AS ... EXECUTE
UtilityContainsQuery looked in CreateTableAsStmt->query for an ExecuteStmt but
transformCreateTableAsStmt always puts it in ->query->utilityStmt.
---
src/backend/tcop/utility.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 97376bb..0d1af0b 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1508,17 +1508,20 @@ UtilityContainsQuery(Node *parsetree)
return qry;
case T_CreateTableAsStmt:
- /* might or might not contain a Query ... */
qry = (Query *) ((CreateTableAsStmt *) parsetree)->query;
- if (IsA(qry, Query))
- {
- /* Recursion currently can't be necessary here */
- Assert(qry->commandType != CMD_UTILITY);
+ Assert(IsA(qry, Query));
+
+ /* CREATE TABLE .. EXECUTE ...*/
+ if (qry->commandType == CMD_UTILITY &&
+ IsA(qry->utilityStmt, ExecuteStmt))
+ return NULL;
+ /* CREATE TABLE .. SELECT ... */
+ else if (qry->commandType == CMD_SELECT)
return qry;
- }
- Assert(IsA(qry, ExecuteStmt));
- return NULL;
+ /* Recursion currently can't be necessary here */
+ elog(ERROR, "unsupported case in CREATE TABLE AS");
+ return NULL; /* silence compiler */
default:
return NULL;
}
--
1.7.10.4
On 19 October 2012 19:01, Andres Freund <andres@2ndquadrant.com> wrote:
Btw, do you plan to submit that psql patch at some point? I repeatedly wished
to be able to use the extended protocol without writing code or misusing
pgbench exactly to test stuff like this.
+1
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
On Fri, Oct 19, 2012 at 2:01 PM, Andres Freund <andres@2ndquadrant.com> wrote:
On Friday, October 19, 2012 06:41:27 PM Peter Geoghegan wrote:
On 19 October 2012 17:19, Robert Haas <robertmhaas@gmail.com> wrote:
Rushabh Lathia of the EnterpriseDB development team and I have been
doing some testing of the extended query protocol and have found a
case where it causes an assertion failure. Here's how to reproduce:1. Apply the attached patch to teach psql how to use the extended
query protocol. Compile, install.2. Start the modified psql and do this:
\set PROTOCOL extended
PREPARE stmt as select 1;
CREATE TEMPORARY TABLE tmptbl AS EXECUTE stmt;The result is:
TRAP: FailedAssertion("!(qry->commandType != CMD_UTILITY)", File:
"utility.c", Line: 1516)fI'm reasonably confident that commit
9dbf2b7d75de5af38d087cbe2b1147dd0fd10f0a caused this breakage.Simple fix attached.
Looks reasonable to me, but I'm not terribly familiar with this code.
Tom, any comments?
Btw, do you plan to submit that psql patch at some point? I repeatedly wished
to be able to use the extended protocol without writing code or misusing
pgbench exactly to test stuff like this.
I didn't think it would be accepted, but if you think it's useful to
have, consider it submitted. If you review the code and it seems OK
(or can be fixed to be OK), I'm happy to write some user
documentation. I'm not sure it actually handles all the cases right
now but perhaps you could have a look.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Oct 19, 2012 at 2:01 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Simple fix attached.
Looks reasonable to me, but I'm not terribly familiar with this code.
Tom, any comments?
Will look shortly.
regards, tom lane
Andres Freund <andres@2ndquadrant.com> writes:
Simple fix attached.
Are you sure this isn't just moving the failure conditions around?
regards, tom lane
On Friday, October 19, 2012 09:05:30 PM Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
Simple fix attached.
Are you sure this isn't just moving the failure conditions around?
Don't think so. Its not that easy to follow though...
The "CREATE OptTemp TABLE create_as_target AS EXECUTE " production puts an
ExecuteStmt into CreateTableAsStmt->query.
"CREATE OptTemp TABLE create_as_target AS SelectStmt" puts a SelectStmt in
there.
then
static Query *
transformCreateTableAsStmt(ParseState *pstate, CreateTableAsStmt *stmt)
{
Query *result;
/* transform contained query */
stmt->query = (Node *) transformStmt(pstate, stmt->query);
/* represent the command as a utility Query */
result = makeNode(Query);
result->commandType = CMD_UTILITY;
result->utilityStmt = (Node *) stmt;
return result;
}
guarantees that we have a Query with IsA(Query->utilityStmt,
CreateTableAsStmt). And CreateTableAsStmt->query is the result of
transformStmt(SelectStmt|ExecuteStmt). A transformed SelectStmt returns a
Query with commandType == CMD_SELECT. A transformed ExecuteStmt returns a new
Query node with commandType == CMD_UTILITY and the original ExecuteStmt as
utilityStmt.
So as far as I can see the new logic is correct? A quick look & test seems to
confirm that.
Greetings,
Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Oct 19, 2012 at 2:01 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Btw, do you plan to submit that psql patch at some point? I repeatedly wished
to be able to use the extended protocol without writing code or misusing
pgbench exactly to test stuff like this.
I didn't think it would be accepted, but if you think it's useful to
have, consider it submitted. If you review the code and it seems OK
(or can be fixed to be OK), I'm happy to write some user
documentation. I'm not sure it actually handles all the cases right
now but perhaps you could have a look.
It's hard to visualize a use for this except for testing purposes, but
that might be sufficient reason to have it. One thing that would be
pretty cool is to be able to run the regression tests in extended
protocol. I hacked that up really quickly with this:
diff --git a/src/test/regress/pg_regress_main.c b/src/test/regress/pg_regress_main.c
index f8e1921e56be90bfa88707e9241afa139f72ac91..03875c917afd1b2339f91fe2b44836e7e6ed283b 100644
*** a/src/test/regress/pg_regress_main.c
--- b/src/test/regress/pg_regress_main.c
*************** psql_start_test(const char *testname,
*** 64,70 ****
"%s ", launcher); snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
! SYSTEMQUOTE "\"%s%spsql\" -X -a -q -d \"%s\" < \"%s\" > \"%s\" 2>&1" SYSTEMQUOTE,
psqldir ? psqldir : "",
psqldir ? "/" : "",
dblist->str,
--- 64,70 ----
"%s ", launcher); snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
! SYSTEMQUOTE "\"%s%spsql\" -X -v PROTOCOL=extended -a -q -d \"%s\" < \"%s\" > \"%s\" 2>&1" SYSTEMQUOTE,
psqldir ? psqldir : "",
psqldir ? "/" : "",
dblist->str,
and tried it, and saw a number of failures. Some of them were readily
explainable (such as the current query showing up in pg_cursors ---
maybe we should prevent that?) and some were not. It might be worth
investigating more carefully.
regards, tom lane
On Saturday, October 20, 2012 12:05:15 AM Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Oct 19, 2012 at 2:01 PM, Andres Freund <andres@2ndquadrant.com>
wrote:
Btw, do you plan to submit that psql patch at some point? I repeatedly
wished to be able to use the extended protocol without writing code or
misusing pgbench exactly to test stuff like this.I didn't think it would be accepted, but if you think it's useful to
have, consider it submitted. If you review the code and it seems OK
(or can be fixed to be OK), I'm happy to write some user
documentation. I'm not sure it actually handles all the cases right
now but perhaps you could have a look.It's hard to visualize a use for this except for testing purposes, but
that might be sufficient reason to have it.
Don't really see any other reason either, but that seems to be more than
enough reason for it. Theres quite a bit of code completely untested because
the regression tests only use the simple protocol. Its also really annoying to
write anything in those codepaths because it means you have to write code to
test.
But I am sure you are way much more aware of those pains than I am ;)
One thing that would be pretty cool is to be able to run the regression
tests in extended protocol. I hacked that up really quickly with this:
Not sure if we want the entire regression suite in general to run in the
extended protocol but as we easily could run parts of it that way...
\"%s\" > \"%s\" 2>&1" SYSTEMQUOTE, psqldir ? psqldir : "",
psqldir ? "/" : "",
dblist->str,and tried it, and saw a number of failures. Some of them were readily
explainable and some were not. It might be worth investigating more
carefully.
Not that I am volunteering but it sounds like a good idea to have a look
there.
(such as the current query showing up in pg_cursors --- maybe we should
prevent that?)
I don't really see an argument for preventing that.
Greetings,
Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
So as far as I can see the new logic is correct? A quick look & test seems to
confirm that.
I think the real problem here is just that the code was trying to be too
specific, and while your version might be more correct it's not doing
anything to fix that misjudgment. We should just make the
CreateTableAsStmt case look like the ExplainStmt case, viz
Assert(IsA(qry, Query));
if (qry->commandType == CMD_UTILITY)
return UtilityContainsQuery(qry->utilityStmt);
return qry;
regards, tom lane
On Saturday, October 20, 2012 12:37:54 AM Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
So as far as I can see the new logic is correct? A quick look & test
seems to confirm that.I think the real problem here is just that the code was trying to be too
specific, and while your version might be more correct it's not doing
anything to fix that misjudgment. We should just make the
CreateTableAsStmt case look like the ExplainStmt case, vizAssert(IsA(qry, Query));
if (qry->commandType == CMD_UTILITY)
return UtilityContainsQuery(qry->utilityStmt);
return qry;
FWIW commit + general principle looks good to me.
Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Oct 19, 2012 at 6:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It's hard to visualize a use for this except for testing purposes, but
that might be sufficient reason to have it. One thing that would be
pretty cool is to be able to run the regression tests in extended
protocol.
Yes, indeed. We came up with an even grottier hack to do this
internally for our fork and found a couple of bugs. It appears, for
example, that the extended protocol exercises copyfuncs/equalfuncs
support a bit better than the simple protocol, and that's certainly
something that would be good to exercise regularly.
What I think is a bit insidious about this whole thing is that the
libpq documentation is not very clear about which functions use the
simple protocol and which ones use the extended protocol; it basically
treats that as an internal implementation detail. And in theory it
should be, but there are significant performance differences between
the two and, as we're now finding out, they're not bug-compatible
either. So +1 from me on finding a way to make the regression tests
run under the extended protocol. If we do that, we should certainly
make sure the buildfarm does it regularly, because otherwise it's
quite likely to get broken again without anyone noticing.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 10/19/2012 09:05 PM, Robert Haas wrote:
On Fri, Oct 19, 2012 at 6:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It's hard to visualize a use for this except for testing purposes, but
that might be sufficient reason to have it. One thing that would be
pretty cool is to be able to run the regression tests in extended
protocol.Yes, indeed. We came up with an even grottier hack to do this
internally for our fork and found a couple of bugs. It appears, for
example, that the extended protocol exercises copyfuncs/equalfuncs
support a bit better than the simple protocol, and that's certainly
something that would be good to exercise regularly.What I think is a bit insidious about this whole thing is that the
libpq documentation is not very clear about which functions use the
simple protocol and which ones use the extended protocol; it basically
treats that as an internal implementation detail. And in theory it
should be, but there are significant performance differences between
the two and, as we're now finding out, they're not bug-compatible
either. So +1 from me on finding a way to make the regression tests
run under the extended protocol. If we do that, we should certainly
make sure the buildfarm does it regularly, because otherwise it's
quite likely to get broken again without anyone noticing.
Put this feature in psql and I'll try to take care of the rest.
cheers
andrew
Andres Freund <andres@2ndquadrant.com> writes:
On Saturday, October 20, 2012 12:05:15 AM Tom Lane wrote:
(such as the current query showing up in pg_cursors --- maybe we should
prevent that?)
I don't really see an argument for preventing that.
Well, the reason it seems peculiar to me is that the current query is in
no way a cursor --- it's just a SELECT in the cases that showed
regression test differences. I didn't go looking in the code yet, but
I suspect the pg_cursors view is displaying all Portals. Arguably, it
should only display those that were created by actual cursor commands.
regards, tom lane
Sorry It might be late here, but just wanted to share the patch
I came up with. Actually with Robert told he reported issues to
pgsql-hacker, I thought he might have also submitted patch.
PFA I came up with, but seems like issue has been already
committed.
Thanks,
On Sat, Oct 20, 2012 at 9:07 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@2ndquadrant.com> writes:
On Saturday, October 20, 2012 12:05:15 AM Tom Lane wrote:
(such as the current query showing up in pg_cursors --- maybe we should
prevent that?)I don't really see an argument for preventing that.
Well, the reason it seems peculiar to me is that the current query is in
no way a cursor --- it's just a SELECT in the cases that showed
regression test differences. I didn't go looking in the code yet, but
I suspect the pg_cursors view is displaying all Portals. Arguably, it
should only display those that were created by actual cursor commands.regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Rushabh Lathia
Attachments:
assertion_pg.patchapplication/octet-stream; name=assertion_pg.patchDownload
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 39bdb19..bdbbb63 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1842,9 +1842,10 @@ QueryReturnsTuples(Query *parsetree)
* We assume it is invoked only on already-parse-analyzed statements
* (else the contained parsetree isn't a Query yet).
*
- * In some cases (currently, only EXPLAIN of CREATE TABLE AS/SELECT INTO),
- * potentially Query-containing utility statements can be nested. This
- * function will drill down to a non-utility Query, or return NULL if none.
+ * In some cases (currently, only EXPLAIN of CREATE TABLE AS/SELECT INTO and
+ * CREATE TEMPORARY TABLE AS/EXECUTE), potentially Query-containing utility
+ * statements can be nested. This function will drill down to a non-utility
+ * Query, or return NULL if none.
*/
Query *
UtilityContainsQuery(Node *parsetree)
@@ -1865,8 +1866,14 @@ UtilityContainsQuery(Node *parsetree)
qry = (Query *) ((CreateTableAsStmt *) parsetree)->query;
if (IsA(qry, Query))
{
- /* Recursion currently can't be necessary here */
- Assert(qry->commandType != CMD_UTILITY);
+ /*
+ * For CREATE TEMPORARY TABLE as EXECUTE commandtype will be
+ * CMD_UTILITY, so call function recursively to get the
+ * contained query.
+ */
+ if (qry->commandType == CMD_UTILITY)
+ return UtilityContainsQuery(qry->utilityStmt);
+
return qry;
}
Assert(IsA(qry, ExecuteStmt));