Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Restructure SELECT INTO's parsetree representation into CreateTableAsStmt.

  1. assertion failure w/extended query protocol

    Robert Haas <robertmhaas@gmail.com> — 2012-10-19T16:19:36Z

    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

  2. Re: assertion failure w/extended query protocol

    Peter Geoghegan <peter@2ndquadrant.com> — 2012-10-19T16:41:27Z

    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

  3. Re: assertion failure w/extended query protocol

    Andres Freund <andres@2ndquadrant.com> — 2012-10-19T16:59:33Z

    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

  4. Re: assertion failure w/extended query protocol

    Andres Freund <andres@2ndquadrant.com> — 2012-10-19T18:01:14Z

    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

  5. Re: assertion failure w/extended query protocol

    Peter Geoghegan <peter@2ndquadrant.com> — 2012-10-19T18:07:59Z

    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

  6. Re: assertion failure w/extended query protocol

    Robert Haas <robertmhaas@gmail.com> — 2012-10-19T18:28:45Z

    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

  7. Re: assertion failure w/extended query protocol

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-19T18:45:16Z

    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

  8. Re: assertion failure w/extended query protocol

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-19T19:05:30Z

    Andres Freund <andres@2ndquadrant.com> writes:
> Simple fix attached. 

Are you sure this isn't just moving the failure conditions around?

			regards, tom lane

  9. Re: assertion failure w/extended query protocol

    Andres Freund <andres@2ndquadrant.com> — 2012-10-19T19:24:25Z

    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

  10. Re: assertion failure w/extended query protocol

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-19T22:05:15Z

    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

  11. Re: assertion failure w/extended query protocol

    Andres Freund <andres@2ndquadrant.com> — 2012-10-19T22:26:28Z

    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

  12. Re: assertion failure w/extended query protocol

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-19T22:37:54Z

    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

  13. Re: assertion failure w/extended query protocol

    Andres Freund <andres@2ndquadrant.com> — 2012-10-19T22:46:10Z

    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

  14. Re: assertion failure w/extended query protocol

    Robert Haas <robertmhaas@gmail.com> — 2012-10-20T01:05:16Z

    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

  15. Re: assertion failure w/extended query protocol

    Andrew Dunstan <andrew@dunslane.net> — 2012-10-20T02:19:11Z

    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

  16. Re: assertion failure w/extended query protocol

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-20T03:37:07Z

    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

  17. Re: assertion failure w/extended query protocol

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2012-10-22T05:24:03Z

    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