Thread
-
patch: assign result of query to psql variable
Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-12T17:00:31Z
---------- Forwarded message ---------- From: Pavel Stehule <pavel.stehule@gmail.com> Date: 2012/10/12 Subject: Re: [HACKERS] proposal - assign result of query to psql variable To: Shigeru HANADA <shigeru.hanada@gmail.com> Kopie: David Fetter <david@fetter.org>, Tom Lane <tgl@sss.pgh.pa.us>, PostgreSQL Hackers <pgsql-hackers@postgresql.org> 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.stehule@gmail.com>: > Hello > > 2012/9/21 Shigeru HANADA <shigeru.hanada@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