Re: [HACKERS] proposal - assign result of query to psql variable
2013/2/2 Tom Lane t...@sss.pgh.pa.us: Shigeru Hanada shigeru.han...@gmail.com writes: On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule pavel.steh...@gmail.com wrote: possible variants a) don't store NULL values - and remove existing variable when NULL be assigned - it is probably best, but should be confusing for users b) implement flag IS NULL - for variables c) use nullPrint d) use empty String +1 for a). If users want to determine whether the result was NULL, or want to use substitute string for NULL result, they can use coalesce in SELECT clause. Anyway the feature should be documented clearly. Yeah, I was considering that one too. Let's do it that way. regards, tom lane possible simple enhancing of this behave (for 9.4). now missing variables is replaced by variable's name. We can implement some pset option - some like define what do with missing variable \pset missing_variable (use_name | use_null | error ) when this option will be active, then missing variable will be replaced by NULL. With this feature sequences of SQL statements joined by some variables can work. SELECT NULL as myvar \gset \pset missing_variable use_null SELECT :'myvar' IS NULL; ideas, opinions ? Regards Pavel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Pavel Stehule pavel.steh...@gmail.com writes: now missing variables is replaced by variable's name. We can implement some pset option - some like define what do with missing variable \pset missing_variable (use_name | use_null | error ) No, it isn't replaced by variable's name. What actually happens is we don't attempt a replacement unless the string after the colon matches an existing variable. Tampering with that seems dangerous and foolish. 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
Re: [HACKERS] proposal - assign result of query to psql variable
2013/2/3 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: now missing variables is replaced by variable's name. We can implement some pset option - some like define what do with missing variable \pset missing_variable (use_name | use_null | error ) No, it isn't replaced by variable's name. What actually happens is we don't attempt a replacement unless the string after the colon matches an existing variable. Tampering with that seems dangerous and foolish. some other ideas? do you think so full NULL support has sense? Regards Pavel 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
Re: [HACKERS] proposal - assign result of query to psql variable
Hello 2013/2/1 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: here is patch related to your proposal I looked at this a bit. It's getting there, though I still don't trust the places where you've chosen to clear the prefix setting. (Looking at it, I'm now not sure that I trust the implementation of \g either.) However, what I wanted to ask about was this: + if (PQgetisnull(result, 0, i)) + value = pset.popt.nullPrint ? pset.popt.nullPrint : ; + else + value = PQgetvalue(result, 0, i); What's the argument for using nullPrint here? ISTM that's effectively a form of escaping, and I'd not expect that to get applied to values going into variables, any more than any other formatting we do when printing results. Admittedly, if we just take the PQgetvalue result blindly, there'll be no way to tell the difference between empty-string and NULL results. But I'm not convinced that this approach is better. It would certainly need more than no documentation. I have not strong opinion about storing NULL value - and nullPrint is a best from simple variants - possible variants a) don't store NULL values - and remove existing variable when NULL be assigned - it is probably best, but should be confusing for users b) implement flag IS NULL - for variables c) use nullPrint d) use empty String @d is subset of @c, and I think so it put some better possibilities with only two lines more @a is probably best - but significant change - not hard to implement it SELECT NULL AS x \g pref_ SELECT :'pref_' IS NULL; is can be nice but it should be premature optimization too - nullPrint is enough for typical use cases Regards Pavel Regards Pavel 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
Re: [HACKERS] proposal - assign result of query to psql variable
On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule pavel.steh...@gmail.com wrote: possible variants a) don't store NULL values - and remove existing variable when NULL be assigned - it is probably best, but should be confusing for users b) implement flag IS NULL - for variables c) use nullPrint d) use empty String +1 for a). If users want to determine whether the result was NULL, or want to use substitute string for NULL result, they can use coalesce in SELECT clause. Anyway the feature should be documented clearly. -- Shigeru HANADA -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
2013/2/2 Shigeru Hanada shigeru.han...@gmail.com: On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule pavel.steh...@gmail.com wrote: possible variants a) don't store NULL values - and remove existing variable when NULL be assigned - it is probably best, but should be confusing for users b) implement flag IS NULL - for variables c) use nullPrint d) use empty String +1 for a). If users want to determine whether the result was NULL, or want to use substitute string for NULL result, they can use coalesce in SELECT clause. Anyway the feature should be documented clearly. yes, this has one other advantage - it doesn't block possible enhancing variables about NULL support in future. And other - it doesn't depends on psql settings Regards Pavel -- Shigeru HANADA -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Shigeru Hanada shigeru.han...@gmail.com writes: On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule pavel.steh...@gmail.com wrote: possible variants a) don't store NULL values - and remove existing variable when NULL be assigned - it is probably best, but should be confusing for users b) implement flag IS NULL - for variables c) use nullPrint d) use empty String +1 for a). If users want to determine whether the result was NULL, or want to use substitute string for NULL result, they can use coalesce in SELECT clause. Anyway the feature should be documented clearly. Yeah, I was considering that one too. Let's do it that way. 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
Re: [HACKERS] proposal - assign result of query to psql variable
Hello 2013/2/2 Tom Lane t...@sss.pgh.pa.us: Shigeru Hanada shigeru.han...@gmail.com writes: On Sat, Feb 2, 2013 at 7:30 PM, Pavel Stehule pavel.steh...@gmail.com wrote: possible variants a) don't store NULL values - and remove existing variable when NULL be assigned - it is probably best, but should be confusing for users b) implement flag IS NULL - for variables c) use nullPrint d) use empty String +1 for a). If users want to determine whether the result was NULL, or want to use substitute string for NULL result, they can use coalesce in SELECT clause. Anyway the feature should be documented clearly. Yeah, I was considering that one too. Let's do it that way. updated version Regards Pavel regards, tom lane gset_20130202.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Pavel Stehule pavel.steh...@gmail.com writes: 2013/2/2 Tom Lane t...@sss.pgh.pa.us: Shigeru Hanada shigeru.han...@gmail.com writes: +1 for a). If users want to determine whether the result was NULL, or want to use substitute string for NULL result, they can use coalesce in SELECT clause. Anyway the feature should be documented clearly. Yeah, I was considering that one too. Let's do it that way. updated version Applied with corrections. 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
Re: [HACKERS] proposal - assign result of query to psql variable
Pavel Stehule pavel.steh...@gmail.com writes: here is patch related to your proposal I looked at this a bit. It's getting there, though I still don't trust the places where you've chosen to clear the prefix setting. (Looking at it, I'm now not sure that I trust the implementation of \g either.) However, what I wanted to ask about was this: + if (PQgetisnull(result, 0, i)) + value = pset.popt.nullPrint ? pset.popt.nullPrint : ; + else + value = PQgetvalue(result, 0, i); What's the argument for using nullPrint here? ISTM that's effectively a form of escaping, and I'd not expect that to get applied to values going into variables, any more than any other formatting we do when printing results. Admittedly, if we just take the PQgetvalue result blindly, there'll be no way to tell the difference between empty-string and NULL results. But I'm not convinced that this approach is better. It would certainly need more than no documentation. 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
Re: [HACKERS] proposal - assign result of query to psql variable
Hello can you look, please, on updated version - it respects Tom's proposal and it is significantly reduced? Thank you Pavel Stehule 2013/1/28 Pavel Stehule pavel.steh...@gmail.com: Hello 2013/1/26 Tom Lane t...@sss.pgh.pa.us: Andrew Dunstan and...@dunslane.net writes: +1. This looks quite nifty. Maybe useful too to have a default prefix via some setting. Meh. I would expect that \gset :foo would work to specify a computed prefix if you wanted it --- isn't that sufficient indirection? I'm not thrilled with further expanding the set of magic variables in psql. here is patch related to your proposal Regards Pavel 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
Re: [HACKERS] proposal - assign result of query to psql variable
Hello 2013/1/26 Tom Lane t...@sss.pgh.pa.us: Andrew Dunstan and...@dunslane.net writes: +1. This looks quite nifty. Maybe useful too to have a default prefix via some setting. Meh. I would expect that \gset :foo would work to specify a computed prefix if you wanted it --- isn't that sufficient indirection? I'm not thrilled with further expanding the set of magic variables in psql. here is patch related to your proposal Regards Pavel regards, tom lane gset_20130128.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
I wrote: Pavel Stehule pavel.steh...@gmail.com writes: [ gset_13.diff ] One more gripe is that the parsing logic for \gset is pretty unintelligible. After further thought, it seems to me that the problem here is an overcomplicated definition of the \gset command; it could be made both more usable and simpler to implement, if we looked at it differently. First off, why is there a provision to omit variable names for some columns, ie why bother with saying that you can write \gset x,,y to store only the first and third columns? If you didn't want the second value, why didn't you leave it out of the SELECT to start with? It seems like the only possible reason for that is if you were lazy and typed SELECT * instead of listing the columns ... but then you still need to list the columns in \gset, and it's pretty error-prone to make sure that the \gset variable list lines up with what * will emit. In fact, it's pretty error-prone to have to make the \gset variable list line up with the SELECT columns in any case. So here's my proposal: let's forget the variable list entirely, and use the column names returned by the server as the variable names to assign to. So instead of select 1, 2 \gset x,y you would write select 1 as x, 2 as y \gset or just select 1 x, 2 y \gset which is exactly as much typing as the existing definition, but much harder to screw up by misaligning the SELECT's values with the target names. It also makes it really trivial to do the SELECT * case --- you just do it, and ignore any variables for columns you don't care about. A probably-useful extension to this basic concept is to allow \gset to specify an optional prefix, that is select 1 as x, 2 as y \gset p_ would set p_x and p_y. This would make it easier to manage results from multiple \gset operations, and to be sure that you didn't accidentally overwrite some built-in variable. So this seems to me to be easier and less error-prone to use than the existing definition. It would also take a lot less code to implement, since the parsing logic for \gset would reduce to a couple of lines, and you'd not need the variable-name-list data structure at all. 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
Re: [HACKERS] proposal - assign result of query to psql variable
On 01/26/2013 11:42 AM, Tom Lane wrote: A probably-useful extension to this basic concept is to allow \gset to specify an optional prefix, that is select 1 as x, 2 as y \gset p_ would set p_x and p_y. This would make it easier to manage results from multiple \gset operations, and to be sure that you didn't accidentally overwrite some built-in variable. +1. This looks quite nifty. Maybe useful too to have a default prefix via some setting. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Andrew Dunstan and...@dunslane.net writes: +1. This looks quite nifty. Maybe useful too to have a default prefix via some setting. Meh. I would expect that \gset :foo would work to specify a computed prefix if you wanted it --- isn't that sufficient indirection? I'm not thrilled with further expanding the set of magic variables in psql. 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
Re: [HACKERS] proposal - assign result of query to psql variable
Hello 2013/1/26 Tom Lane t...@sss.pgh.pa.us: I wrote: Pavel Stehule pavel.steh...@gmail.com writes: [ gset_13.diff ] One more gripe is that the parsing logic for \gset is pretty unintelligible. After further thought, it seems to me that the problem here is an overcomplicated definition of the \gset command; it could be made both more usable and simpler to implement, if we looked at it differently. First off, why is there a provision to omit variable names for some columns, ie why bother with saying that you can write \gset x,,y to store only the first and third columns? If you didn't want the second value, why didn't you leave it out of the SELECT to start with? It seems like the only possible reason for that is if you were lazy and typed SELECT * instead of listing the columns ... but then you still need to list the columns in \gset, and it's pretty error-prone to make sure that the \gset variable list lines up with what * will emit. possibility to skip some variables is David Fetter's idea. I see only one possible use case - it enable use some query from history without necessity to modify query or creating some auxiliary variables. Personally, I can live without this feature, but it question for David mainly. In fact, it's pretty error-prone to have to make the \gset variable list line up with the SELECT columns in any case. So here's my proposal: let's forget the variable list entirely, and use the column names returned by the server as the variable names to assign to. So instead of select 1, 2 \gset x,y you would write select 1 as x, 2 as y \gset or just select 1 x, 2 y \gset which is exactly as much typing as the existing definition, but much harder to screw up by misaligning the SELECT's values with the target names. It also makes it really trivial to do the SELECT * case --- you just do it, and ignore any variables for columns you don't care about. hard to say your proposal has advantages - and implementation is simple, but it is looking little bit strange - but like other psql features. I have no strong opinion - I prefer original design, as more explicit with clean separation line between query and console part, but I am able to see advantages of your proposal - so depends what will speak others. I have no problem with your design, although I am thinking so original design is little bit more safer (but not with significant difference). A probably-useful extension to this basic concept is to allow \gset to specify an optional prefix, that is select 1 as x, 2 as y \gset p_ would set p_x and p_y. This would make it easier to manage results from multiple \gset operations, and to be sure that you didn't accidentally overwrite some built-in variable. I understand to motivation - but I am not enthused. Now - a work with variables is strange - and with it will be more stranger. So this seems to me to be easier and less error-prone to use than the existing definition. It would also take a lot less code to implement, since the parsing logic for \gset would reduce to a couple of lines, and you'd not need the variable-name-list data structure at all. I will waiting for others - I can live with this proposal. Regards Pavel 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
Re: [HACKERS] proposal - assign result of query to psql variable
Pavel Stehule pavel.steh...@gmail.com writes: [ gset_13.diff ] I looked at this a bit. I think you need to reconsider when and how the \gset state gets cleaned up. Doing it inside StoreQueryResult is not very good because that only gets reached upon successful execution. Consider for example select 1/0 \gset x You'll get an ERROR from this, and a reasonable user would suppose that that was that and the \gset is no longer in effect. But guess what, it's still lurking under the surface, waiting to capture his next command. This is also causing you unnecessary complication in ExecQueryUsingCursor, which has to work around the fact that StoreQueryResult destroys state. I think it would be better to remove that responsibility from StoreQueryResult and instead put the gset-list cleanup somewhere at the end of query processing. Didn't really look into where would be the best place, but it should be someplace that control passes through no matter what came back from the server. BTW, is StoreQueryResult in itself (that is, the switch on PQresultStatus) actually doing anything useful? It appears to me that the error cases are handled elsewhere, such that control never gets to it unless the PQresultStatus is an expected value. If that were not the case, printouts as laconic as bad response\n would certainly not be acceptable --- people would want to see the underlying error message. Also, I'm not sure I like the PSQL_CMD_NOSEND business, ie, trashing the query buffer if anything can be found wrong with the \gset itself. If I've done big long multiline query here \gset x y I'd expect that the error only discards the \gset and not the query. An error in some other sort of backslash command in that situation wouldn't discard the query buffer. For instance try this: regression=# select 3+2 regression-# \goofus Invalid command \goofus. Try \? for help. regression-# ; ?column? -- 5 (1 row) regression=# So AFAICS, PSQL_CMD_NOSEND just represents extra code that's making things worse not better. One more gripe is that the parsing logic for \gset is pretty unintelligible. You've got a state variable with only two values, whose names seem to boil down to whether a comma is expected or not. And then you've got a separate process_comma flag, which means ... well, I'm not sure, but possibly the very same thing. For sure it's not clear whether all four possible combinations of those two variables are valid and what they'd mean. This could use another round of thinking and rewriting. Or at least better comments. 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
Re: [HACKERS] proposal - assign result of query to psql variable
2012/12/19 Shigeru Hanada shigeru.han...@gmail.com: On Tue, Dec 18, 2012 at 2:52 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Attached updated patch Seems fine. I'll mark this as ready for committer. Nice work! thank you very much Regards Pavel -- Shigeru HANADA -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
On Tue, Dec 18, 2012 at 2:52 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Attached updated patch Seems fine. I'll mark this as ready for committer. Nice work! -- Shigeru HANADA -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
On Sun, Oct 28, 2012 at 7:16 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello here is updated patch main change - it doesn't touch psql lexer - like Tom proposed other points respect Phil's notices I reviewed v12 patch. It provides gset command which works consistently with other psql commands, such as \g and \set, and implementation seems reasonable, and follows other reviewer's comments properly. I think we can mark it as ready for committer, once you have fixed some minor issues below. * Skipping leading blank in inner while loop of command.c seems unnecessary, because (IIUC) psql's scanner skips blanks. Is there any case that scanner returns token with leading/trailing blank? * ISTM that VARLIST_INITIAL can be removed. AFAIS it's same state as VARLIST_EXPECTED_COMMA_OR_IDENT. * I found some cosmetic flaw and typo. Please see attached patch for details. * How about pulling up codes for PGRES_TUPLES_OK case in StoreQueryResult to new static function, say StoreQueryTuple? It would make StoreQueryResult more similar to PrintQueryResult's style, and IMO it makes the code more readable. Regards, -- Shigeru HANADA gset_fix.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
2012/12/17 Shigeru Hanada shigeru.han...@gmail.com: On Sun, Oct 28, 2012 at 7:16 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello here is updated patch main change - it doesn't touch psql lexer - like Tom proposed other points respect Phil's notices I reviewed v12 patch. It provides gset command which works consistently with other psql commands, such as \g and \set, and implementation seems reasonable, and follows other reviewer's comments properly. I think we can mark it as ready for committer, once you have fixed some minor issues below. * Skipping leading blank in inner while loop of command.c seems unnecessary, because (IIUC) psql's scanner skips blanks. Is there any case that scanner returns token with leading/trailing blank? removed * ISTM that VARLIST_INITIAL can be removed. AFAIS it's same state as VARLIST_EXPECTED_COMMA_OR_IDENT. removed * I found some cosmetic flaw and typo. Please see attached patch for details. it is ok, merged * How about pulling up codes for PGRES_TUPLES_OK case in StoreQueryResult to new static function, say StoreQueryTuple? It would make StoreQueryResult more similar to PrintQueryResult's style, and IMO it makes the code more readable. good idea done Attached updated patch Regards Pavel Regards, -- Shigeru HANADA gset_13.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Hello
here is updated patch
main change - it doesn't touch psql lexer - like Tom proposed
other points respect Phil's notices
My first review...
Patch applied cleanly to master and make check was fine.
I tested it out and it operates as advertised. There were a couple
things that stood out to me though.
1) NULL values are not displayed properly after \pset null is run.
postgres=# \pset null '(null)'
Null display is (null).
postgres=# select NULL \gset var1
postgres=# \echo :var1
postgres=# select NULL;
?column?
--
(null)
(1 row)
I know this doesn't come back from the server like this, but you
should be able to pull this from the options and display
appropriately. Not sure if it should be in variable display code, or
when you store it into the variable.
fixed and add to regress test
2) The error messages seemed kind of terse. Other error messages are
capitalized and have punctuation.
After some thinking I didn't change it - it is consistent with other
messages in psql - short messages that are not complete sentence are
no capitalized and have not punctuation like other short messages in
psql
3) We don't find out about incorrect number of columns until after
query returns. I know this is hard/impossible to fix, but it might be
nice to print out the result normally if you can't store it in the
variables.
I didn't change it because a) I don't think so change behave after
error is good idea, b) \gset doesn't remove SQL from query buffer, so
you can repeat it
postgres= select 10,20,30
postgres- \gset a,b
too few target variables
postgres= \g
?column? │ ?column? │ ?column?
──┼──┼──
10 │ 20 │ 30
(1 row)
postgres= \gset a,b,c
3b) You throw an error on too many variables, but still store the data
since you have fewer columns than variables. This makes sense, but you
don't inform the user at all.
it is commented in doc
+ para
+ When this command fails, then related replaceable
+ class=parametervariables/replaceable has undefined content.
+ /para
On to the code:
1) Introduction of random newlines:
*** cleanup:
*** 1254,1259
--- 1383,1389
PQclear(results);
}
+
if (pset.timing)
{
INSTR_TIME_SET_CURRENT(after);
I saw that in a couple places, but that was the most obvious.
I hope so I moved to /dev/null all
2) TargetList - Why not use the built in linked list operations rather
than creating your own? Are they not accessible to client binaries
like this?
There was not support for lists on client part to today. So I had to
created own simple implementation.
Overall I think this is a useful feature and I think you integrated it
well within the existing infrastructure, ie combining concepts of \set
and \g.
Thank you very much again
Regards
Pavel
gset_12.diff
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
I gave this a look. I think it needs to be revised by somebody with a better understanding of scanner (flex) than me, but I didn't like the changes in psqlscan.l at all; the new xvl pattern is too unlike the rest of the surrounding patterns, and furthermore it has been placed within the block that says it mirrors the backend scanner, when it obviously has no equivalent there. I assume there's a better way to do this. Hints would be appreciated. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Alvaro Herrera alvhe...@2ndquadrant.com writes:
I gave this a look. I think it needs to be revised by somebody with a
better understanding of scanner (flex) than me, but I didn't like the
changes in psqlscan.l at all; the new xvl pattern is too unlike the
rest of the surrounding patterns, and furthermore it has been placed
within the block that says it mirrors the backend scanner, when it
obviously has no equivalent there.
I assume there's a better way to do this. Hints would be appreciated.
Personally I saw no reason for this patch to touch psqlscan.l in the
first place. Commands such as \set just scan variable names with
psql_scan_slash_option(OT_NORMAL); why shouldn't this act the same?
Moreover, the proposed lexer rules are flat out *wrong*, in that they
insist on the target variable names being {identifier}s, a restriction
not imposed by \set.
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
Re: [HACKERS] proposal - assign result of query to psql variable
Tom Lane escribió:
Alvaro Herrera alvhe...@2ndquadrant.com writes:
I gave this a look. I think it needs to be revised by somebody with a
better understanding of scanner (flex) than me, but I didn't like the
changes in psqlscan.l at all; the new xvl pattern is too unlike the
rest of the surrounding patterns, and furthermore it has been placed
within the block that says it mirrors the backend scanner, when it
obviously has no equivalent there.
I assume there's a better way to do this. Hints would be appreciated.
Personally I saw no reason for this patch to touch psqlscan.l in the
first place. Commands such as \set just scan variable names with
psql_scan_slash_option(OT_NORMAL); why shouldn't this act the same?
Moreover, the proposed lexer rules are flat out *wrong*, in that they
insist on the target variable names being {identifier}s, a restriction
not imposed by \set.
Great, thanks for the feedback. Marking as returned in CF. I hope to
see a new version after pgconf.eu.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
2012/10/25 Tom Lane t...@sss.pgh.pa.us:
Alvaro Herrera alvhe...@2ndquadrant.com writes:
I gave this a look. I think it needs to be revised by somebody with a
better understanding of scanner (flex) than me, but I didn't like the
changes in psqlscan.l at all; the new xvl pattern is too unlike the
rest of the surrounding patterns, and furthermore it has been placed
within the block that says it mirrors the backend scanner, when it
obviously has no equivalent there.
I assume there's a better way to do this. Hints would be appreciated.
Personally I saw no reason for this patch to touch psqlscan.l in the
first place. Commands such as \set just scan variable names with
psql_scan_slash_option(OT_NORMAL); why shouldn't this act the same?
it cannot be same, because current scan doesn't know comma as
separator. So if you don't like changes in scanner, than we can't to
use var1, var2, syntax and we can't to use leaky list syntax ,x,
Moreover, the proposed lexer rules are flat out *wrong*, in that they
insist on the target variable names being {identifier}s, a restriction
not imposed by \set.
do you like to support referenced varnames??
postgres=# \varname xxx
Invalid command \varname. Try \? for help.
postgres=# \set varname xxx
postgres=# \set :varname Hello
postgres=# \set
varname = 'xxx'
xxx = 'Hello'
yes, \set support it, but this can be source of strange behave for
some people, because people use :varname like $varname in classic
scripting languages, and it is significantly different - so I didn't
support it as little bit dangerous feature. It is easy support it,
although I am thinking, so it is not good idea, because behave is
really different than users expect and I don't know any use case for
this indirect referencing. But I would to talk about it, and I invite
opinion of others.
Regards
Pavel
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
Re: [HACKERS] proposal - assign result of query to psql variable
Pavel Stehule pavel.steh...@gmail.com writes:
2012/10/25 Tom Lane t...@sss.pgh.pa.us:
Personally I saw no reason for this patch to touch psqlscan.l in the
first place. Commands such as \set just scan variable names with
psql_scan_slash_option(OT_NORMAL); why shouldn't this act the same?
it cannot be same, because current scan doesn't know comma as
separator. So if you don't like changes in scanner, than we can't to
use var1, var2, syntax and we can't to use leaky list syntax ,x,
Uh, no, that doesn't follow. It wouldn't be any more code to have
command.c process the commas (or even more likely, just save the \gset
argument(s) as a string, and split on commas after we've done the
command). Even if we wanted to do that in psqlscan.l, this was a pretty
bad/ugly implementation of it.
Moreover, the proposed lexer rules are flat out *wrong*, in that they
insist on the target variable names being {identifier}s, a restriction
not imposed by \set.
yes, \set support it, but this can be source of strange behave for
some people, because people use :varname like $varname in classic
scripting languages, and it is significantly different - so I didn't
support it as little bit dangerous feature.
[ shrug... ] If you want to argue for imposing a restriction on
psql variable names across-the-board, we could have that discussion;
but personally I've not seen even one user complaint that could be
traced to \set's laxity in the matter, so I don't see a need for
a restriction. In any case, having \gset enforce a restriction
that \set doesn't is useless and inconsistent.
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
Re: [HACKERS] proposal - assign result of query to psql variable
2012/10/25 Tom Lane t...@sss.pgh.pa.us:
Pavel Stehule pavel.steh...@gmail.com writes:
2012/10/25 Tom Lane t...@sss.pgh.pa.us:
Personally I saw no reason for this patch to touch psqlscan.l in the
first place. Commands such as \set just scan variable names with
psql_scan_slash_option(OT_NORMAL); why shouldn't this act the same?
it cannot be same, because current scan doesn't know comma as
separator. So if you don't like changes in scanner, than we can't to
use var1, var2, syntax and we can't to use leaky list syntax ,x,
Uh, no, that doesn't follow. It wouldn't be any more code to have
command.c process the commas (or even more likely, just save the \gset
argument(s) as a string, and split on commas after we've done the
command). Even if we wanted to do that in psqlscan.l, this was a pretty
bad/ugly implementation of it.
I don't understand, why we have to move lexer work from scanner to
command processing?
then I afraid of another issue - when we do late separation in command
somebody can do
\set targetvars a,b,c
select
\gset x1,x2,:targetvars,x3
We would to do this? Then we moving to TeX liked languages. I am asking.
Moreover, the proposed lexer rules are flat out *wrong*, in that they
insist on the target variable names being {identifier}s, a restriction
not imposed by \set.
yes, \set support it, but this can be source of strange behave for
some people, because people use :varname like $varname in classic
scripting languages, and it is significantly different - so I didn't
support it as little bit dangerous feature.
[ shrug... ] If you want to argue for imposing a restriction on
psql variable names across-the-board, we could have that discussion;
but personally I've not seen even one user complaint that could be
traced to \set's laxity in the matter, so I don't see a need for
a restriction. In any case, having \gset enforce a restriction
that \set doesn't is useless and inconsistent.
ok, it has a sense
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
Re: [HACKERS] proposal - assign result of query to psql variable
On Tue, Oct 16, 2012 at 12:24 PM, Pavel Stehule pavel.steh...@gmail.com wrote:
2012/10/16 Shigeru HANADA shigeru.han...@gmail.com:
Hi Pavel,
On Tue, Oct 16, 2012 at 6:59 AM, Pavel Stehule pavel.steh...@gmail.com
wrote:
here is updated patch, I moved lot of code from lexer to command.com,
and now more \gset doesn't disable other backslash commands on same
line.
* lexer changes
IIUC, new function psql_scan_varlist_varname scans input and returns a
variable name or a comma at each call, and command.c handles the error
such as invalid # of variables. This new design seems better than old one.
However, IMHO the name psql_scan_varlist_varname sounds redundant and
unintuitive. I'd prefer psql_scan_slash_varlist, because it indicates
that that function is expected to be used for arguments of backslash
commands, like psql_scan_slash_command and psql_scan_slash_option.
Thoughts?
* multiple meta command
Now both of the command sequences
$ SELECT 1, 2 \gset var1, var2 \g foo.txt
$ SELECT 1, 2 \g foo.txt \gset var1, var2
set var1 and v2 to 1 and 2 respectively, and also write the result
into foo.txt. This would be what users expected.
* Duplication of variables
I found an issue we have not discussed. Currently \gset accepts same
variable names in the list, and stores last SELECT item in duplicated
variables. For instance,
$ SELECT 1, 2 \gset var, var
stores 2 into var. I think this behavior is acceptable, but it might
be worth mentioning in document.
* extra fixes
I fixed some minor issues below. Please see attached v10 patch for details.
* remove unused macro OT_VARLIST
* remove unnecessary #include directive for common.h
* fill comment within 80 columns
* indent short variable name with tab
* add regression test case for combination of \g and \gset
* bug on FETCH_COUNT = 1
When FETCH_COUNT is set to 1, and the number of rows returned is 1 too,
\gset shows extra (1 row). This would be a bug in
ExecQueryUsingCursor. Please see the last test case in regression test
psql_cmd.
I fixed this bug
Regards
Pavel
I'll mark this patch as waiting author.
Regards,
--
Shigeru HANADA
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
My first review...
Patch applied cleanly to master and make check was fine.
I tested it out and it operates as advertised. There were a couple
things that stood out to me though.
1) NULL values are not displayed properly after \pset null is run.
postgres=# \pset null '(null)'
Null display is (null).
postgres=# select NULL \gset var1
postgres=# \echo :var1
postgres=# select NULL;
?column?
--
(null)
(1 row)
I know this doesn't come back from the server like this, but you
should be able to pull this from the options and display
appropriately. Not sure if it should be in variable display code, or
when you store it into the variable.
2) The error messages seemed kind of terse. Other error messages are
capitalized and have punctuation.
3) We don't find out about incorrect number of columns until after
query returns. I know this is hard/impossible to fix, but it might be
nice to print out the result normally if you can't store it in the
variables.
3b) You throw an error on too many variables, but still store the data
since you have fewer columns than variables. This makes sense, but you
don't inform the user at all.
On to the code:
1) Introduction of random newlines:
*** cleanup:
*** 1254,1259
--- 1383,1389
PQclear(results);
}
+
if (pset.timing)
{
INSTR_TIME_SET_CURRENT(after);
I saw that in a couple places, but that was the most obvious.
2) TargetList - Why not use the built in linked list operations rather
than creating your own? Are they not accessible to client binaries
like this?
Overall I think this is a useful feature and I think you integrated it
well within the existing infrastructure, ie combining concepts of \set
and \g.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Hi Pavel,
On Tue, Oct 16, 2012 at 6:59 AM, Pavel Stehule pavel.steh...@gmail.com
wrote:
here is updated patch, I moved lot of code from lexer to command.com,
and now more \gset doesn't disable other backslash commands on same
line.
* lexer changes
IIUC, new function psql_scan_varlist_varname scans input and returns a
variable name or a comma at each call, and command.c handles the error
such as invalid # of variables. This new design seems better than old one.
However, IMHO the name psql_scan_varlist_varname sounds redundant and
unintuitive. I'd prefer psql_scan_slash_varlist, because it indicates
that that function is expected to be used for arguments of backslash
commands, like psql_scan_slash_command and psql_scan_slash_option.
Thoughts?
* multiple meta command
Now both of the command sequences
$ SELECT 1, 2 \gset var1, var2 \g foo.txt
$ SELECT 1, 2 \g foo.txt \gset var1, var2
set var1 and v2 to 1 and 2 respectively, and also write the result
into foo.txt. This would be what users expected.
* Duplication of variables
I found an issue we have not discussed. Currently \gset accepts same
variable names in the list, and stores last SELECT item in duplicated
variables. For instance,
$ SELECT 1, 2 \gset var, var
stores 2 into var. I think this behavior is acceptable, but it might
be worth mentioning in document.
* extra fixes
I fixed some minor issues below. Please see attached v10 patch for details.
* remove unused macro OT_VARLIST
* remove unnecessary #include directive for common.h
* fill comment within 80 columns
* indent short variable name with tab
* add regression test case for combination of \g and \gset
* bug on FETCH_COUNT = 1
When FETCH_COUNT is set to 1, and the number of rows returned is 1 too,
\gset shows extra (1 row). This would be a bug in
ExecQueryUsingCursor. Please see the last test case in regression test
psql_cmd.
I'll mark this patch as waiting author.
Regards,
--
Shigeru HANADA
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
***
*** 1483,1490 testdb=gt;
way. Use command\i/command for that.) This means that
if the query ends with (or contains) a semicolon, it is
immediately executed. Otherwise it will merely wait in the
! query buffer; type semicolon or literal\g/ to send it, or
! literal\r/ to cancel.
/para
para
--- 1483,1490
way. Use command\i/command for that.) This means that
if the query ends with (or contains) a semicolon, it is
immediately executed. Otherwise it will merely wait in the
! query buffer; type semicolon, literal\g/ or
! literal\gset/literal to send it, or literal\r/ to cancel.
/para
para
***
*** 1617,1622 Tue Oct 26 21:40:57 CEST 1999
--- 1617,1644
/varlistentry
varlistentry
+ termliteral\gset/literal replaceable class=parametervariable/replaceable [ ,replaceable class=parametervariable/replaceable ... ] /term
+
+ listitem
+ para
+ Sends the current query input buffer to the server and stores the
+ query's output into corresponding replaceable
+ class=parametervariable/replaceable. The preceding query must
+ return only one row, and the number of variables must be same as the
+ number of elements in commandSELECT/command list. If you don't
+ need any of items in commandSELECT/command list, you can omit
+ corresponding replaceable class=parametervariable/replaceable.
+ Example:
+ programlisting
+ foo=gt; SELECT 'hello', 'wonderful', 'world!' \gset var1,,var3
+ foo=gt; \echo :var1 :var3
+ hello world!
+ /programlisting
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termliteral\h/literal or literal\help/literal literal[ replaceable class=parametercommand/replaceable ]/literal/term
listitem
para
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***
*** 71,77 static void printSSLInfo(void);
static void checkWin32Codepage(void);
#endif
!
/*--
* HandleSlashCmds:
--- 71,86
static void checkWin32Codepage(void);
#endif
! /*
! * Possible states for simple state machine, that is used for
! * parsing target list - list of varnames separated by comma.
! */
! typedef enum
! {
! VARLIST_INITIAL,
! VARLIST_EXPECTED_COMMA,
! VARLIST_EXPECTED_COMMA_OR_IDENT
! } VarlistParserState;
/*--
* HandleSlashCmds:
***
*** 748,753 exec_command(const char *cmd,
--- 757,831
status = PSQL_CMD_SEND;
}
+ /* \gset send query and store result */
+ else if (strcmp(cmd, gset) == 0)
+ {
+ bool value_is_valid;
+ char *value;
+ VarlistParserState state = VARLIST_INITIAL;
+
+ /* expected valid target list */
+ success = true;
+
+ pset.gvars = NULL;
+
Re: [HACKERS] proposal - assign result of query to psql variable
Hello here is updated patch, I moved lot of code from lexer to command.com, and now more \gset doesn't disable other backslash commands on same line. Regards Pavel 2012/10/15 Pavel Stehule pavel.steh...@gmail.com: 2012/10/15 Pavel Stehule pavel.steh...@gmail.com: 2012/10/15 Shigeru HANADA shigeru.han...@gmail.com: Hi Pavel, First of all, I'm sorry that my previous review was rough. I looked your patch and existing code closely again. On 2012/10/15, at 12:57, Pavel Stehule pavel.steh...@gmail.com wrote: 2012/10/14 Tom Lane t...@sss.pgh.pa.us: * ExecQueryUsingCursor's concept of how to process command results (why? surely we don't need \gset to use a cursor) There was two possibilities, but hardly non using cursor is better way +1 for supporting the case when FETCH_COUNT 0, because user might set so mainly for other queries, and they would want to avoid changing FETCH_COUNT setting during every query followed by \gset. * the psql lexer (adding a whole bunch of stuff that probably doesn't belong there) ?? I think that Tom is talking about psql_scan_slash_vars(). It seems too specific to \gset command. How about to improve psql_scan_slash_options() so that it can handle comma-separated variable list? Although you might have tried it before. # Unused OT_VARLIST macro gave me the idea. yes, it is possible - I'll look on it at evening a reuse of psql_scan_slash_options is not good idea - a interface of this function is out of my purposes - and I cannot to signalise error from this procedure. But I can minimize psql_scan_slash_var and I can move lot of code out of lexer file. * the core psql settings construct (to store something that is in no way a persistent setting) ?? I thought that having \gset arguments in pset is reasonable, since \g uses pest.gfname to hold its one-shot setting. Or, we should refactor \g to fit with \gset? I might be missing Tom's point... Surely there is a less ugly and invasive way to do this. The fact that the reviewer keeps finding bizarre bugs like another backslash command on the same line doesn't work seems to me to be a good indication that this is touching things it shouldn't. - all these bugs are based on lexer construct. A little modification of lexer is possible IMHO those issues come from the design rather than the implementation of lexer. AFAIK we don't have consensus about the case that both of \g and \gset are used for a query like this: postgres=# SELECT 1 \gset var \\ \g foo.txt This seems regal. Should we store 1 into var and write the result into foo.txt? Or, only either of them? It's just an idea and it requires new special character, but how about use \g command for file, pipe, and variable? In the case we choose '' for variable prefix: postgres=# SELECT 'hello', 'wonderful', 'world!' \g var1,,var2 Anyway, we've had no psql's meta command which processes query result other than \g. So, we might have more considerable issues about design. a current design is rigid - a small implementation can stop parsing target list, when other backslash statement is detected BTW, what the word comma_expected means? It's in the comment above psql_scan_slash_vars(). It might be a remaining of old implementation. This identifier is mistaken - etc this comment is wrong and related to old implementation - sorry. A first design was replaced by state machine described by VarListParserState Regards, -- Shigeru HANADA shigeru.han...@gmail.com gset09.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
2012/10/16 Shigeru HANADA shigeru.han...@gmail.com: Hi Pavel, On Tue, Oct 16, 2012 at 6:59 AM, Pavel Stehule pavel.steh...@gmail.com wrote: here is updated patch, I moved lot of code from lexer to command.com, and now more \gset doesn't disable other backslash commands on same line. * lexer changes IIUC, new function psql_scan_varlist_varname scans input and returns a variable name or a comma at each call, and command.c handles the error such as invalid # of variables. This new design seems better than old one. However, IMHO the name psql_scan_varlist_varname sounds redundant and unintuitive. I'd prefer psql_scan_slash_varlist, because it indicates that that function is expected to be used for arguments of backslash commands, like psql_scan_slash_command and psql_scan_slash_option. Thoughts? * multiple meta command Now both of the command sequences $ SELECT 1, 2 \gset var1, var2 \g foo.txt $ SELECT 1, 2 \g foo.txt \gset var1, var2 set var1 and v2 to 1 and 2 respectively, and also write the result into foo.txt. This would be what users expected. * Duplication of variables I found an issue we have not discussed. Currently \gset accepts same variable names in the list, and stores last SELECT item in duplicated variables. For instance, $ SELECT 1, 2 \gset var, var stores 2 into var. I think this behavior is acceptable, but it might be worth mentioning in document. * extra fixes I fixed some minor issues below. Please see attached v10 patch for details. * remove unused macro OT_VARLIST * remove unnecessary #include directive for common.h * fill comment within 80 columns * indent short variable name with tab * add regression test case for combination of \g and \gset * bug on FETCH_COUNT = 1 When FETCH_COUNT is set to 1, and the number of rows returned is 1 too, \gset shows extra (1 row). This would be a bug in ExecQueryUsingCursor. Please see the last test case in regression test psql_cmd. I fixed this bug Regards Pavel I'll mark this patch as waiting author. Regards, -- Shigeru HANADA gset_11.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Hello 2012/10/14 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: [ gset_08.diff ] In the course of adding a new backslash command, this patch manages to touch: * the main loop's concept of what types of backslash commands exist (PSQL_CMD_NOSEND ... what's the point of that, rather than making this work the same as \g?) This is necessary, because there is a new possible state - query is complete, but command is wrong - so I cannot use \g implementation, because there is no possible error in \g or ';' * SendQuery's concept of how to process command results (again, why isn't this more like \g?) it is similar - difference is only in work with result - \gset uses StoreQueryResult instead PrintQueryResults, but other is same * ExecQueryUsingCursor's concept of how to process command results (why? surely we don't need \gset to use a cursor) It is my mistake - simply and correct way is not using cursor in this use case * the psql lexer (adding a whole bunch of stuff that probably doesn't belong there) I had to modify lexer - current lexer supports symbols separated by space, but not symbols separated by comma. We don't would to use variable evaluation in target variable list. * the core psql settings construct (to store something that is in no way a persistent setting) sorry, I don't understand to this issue Surely there is a less ugly and invasive way to do this. The fact that the reviewer keeps finding bizarre bugs like another backslash command on the same line doesn't work seems to me to be a good indication that this is touching things it shouldn't. I had too strong in checking and raising errors. Is relative simple to modify patch to enable more backslash statements on same line I'll send updated patch early Regards Pavel 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
Re: [HACKERS] proposal - assign result of query to psql variable
2012/10/15 Pavel Stehule pavel.steh...@gmail.com: Hello 2012/10/14 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: [ gset_08.diff ] In the course of adding a new backslash command, this patch manages to touch: * the main loop's concept of what types of backslash commands exist (PSQL_CMD_NOSEND ... what's the point of that, rather than making this work the same as \g?) This is necessary, because there is a new possible state - query is complete, but command is wrong - so I cannot use \g implementation, because there is no possible error in \g or ';' * SendQuery's concept of how to process command results (again, why isn't this more like \g?) it is similar - difference is only in work with result - \gset uses StoreQueryResult instead PrintQueryResults, but other is same * ExecQueryUsingCursor's concept of how to process command results (why? surely we don't need \gset to use a cursor) It is my mistake - simply and correct way is not using cursor in this use case and it is question if cursor support is bad decision, because cursors can help with large queries, that can returns thousands rows - and we can raise error early, before we fall on out of memory regards Pavel * the psql lexer (adding a whole bunch of stuff that probably doesn't belong there) I had to modify lexer - current lexer supports symbols separated by space, but not symbols separated by comma. We don't would to use variable evaluation in target variable list. * the core psql settings construct (to store something that is in no way a persistent setting) sorry, I don't understand to this issue Surely there is a less ugly and invasive way to do this. The fact that the reviewer keeps finding bizarre bugs like another backslash command on the same line doesn't work seems to me to be a good indication that this is touching things it shouldn't. I had too strong in checking and raising errors. Is relative simple to modify patch to enable more backslash statements on same line I'll send updated patch early Regards Pavel 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
Re: [HACKERS] proposal - assign result of query to psql variable
2012/10/15 Shigeru HANADA shigeru.han...@gmail.com: Hi Pavel, First of all, I'm sorry that my previous review was rough. I looked your patch and existing code closely again. On 2012/10/15, at 12:57, Pavel Stehule pavel.steh...@gmail.com wrote: 2012/10/14 Tom Lane t...@sss.pgh.pa.us: * ExecQueryUsingCursor's concept of how to process command results (why? surely we don't need \gset to use a cursor) There was two possibilities, but hardly non using cursor is better way +1 for supporting the case when FETCH_COUNT 0, because user might set so mainly for other queries, and they would want to avoid changing FETCH_COUNT setting during every query followed by \gset. * the psql lexer (adding a whole bunch of stuff that probably doesn't belong there) ?? I think that Tom is talking about psql_scan_slash_vars(). It seems too specific to \gset command. How about to improve psql_scan_slash_options() so that it can handle comma-separated variable list? Although you might have tried it before. # Unused OT_VARLIST macro gave me the idea. yes, it is possible - I'll look on it at evening * the core psql settings construct (to store something that is in no way a persistent setting) ?? I thought that having \gset arguments in pset is reasonable, since \g uses pest.gfname to hold its one-shot setting. Or, we should refactor \g to fit with \gset? I might be missing Tom's point... Surely there is a less ugly and invasive way to do this. The fact that the reviewer keeps finding bizarre bugs like another backslash command on the same line doesn't work seems to me to be a good indication that this is touching things it shouldn't. - all these bugs are based on lexer construct. A little modification of lexer is possible IMHO those issues come from the design rather than the implementation of lexer. AFAIK we don't have consensus about the case that both of \g and \gset are used for a query like this: postgres=# SELECT 1 \gset var \\ \g foo.txt This seems regal. Should we store 1 into var and write the result into foo.txt? Or, only either of them? It's just an idea and it requires new special character, but how about use \g command for file, pipe, and variable? In the case we choose '' for variable prefix: postgres=# SELECT 'hello', 'wonderful', 'world!' \g var1,,var2 Anyway, we've had no psql's meta command which processes query result other than \g. So, we might have more considerable issues about design. a current design is rigid - a small implementation can stop parsing target list, when other backslash statement is detected BTW, what the word comma_expected means? It's in the comment above psql_scan_slash_vars(). It might be a remaining of old implementation. This identifier is mistaken - etc this comment is wrong and related to old implementation - sorry. A first design was replaced by state machine described by VarListParserState Regards, -- Shigeru HANADA shigeru.han...@gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
2012/10/15 Pavel Stehule pavel.steh...@gmail.com: 2012/10/15 Shigeru HANADA shigeru.han...@gmail.com: Hi Pavel, First of all, I'm sorry that my previous review was rough. I looked your patch and existing code closely again. On 2012/10/15, at 12:57, Pavel Stehule pavel.steh...@gmail.com wrote: 2012/10/14 Tom Lane t...@sss.pgh.pa.us: * ExecQueryUsingCursor's concept of how to process command results (why? surely we don't need \gset to use a cursor) There was two possibilities, but hardly non using cursor is better way +1 for supporting the case when FETCH_COUNT 0, because user might set so mainly for other queries, and they would want to avoid changing FETCH_COUNT setting during every query followed by \gset. * the psql lexer (adding a whole bunch of stuff that probably doesn't belong there) ?? I think that Tom is talking about psql_scan_slash_vars(). It seems too specific to \gset command. How about to improve psql_scan_slash_options() so that it can handle comma-separated variable list? Although you might have tried it before. # Unused OT_VARLIST macro gave me the idea. yes, it is possible - I'll look on it at evening a reuse of psql_scan_slash_options is not good idea - a interface of this function is out of my purposes - and I cannot to signalise error from this procedure. But I can minimize psql_scan_slash_var and I can move lot of code out of lexer file. * the core psql settings construct (to store something that is in no way a persistent setting) ?? I thought that having \gset arguments in pset is reasonable, since \g uses pest.gfname to hold its one-shot setting. Or, we should refactor \g to fit with \gset? I might be missing Tom's point... Surely there is a less ugly and invasive way to do this. The fact that the reviewer keeps finding bizarre bugs like another backslash command on the same line doesn't work seems to me to be a good indication that this is touching things it shouldn't. - all these bugs are based on lexer construct. A little modification of lexer is possible IMHO those issues come from the design rather than the implementation of lexer. AFAIK we don't have consensus about the case that both of \g and \gset are used for a query like this: postgres=# SELECT 1 \gset var \\ \g foo.txt This seems regal. Should we store 1 into var and write the result into foo.txt? Or, only either of them? It's just an idea and it requires new special character, but how about use \g command for file, pipe, and variable? In the case we choose '' for variable prefix: postgres=# SELECT 'hello', 'wonderful', 'world!' \g var1,,var2 Anyway, we've had no psql's meta command which processes query result other than \g. So, we might have more considerable issues about design. a current design is rigid - a small implementation can stop parsing target list, when other backslash statement is detected BTW, what the word comma_expected means? It's in the comment above psql_scan_slash_vars(). It might be a remaining of old implementation. This identifier is mistaken - etc this comment is wrong and related to old implementation - sorry. A first design was replaced by state machine described by VarListParserState Regards, -- Shigeru HANADA shigeru.han...@gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Hello 2012/10/14 Erik Rijkers e...@xs4all.nl: On Sat, October 13, 2012 19:26, Pavel Stehule wrote: 2012/10/13 Shigeru HANADA shigeru.han...@gmail.com: After you determine whether it's ok or unnecessary, I'll mark this patch as Ready for committer. I found this behaviour which I think must count as a bug. \gset doesn't allow more \\-separated lines behind it: Only the last of these commands is problematic, and giving the syntax error $ psql psql (9.3devel-psql_var-20121012_2345-8b728e5c6e0ce6b6d6f54b92b390f14aa1aca6db) Type help for help. testdb=# select 1,2 \gset x,y testdb=# \echo :x 1 testdb=# \echo :y 2 testdb=# \echo :x \\ \echo :y 1 2 testdb=# select 1,2 \gset x,y \\ \echo :x \gset: syntax error testdb=# It'd be nice if it could be made to work done Regards Pavel Thanks Erik Rijkers gset_08.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Hi Pavel,
On Sat, Oct 13, 2012 at 12:58 AM, Pavel Stehule
pavel.steh...@gmail.com wrote:
* merge Shigeru's doc patch
* rename psql regression test from psql to psql_cmd
Those changes seem good.
Besides, I found an error message which doesn't end with '¥n' in
common.c. In general, messages passed to psql_error end with '¥n',
unless additional information follows. Please see attached patch for
additional change.
After you determine whether it's ok or unnecessary, I'll mark this patch
as Ready for committer.
Regards,
--
Shigeru HANADA
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
***
*** 1483,1490 testdb=gt;
way. Use command\i/command for that.) This means that
if the query ends with (or contains) a semicolon, it is
immediately executed. Otherwise it will merely wait in the
! query buffer; type semicolon or literal\g/ to send it, or
! literal\r/ to cancel.
/para
para
--- 1483,1490
way. Use command\i/command for that.) This means that
if the query ends with (or contains) a semicolon, it is
immediately executed. Otherwise it will merely wait in the
! query buffer; type semicolon, literal\g/ or
! literal\gset/literal to send it, or literal\r/ to cancel.
/para
para
***
*** 1617,1622 Tue Oct 26 21:40:57 CEST 1999
--- 1617,1644
/varlistentry
varlistentry
+ termliteral\gset/literal replaceable
class=parametervariable/replaceable [ ,replaceable
class=parametervariable/replaceable ... ] /term
+
+ listitem
+ para
+ Sends the current query input buffer to the server and stores the
+ query's output into corresponding replaceable
+ class=parametervariable/replaceable. The preceding query must
+ return only one row, and the number of variables must be same as the
+ number of elements in commandSELECT/command list. If you don't
+ need any of items in commandSELECT/command list, you can omit
+ corresponding replaceable class=parametervariable/replaceable.
+ Example:
+ programlisting
+ foo=gt; SELECT 'hello', 'wonderful', 'world!' \gset var1,,var3
+ foo=gt; \echo :var1 :var3
+ hello world!
+ /programlisting
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termliteral\h/literal or literal\help/literal literal[
replaceable class=parametercommand/replaceable ]/literal/term
listitem
para
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***
*** 748,753 exec_command(const char *cmd,
--- 748,776
status = PSQL_CMD_SEND;
}
+ /* \gset send query and store result */
+ else if (strcmp(cmd, gset) == 0)
+ {
+ boolerror;
+
+ pset.gvars = psql_scan_slash_vars(scan_state, error);
+
+ if (!pset.gvars !error)
+ {
+ psql_error(\\%s: missing required argument\n, cmd);
+ status = PSQL_CMD_NOSEND;
+ }
+ else if (error)
+ {
+ psql_error(\\%s: syntax error\n, cmd);
+ status = PSQL_CMD_NOSEND;
+ tglist_free(pset.gvars);
+ pset.gvars = NULL;
+ }
+ else
+ status = PSQL_CMD_SEND;
+ }
+
/* help */
else if (strcmp(cmd, h) == 0 || strcmp(cmd, help) == 0)
{
*** a/src/bin/psql/command.h
--- b/src/bin/psql/command.h
***
*** 16,21 typedef enum _backslashResult
--- 16,22
{
PSQL_CMD_UNKNOWN = 0, /* not done parsing yet (internal only)
*/
PSQL_CMD_SEND, /* query complete; send off */
+ PSQL_CMD_NOSEND,/* query complete, don't send */
PSQL_CMD_SKIP_LINE, /* keep building query */
PSQL_CMD_TERMINATE, /* quit program */
PSQL_CMD_NEWEDIT, /* query buffer was changed
(e.g., via \e) */
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***
*** 816,821 PrintQueryResults(PGresult *results)
--- 816,919
return success;
}
+ /*
+ * StoreQueryResult: store first row of result to selected variables
+ *
+ * Note: Utility function for use by SendQuery() only.
+ *
+ * Returns true if the query executed successfully, false otherwise.
+ */
+ static bool
+ StoreQueryResult(PGresult *result)
+ {
+ bool success;
+
+ switch (PQresultStatus(result))
+ {
+ case PGRES_TUPLES_OK:
+ {
+ int i;
+
+ if (PQntuples(result) 1)
+
Re: [HACKERS] proposal - assign result of query to psql variable
2012/10/13 Shigeru HANADA shigeru.han...@gmail.com: Hi Pavel, On Sat, Oct 13, 2012 at 12:58 AM, Pavel Stehule pavel.steh...@gmail.com wrote: * merge Shigeru's doc patch * rename psql regression test from psql to psql_cmd Those changes seem good. Besides, I found an error message which doesn't end with '¥n' in common.c. In general, messages passed to psql_error end with '¥n', unless additional information follows. Please see attached patch for additional change. After you determine whether it's ok or unnecessary, I'll mark this patch as Ready for committer. it is ok, thank you Pavel Regards, -- Shigeru HANADA -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
On Sat, October 13, 2012 19:26, Pavel Stehule wrote: 2012/10/13 Shigeru HANADA shigeru.han...@gmail.com: After you determine whether it's ok or unnecessary, I'll mark this patch as Ready for committer. I found this behaviour which I think must count as a bug. \gset doesn't allow more \\-separated lines behind it: Only the last of these commands is problematic, and giving the syntax error $ psql psql (9.3devel-psql_var-20121012_2345-8b728e5c6e0ce6b6d6f54b92b390f14aa1aca6db) Type help for help. testdb=# select 1,2 \gset x,y testdb=# \echo :x 1 testdb=# \echo :y 2 testdb=# \echo :x \\ \echo :y 1 2 testdb=# select 1,2 \gset x,y \\ \echo :x \gset: syntax error testdb=# It'd be nice if it could be made to work Thanks Erik Rijkers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Hello here is updated version of gset patch. * merge Shigeru's doc patch * rename psql regression test from psql to psql_cmd Regards Pavel Stehule 2012/9/27 Pavel Stehule pavel.steh...@gmail.com: Hello 2012/9/21 Shigeru HANADA shigeru.han...@gmail.com: Hi Pavel, (2012/09/21 2:01), Pavel Stehule wrote: - Is it intentional that \gset can set special variables such as AUTOCOMMIT and HOST? I don't see any downside for this behavior, because \set also can do that, but it is not documented nor tested at all. I use a same SetVariable function, so a behave should be same It seems reasonable. Document - Adding some description of \gset command, especially about limitation of variable list, seems necessary. - In addition to the meta-command section, Advanced features section mentions how to set psql's variables, so we would need some mention there too. - The term target list might not be familiar to users, since it appears in only sections mentioning PG internal relatively. I think that the feature described in the section Retrieving Query Results in ECPG document is similar to this feature. http://www.postgresql.org/docs/devel/static/ecpg-variables.html I invite any proposals about enhancing documentation. Personally I am a PostgreSQL developer, so I don't known any different term other than target list - but any user friendly description is welcome. How about to say stores the query's result output into variable? Please see attached file for my proposal. I also mentioned about 1-row limit and omit of variable. should be Coding == The code follows our coding conventions. Here are comments for coding. - Some typo found in comments, please see attached patch. - There is a code path which doesn't print error message even if libpq reported error (PGRES_BAD_RESPONSE, PGRES_NONFATAL_ERROR, PGRES_FATAL_ERROR) in StoreQueryResult. Is this intentional? FYI, ecpg prints bad response message for those errors. yes - it is question. I use same pattern like PrintQueryResult, but bad response message should be used. I am sending updated patch It seems ok. BTW, as far as I see, no psql backslash command including \setenv (it was added in 9.2) has regression test in core (I mean src/test/regress). Is there any convention about this issue? If psql backslash commands (or any psql feature else) don't need regression test, we can remove psql.(sql|out). # Of course we need to test new feature by hand. It is question for Tom or David - only server side functionalities has regress tests. But result of some backslash command is verified in other regress tests. I would to see some regression tests for this functionality. Anyway, IMO the name psql impresses larger area than the patch implements. How about to rename psql to psql_cmd or backslash_cmd than psql as regression test name? I have no idea - psql_cmd is good name Regards Pavel -- Shigeru HANADA gset_06.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Hello 2012/9/21 Shigeru HANADA shigeru.han...@gmail.com: Hi Pavel, (2012/09/21 2:01), Pavel Stehule wrote: - Is it intentional that \gset can set special variables such as AUTOCOMMIT and HOST? I don't see any downside for this behavior, because \set also can do that, but it is not documented nor tested at all. I use a same SetVariable function, so a behave should be same It seems reasonable. Document - Adding some description of \gset command, especially about limitation of variable list, seems necessary. - In addition to the meta-command section, Advanced features section mentions how to set psql's variables, so we would need some mention there too. - The term target list might not be familiar to users, since it appears in only sections mentioning PG internal relatively. I think that the feature described in the section Retrieving Query Results in ECPG document is similar to this feature. http://www.postgresql.org/docs/devel/static/ecpg-variables.html I invite any proposals about enhancing documentation. Personally I am a PostgreSQL developer, so I don't known any different term other than target list - but any user friendly description is welcome. How about to say stores the query's result output into variable? Please see attached file for my proposal. I also mentioned about 1-row limit and omit of variable. should be Coding == The code follows our coding conventions. Here are comments for coding. - Some typo found in comments, please see attached patch. - There is a code path which doesn't print error message even if libpq reported error (PGRES_BAD_RESPONSE, PGRES_NONFATAL_ERROR, PGRES_FATAL_ERROR) in StoreQueryResult. Is this intentional? FYI, ecpg prints bad response message for those errors. yes - it is question. I use same pattern like PrintQueryResult, but bad response message should be used. I am sending updated patch It seems ok. BTW, as far as I see, no psql backslash command including \setenv (it was added in 9.2) has regression test in core (I mean src/test/regress). Is there any convention about this issue? If psql backslash commands (or any psql feature else) don't need regression test, we can remove psql.(sql|out). # Of course we need to test new feature by hand. It is question for Tom or David - only server side functionalities has regress tests. But result of some backslash command is verified in other regress tests. I would to see some regression tests for this functionality. Anyway, IMO the name psql impresses larger area than the patch implements. How about to rename psql to psql_cmd or backslash_cmd than psql as regression test name? I have no idea - psql_cmd is good name Regards Pavel -- Shigeru HANADA -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Hello 2012/9/19 Shigeru HANADA shigeru.han...@gmail.com: On Fri, Aug 10, 2012 at 3:21 AM, Pavel Stehule pavel.steh...@gmail.com wrote: there is new version of this patch * cleaned var list parser * new regress tests * support FETCH_COUNT 0 Here are my review comments. Submission == The patch is formatted in context diff style, and it could be applied cleanly against latest master. This patch include document and tests, but IMO they need some enhancement. Usability = This patch provides new psql command \gset which sends content of query buffer to server, and stores result of the query into psql variables. The name \gset is mixture of \g, which sends result to file or pipe, and \set, which sets variable to some value, so it would sound natural to psql users. Freature test = Compile completed without warning. Regression tests for \gset passed, but I have some comments on them. - Other regression tests have comment -- ERROR just after queries which should fail. It would be nice to follow this manner. - Typo to few in expected file and source file. - How about adding testing \gset (no variable list) to should fail? - Is it intentional that \gset can set special variables such as AUTOCOMMIT and HOST? I don't see any downside for this behavior, because \set also can do that, but it is not documented nor tested at all. I use a same SetVariable function, so a behave should be same Document - Adding some description of \gset command, especially about limitation of variable list, seems necessary. - In addition to the meta-command section, Advanced features section mentions how to set psql's variables, so we would need some mention there too. - The term target list might not be familiar to users, since it appears in only sections mentioning PG internal relatively. I think that the feature described in the section Retrieving Query Results in ECPG document is similar to this feature. http://www.postgresql.org/docs/devel/static/ecpg-variables.html I invite any proposals about enhancing documentation. Personally I am a PostgreSQL developer, so I don't known any different term other than target list - but any user friendly description is welcome. Coding == The code follows our coding conventions. Here are comments for coding. - Some typo found in comments, please see attached patch. - There is a code path which doesn't print error message even if libpq reported error (PGRES_BAD_RESPONSE, PGRES_NONFATAL_ERROR, PGRES_FATAL_ERROR) in StoreQueryResult. Is this intentional? FYI, ecpg prints bad response message for those errors. yes - it is question. I use same pattern like PrintQueryResult, but bad response message should be used. I am sending updated patch Although I'll look the code more closely later, but anyway I marked the patch Waiting on Author for comments above. Regards, -- Shigeru HANADA gset_04.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Hi Pavel, (2012/09/21 2:01), Pavel Stehule wrote: - Is it intentional that \gset can set special variables such as AUTOCOMMIT and HOST? I don't see any downside for this behavior, because \set also can do that, but it is not documented nor tested at all. I use a same SetVariable function, so a behave should be same It seems reasonable. Document - Adding some description of \gset command, especially about limitation of variable list, seems necessary. - In addition to the meta-command section, Advanced features section mentions how to set psql's variables, so we would need some mention there too. - The term target list might not be familiar to users, since it appears in only sections mentioning PG internal relatively. I think that the feature described in the section Retrieving Query Results in ECPG document is similar to this feature. http://www.postgresql.org/docs/devel/static/ecpg-variables.html I invite any proposals about enhancing documentation. Personally I am a PostgreSQL developer, so I don't known any different term other than target list - but any user friendly description is welcome. How about to say stores the query's result output into variable? Please see attached file for my proposal. I also mentioned about 1-row limit and omit of variable. Coding == The code follows our coding conventions. Here are comments for coding. - Some typo found in comments, please see attached patch. - There is a code path which doesn't print error message even if libpq reported error (PGRES_BAD_RESPONSE, PGRES_NONFATAL_ERROR, PGRES_FATAL_ERROR) in StoreQueryResult. Is this intentional? FYI, ecpg prints bad response message for those errors. yes - it is question. I use same pattern like PrintQueryResult, but bad response message should be used. I am sending updated patch It seems ok. BTW, as far as I see, no psql backslash command including \setenv (it was added in 9.2) has regression test in core (I mean src/test/regress). Is there any convention about this issue? If psql backslash commands (or any psql feature else) don't need regression test, we can remove psql.(sql|out). # Of course we need to test new feature by hand. Anyway, IMO the name psql impresses larger area than the patch implements. How about to rename psql to psql_cmd or backslash_cmd than psql as regression test name? -- Shigeru HANADA diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 3693a5a..c4ac674 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -1483,8 +1483,8 @@ testdb=gt; way. Use command\i/command for that.) This means that if the query ends with (or contains) a semicolon, it is immediately executed. Otherwise it will merely wait in the -query buffer; type semicolon, literal\g/ or literal\gset/literal to send it, or -literal\r/ to cancel. +query buffer; type semicolon, literal\g/ or +literal\gset/literal to send it, or literal\r/ to cancel. /para para @@ -1621,9 +1621,19 @@ Tue Oct 26 21:40:57 CEST 1999 listitem para -Sends the current query input buffer to the server and stores -the query's target list a corresponding list of psql -variables. + Sends the current query input buffer to the server and stores the + query's output into corresponding replaceable + class=parametervariable/replaceable. The preceding query must + return only one row, and the number of variables must be same as the + number of elements in commandSELECT/command list. If you don't + need any of items in commandSELECT/command list, you can omit + corresponding replaceable class=parametervariable/replaceable. + Example: +programlisting +foo=gt; SELECT 'hello', 'wonderful', 'world!' \gset var1,,var3 +foo=gt; \echo :var1 :var3 +hello world! +/programlisting /para /listitem /varlistentry -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
On Fri, Aug 10, 2012 at 3:21 AM, Pavel Stehule pavel.steh...@gmail.com
wrote:
there is new version of this patch
* cleaned var list parser
* new regress tests
* support FETCH_COUNT 0
Here are my review comments.
Submission
==
The patch is formatted in context diff style, and it could be applied
cleanly against latest master. This patch include document and tests,
but IMO they need some enhancement.
Usability
=
This patch provides new psql command \gset which sends content of query
buffer to server, and stores result of the query into psql variables.
The name \gset is mixture of \g, which sends result to file or pipe,
and \set, which sets variable to some value, so it would sound natural
to psql users.
Freature test
=
Compile completed without warning. Regression tests for \gset passed,
but I have some comments on them.
- Other regression tests have comment -- ERROR just after queries
which should fail. It would be nice to follow this manner.
- Typo to few in expected file and source file.
- How about adding testing \gset (no variable list) to should fail?
- Is it intentional that \gset can set special variables such as
AUTOCOMMIT and HOST? I don't see any downside for this behavior,
because \set also can do that, but it is not documented nor tested at all.
Document
- Adding some description of \gset command, especially about limitation
of variable list, seems necessary.
- In addition to the meta-command section, Advanced features section
mentions how to set psql's variables, so we would need some mention
there too.
- The term target list might not be familiar to users, since it
appears in only sections mentioning PG internal relatively. I think
that the feature described in the section Retrieving Query Results in
ECPG document is similar to this feature.
http://www.postgresql.org/docs/devel/static/ecpg-variables.html
Coding
==
The code follows our coding conventions. Here are comments for coding.
- Some typo found in comments, please see attached patch.
- There is a code path which doesn't print error message even if libpq
reported error (PGRES_BAD_RESPONSE, PGRES_NONFATAL_ERROR,
PGRES_FATAL_ERROR) in StoreQueryResult. Is this intentional? FYI, ecpg
prints bad response message for those errors.
Although I'll look the code more closely later, but anyway I marked the
patch Waiting on Author for comments above.
Regards,
--
Shigeru HANADA
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 0e9b408..a76b84d 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -832,7 +832,7 @@ PrintQueryResults(PGresult *results)
*
* Note: Utility function for use by SendQuery() only.
*
- * Returns true if the query executed sucessfully, false otherwise.
+ * Returns true if the query executed successfully, false otherwise.
*/
static bool
StoreQueryResult(PGresult *result)
@@ -865,7 +865,7 @@ StoreQueryResult(PGresult *result)
{
if (!iter)
{
- psql_error(to few
target variables\n);
+ psql_error(too few
target variables\n);
success = false;
break;
}
@@ -902,7 +902,7 @@ StoreQueryResult(PGresult *result)
case PGRES_COPY_OUT:
case PGRES_COPY_IN:
- psql_error(COPY isnot supported by \\gset command\n);
+ psql_error(COPY is not supported by \\gset command\n);
success = false;
break;
@@ -1797,7 +1797,7 @@ expand_tilde(char **filename)
/*
- * Add name of internal variable to query targer list
+ * Add name of internal variable to query target list
*
*/
TargetList
diff --git a/src/test/regress/expected/psql.out
b/src/test/regress/expected/psql.out
index 90ab9bd..54fa490 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -15,7 +15,7 @@ select 10, 20, 'Hello World'
-- should fail
\gset ,,
\gset ,
-to few target variables
+too few target variables
\gset ,,,
too many target variables
-- should be ok
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Hello there is new version of this patch * cleaned var list parser * new regress tests * support FETCH_COUNT 0 Regards Pavel Stehule 2012/8/1 David Fetter da...@fetter.org: On Sat, Jul 28, 2012 at 06:11:21PM +0200, Pavel Stehule wrote: Hello 2012/7/27 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: 2012/7/26 David Fetter da...@fetter.org: How about \gset var1,,,var2,var3... I don't like this - you can use fake variable - and ignoring some variable has no big effect on client Why assign to a variable you'll never use? so why you get data from server, when you would not to use it ? Yeah. I don't see why you'd be likely to write a select that computes columns you don't actually want. Tom - your proposal release of stored dataset just before next statement, not like now on the end of statement? Huh? I think you'd assign the values to the variables and then PQclear the result right away. yes - I didn't understand \g mechanism well. Here is patch - it is not nice at this moment and it is little bit longer than I expected - but it works It supports David's syntax postgres=# select 'Hello', 'World' \gset a,b postgres=# \echo :'a' :'b' 'Hello' 'World' postgres=# select 'Hello', 'World'; ?column? │ ?column? ──┼── Hello│ World (1 row) postgres=# \gset a to few target variables postgres=# \gset a, postgres=# \echo :'a' 'Hello' Regards Pavel Teensy code cleanup (trailing space) and SGML docs added. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate gset_03.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
On Sat, Jul 28, 2012 at 06:11:21PM +0200, Pavel Stehule wrote:
Hello
2012/7/27 Tom Lane t...@sss.pgh.pa.us:
Pavel Stehule pavel.steh...@gmail.com writes:
2012/7/26 David Fetter da...@fetter.org:
How about
\gset var1,,,var2,var3...
I don't like this - you can use fake variable - and ignoring some
variable has no big effect on client
Why assign to a variable you'll never use?
so why you get data from server, when you would not to use it ?
Yeah. I don't see why you'd be likely to write a select that computes
columns you don't actually want.
Tom - your proposal release of stored dataset just before next
statement, not like now on the end of statement?
Huh? I think you'd assign the values to the variables and then PQclear
the result right away.
yes - I didn't understand \g mechanism well.
Here is patch - it is not nice at this moment and it is little bit
longer than I expected - but it works
It supports David's syntax
postgres=# select 'Hello', 'World' \gset a,b
postgres=# \echo :'a' :'b'
'Hello' 'World'
postgres=# select 'Hello', 'World';
?column? │ ?column?
──┼──
Hello│ World
(1 row)
postgres=# \gset a
to few target variables
postgres=# \gset a,
postgres=# \echo :'a'
'Hello'
Regards
Pavel
Teensy code cleanup (trailing space) and SGML docs added.
Cheers,
David.
--
David Fetter da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
***
*** 1489,1495 testdb=gt;
way. Use command\i/command for that.) This means that
if the query ends with (or contains) a semicolon, it is
immediately executed. Otherwise it will merely wait in the
! query buffer; type semicolon or literal\g/ to send it, or
literal\r/ to cancel.
/para
--- 1489,1495
way. Use command\i/command for that.) This means that
if the query ends with (or contains) a semicolon, it is
immediately executed. Otherwise it will merely wait in the
! query buffer; type semicolon, literal\g/ or
literal\gset/literal to send it, or
literal\r/ to cancel.
/para
***
*** 1623,1628 Tue Oct 26 21:40:57 CEST 1999
--- 1623,1640
/varlistentry
varlistentry
+ termliteral\gset/literal replaceable
class=parametervariable/replaceable [ ,replaceable
class=parametervariable/replaceable ... ] /term
+
+ listitem
+ para
+ Sends the current query input buffer to the server and stores
+ the query's target list a corresponding list of psql
+ variables.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termliteral\h/literal or literal\help/literal literal[
replaceable class=parametercommand/replaceable ]/literal/term
listitem
para
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***
*** 748,753 exec_command(const char *cmd,
--- 748,776
status = PSQL_CMD_SEND;
}
+ /* \gset send query and store result */
+ else if (strcmp(cmd, gset) == 0)
+ {
+ boolerror;
+
+ pset.gvars = psql_scan_slash_vars(scan_state, error);
+
+ if (!pset.gvars)
+ {
+ psql_error(\\%s: missing required argument\n, cmd);
+ status = PSQL_CMD_NOSEND;
+ }
+ else if (error)
+ {
+ psql_error(\\%s: syntax error\n, cmd);
+ status = PSQL_CMD_NOSEND;
+ tglist_free(pset.gvars);
+ pset.gvars = NULL;
+ }
+ else
+ status = PSQL_CMD_SEND;
+ }
+
/* help */
else if (strcmp(cmd, h) == 0 || strcmp(cmd, help) == 0)
{
*** a/src/bin/psql/command.h
--- b/src/bin/psql/command.h
***
*** 16,21 typedef enum _backslashResult
--- 16,22
{
PSQL_CMD_UNKNOWN = 0, /* not done parsing yet (internal only)
*/
PSQL_CMD_SEND, /* query complete; send off */
+ PSQL_CMD_NOSEND,/* query complete, don't send */
PSQL_CMD_SKIP_LINE, /* keep building query */
PSQL_CMD_TERMINATE, /* quit program */
PSQL_CMD_NEWEDIT, /* query buffer was changed
(e.g., via \e) */
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***
*** 826,831 PrintQueryResults(PGresult *results)
Re: [HACKERS] proposal - assign result of query to psql variable
Hello 2012/7/27 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: 2012/7/26 David Fetter da...@fetter.org: How about \gset var1,,,var2,var3... I don't like this - you can use fake variable - and ignoring some variable has no big effect on client Why assign to a variable you'll never use? so why you get data from server, when you would not to use it ? Yeah. I don't see why you'd be likely to write a select that computes columns you don't actually want. Tom - your proposal release of stored dataset just before next statement, not like now on the end of statement? Huh? I think you'd assign the values to the variables and then PQclear the result right away. yes - I didn't understand \g mechanism well. Here is patch - it is not nice at this moment and it is little bit longer than I expected - but it works It supports David's syntax postgres=# select 'Hello', 'World' \gset a,b postgres=# \echo :'a' :'b' 'Hello' 'World' postgres=# select 'Hello', 'World'; ?column? │ ?column? ──┼── Hello│ World (1 row) postgres=# \gset a to few target variables postgres=# \gset a, postgres=# \echo :'a' 'Hello' Regards Pavel regards, tom lane gset.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote: Pavel Stehule pavel.steh...@gmail.com writes: \eset variable [, variable [..]] query -- it raise exception when more than one row is returned or when no row is returned Better would be a variant on \g, that is you type in the query and then tell it where to put the result. We have learned the hard way that putting SQL commands into the arguments of backslash commands is a horrid idea. Maybe select x,y,... from ... \gset var1 var2 ... How about \gset var1,,,var2,var3... The above shows how one would skip assigning variables in the target list, which one might want to do. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
2012/7/26 David Fetter da...@fetter.org: On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote: Pavel Stehule pavel.steh...@gmail.com writes: \eset variable [, variable [..]] query -- it raise exception when more than one row is returned or when no row is returned Better would be a variant on \g, that is you type in the query and then tell it where to put the result. We have learned the hard way that putting SQL commands into the arguments of backslash commands is a horrid idea. Maybe select x,y,... from ... \gset var1 var2 ... How about \gset var1,,,var2,var3... I don't like this - you can use fake variable - and ignoring some variable has no big effect on client Pavel The above shows how one would skip assigning variables in the target list, which one might want to do. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
On Thu, Jul 26, 2012 at 08:31:13AM +0200, Pavel Stehule wrote: 2012/7/26 David Fetter da...@fetter.org: On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote: Pavel Stehule pavel.steh...@gmail.com writes: \eset variable [, variable [..]] query -- it raise exception when more than one row is returned or when no row is returned Better would be a variant on \g, that is you type in the query and then tell it where to put the result. We have learned the hard way that putting SQL commands into the arguments of backslash commands is a horrid idea. Maybe select x,y,... from ... \gset var1 var2 ... How about \gset var1,,,var2,var3... I don't like this - you can use fake variable - and ignoring some variable has no big effect on client Why assign to a variable you'll never use? Cheers, David. P.S. The bike shed should be puce with blaze orange pin-striping. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
2012/7/26 David Fetter da...@fetter.org: On Thu, Jul 26, 2012 at 08:31:13AM +0200, Pavel Stehule wrote: 2012/7/26 David Fetter da...@fetter.org: On Thu, Jul 26, 2012 at 01:36:17AM -0400, Tom Lane wrote: Pavel Stehule pavel.steh...@gmail.com writes: \eset variable [, variable [..]] query -- it raise exception when more than one row is returned or when no row is returned Better would be a variant on \g, that is you type in the query and then tell it where to put the result. We have learned the hard way that putting SQL commands into the arguments of backslash commands is a horrid idea. Maybe select x,y,... from ... \gset var1 var2 ... How about \gset var1,,,var2,var3... I don't like this - you can use fake variable - and ignoring some variable has no big effect on client Why assign to a variable you'll never use? so why you get data from server, when you would not to use it ? no offence, probably it is not hard to implement it - because we use own parser, but I see this proposal little bit obscure Tom - your proposal release of stored dataset just before next statement, not like now on the end of statement? Regards Pavel Cheers, David. P.S. The bike shed should be puce with blaze orange pin-striping. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Pavel Stehule pavel.steh...@gmail.com writes: 2012/7/26 David Fetter da...@fetter.org: How about \gset var1,,,var2,var3... I don't like this - you can use fake variable - and ignoring some variable has no big effect on client Why assign to a variable you'll never use? so why you get data from server, when you would not to use it ? Yeah. I don't see why you'd be likely to write a select that computes columns you don't actually want. Tom - your proposal release of stored dataset just before next statement, not like now on the end of statement? Huh? I think you'd assign the values to the variables and then PQclear the result right away. 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
[HACKERS] proposal - assign result of query to psql variable
Hello we cannot actually store result of query to psql variable I propose a new slash statement \eset for this purpose Syntax: \eset variable [, variable [..]] query -- it raise exception when more than one row is returned or when no row is returned Usage: \eset var1, var2 select version(), current_date Current workaround is not friendly and it is not usable for more than one target variable http://stackoverflow.com/questions/11654244/how-to-bind-a-sql-query-return-value-to-a-psql-variable/11654676#11654676 postgres=# \set myvar `psql -A -t -c select version() postgres ` postgres=# \echo :myvar PostgreSQL 9.1.4 on x86_64-unknown-linux-gnu, compiled by gcc (GCC) 4.7.0 20120507 (Red Hat 4.7.0-5), 64-bit Regards Pavel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
Pavel Stehule pavel.steh...@gmail.com writes: \eset variable [, variable [..]] query -- it raise exception when more than one row is returned or when no row is returned Better would be a variant on \g, that is you type in the query and then tell it where to put the result. We have learned the hard way that putting SQL commands into the arguments of backslash commands is a horrid idea. Maybe select x,y,... from ... \gset var1 var2 ... 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
Re: [HACKERS] proposal - assign result of query to psql variable
2012/7/26 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: \eset variable [, variable [..]] query -- it raise exception when more than one row is returned or when no row is returned Better would be a variant on \g, that is you type in the query and then tell it where to put the result. We have learned the hard way that putting SQL commands into the arguments of backslash commands is a horrid idea. Maybe select x,y,... from ... \gset var1 var2 ... it could be Pavel 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
