pgbench - allow to store select results into variables

classic Classic list List threaded Threaded
120 messages Options
1234 ... 6
Reply | Threaded
Open this post in threaded view
|

pgbench - allow to store select results into variables

Fabien COELHO-3

Hello devs,

I mentionned my intention to add some features to pgbench back in March:
https://www.postgresql.org/message-id/alpine.DEB.2.10.1603301618570.5677@sto

The attached patch adds an \into meta command to store results of
preceding SELECTs into pgbench variables, so that they can be reused
afterwards.

The feature is useful to make more realistic scripts, currently pgbench
script cannot really interact with the database as results are discarded.

The chosen syntax is easy to understand and the implementation is quite
light, with minimal impact on the code base. I think that this is a
reasonnable compromise.

The SELECTs must yield exactly one row, the number of variables must be
less than the number of columns.

Also attached a set of test scripts, especially to trigger various error
cases.

--
Fabien.

--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

pgbench-into-1.patch (9K) Download Attachment
into.sql (414 bytes) Download Attachment
into-err-1.sql (46 bytes) Download Attachment
into-err-2.sql (68 bytes) Download Attachment
into-err-3.sql (108 bytes) Download Attachment
into-err-4.sql (58 bytes) Download Attachment
into-err-5.sql (104 bytes) Download Attachment
into-err-6.sql (124 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Pavel Stehule
Hi

2016-07-09 10:20 GMT+02:00 Fabien COELHO <[hidden email]>:

Hello devs,

I mentionned my intention to add some features to pgbench back in March:
https://www.postgresql.org/message-id/alpine.DEB.2.10.1603301618570.5677@sto

The attached patch adds an \into meta command to store results of preceding SELECTs into pgbench variables, so that they can be reused afterwards.

The feature is useful to make more realistic scripts, currently pgbench script cannot really interact with the database as results are discarded.

The chosen syntax is easy to understand and the implementation is quite light, with minimal impact on the code base. I think that this is a reasonnable compromise.

The SELECTs must yield exactly one row, the number of variables must be less than the number of columns.

Also attached a set of test scripts, especially to trigger various error cases.


Why you are introducing \into and not \gset like psql does?

Regards

Pavel
 
--
Fabien.


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Fabien COELHO-3

Hello Pavel,

> Why you are introducing \into and not \gset like psql does?

Good question.

The \into syntax I implemented is more generic, you can send a bunch of
queries together and extract the results, which makes sense from a client
perspective where reducing latency is important:

    SELECT 1, 2 \; SELECT 3;
    \into one two three

However "gset" only works on the last SELECT and if all columns have a
name. This feature probably makes sense interactively, but for a script it
seems more useful to allow batch processing and collect results
afterwards.

Also a more subjective argument: I do not like the gset automagic naming
feature. I got more inspired by PL/pgSQL and ECPG which both have an
"into" syntax with explicit variable names that let nothing to guessing. I
like things to be simple and explicit, hence the proposed into.

--
Fabien.


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Pavel Stehule


2016-07-09 11:19 GMT+02:00 Fabien COELHO <[hidden email]>:

Hello Pavel,

Why you are introducing \into and not \gset like psql does?

Good question.

The \into syntax I implemented is more generic, you can send a bunch of queries together and extract the results, which makes sense from a client perspective where reducing latency is important:

   SELECT 1, 2 \; SELECT 3;
   \into one two three

I understand, but it looks little bit scary - but the argument of reducing latency can be valid

However "gset" only works on the last SELECT and if all columns have a name. This feature probably makes sense interactively, but for a script it seems more useful to allow batch processing and collect results afterwards.

Also a more subjective argument: I do not like the gset automagic naming feature. I got more inspired by PL/pgSQL and ECPG which both have an "into" syntax with explicit variable names that let nothing to guessing. I like things to be simple and explicit, hence the proposed into.

the gset was originally designed differently - but now it is here - and it is not practical to have two different, but pretty similar statements in psql and pgbench.

Regards

Pavel

 

--
Fabien.

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Fabien COELHO-3

>> Also a more subjective argument: I do not like the gset automagic naming
>> feature. I got more inspired by PL/pgSQL and ECPG which both have an "into"
>> syntax with explicit variable names that let nothing to guessing. I like
>> things to be simple and explicit, hence the proposed into.
>
> the gset was originally designed differently - but now it is here - and it
> is not practical to have two different, but pretty similar statements in
> psql and pgbench.

In my view they are unrelated: on the one hand "gset" is really an
interactive feature, where typing is costly so "automagic" might make
sense; on the other hand "into" is a scripting feature, where you want to
understand the code and have something as readable as possible, without
surprises.

The commands are named differently and behave differently.

If someone thinks that "gset" is a good idea for pgbench, which I don't,
it could be implemented. I think that an "into" feature, like PL/pgSQL &
ECPG, makes more sense for scripting.

--
Fabien.


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Robert Haas
On Sat, Jul 9, 2016 at 7:52 AM, Fabien COELHO <[hidden email]> wrote:
> If someone thinks that "gset" is a good idea for pgbench, which I don't, it
> could be implemented. I think that an "into" feature, like PL/pgSQL & ECPG,
> makes more sense for scripting.

I agree: I like \into.

But:

> SELECT 1, 2 \; SELECT 3;
>  \into one two three

I think that's pretty weird.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Tom Lane-2
Robert Haas <[hidden email]> writes:
> On Sat, Jul 9, 2016 at 7:52 AM, Fabien COELHO <[hidden email]> wrote:
>> If someone thinks that "gset" is a good idea for pgbench, which I don't, it
>> could be implemented. I think that an "into" feature, like PL/pgSQL & ECPG,
>> makes more sense for scripting.

> I agree: I like \into.

> But:

>> SELECT 1, 2 \; SELECT 3;
>> \into one two three

> I think that's pretty weird.

Yeah, that's seriously nasty action-at-a-distance in my view.  I'd be okay
with

SELECT 1, 2 \into one two
SELECT 3 \into three

but I do not think that a metacommand on a following line should
retroactively affect the execution of a prior command, much less commands
before the last one.  Even if this happens to be easy to do in pgbench's
existing over-contorted logic, it's tremendously confusing to the user;
and it might be much less easy if we try to refactor that logic.

And I'm with Pavel on this: it should work exactly like \gset.  Inventing
\into to do almost the same thing in a randomly different way exhibits a
bad case of NIH syndrome.  Sure, you can argue about how it's not quite
the same use-case and so you could micro-optimize by doing it differently,
but that's ignoring the cognitive load on users who have to remember two
different commands.  Claiming that plpgsql's SELECT INTO is a closer
analogy than psql's \gset is quite bogus, too: the environment is
different (client side vs server side, declared vs undeclared target
variables), and the syntax is different (backslash or not, commas or not,
just for starters).  I note also that we were talking a couple months ago
about trying to align psql and pgbench backslash commands more closely.
This would not be a good step in that direction.

                        regards, tom lane


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Robert Haas
On Wed, Jul 13, 2016 at 3:02 PM, Tom Lane <[hidden email]> wrote:

> Robert Haas <[hidden email]> writes:
>> On Sat, Jul 9, 2016 at 7:52 AM, Fabien COELHO <[hidden email]> wrote:
>>> If someone thinks that "gset" is a good idea for pgbench, which I don't, it
>>> could be implemented. I think that an "into" feature, like PL/pgSQL & ECPG,
>>> makes more sense for scripting.
>
>> I agree: I like \into.
>
>> But:
>
>>> SELECT 1, 2 \; SELECT 3;
>>> \into one two three
>
>> I think that's pretty weird.
>
> Yeah, that's seriously nasty action-at-a-distance in my view.  I'd be okay
> with
>
> SELECT 1, 2 \into one two
> SELECT 3 \into three
>
> but I do not think that a metacommand on a following line should
> retroactively affect the execution of a prior command, much less commands
> before the last one.  Even if this happens to be easy to do in pgbench's
> existing over-contorted logic, it's tremendously confusing to the user;
> and it might be much less easy if we try to refactor that logic.
>
> And I'm with Pavel on this: it should work exactly like \gset.  Inventing
> \into to do almost the same thing in a randomly different way exhibits a
> bad case of NIH syndrome.  Sure, you can argue about how it's not quite
> the same use-case and so you could micro-optimize by doing it differently,
> but that's ignoring the cognitive load on users who have to remember two
> different commands.  Claiming that plpgsql's SELECT INTO is a closer
> analogy than psql's \gset is quite bogus, too: the environment is
> different (client side vs server side, declared vs undeclared target
> variables), and the syntax is different (backslash or not, commas or not,
> just for starters).  I note also that we were talking a couple months ago
> about trying to align psql and pgbench backslash commands more closely.
> This would not be a good step in that direction.

True, but I'd still argue that \into is a lot more readable than
\gset.  Maybe both programs should support both commands.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Tom Lane-2
Robert Haas <[hidden email]> writes:
> On Wed, Jul 13, 2016 at 3:02 PM, Tom Lane <[hidden email]> wrote:
>> I note also that we were talking a couple months ago
>> about trying to align psql and pgbench backslash commands more closely.
>> This would not be a good step in that direction.

> True, but I'd still argue that \into is a lot more readable than
> \gset.  Maybe both programs should support both commands.

Meh, personal preference there no doubt.  I'd be okay with both programs
supporting both commands, but the \into command as described here would
be quite unreasonable to implement in psql.  It needs to act more like
a semicolon-substitute, as \g and the rest of its family do.  (I think
I'd also lobby for spelling it \ginto.)

                        regards, tom lane


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Fabien COELHO-3
In reply to this post by Robert Haas

Hello Robert,

> I agree: I like \into.

Great!

> But:
>
>> SELECT 1, 2 \; SELECT 3;
>>  \into one two three
>
> I think that's pretty weird.

I agree that it is weird, but I do not think that it is bad.

Sending a batch of requests is a feature of libpq which is accessible
through pgbench by using "\;", although the fact is not documented. It
makes sense for a client to send independent queries together so as to
reduce latency.

From pgbench perspective, I would find it pretty weird as well that one
can send several queries together but could only read the answer from...
say the first one, and the others would be lost.

From an implementation perspective doing it is straightforward, and
rejecting it would require some more logic.

An obvious nicer feature would be to allow intermixing \into & \; but ISTM
that it would require to rethink deeply pgbench lexing/parsing which has
just been merged with psql by Tom and others...

If I had not pointed out the fact that it works, maybe no one would have
noticed... so a compromise could be not to advertise the fact that it
works (as the \; feature is not advertised anyway), but letting the
implementation do it because it is simple and may be useful, and rephrase
the documentation so that it is just about the previous select and not
previous select*S*?

--
Fabien.


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Tom Lane-2
Fabien COELHO <[hidden email]> writes:
> Sending a batch of requests is a feature of libpq which is accessible
> through pgbench by using "\;", although the fact is not documented. It
> makes sense for a client to send independent queries together so as to
> reduce latency.

You're putting an awful lot of weight on an unsupported assertion about
latency.  If a user cares about that, why would she not simply merge the
commands into "SELECT 1, 2, 3 \into one two three" ?

And I still say that what you're proposing might be easy right now, but
it might also be next door to impossible in a refactored implementation.
I don't think we should go there on the basis of a weak argument about
latency.  \into should retrieve data only from the last PGresult.

                        regards, tom lane


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Fabien COELHO-3

Hello Tom,

>> Sending a batch of requests is a feature of libpq which is accessible
>> through pgbench by using "\;", although the fact is not documented. It
>> makes sense for a client to send independent queries together so as to
>> reduce latency.
>
> You're putting an awful lot of weight on an unsupported assertion about
> latency.

For support, I would submit that many applications today are web/mobile
apps which are quite sensitive to latency. See for instance the Fast 2016
white paper by people at Google which discusses in depth "tail latency" as
a key measure of quality for IO systems used for live services, or the new
HTTP2 protocole (based on Google spdy) which aims at reducing latency
through multiple features (compression, serveur push, pipelining...).

> If a user cares about that, why would she not simply merge the
> commands into "SELECT 1, 2, 3 \into one two three" ?

Because the code would look pretty awful:

   SELECT (SELECT first data FROM ... JOIN ... WHERE ... ),
          (SELECT second data FROM ... JOIN ... WHERE ...),
          (SELECT third data FROM ... JOIN ... WHERE ...);

> And I still say that what you're proposing might be easy right now, but
> it might also be next door to impossible in a refactored implementation.

I do not understand. There is one "multi" sql-command followed by a meta
command, and somehow a refactor implementation would have troubled with
that?

> I don't think we should go there on the basis of a weak argument about
> latency.  \into should retrieve data only from the last PGresult.

This looks pretty arbitrary: Why not the first one, as I asked for it
first? Anyway, why allowing to send several queries if you are not allowed
to extract their results.

--
Fabien.


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

David G Johnston
In reply to this post by Tom Lane-2
On Wed, Jul 13, 2016 at 4:02 PM, Tom Lane <[hidden email]> wrote:
Robert Haas <[hidden email]> writes:
> On Sat, Jul 9, 2016 at 7:52 AM, Fabien COELHO <[hidden email]> wrote:
>> If someone thinks that "gset" is a good idea for pgbench, which I don't, it
>> could be implemented. I think that an "into" feature, like PL/pgSQL & ECPG,
>> makes more sense for scripting.

> I agree: I like \into.

> But:

>> SELECT 1, 2 \; SELECT 3;
>> \into one two three

> I think that's pretty weird.

Yeah, that's seriously nasty action-at-a-distance in my view.  I'd be okay
with

SELECT 1, 2 \into one two
SELECT 3 \into three

but I do not think that a metacommand on a following line should
retroactively affect the execution of a prior command, much less commands
before the last one. 

You need a test and a definition for:

​SELECT 1, 2;
SELECT 3;
\into x, y, z

It should fail - "too many variables" - right?

David J.
 
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Fabien COELHO-3
In reply to this post by Tom Lane-2

Hello Tom,

>>> SELECT 1, 2 \; SELECT 3;
>>> \into one two three
>
> Yeah, that's seriously nasty action-at-a-distance in my view.  I'd be okay
> with
>
> SELECT 1, 2 \into one two
> SELECT 3 \into three

ISTM that is not the same, because then you would have two queries (over
the network) instead of one, so you pay the network latency twice?

> but I do not think that a metacommand on a following line should
> retroactively affect the execution of a prior command, much less commands
> before the last one.

Nope. The meta-command applies to the preceeding SQL command... which
happens to be a \;-compound command. ISTM that all is logically fine.


Some motivation about the feature (not its syntax or implementation), from
a benchmarking perspective:

- clients MUST read the server answers and possibly reuse them, hence a
proposed \into feature. Discarding the answer as pgbench does not really
comply with typical benchmark rules, eg from tpc-b:

   """1.3.2 Each transaction shall return to the driver the Account_Balance
      resulting from successful commit of the transaction.

   Comment: It is the intent of this clause that the account balance in the
   database be returned to the driver, i.e., that the application retrieve
   the account balance."""

- latency is important to applications (eg web applications), thus the
ability to compound statements is a good thing. However, if in a bench one
can compound statements but not retrieve their values, it fails the
previous "retrieve the value" requirement.

So basically I wish to avoid splitting compound queries and paying latency
just because of a lack of syntax to do the right thing, hence the proposed
feature which can retrieve data from various parts of a compound
statement.

--
Fabien.


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Fabien COELHO-3
In reply to this post by Tom Lane-2

Hello Tom,

> Yeah, that's seriously nasty action-at-a-distance in my view.  I'd be okay
> with
>
> SELECT 1, 2 \into one two
> SELECT 3 \into three

After giving it some thoughts, it could work on compound commands if \into
does not close the current sql command. Something like:

   SELECT 1, 2 ; \into one two
   SELECT 3 ; \into three
   => 2 SQL commands

   SELECT 1, 2 \; \into one two
   SELECT 3 ; \into three
   => 1 compound SQL command

I'd like \; or ; to stay mandatory as separators, though. Or at least to
be allowed.

I'm not quite sure how it could be implemented, though.

> And I'm with Pavel on this: it should work exactly like \gset.

Hmmm. Maybe I'll do that thing in the end, but I really think than gset
only makes sense in interactive context, and is pretty ugly for scripting.

> Inventing \into to do almost the same thing in a randomly different way
> exhibits a bad case of NIH syndrome.

No, it is a question of design suitable to programming:

   > SELECT 1, 2 \gset v
   could not set variable "?column?"

> Sure, you can argue about how it's not quite the same use-case

Indeed, that is my point.

> and so you could micro-optimize by doing it differently,

No, the underlying implementation is basically the same.

> but that's ignoring the cognitive load on users who have to remember two
> different commands.

I do not buy this argument: It is easier for me to remember that keyword
INTO happens to do the same thing the same way in PL/pgSQL and ECPG,
although with slightly different syntaxes, than to have to remember
psql-specific "gset" which does the same thing but in quite a different
way, because it means both another name and another concept.

> Claiming that plpgsql's SELECT INTO is a closer analogy than psql's
> \gset is quite bogus, too:

I disagree. I mentionned ECPG as well. Both ECPG & PLpgSQL are
"programming", psql is interactive.

> the environment is different (client side vs server side,

ECPG is client side. I think that the side does not matter.

> declared vs undeclared target variables),

Sure, the "gset" hack is only possible for a language without variable
declarations... but that does not make it a good idea.

> and the syntax is different (backslash or not, commas or not, just for
> starters).

Sure, different languages do not have the same syntax.

--
Fabien.


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Fabien COELHO-3
In reply to this post by Tom Lane-2

Hello again,

> I'd be okay with
>
> SELECT 1, 2 \into one two
> SELECT 3 \into three

Here is a v2 with more or less this approach, although \into does not end
the query, but applies to the current or last sql command. A query is
still terminated with a ";".

Now it handles things like :

   -- standard sql command
   SELECT balance FROM bank WHERE id=1;
   \into balance

   -- compound sql command, three == 3.
   SELECT 1, 2 \; SELECT 3 ;
   \into three

   -- compound query with 2 selects & 3 variables
   SELECT i \into one
     FROM generate_series(1, 1) AS i \;
   SELECT i+1, i+2 \into two three
     FROM generate_series(1, 1) AS i ;


I had to add a few lines in psql scanner to count "\;", so the parsing
logic is a little more complicated than before.

--
Fabien.

--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

pgbench-into-2.patch (22K) Download Attachment
into.sql (410 bytes) Download Attachment
into2.sql (1014 bytes) Download Attachment
into-err-1.sql (46 bytes) Download Attachment
into-err-2.sql (68 bytes) Download Attachment
into-err-3.sql (108 bytes) Download Attachment
into-err-4.sql (58 bytes) Download Attachment
into-err-5.sql (104 bytes) Download Attachment
into-err-6.sql (124 bytes) Download Attachment
into-err-7.sql (68 bytes) Download Attachment
into-err-8.sql (80 bytes) Download Attachment
into-err-9.sql (54 bytes) Download Attachment
into-err-10.sql (50 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Amit Langote-2

Hi Fabien,

On 2016/07/16 1:33, Fabien COELHO wrote:
> Here is a v2 with more or less this approach, although \into does not end
> the query, but applies to the current or last sql command. A query is
> still terminated with a ";".

This patch needs to be rebased because of commit 64710452 (on 2016-08-19).

Thanks,
Amit




--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Fabien COELHO-3

Hello Amit,

> This patch needs to be rebased because of commit 64710452 (on 2016-08-19).

Here it is!

--
Fabien.

--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

pgbench-into-3.patch (22K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Amit Langote-2

Hi Fabien,

On 2016/09/03 2:47, Fabien COELHO wrote:
>> This patch needs to be rebased because of commit 64710452 (on 2016-08-19).
>
> Here it is!

Thanks for sending the updated patch. Here are some (mostly cosmetic)
comments.  Before the comments, let me confirm whether the following
result is odd (a bug) or whether I am just using it wrong:

Custom script looks like:

\;
select a \into a
    from tab where a = 1;
\set i debug(:a)

I get the following error:

undefined variable "a"
client 0 aborted in state 1; execution of meta-command failed

Even the following script gives the same result:

\;
select a from a where a = 1;
\into a
\set t debug(:a)

However with the following there is no error and a gets set to 2:

select a from a where a = 1
\;
select a+1 from a where a = 1;
\into a
\set t debug(:a)


Comments on the patch follow:

+    <listitem>
+     <para>
+      Stores the first fields of the resulting row from the current or
preceding
+      <command>SELECT</> command into these variables.

Any command returning rows ought to work, no?  For example, the following
works:

update a set a = a+1 returning *;
\into a
\set t debug(:a)


-    char       *line;           /* text of command line */
+    char       *line;           /* first line for short display */
+    char       *lines;          /* full multi-line text of command */

When I looked at this and related hunks (and also the hunks related to
semicolons), it made me wonder whether this patch contains two logical
changes.  Is this a just a refactoring for the \into implementation or
does this provide some new externally visible feature?


     char       *argv[MAX_ARGS]; /* command word list */
+    int         compound;      /* last compound command (number of \;) */
+    char    ***intos;          /* per-compound command \into variables */

Need an extra space for intos to align with earlier fields.  Also I wonder
if intonames or intoargs would be a slightly better name for the field.


+static bool
+read_response(CState *st, char ** intos[])

Usual style seems to be to use ***intos here.


+                fprintf(stderr,
+                        "client %d state %d compound %d: "
+                        "cannot apply \\into to non SELECT statement\n",
+                        st->id, st->state, compound);

How about make this error message say something like other messages
related to \into, perhaps something like: "\\into cannot follow non SELECT
statements\n"


             /*
              * Read and discard the query result; note this is not
included in
-             * the statement latency numbers.
+             * the statement latency numbers (above), thus if reading the
+             * response fails the transaction is counted nevertheless.
              */

Does this comment need to mention that the result is not discarded when
\into is specified?


+    my_command->intos = pg_malloc0(sizeof(char**) * (compounds+1));

Need space: s/char**/char **/g

This comments applies also to a couple of nearby places.


-        my_command->line = pg_malloc(nlpos - p + 1);
+        my_command->line = pg_malloc(nlpos - p  + 1 + 3);

A comment mentioning what the above means would be helpful.


+    bool        sql_command_in_progress = false;

This variable's name could be slightly confusing to readers. If I
understand its purpose correctly, perhaps it could be called
sql_command_continues.


+                    if (index == 0)
+                        syntax_error(desc, lineno, NULL, NULL,
+                                     "\\into cannot start a script",
+                                     NULL, -1);

How about: "\\into cannot be at the beginning of a script" ?

Thanks,
Amit




--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - allow to store select results into variables

Fabien COELHO-3

Hello Amit,

> Custom script looks like:
>
> \;
> select a \into a
>    from tab where a = 1;
> \set i debug(:a)
>
> I get the following error:
>
> undefined variable "a"
> client 0 aborted in state 1; execution of meta-command failed
Good catch!

Indeed, there is a problem with empty commands which are simply ignored by
libpq/postgres if there are other commands around, so my synchronization
between results & commands was too naive.

In order to fix this, I made the scanner also count empty commands and
ignore comments. I guessed that proposing to change libpq/postgres
behavior was not an option.

> Comments on the patch follow:
>
> +    <listitem>
> +     <para>
> +      Stores the first fields of the resulting row from the current or
> preceding
> +      <command>SELECT</> command into these variables.
>
> Any command returning rows ought to work, no?

Yes. I put "SQL command" instead.

>
> -    char       *line;           /* text of command line */
> +    char       *line;           /* first line for short display */
> +    char       *lines;          /* full multi-line text of command */
>
> When I looked at this and related hunks (and also the hunks related to
> semicolons), it made me wonder whether this patch contains two logical
> changes.  Is this a just a refactoring for the \into implementation or
> does this provide some new externally visible feature?

There is essentially a refactoring that I did when updating how Command is
managed because it has to be done in several stages to fit "into" into it
and to take care of compounds.

However there was small "new externally visible feature": on -r, instead
of cutting abruptly a multiline command at the end of the first line it
appended "..." as an ellipsis because it looked nicer.

I've removed this small visible changed.

>     char       *argv[MAX_ARGS]; /* command word list */
> +    int         compound;      /* last compound command (number of \;) */
> +    char    ***intos;          /* per-compound command \into variables */

> Need an extra space for intos to align with earlier fields.

Ok.

> Also I wonder if intonames or intoargs would be a slightly better name
> for the field.

I put "intovars" as they are variable names.

> +static bool
> +read_response(CState *st, char ** intos[])
>
> Usual style seems to be to use ***intos here.

Ok.

> +                fprintf(stderr,
> +                        "client %d state %d compound %d: "
> +                        "cannot apply \\into to non SELECT statement\n",
> +                        st->id, st->state, compound);
>
> How about make this error message say something like other messages
> related to \into, perhaps something like: "\\into cannot follow non SELECT
> statements\n"

As you pointed out above, there may be statements without "SELECT" which
which return a row. I wrote "\\into expects a row" instead.

>             /*
>              * Read and discard the query result; note this is not included in
> -             * the statement latency numbers.
> +             * the statement latency numbers (above), thus if reading the
> +             * response fails the transaction is counted nevertheless.
>              */
>
> Does this comment need to mention that the result is not discarded when
> \into is specified?

Hmmm. The result structure is discarded, but the results are copied into
variables before that, so the comments seems ok...

> +    my_command->intos = pg_malloc0(sizeof(char**) * (compounds+1));
>
> Need space: s/char**/char **/g

Ok.

> This comments applies also to a couple of nearby places.

Indeed.

> -        my_command->line = pg_malloc(nlpos - p + 1);
> +        my_command->line = pg_malloc(nlpos - p  + 1 + 3);
>
> A comment mentioning what the above means would be helpful.

Ok. I removed the "+ 3" anyway.

> +    bool        sql_command_in_progress = false;
>
> This variable's name could be slightly confusing to readers. If I
> understand its purpose correctly, perhaps it could be called
> sql_command_continues.

It is possible. I like 'in progress' though. Why is it confusing?

It means that the current command is not finished yet and more is
expected, that is the final ';' has not been encountered.

> +                    if (index == 0)
> +                        syntax_error(desc, lineno, NULL, NULL,
> +                                     "\\into cannot start a script",
> +                                     NULL, -1);
>
> How about: "\\into cannot be at the beginning of a script" ?

It is possible, but it's quite longer... I'm not a native speaker, so I'm
do not know whether it would be better.

The attached patch takes into all your comments but:
  - comment about discarded results...
  - the sql_command_in_progress variable name change
  - the longer message on into at the start of a script

--
Fabien.

--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

pgbench-into-4.patch (25K) Download Attachment
into-8.sql (266 bytes) Download Attachment
1234 ... 6