plpgsql: open for execute - add USING clause
Hello,
this small patch add missing USING clause to OPEN FOR EXECUTE statement
+ cleaning part of exec_stmt_open function
see http://archives.postgresql.org/pgsql-hackers/2009-11/msg00713.php
Regards
Pavel Stehule
Attachments:
openexecusing.difftext/x-patch; charset=US-ASCII; name=openexecusing.diffDownload
*** ./doc/src/sgml/plpgsql.sgml.orig 2009-11-13 23:43:39.000000000 +0100
--- ./doc/src/sgml/plpgsql.sgml 2009-11-17 20:30:10.656208300 +0100
***************
*** 2488,2494 ****
<title><command>OPEN FOR EXECUTE</command></title>
<synopsis>
! OPEN <replaceable>unbound_cursorvar</replaceable> <optional> <optional> NO </optional> SCROLL </optional> FOR EXECUTE <replaceable class="command">query_string</replaceable>;
</synopsis>
<para>
--- 2488,2494 ----
<title><command>OPEN FOR EXECUTE</command></title>
<synopsis>
! OPEN <replaceable>unbound_cursorvar</replaceable> <optional> <optional> NO </optional> SCROLL </optional> FOR EXECUTE <replaceable class="command">query_string</replaceable> <optional> USING <replaceable>expression</replaceable> <optional>, ...</optional> </optional>;
</synopsis>
<para>
***************
*** 2500,2506 ****
command. As usual, this gives flexibility so the query plan can vary
from one run to the next (see <xref linkend="plpgsql-plan-caching">),
and it also means that variable substitution is not done on the
! command string.
The <literal>SCROLL</> and
<literal>NO SCROLL</> options have the same meanings as for a bound
cursor.
--- 2500,2507 ----
command. As usual, this gives flexibility so the query plan can vary
from one run to the next (see <xref linkend="plpgsql-plan-caching">),
and it also means that variable substitution is not done on the
! command string. As with <command>EXECUTE</command>, parameter values
! can be inserted into the dynamic command via <literal>USING</>.
The <literal>SCROLL</> and
<literal>NO SCROLL</> options have the same meanings as for a bound
cursor.
***************
*** 2509,2515 ****
<para>
An example:
<programlisting>
! OPEN curs1 FOR EXECUTE 'SELECT * FROM ' || quote_ident($1);
</programlisting>
</para>
</sect3>
--- 2510,2516 ----
<para>
An example:
<programlisting>
! OPEN curs1 FOR EXECUTE 'SELECT * FROM ' || quote_ident($1) ' WHERE col1 = $1' USING var1;
</programlisting>
</para>
</sect3>
*** ./src/pl/plpgsql/src/gram.y.orig 2009-11-13 23:43:40.000000000 +0100
--- ./src/pl/plpgsql/src/gram.y 2009-11-17 14:37:33.927208178 +0100
***************
*** 1686,1692 ****
tok = yylex();
if (tok == K_EXECUTE)
{
! new->dynquery = read_sql_stmt("SELECT ");
}
else
{
--- 1686,1712 ----
tok = yylex();
if (tok == K_EXECUTE)
{
! int endtoken;
!
! new->dynquery = read_sql_construct(K_USING, ';', 0,
! "USING or ;",
! "SELECT ",
! true, true,
! NULL, &endtoken);
!
! /* If we found "USING", collect the argument(s) */
! if (endtoken == K_USING)
! {
! PLpgSQL_expr *expr;
!
! do
! {
! expr = read_sql_expression2(',', ';',
! ", or ;",
! &endtoken);
! new->params = lappend(new->params, expr);
! } while (endtoken == ',');
! }
}
else
{
*** ./src/pl/plpgsql/src/pl_exec.c.orig 2009-11-09 01:26:55.000000000 +0100
--- ./src/pl/plpgsql/src/pl_exec.c 2009-11-17 19:48:47.209207349 +0100
***************
*** 199,206 ****
static PreparedParamsData *exec_eval_using_params(PLpgSQL_execstate *estate,
List *params);
static void free_params_data(PreparedParamsData *ppd);
! static Portal exec_dynquery_with_params(PLpgSQL_execstate *estate,
! PLpgSQL_expr *query, List *params);
/* ----------
--- 199,206 ----
static PreparedParamsData *exec_eval_using_params(PLpgSQL_execstate *estate,
List *params);
static void free_params_data(PreparedParamsData *ppd);
! static Portal exec_dynquery_with_params(PLpgSQL_execstate *estate, char *posrtalname,
! PLpgSQL_expr *query, List *params, int cursorOption);
/* ----------
***************
*** 2343,2350 ****
{
/* RETURN QUERY EXECUTE */
Assert(stmt->dynquery != NULL);
! portal = exec_dynquery_with_params(estate, stmt->dynquery,
! stmt->params);
}
tupmap = convert_tuples_by_position(portal->tupDesc,
--- 2343,2350 ----
{
/* RETURN QUERY EXECUTE */
Assert(stmt->dynquery != NULL);
! portal = exec_dynquery_with_params(estate, NULL, stmt->dynquery,
! stmt->params, 0);
}
tupmap = convert_tuples_by_position(portal->tupDesc,
***************
*** 3123,3129 ****
Portal portal;
int rc;
! portal = exec_dynquery_with_params(estate, stmt->query, stmt->params);
/*
* Execute the loop
--- 3123,3129 ----
Portal portal;
int rc;
! portal = exec_dynquery_with_params(estate, NULL, stmt->query, stmt->params, 0);
/*
* Execute the loop
***************
*** 3191,3234 ****
* This is an OPEN refcursor FOR EXECUTE ...
* ----------
*/
! Datum queryD;
! Oid restype;
! char *querystr;
! SPIPlanPtr curplan;
!
! /* ----------
! * We evaluate the string expression after the
! * EXECUTE keyword. It's result is the querystring we have
! * to execute.
! * ----------
! */
! queryD = exec_eval_expr(estate, stmt->dynquery, &isnull, &restype);
! if (isnull)
! ereport(ERROR,
! (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
! errmsg("query string argument of EXECUTE is null")));
!
! /* Get the C-String representation */
! querystr = convert_value_to_string(queryD, restype);
!
! exec_eval_cleanup(estate);
!
! /* ----------
! * Now we prepare a query plan for it and open a cursor
! * ----------
! */
! curplan = SPI_prepare_cursor(querystr, 0, NULL, stmt->cursor_options);
! if (curplan == NULL)
! elog(ERROR, "SPI_prepare_cursor failed for \"%s\": %s",
! querystr, SPI_result_code_string(SPI_result));
! portal = SPI_cursor_open(curname, curplan, NULL, NULL,
! estate->readonly_func);
! if (portal == NULL)
! elog(ERROR, "could not open cursor for query \"%s\": %s",
! querystr, SPI_result_code_string(SPI_result));
! pfree(querystr);
! SPI_freeplan(curplan);
!
/*
* If cursor variable was NULL, store the generated portal name in it
*/
--- 3191,3201 ----
* This is an OPEN refcursor FOR EXECUTE ...
* ----------
*/
! portal = exec_dynquery_with_params(estate, curname,
! stmt->dynquery,
! stmt->params,
! stmt->cursor_options);
!
/*
* If cursor variable was NULL, store the generated portal name in it
*/
***************
*** 5520,5527 ****
* Open portal for dynamic query
*/
static Portal
! exec_dynquery_with_params(PLpgSQL_execstate *estate, PLpgSQL_expr *dynquery,
! List *params)
{
Portal portal;
Datum query;
--- 5487,5496 ----
* Open portal for dynamic query
*/
static Portal
! exec_dynquery_with_params(PLpgSQL_execstate *estate, char *portalname,
! PLpgSQL_expr *dynquery,
! List *params,
! int cursorOption)
{
Portal portal;
Datum query;
***************
*** 5554,5573 ****
PreparedParamsData *ppd;
ppd = exec_eval_using_params(estate, params);
! portal = SPI_cursor_open_with_args(NULL,
querystr,
ppd->nargs, ppd->types,
ppd->values, ppd->nulls,
! estate->readonly_func, 0);
free_params_data(ppd);
}
else
{
! portal = SPI_cursor_open_with_args(NULL,
querystr,
0, NULL,
NULL, NULL,
! estate->readonly_func, 0);
}
if (portal == NULL)
--- 5523,5544 ----
PreparedParamsData *ppd;
ppd = exec_eval_using_params(estate, params);
! portal = SPI_cursor_open_with_args(portalname,
querystr,
ppd->nargs, ppd->types,
ppd->values, ppd->nulls,
! estate->readonly_func,
! cursorOption);
free_params_data(ppd);
}
else
{
! portal = SPI_cursor_open_with_args(portalname,
querystr,
0, NULL,
NULL, NULL,
! estate->readonly_func,
! cursorOption);
}
if (portal == NULL)
*** ./src/pl/plpgsql/src/pl_funcs.c.orig 2009-11-12 01:13:00.000000000 +0100
--- ./src/pl/plpgsql/src/pl_funcs.c 2009-11-17 21:03:12.172210054 +0100
***************
*** 619,627 ****
printf(" execute = '");
dump_expr(stmt->dynquery);
printf("'\n");
}
dump_indent -= 2;
-
}
static void
--- 619,646 ----
printf(" execute = '");
dump_expr(stmt->dynquery);
printf("'\n");
+
+ if (stmt->params != NIL)
+ {
+ ListCell *lc;
+ int i;
+
+ dump_indent += 2;
+ dump_ind();
+ printf(" USING\n");
+ dump_indent += 2;
+ i = 1;
+ foreach(lc, stmt->params)
+ {
+ dump_ind();
+ printf(" parameter $%d: ", i++);
+ dump_expr((PLpgSQL_expr *) lfirst(lc));
+ printf("\n");
+ }
+ dump_indent -= 4;
+ }
}
dump_indent -= 2;
}
static void
*** ./src/pl/plpgsql/src/plpgsql.h.orig 2009-11-13 23:43:42.000000000 +0100
--- ./src/pl/plpgsql/src/plpgsql.h 2009-11-17 16:35:35.075209934 +0100
***************
*** 503,508 ****
--- 503,509 ----
PLpgSQL_expr *argquery;
PLpgSQL_expr *query;
PLpgSQL_expr *dynquery;
+ List *params; /* USING expressions */
} PLpgSQL_stmt_open;
*** ./src/test/regress/expected/plpgsql.out.orig 2009-11-13 23:43:42.000000000 +0100
--- ./src/test/regress/expected/plpgsql.out 2009-11-17 20:11:14.000000000 +0100
***************
*** 3189,3194 ****
--- 3189,3223 ----
26
(1 row)
+ drop function exc_using(int, text);
+ create or replace function exc_using(int) returns void as $$
+ declare
+ c refcursor;
+ i int;
+ begin
+ open c for execute 'select * from generate_series(1,$1)' using $1+1;
+ loop
+ fetch c into i;
+ exit when not found;
+ raise notice '%', i;
+ end loop;
+ close c;
+ return;
+ end;
+ $$ language plpgsql;
+ select exc_using(5);
+ NOTICE: 1
+ NOTICE: 2
+ NOTICE: 3
+ NOTICE: 4
+ NOTICE: 5
+ NOTICE: 6
+ exc_using
+ -----------
+
+ (1 row)
+
+ drop function exc_using(int);
-- test FOR-over-cursor
create or replace function forc01() returns void as $$
declare
*** ./src/test/regress/sql/plpgsql.sql.orig 2009-11-13 23:43:42.000000000 +0100
--- ./src/test/regress/sql/plpgsql.sql 2009-11-17 20:09:57.030208080 +0100
***************
*** 2629,2634 ****
--- 2629,2656 ----
select exc_using(5, 'foobar');
+ drop function exc_using(int, text);
+
+ create or replace function exc_using(int) returns void as $$
+ declare
+ c refcursor;
+ i int;
+ begin
+ open c for execute 'select * from generate_series(1,$1)' using $1+1;
+ loop
+ fetch c into i;
+ exit when not found;
+ raise notice '%', i;
+ end loop;
+ close c;
+ return;
+ end;
+ $$ language plpgsql;
+
+ select exc_using(5);
+
+ drop function exc_using(int);
+
-- test FOR-over-cursor
create or replace function forc01() returns void as $$
On Tue, Nov 17, 2009 at 3:04 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Hello,
this small patch add missing USING clause to OPEN FOR EXECUTE statement
+ cleaning part of exec_stmt_open functionsee http://archives.postgresql.org/pgsql-hackers/2009-11/msg00713.php
This is now the fourth patch you've submitted since the start of the
CommitFest...
...Robert
2009/11/17 Robert Haas <robertmhaas@gmail.com>:
On Tue, Nov 17, 2009 at 3:04 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Hello,
this small patch add missing USING clause to OPEN FOR EXECUTE statement
+ cleaning part of exec_stmt_open functionsee http://archives.postgresql.org/pgsql-hackers/2009-11/msg00713.php
This is now the fourth patch you've submitted since the start of the
CommitFest...
These patches are for next commitfest. What I know, the current
commitfest is closed for new patches. Is it ok?
Pavel
Show quoted text
...Robert
2009/11/17 Pavel Stehule <pavel.stehule@gmail.com>:
2009/11/17 Robert Haas <robertmhaas@gmail.com>:
On Tue, Nov 17, 2009 at 3:04 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Hello,
this small patch add missing USING clause to OPEN FOR EXECUTE statement
+ cleaning part of exec_stmt_open functionsee http://archives.postgresql.org/pgsql-hackers/2009-11/msg00713.php
This is now the fourth patch you've submitted since the start of the
CommitFest...These patches are for next commitfest. What I know, the current
commitfest is closed for new patches. Is it ok?
typmode support is for this commitfest. Others for next commitfest.
Pavel
Show quoted text
Pavel
...Robert
Pavel Stehule <pavel.stehule@gmail.com> wrote:
2009/11/17 Robert Haas <robertmhaas@gmail.com>:
This is now the fourth patch you've submitted since the start of
the CommitFest...These patches are for next commitfest. What I know, the current
commitfest is closed for new patches. Is it ok?
Until this moment I was unconvinced of the need for a strict rule that
patches from regular submitters who don't suspend patch development
to contribute to the commitfest reviews should be ignored.
-Kevin
2009/11/17 Kevin Grittner <Kevin.Grittner@wicourts.gov>:
Pavel Stehule <pavel.stehule@gmail.com> wrote:
2009/11/17 Robert Haas <robertmhaas@gmail.com>:
This is now the fourth patch you've submitted since the start of
the CommitFest...These patches are for next commitfest. What I know, the current
commitfest is closed for new patches. Is it ok?Until this moment I was unconvinced of the need for a strict rule that
patches from regular submitters who don't suspend patch development
to contribute to the commitfest reviews should be ignored.
what is wrong?
Patches typmodes for functions and enhancing psql was notificated in proposal:
*http://archives.postgresql.org/pgsql-hackers/2009-11/msg00934.php
*http://archives.postgresql.org/pgsql-hackers/2009-10/msg00519.php
(more than one moth old)
patch for OPEN EXECUTE USING is reaction on Tom's mail
http://archives.postgresql.org/pgsql-hackers/2009-11/msg00713.php (for
me it is like a proposal) and this patch is +/- bugfix.
I don't wont to apply these patches tomorrow, I don't sending these
patches for last moment. If I have to wait one weak or two weeks, ok.
Declare it. I'll respect it. But actually I respecting all rules, what
I know.
I don't would to generate thousand patches now. Simply I have a time
for postgres now. I wrote three patches. And I put it to commitfest,
because I thing so this work is serious. So anybody can comment it, so
anybody can test it. I put it to commitfest application, because this
code is finished (or finished for reviewing) and I would lost these
patches in this mailing list. Tomorrow I could be killed (maybe),
tomorrow I could to lost data in my hardisc. I have not other patches.
Don't afraid.
Pavel
Show quoted text
-Kevin
On Tue, 2009-11-17 at 14:33 -0600, Kevin Grittner wrote:
Pavel Stehule <pavel.stehule@gmail.com> wrote:
2009/11/17 Robert Haas <robertmhaas@gmail.com>:
This is now the fourth patch you've submitted since the start of
the CommitFest...These patches are for next commitfest. What I know, the current
commitfest is closed for new patches. Is it ok?Until this moment I was unconvinced of the need for a strict rule that
patches from regular submitters who don't suspend patch development
to contribute to the commitfest reviews should be ignored.
I agree they should be ignored until the NEXT commitfest. I do not agree
that they should be dropped into a bucket.
Joshua D. Drake
-Kevin
--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering
If the world pushes look it in the eye and GRR. Then push back harder. - Salamander
2009/11/17 Joshua D. Drake <jd@commandprompt.com>:
On Tue, 2009-11-17 at 14:33 -0600, Kevin Grittner wrote:
Pavel Stehule <pavel.stehule@gmail.com> wrote:
2009/11/17 Robert Haas <robertmhaas@gmail.com>:
This is now the fourth patch you've submitted since the start of
the CommitFest...These patches are for next commitfest. What I know, the current
commitfest is closed for new patches. Is it ok?Until this moment I was unconvinced of the need for a strict rule that
patches from regular submitters who don't suspend patch development
to contribute to the commitfest reviews should be ignored.I agree they should be ignored until the NEXT commitfest. I do not agree
that they should be dropped into a bucket.
I never sent these (last two) patches to THIS commitfest. Is it clean?
I am maybe crazy, but I know when commitfest starting. Have I next
month be quite? First patch I resent, because patch was broken. But
this patch was sent to 2009-11-04. Really, I don't would to push my
patches to this commitfest.
Pavel
Show quoted text
Joshua D. Drake
-Kevin
--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering
If the world pushes look it in the eye and GRR. Then push back harder. - Salamander
Pavel Stehule <pavel.stehule@gmail.com> wrote:
I never sent these (last two) patches to THIS commitfest. Is it
clean?
Counting the "In Progress" commitfest and the two preceding ones, you
have submitted nine patches and contributed to the review of none.
Surely you noticed recent threads about how the review and commit
steps are the bottleneck, help is desperately needed for the review
process, and the point of commitfests is to get everyone to take a
break from coding to help review the work of others? Several regular
contributors have expressed frustration that while they are taking
time off from their preferred activity of coding to contribute to the
review process, others are stacking up a pile of patches for the next
review cycle.
Robert in particular has been burning himself out trying to keep the
patch reviews rolling through so that everyone's patches can get
proper consideration. I certainly appreciate that you are making
contributions of patches to help make PostgreSQL better; but since the
review process is the bottleneck, if you don't help review patches,
any time spent by someone reviewing your patches comes out of the time
they would be writing patches themselves.
I'm sure it would be much appreciated, and help to alleviate the
frustration and burnout of some other contributors, if you could take
a turn at reviewing -- at least one patch each commitfest.
-Kevin
On Tue, 2009-11-17 at 15:40 -0600, Kevin Grittner wrote:
Pavel Stehule <pavel.stehule@gmail.com> wrote:
I never sent these (last two) patches to THIS commitfest. Is it
clean?I'm sure it would be much appreciated, and help to alleviate the
frustration and burnout of some other contributors, if you could take
a turn at reviewing -- at least one patch each commitfest.
In short Pavel,
Nobody is complaining about your patches. It would just be really nice
if you could help review some existing patches in this commit fest.
Would you be willing to do so?
Sincerely,
Joshua D. Drake
--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering
If the world pushes look it in the eye and GRR. Then push back harder. - Salamander
Pavel Stehule wrote:
I don't wont to apply these patches tomorrow, I don't sending these
patches for last moment. If I have to wait one weak or two weeks, ok.
Declare it. I'll respect it. But actually I respecting all rules, what
I know.
If you're sending stuff intended for the next CommitFest in the middle
of an active one (which we'd prefer not to see at all but you have your
own schedule limitations), it would be helpful if you were to label
those patches as such. It's difficult for the rest of us to tell which
of the ones you're generating are in response to patches that are active
during this one, and which are intended for future review but you're
just dropping them off now. Had your new stuff been labeled "This is
for the next CommitFest, I'm just sending it to the list now", it would
have made it easier on everyone else to figure out which of your
messages we need to pay attention to and what should be ignored for now.
--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.com
On Tue, Nov 17, 2009 at 5:34 PM, Greg Smith <greg@2ndquadrant.com> wrote:
Pavel Stehule wrote:
I don't wont to apply these patches tomorrow, I don't sending these
patches for last moment. If I have to wait one weak or two weeks, ok.
Declare it. I'll respect it. But actually I respecting all rules, what
I know.If you're sending stuff intended for the next CommitFest in the middle of an
active one (which we'd prefer not to see at all but you have your own
schedule limitations), it would be helpful if you were to label those
patches as such. It's difficult for the rest of us to tell which of the
ones you're generating are in response to patches that are active during
this one, and which are intended for future review but you're just dropping
them off now. Had your new stuff been labeled "This is for the next
CommitFest, I'm just sending it to the list now", it would have made it
easier on everyone else to figure out which of your messages we need to pay
attention to and what should be ignored for now.
This expresses my feelings on the topic exactly, and perhaps merits
inclusion in a Wiki page someplace. Maybe we need to have a wiki page
on commitfest rules & expectations.
...Robert
Robert Haas wrote:
This expresses my feelings on the topic exactly, and perhaps merits
inclusion in a Wiki page someplace. Maybe we need to have a wiki page
on commitfest rules & expectations.
I put a note at
http://wiki.postgresql.org/wiki/Submitting_a_Patch#Submission_timing
which seems the logical place to warn people about patch submission
guidelines; there was already one there about avoiding submissions
during the beta I moved into the new section.
There's still some debris from the old wiki-based CommitFest approach
floating around the wiki that makes it harder to figure out how things
fit together than it should be. I started cleaning that up with
refreshing http://wiki.postgresql.org/wiki/CommitFest , which is
probably the right place to document general rules and expectations better.
--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.com
2009/11/17 Joshua D. Drake <jd@commandprompt.com>:
On Tue, 2009-11-17 at 15:40 -0600, Kevin Grittner wrote:
Pavel Stehule <pavel.stehule@gmail.com> wrote:
I never sent these (last two) patches to THIS commitfest. Is it
clean?I'm sure it would be much appreciated, and help to alleviate the
frustration and burnout of some other contributors, if you could take
a turn at reviewing -- at least one patch each commitfest.In short Pavel,
Nobody is complaining about your patches. It would just be really nice
if you could help review some existing patches in this commit fest.
Would you be willing to do so?
I understand so there are missing people who can do a review. I could
to help with plpgsql or psql code - or some catalog code.
Pavel
Show quoted text
Sincerely,
Joshua D. Drake
--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering
If the world pushes look it in the eye and GRR. Then push back harder. - Salamander
2009/11/17 Robert Haas <robertmhaas@gmail.com>:
On Tue, Nov 17, 2009 at 5:34 PM, Greg Smith <greg@2ndquadrant.com> wrote:
Pavel Stehule wrote:
I don't wont to apply these patches tomorrow, I don't sending these
patches for last moment. If I have to wait one weak or two weeks, ok.
Declare it. I'll respect it. But actually I respecting all rules, what
I know.If you're sending stuff intended for the next CommitFest in the middle of an
active one (which we'd prefer not to see at all but you have your own
schedule limitations), it would be helpful if you were to label those
patches as such. It's difficult for the rest of us to tell which of the
ones you're generating are in response to patches that are active during
this one, and which are intended for future review but you're just dropping
them off now. Had your new stuff been labeled "This is for the next
CommitFest, I'm just sending it to the list now", it would have made it
easier on everyone else to figure out which of your messages we need to pay
attention to and what should be ignored for now.This expresses my feelings on the topic exactly, and perhaps merits
inclusion in a Wiki page someplace. Maybe we need to have a wiki page
on commitfest rules & expectations.
Ok, It's my mistake. I didn't would to attack anybody. I though so is
sufficient information is registration in commitfest application.
Patch in mailing list is one thing, but registration in second -
crucial. And when commitfest is closed, then is clean, so new patches
goes to next commitfest. I agree - It should frustrating - and it
means some work more (for reades of mailing list). I have not a
problem with labeling, when patch isn't used for current commitfest.
Pavel
Show quoted text
...Robert
Hi, I'm reviewing OPEN FOR EXECUTE USING patch and have a couple of
trivial comments.
Pavel Stehule <pavel.stehule@gmail.com> wrote:
this small patch add missing USING clause to OPEN FOR EXECUTE statement
+ cleaning part of exec_stmt_open function
- Can we use read_sql_expression2() instead of read_sql_construct()
in gram.y? It could simplify the code a bit.
- I'd prefer to change the new argument for exec_dynquery_with_params()
from "char *portalname" to "const char *curname".
Other than the above minor issues, this patch is ready to commit.
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
2010/1/12 Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp>:
Hi, I'm reviewing OPEN FOR EXECUTE USING patch and have a couple of
trivial comments.Pavel Stehule <pavel.stehule@gmail.com> wrote:
this small patch add missing USING clause to OPEN FOR EXECUTE statement
+ cleaning part of exec_stmt_open function- Can we use read_sql_expression2() instead of read_sql_construct()
in gram.y? It could simplify the code a bit.- I'd prefer to change the new argument for exec_dynquery_with_params()
from "char *portalname" to "const char *curname".
ok, I accept all comments.
revised version are attached.
Thank you,
Pavel Stehule
Show quoted text
Other than the above minor issues, this patch is ready to commit.
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
Attachments:
openexecusing-after-rev.diffapplication/octet-stream; name=openexecusing-after-rev.diffDownload
*** ./doc/src/sgml/plpgsql.sgml.orig 2009-12-28 20:11:51.000000000 +0100
--- ./doc/src/sgml/plpgsql.sgml 2010-01-12 15:54:24.451578961 +0100
***************
*** 2495,2501 ****
<title><command>OPEN FOR EXECUTE</command></title>
<synopsis>
! OPEN <replaceable>unbound_cursorvar</replaceable> <optional> <optional> NO </optional> SCROLL </optional> FOR EXECUTE <replaceable class="command">query_string</replaceable>;
</synopsis>
<para>
--- 2495,2501 ----
<title><command>OPEN FOR EXECUTE</command></title>
<synopsis>
! OPEN <replaceable>unbound_cursorvar</replaceable> <optional> <optional> NO </optional> SCROLL </optional> FOR EXECUTE <replaceable class="command">query_string</replaceable> <optional> USING <replaceable>expression</replaceable> <optional>, ...</optional> </optional>;
</synopsis>
<para>
***************
*** 2507,2513 ****
command. As usual, this gives flexibility so the query plan can vary
from one run to the next (see <xref linkend="plpgsql-plan-caching">),
and it also means that variable substitution is not done on the
! command string.
The <literal>SCROLL</> and
<literal>NO SCROLL</> options have the same meanings as for a bound
cursor.
--- 2507,2514 ----
command. As usual, this gives flexibility so the query plan can vary
from one run to the next (see <xref linkend="plpgsql-plan-caching">),
and it also means that variable substitution is not done on the
! command string. As with <command>EXECUTE</command>, parameter values
! can be inserted into the dynamic command via <literal>USING</>.
The <literal>SCROLL</> and
<literal>NO SCROLL</> options have the same meanings as for a bound
cursor.
***************
*** 2516,2522 ****
<para>
An example:
<programlisting>
! OPEN curs1 FOR EXECUTE 'SELECT * FROM ' || quote_ident($1);
</programlisting>
</para>
</sect3>
--- 2517,2523 ----
<para>
An example:
<programlisting>
! OPEN curs1 FOR EXECUTE 'SELECT * FROM ' || quote_ident($1) ' WHERE col1 = $1' USING var1;
</programlisting>
</para>
</sect3>
*** ./src/pl/plpgsql/src/gram.y.orig 2010-01-10 18:56:50.000000000 +0100
--- ./src/pl/plpgsql/src/gram.y 2010-01-13 08:50:45.974122460 +0100
***************
*** 1704,1710 ****
tok = yylex();
if (tok == K_EXECUTE)
{
! new->dynquery = read_sql_stmt("SELECT ");
}
else
{
--- 1704,1728 ----
tok = yylex();
if (tok == K_EXECUTE)
{
! int endtoken;
!
! new->dynquery = read_sql_expression2(K_USING, ';',
! "USING or ;",
! &endtoken);
!
! /* If we found "USING", collect the argument(s) */
! if (endtoken == K_USING)
! {
! PLpgSQL_expr *expr;
!
! do
! {
! expr = read_sql_expression2(',', ';',
! ", or ;",
! &endtoken);
! new->params = lappend(new->params, expr);
! } while (endtoken == ',');
! }
}
else
{
*** ./src/pl/plpgsql/src/pl_exec.c.orig 2010-01-02 17:58:13.000000000 +0100
--- ./src/pl/plpgsql/src/pl_exec.c 2010-01-13 09:01:30.137122346 +0100
***************
*** 199,206 ****
static PreparedParamsData *exec_eval_using_params(PLpgSQL_execstate *estate,
List *params);
static void free_params_data(PreparedParamsData *ppd);
! static Portal exec_dynquery_with_params(PLpgSQL_execstate *estate,
! PLpgSQL_expr *query, List *params);
/* ----------
--- 199,206 ----
static PreparedParamsData *exec_eval_using_params(PLpgSQL_execstate *estate,
List *params);
static void free_params_data(PreparedParamsData *ppd);
! static Portal exec_dynquery_with_params(PLpgSQL_execstate *estate, const char *curname,
! PLpgSQL_expr *query, List *params, int cursorOption);
/* ----------
***************
*** 2336,2343 ****
{
/* RETURN QUERY EXECUTE */
Assert(stmt->dynquery != NULL);
! portal = exec_dynquery_with_params(estate, stmt->dynquery,
! stmt->params);
}
tupmap = convert_tuples_by_position(portal->tupDesc,
--- 2336,2343 ----
{
/* RETURN QUERY EXECUTE */
Assert(stmt->dynquery != NULL);
! portal = exec_dynquery_with_params(estate, NULL, stmt->dynquery,
! stmt->params, 0);
}
tupmap = convert_tuples_by_position(portal->tupDesc,
***************
*** 3133,3139 ****
Portal portal;
int rc;
! portal = exec_dynquery_with_params(estate, stmt->query, stmt->params);
/*
* Execute the loop
--- 3133,3139 ----
Portal portal;
int rc;
! portal = exec_dynquery_with_params(estate, NULL, stmt->query, stmt->params, 0);
/*
* Execute the loop
***************
*** 3161,3167 ****
PLpgSQL_expr *query;
Portal portal;
ParamListInfo paramLI;
- bool isnull;
/* ----------
* Get the cursor variable and if it has an assigned name, check
--- 3161,3166 ----
***************
*** 3201,3244 ****
* This is an OPEN refcursor FOR EXECUTE ...
* ----------
*/
! Datum queryD;
! Oid restype;
! char *querystr;
! SPIPlanPtr curplan;
!
! /* ----------
! * We evaluate the string expression after the
! * EXECUTE keyword. It's result is the querystring we have
! * to execute.
! * ----------
! */
! queryD = exec_eval_expr(estate, stmt->dynquery, &isnull, &restype);
! if (isnull)
! ereport(ERROR,
! (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
! errmsg("query string argument of EXECUTE is null")));
!
! /* Get the C-String representation */
! querystr = convert_value_to_string(queryD, restype);
!
! exec_eval_cleanup(estate);
!
! /* ----------
! * Now we prepare a query plan for it and open a cursor
! * ----------
! */
! curplan = SPI_prepare_cursor(querystr, 0, NULL, stmt->cursor_options);
! if (curplan == NULL)
! elog(ERROR, "SPI_prepare_cursor failed for \"%s\": %s",
! querystr, SPI_result_code_string(SPI_result));
! portal = SPI_cursor_open(curname, curplan, NULL, NULL,
! estate->readonly_func);
! if (portal == NULL)
! elog(ERROR, "could not open cursor for query \"%s\": %s",
! querystr, SPI_result_code_string(SPI_result));
! pfree(querystr);
! SPI_freeplan(curplan);
!
/*
* If cursor variable was NULL, store the generated portal name in it
*/
--- 3200,3210 ----
* This is an OPEN refcursor FOR EXECUTE ...
* ----------
*/
! portal = exec_dynquery_with_params(estate, curname,
! stmt->dynquery,
! stmt->params,
! stmt->cursor_options);
!
/*
* If cursor variable was NULL, store the generated portal name in it
*/
***************
*** 5530,5537 ****
* Open portal for dynamic query
*/
static Portal
! exec_dynquery_with_params(PLpgSQL_execstate *estate, PLpgSQL_expr *dynquery,
! List *params)
{
Portal portal;
Datum query;
--- 5496,5505 ----
* Open portal for dynamic query
*/
static Portal
! exec_dynquery_with_params(PLpgSQL_execstate *estate, const char *curname,
! PLpgSQL_expr *dynquery,
! List *params,
! int cursorOption)
{
Portal portal;
Datum query;
***************
*** 5564,5583 ****
PreparedParamsData *ppd;
ppd = exec_eval_using_params(estate, params);
! portal = SPI_cursor_open_with_args(NULL,
querystr,
ppd->nargs, ppd->types,
ppd->values, ppd->nulls,
! estate->readonly_func, 0);
free_params_data(ppd);
}
else
{
! portal = SPI_cursor_open_with_args(NULL,
querystr,
0, NULL,
NULL, NULL,
! estate->readonly_func, 0);
}
if (portal == NULL)
--- 5532,5553 ----
PreparedParamsData *ppd;
ppd = exec_eval_using_params(estate, params);
! portal = SPI_cursor_open_with_args(curname,
querystr,
ppd->nargs, ppd->types,
ppd->values, ppd->nulls,
! estate->readonly_func,
! cursorOption);
free_params_data(ppd);
}
else
{
! portal = SPI_cursor_open_with_args(curname,
querystr,
0, NULL,
NULL, NULL,
! estate->readonly_func,
! cursorOption);
}
if (portal == NULL)
*** ./src/pl/plpgsql/src/pl_funcs.c.orig 2010-01-12 15:54:05.281703649 +0100
--- ./src/pl/plpgsql/src/pl_funcs.c 2010-01-12 15:54:24.475579422 +0100
***************
*** 619,627 ****
printf(" execute = '");
dump_expr(stmt->dynquery);
printf("'\n");
}
dump_indent -= 2;
-
}
static void
--- 619,646 ----
printf(" execute = '");
dump_expr(stmt->dynquery);
printf("'\n");
+
+ if (stmt->params != NIL)
+ {
+ ListCell *lc;
+ int i;
+
+ dump_indent += 2;
+ dump_ind();
+ printf(" USING\n");
+ dump_indent += 2;
+ i = 1;
+ foreach(lc, stmt->params)
+ {
+ dump_ind();
+ printf(" parameter $%d: ", i++);
+ dump_expr((PLpgSQL_expr *) lfirst(lc));
+ printf("\n");
+ }
+ dump_indent -= 4;
+ }
}
dump_indent -= 2;
}
static void
*** ./src/pl/plpgsql/src/plpgsql.h.orig 2010-01-12 15:54:05.282705735 +0100
--- ./src/pl/plpgsql/src/plpgsql.h 2010-01-12 15:54:24.476578504 +0100
***************
*** 503,508 ****
--- 503,509 ----
PLpgSQL_expr *argquery;
PLpgSQL_expr *query;
PLpgSQL_expr *dynquery;
+ List *params; /* USING expressions */
} PLpgSQL_stmt_open;
*** ./src/test/regress/expected/plpgsql.out.orig 2010-01-12 15:54:05.283703630 +0100
--- ./src/test/regress/expected/plpgsql.out 2010-01-12 15:54:24.478578834 +0100
***************
*** 3189,3194 ****
--- 3189,3223 ----
26
(1 row)
+ drop function exc_using(int, text);
+ create or replace function exc_using(int) returns void as $$
+ declare
+ c refcursor;
+ i int;
+ begin
+ open c for execute 'select * from generate_series(1,$1)' using $1+1;
+ loop
+ fetch c into i;
+ exit when not found;
+ raise notice '%', i;
+ end loop;
+ close c;
+ return;
+ end;
+ $$ language plpgsql;
+ select exc_using(5);
+ NOTICE: 1
+ NOTICE: 2
+ NOTICE: 3
+ NOTICE: 4
+ NOTICE: 5
+ NOTICE: 6
+ exc_using
+ -----------
+
+ (1 row)
+
+ drop function exc_using(int);
-- test FOR-over-cursor
create or replace function forc01() returns void as $$
declare
*** ./src/test/regress/sql/plpgsql.sql.orig 2010-01-12 15:54:05.285705775 +0100
--- ./src/test/regress/sql/plpgsql.sql 2010-01-12 15:54:24.480578884 +0100
***************
*** 2629,2634 ****
--- 2629,2656 ----
select exc_using(5, 'foobar');
+ drop function exc_using(int, text);
+
+ create or replace function exc_using(int) returns void as $$
+ declare
+ c refcursor;
+ i int;
+ begin
+ open c for execute 'select * from generate_series(1,$1)' using $1+1;
+ loop
+ fetch c into i;
+ exit when not found;
+ raise notice '%', i;
+ end loop;
+ close c;
+ return;
+ end;
+ $$ language plpgsql;
+
+ select exc_using(5);
+
+ drop function exc_using(int);
+
-- test FOR-over-cursor
create or replace function forc01() returns void as $$
Pavel Stehule <pavel.stehule@gmail.com> wrote:
ok, I accept all comments.
revised version are attached.
Good. This patch is ready to commit. I'll do it soon if no objections.
BTW, I found inconsistent parameter dumps in the codes. Some of them
add '$', but others does not. Are they intentional? Or, should we
adjust them to use one of the formats?
[pl_funcs.c]
dump_dynexecute()
dump_raise()
printf(" parameter %d: ", i++);
dump_dynfors()
dump_open()
dump_return_query()
printf(" parameter $%d: ", i++);
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
2010/1/14 Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp>:
Pavel Stehule <pavel.stehule@gmail.com> wrote:
ok, I accept all comments.
revised version are attached.Good. This patch is ready to commit. I'll do it soon if no objections.
BTW, I found inconsistent parameter dumps in the codes. Some of them
add '$', but others does not. Are they intentional? Or, should we
adjust them to use one of the formats?[pl_funcs.c]
dump_dynexecute()
dump_raise()
printf(" parameter %d: ", i++);
dump_dynfors()
dump_open()
dump_return_query()
printf(" parameter $%d: ", i++);
isn't parameter of raise statement different than query parameter?
I thing so $x convention respects parameter holder syntax.
Regards
Pavel
Show quoted text
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
Pavel Stehule <pavel.stehule@gmail.com> writes:
ok, I accept all comments.
revised version are attached.
Applied with minor editorialization.
regards, tom lane