Обсуждение: 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
Вложения
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)f
>
> I'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
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)f
>>
>> I'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
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: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
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, viz > > Assert(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,
--
Rushabh Lathia
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 shouldWell, the reason it seems peculiar to me is that the current query is in
>> prevent that?)
> I don't really see an argument for preventing that.
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