Thread: pgbench - add \aset to store results of a combined query

Hello devs,

A long time ago I submitted a pgbench \into command to store results of 
queries into variables independently of the query being processed, which 
got turn into \gset (;) and \cset (\;), which got committed, then \cset 
was removed because it was not "up to standard", as it could not work with 
empty query (the underlying issue is that pg silently skips empty queries, 
so that "\; SELECT 1 \; \; SELECT 3," returns 2 results instead of 4, a 
misplaced optimisation from my point of view).

Now there is a pgbench \gset which allows to extract the results of 
variables of the last query, but as it does both setting and ending a 
query at the same time, there is no way to set variables out of a combined 
(\;) query but the last, which is the kind of non orthogonal behavior that 
I dislike much.

This annoys me because testing the performance of combined queries cannot 
be tested if the script needs to extract variables.

To make the feature somehow accessible to combined queries, the attached 
patch adds the "\aset" (all set) command to store all results of queries 
which return just one row into variables, i.e.:

   SELECT 1 AS one \;
   SELECT 2 AS two UNION SELECT 2 \;
   SELECT 3 AS three \aset

will set both "one" and "three", while "two" is not set because there were 
two rows. It is a kind of more permissive \gset.

Because it does it for all queries, there is no need for synchronizing 
with the underlying queries, which made the code for \cset both awkward 
and with limitations. Hopefully this version might be "up to standard".
I'll see. I'm in no hurry:-)

-- 
Fabien.

V2 is a rebase.

> A long time ago I submitted a pgbench \into command to store results of 
> queries into variables independently of the query being processed, which got 
> turn into \gset (;) and \cset (\;), which got committed, then \cset was 
> removed because it was not "up to standard", as it could not work with empty 
> query (the underlying issue is that pg silently skips empty queries, so that 
> "\; SELECT 1 \; \; SELECT 3," returns 2 results instead of 4, a misplaced 
> optimisation from my point of view).
>
> Now there is a pgbench \gset which allows to extract the results of variables 
> of the last query, but as it does both setting and ending a query at the same 
> time, there is no way to set variables out of a combined (\;) query but the 
> last, which is the kind of non orthogonal behavior that I dislike much.
>
> This annoys me because testing the performance of combined queries cannot be 
> tested if the script needs to extract variables.
>
> To make the feature somehow accessible to combined queries, the attached 
> patch adds the "\aset" (all set) command to store all results of queries 
> which return just one row into variables, i.e.:
>
>  SELECT 1 AS one \;
>  SELECT 2 AS two UNION SELECT 2 \;
>  SELECT 3 AS three \aset
>
> will set both "one" and "three", while "two" is not set because there were 
> two rows. It is a kind of more permissive \gset.
>
> Because it does it for all queries, there is no need for synchronizing with 
> the underlying queries, which made the code for \cset both awkward and with 
> limitations. Hopefully this version might be "up to standard".
> I'll see. I'm in no hurry:-)
>
>

-- 
Fabien.

>  SELECT 1 AS one \;
>  SELECT 2 AS two UNION SELECT 2 \;
>  SELECT 3 AS three \aset
>
> will set both "one" and "three", while "two" is not set because there were 
> two rows. It is a kind of more permissive \gset.

Are you sure two is not set :)? 
SELECT 2 AS two UNION SELECT 2;   -- only returns one row.
but
SELECT 2 AS two UNION SELECT 10;  -- returns the two rows.


Is this the expected behavior with \aset? In my opinion throwing a valid error like "client 0 script 0 command 0 query
0:expected one row, got 2" make more sense.
 


 - With \gset 

SELECT 2 AS two UNION SELECT 10 \gset
INSERT INTO test VALUES(:two,0,0);

$ pgbench postgres -f pgbench_aset.sql -T 1 -j 1 -c 1 -s 10
starting vacuum...end.
client 0 script 0 command 0 query 0: expected one row, got 2
transaction type: pgbench_aset.sql
scaling factor: 10
query mode: simple
number of clients: 1
number of threads: 1
duration: 1 s
number of transactions actually processed: 0
Run was aborted; the above results are incomplete.


- With \aset

SELECT 2 AS two UNION SELECT 10 \aset
INSERT INTO test VALUES(:two,0,0);

vagrant@vagrant:~/percona/postgresql$ pgbench postgres -f pgbench_aset.sql -T 1 -j 1 -c 1 -s 10
starting vacuum...end.
client 0 script 0 aborted in command 1 query 0: ERROR:  syntax error at or near ":"
LINE 1: INSERT INTO test VALUES(:two,0,0);
                                ^
transaction type: pgbench_aset.sql
scaling factor: 10
query mode: simple
number of clients: 1
number of threads: 1
duration: 1 s
number of transactions actually processed: 0
Run was aborted; the above results are incomplete.

The new status of this patch is: Waiting on Author

Hello Ibrar,

>>  SELECT 1 AS one \;
>>  SELECT 2 AS two UNION SELECT 2 \;
>>  SELECT 3 AS three \aset
>>
>> will set both "one" and "three", while "two" is not set because there were
>> two rows. It is a kind of more permissive \gset.
>
> Are you sure two is not set :)?
>
> SELECT 2 AS two UNION SELECT 2;   -- only returns one row.
> but
> SELECT 2 AS two UNION SELECT 10;  -- returns the two rows.

Indeed, my intension was to show an example like the second.

> Is this the expected behavior with \aset?

> In my opinion throwing a valid error like "client 0 script 0 command 0 
> query 0: expected one row, got 2" make more sense.

Hmmm. My intention with \aset is really NOT to throw an error. With 
pgbench, the existence of the variable can be tested later to know whether 
it was assigned or not, eg:

   SELECT 1 AS x \;
   -- 2 rows, no assignment
   SELECT 'calvin' AS firstname UNION SELECT 'hobbes' \;
   SELECT 2 AS z \aset
   -- next test is false
   \if :{?firstname}
     ...
   \endif

The rational is that one may want to benefit from combined queries (\;)
which result in less communication thus has lower latency, but still be 
interested in extracting some results.

The question is what to do if the query returns 0 or >1 rows. If an error 
is raised, the construct cannot be used for testing whether there is one 
result or not, eg for a query returning 0 or 1 row, you could not write:

   \set id random(1, :number_of_users)
   SELECT firtname AS fn FROM user WHERE id = :id \aset
   \if :{?fn}
     -- the user exists, proceed with further queries
   \else
     -- no user, maybe it was removed, it is not an error
   \endif

Another option would to just assign the value so that
  - on 0 row no assignment is made, and it can be tested afterwards.
  - on >1 rows the last (first?) value is kept. I took last so to
    ensure that all results are received.

I think that having some permissive behavior allows to write some more 
interesting test scripts that use combined queries and extract values.

What do you think?

> - With \gset
>
> SELECT 2 AS two UNION SELECT 10 \gset
> INSERT INTO test VALUES(:two,0,0);
>
> client 0 script 0 command 0 query 0: expected one row, got 2
> Run was aborted; the above results are incomplete.

Yes, that is the intented behavior.

> - With \aset
>
> SELECT 2 AS two UNION SELECT 10 \aset
> INSERT INTO test VALUES(:two,0,0);
> [...]
> client 0 script 0 aborted in command 1 query 0: ERROR:  syntax error at or near ":"

Indeed, the user should test whether the variable was assigned before 
using it if the result is not warranted to return one row.

> The new status of this patch is: Waiting on Author

The attached patch implements the altered behavior described above.

-- 
Fabien.

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

The patch passed my review, I have not reviewed the documentation changes.

The new status of this patch is: Ready for Committer

On Thu, Aug 15, 2019 at 06:30:13PM +0000, Ibrar Ahmed wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            not tested
>
> The patch passed my review, I have not reviewed the documentation changes.
>
> The new status of this patch is: Ready for Committer

@@ -524,6 +526,7 @@ typedef struct Command
    int         argc;
    char       *argv[MAX_ARGS];
    char       *varprefix;
+   bool        aset;

It seems to me that there is no point to have the variable aset in
Command because this structure includes already MetaCommand, so the
information is duplicated.  And I would suggest to change
readCommandResponse() to use a MetaCommand in argument.  Perhaps I am
missing something?
--
Michael

Michaël,

> +   bool        aset;
>
> It seems to me that there is no point to have the variable aset in 
> Command because this structure includes already MetaCommand, so the 
> information is duplicated. [...] Perhaps I am missing something?

Yep. ISTM that you are missing that aset is not an independent meta 
command like most others but really changes the state of the previous SQL 
command, so that it needs to be stored into that with some additional 
fields. This is the same with "gset" which is tagged by a non-null 
"varprefix".

So I cannot remove the "aset" field.

> And I would suggest to change readCommandResponse() to use a MetaCommand 
> in argument.

MetaCommand is not enough: we need varprefix, and then distinguishing 
between aset and gset. Although this last point can be done with a 
MetaCommand, ISTM that a bool is_aset is clear and good enough. It is 
possible to switch if you insist on it, but I do not think it is 
desirable.

Attached v4 removes an unwanted rebased comment duplication and does minor 
changes while re-reading the code.

-- 
Fabien.

On 11/29/19 4:34 AM, Fabien COELHO wrote:
>>
>> It seems to me that there is no point to have the variable aset in 
>> Command because this structure includes already MetaCommand, so the 
>> information is duplicated. [...] Perhaps I am missing something?
> 
> Yep. ISTM that you are missing that aset is not an independent meta 
> command like most others but really changes the state of the previous 
> SQL command, so that it needs to be stored into that with some 
> additional fields. This is the same with "gset" which is tagged by a 
> non-null "varprefix".
> 
> So I cannot remove the "aset" field.
> 
>> And I would suggest to change readCommandResponse() to use a 
>> MetaCommand in argument.
> 
> MetaCommand is not enough: we need varprefix, and then distinguishing 
> between aset and gset. Although this last point can be done with a 
> MetaCommand, ISTM that a bool is_aset is clear and good enough. It is 
> possible to switch if you insist on it, but I do not think it is desirable.

Michael, do you agree with Fabien's comments?

> Attached v4 removes an unwanted rebased comment duplication and does 
> minor changes while re-reading the code.

This patch no longer applies: http://cfbot.cputube.org/patch_27_2091.log

CF entry has been updated to Waiting on Author.

Regards,
-- 
-David
david@pgmasters.net

On Tue, Mar 24, 2020 at 11:04:45AM -0400, David Steele wrote:
> On 11/29/19 4:34 AM, Fabien COELHO wrote:
>> MetaCommand is not enough: we need varprefix, and then distinguishing
>> between aset and gset. Although this last point can be done with a
>> MetaCommand, ISTM that a bool is_aset is clear and good enough. It is
>> possible to switch if you insist on it, but I do not think it is
>> desirable.
>
> Michael, do you agree with Fabien's comments?

Thanks for the reminder.  I am following up with Fabien's comments.
--
Michael

On Fri, Nov 29, 2019 at 10:34:05AM +0100, Fabien COELHO wrote:
>> It seems to me that there is no point to have the variable aset in
>> Command because this structure includes already MetaCommand, so the
>> information is duplicated. [...] Perhaps I am missing something?
>
> Yep. ISTM that you are missing that aset is not an independent meta command
> like most others but really changes the state of the previous SQL command,
> so that it needs to be stored into that with some additional fields. This is
> the same with "gset" which is tagged by a non-null "varprefix".
>
> So I cannot remove the "aset" field.

Still sounds strange to me to invent a new variable to this structure
if it is possible to track the exact same thing with an existing part
of a Command, or it would make sense to split Command into two
different structures with an extra structure used after the parsing
for clarity?

>> And I would suggest to change readCommandResponse() to use a MetaCommand
>> in argument.
>
> MetaCommand is not enough: we need varprefix, and then distinguishing
> between aset and gset. Although this last point can be done with a
> MetaCommand, ISTM that a bool is_aset is clear and good enough. It is
> possible to switch if you insist on it, but I do not think it is desirable.
>
> Attached v4 removes an unwanted rebased comment duplication and does minor
> changes while re-reading the code.

Well, it still looks cleaner to me to just assign the meta field
properly within ParseScript(), and you get the same result.  And it is
also possible to use "meta" to do more sanity checks around META_GSET
for some code paths.  So I'd actually find the addition of a new
argument using a meta command within readCommandResponse() cleaner.

- * varprefix   SQL commands terminated with \gset have this set
+ * varprefix   SQL commands terminated with \gset or \aset have this set
Nit from v4: varprefix can be used for \gset and \aset, and the
comment was not updated.

+                   /* coldly skip empty result under \aset */
+                   if (ntuples <= 0)
+                       break;
Shouldn't this check after \aset?  And it seems to me that this code
path is not taken, so a test would be nice.

-   } while (res);
+   } while (res != NULL);
Useless diff.

The conflicts came from the switch to the common logging of
src/common/.  That was no big deal to solve.
--
Michael

Bonjour Michaël,

> [...] Still sounds strange to me to invent a new variable to this 
> structure if it is possible to track the exact same thing with an 
> existing part of a Command, or it would make sense to split Command into 
> two different structures with an extra structure used after the parsing 
> for clarity?

Hmmm.

Your point is to store the gset/aset status into the meta field, even if 
the command type is SQL. This is not done for gset, which relies on the 
non-null prefix, and breaks the assumption that meta is set to something 
only when the command is a meta command. Why not. I updated the comment, 
so now meta is none/gset/aset when command type is sql, and I removed the 
aset field.

> Well, it still looks cleaner to me to just assign the meta field
> properly within ParseScript(), and you get the same result.  And it is
> also possible to use "meta" to do more sanity checks around META_GSET
> for some code paths.  So I'd actually find the addition of a new
> argument using a meta command within readCommandResponse() cleaner.

I tried to do that.

> - * varprefix   SQL commands terminated with \gset have this set
> + * varprefix   SQL commands terminated with \gset or \aset have this set

> Nit from v4: varprefix can be used for \gset and \aset, and the
> comment was not updated.

It is now updated.

> +                   /* coldly skip empty result under \aset */
> +                   if (ntuples <= 0)
> +                       break;
> Shouldn't this check after \aset?  And it seems to me that this code
> path is not taken, so a test would be nice.

Added (I think, if I understood what you suggested.).

> -   } while (res);
> +   } while (res != NULL);
> Useless diff.

Yep.

Attached an updated v5.

-- 
Fabien.

On Thu, Mar 26, 2020 at 10:35:03PM +0100, Fabien COELHO wrote:
> Your point is to store the gset/aset status into the meta field, even if the
> command type is SQL. This is not done for gset, which relies on the non-null
> prefix, and breaks the assumption that meta is set to something only when
> the command is a meta command. Why not. I updated the comment, so now meta
> is none/gset/aset when command type is sql, and I removed the aset field.

Yes, that's the point I was trying to make.  Thanks for sending a new
version.

> - * meta            The type of meta-command, or META_NONE if command is SQL
> + * meta            The type of meta-command, if command is SQL META_NONE,
> + *                META_GSET or META_ASET which dictate what to do with the
> + *                 SQL query result.

I did not quite get why you need to update this comment.  The same
concepts as before apply.

>> +                   /* coldly skip empty result under \aset */
>> +                   if (ntuples <= 0)
>> +                       break;
>> Shouldn't this check after \aset?  And it seems to me that this code
>> path is not taken, so a test would be nice.
>
> Added (I think, if I understood what you suggested.).

+                    /* coldly skip empty result under \aset */
+                    else if (meta == META_ASET && ntuples <= 0)
+                        break;
Yes, that's what I meant.  Now it happens that we don't have a
regression test to cover the path where we have no tuples.  Could it
be possible to add one?

+    Assert((meta == META_NONE && varprefix == NULL) ||
+           ((meta == META_GSET || meta == META_ASET) && varprefix != NULL));
+
Good addition.  That would blow up if meta is set to something else
than {META_NONE,META_GSET,META_ASET}, so anybody changing this code
path will need to question if he/she needs to do something here.

Except for the addition of a test case to skip empty results when
\aset is used, I think that we are pretty good here.
--
Michael

On Mon, Mar 30, 2020 at 03:30:58PM +0900, Michael Paquier wrote:
> Except for the addition of a test case to skip empty results when
> \aset is used, I think that we are pretty good here.

While hacking on the patch more by myself, I found that mixing tests
for \gset and \aset was rather messy.  A test for an empty result
leads also to a failure with the pgbench command as we want to make
sure that the variable does not exist in this case using debug().  So
let's split the tests in three parts:
- the set for \get is left alone.
- addition of a new set for the valid cases of \aset.
- addition of an invalid test for \aset (the empty set one).

Fabien, what do you think about the attached?  Perhaps we should also
have a test where we return more than 1 row for \get?  The last point
is unrelated to this thread though.
--
Michael

Bonjour Michaël,

>> Except for the addition of a test case to skip empty results when
>> \aset is used, I think that we are pretty good here.
>
> While hacking on the patch more by myself, I found that mixing tests
> for \gset and \aset was rather messy.  A test for an empty result
> leads also to a failure with the pgbench command as we want to make
> sure that the variable does not exist in this case using debug().

ISTM that I submitted a patch to test whether a variable exists in 
pgbench, like available in psql (:{?var} I think), but AFAICS it did not 
pass. Maybe I should resurect it as it would allow to test simply whether 
an empty result was returned to aset, which could make sense in a bench 
script (get something, if it does not exist skip remainder… I can see some 
interesting use cases).

> So let's split the tests in three parts:
> - the set for \get is left alone.
> - addition of a new set for the valid cases of \aset.
> - addition of an invalid test for \aset (the empty set one).

Ok.

> Fabien, what do you think about the attached?

It does not need to create an UNLOGGED table, a mere "WHERE FALSE" 
suffices.

I do not understand why you removed the comment about meta which makes it 
false, so I added something minimal back.

> Perhaps we should also have a test where we return more than 1 row for 
> \get?  The last point is unrelated to this thread though.

Yes, but ISTM that it is not worth a dedicated patch… so I added a test 
similar to the one about empty aset.

See v7 attached.

-- 
Fabien.

On Thu, Apr 02, 2020 at 08:08:08AM +0200, Fabien COELHO wrote:
> ISTM that I submitted a patch to test whether a variable exists in pgbench,
> like available in psql (:{?var} I think), but AFAICS it did not pass. Maybe
> I should resurect it as it would allow to test simply whether an empty
> result was returned to aset, which could make sense in a bench script (get
> something, if it does not exist skip remainder… I can see some interesting
> use cases).

Not sure if improving the readability of the tests is a reason for
this patch.  So I would suggest to just live with relying on debug()
for now to check that a variable with a given prefix exists.

> It does not need to create an UNLOGGED table, a mere "WHERE FALSE" suffices.

Good point, that's cheaper.

> I do not understand why you removed the comment about meta which makes it
> false, so I added something minimal back.

  * type            SQL_COMMAND or META_COMMAND
- * meta            The type of meta-command, or META_NONE if command is SQL
+ * meta            The type of meta-command. On SQL_COMMAND: META_NONE/GSET/ASET.

Oh, OK.  I see your point.  Sorry about that.

>> Perhaps we should also have a test where we return more than 1 row for
>> \get?  The last point is unrelated to this thread though.
>
> Yes, but ISTM that it is not worth a dedicated patch… so I added a test
> similar to the one about empty aset.

Thanks.  So, it looks like everything has been addressed.  Do you have
anything else in mind?

NB: I think that it is really strange to not use an array for the
options in subroutine pgbench() of 001_pgbench_with_server.pl.
Shouldn't this be an array of options instead?  The current logic of
using a splitted string is weak when it comes to option quoting in
perl and command handling in general.
--
Michael

Michaël,

>> ISTM that I submitted a patch to test whether a variable exists in pgbench,
>> like available in psql (:{?var} I think),
>
> Not sure if improving the readability of the tests is a reason for
> this patch.  So I would suggest to just live with relying on debug()
> for now to check that a variable with a given prefix exists.

Sure. I meant that the feature would make sense to write benchmark scripts 
which would use aset and be able to act on the success or not of this 
aset, not to resurrect it for a hidden coverage test.

> Thanks.  So, it looks like everything has been addressed.  Do you have
> anything else in mind?

Nope.

> NB: I think that it is really strange to not use an array for the
> options in subroutine pgbench() of 001_pgbench_with_server.pl.
> Shouldn't this be an array of options instead?  The current logic of
> using a splitted string is weak when it comes to option quoting in
> perl and command handling in general.

The idea is that a scalar is simpler and readable to write in the simple 
case than a perl array. Now maybe qw() could have done the trick.

-- 
Fabien.

On Thu, Apr 02, 2020 at 03:58:50PM +0200, Fabien COELHO wrote:
> Sure. I meant that the feature would make sense to write benchmark scripts
> which would use aset and be able to act on the success or not of this aset,
> not to resurrect it for a hidden coverage test.

This could always be discussed for v14.  We'll see.

>> Thanks.  So, it looks like everything has been addressed.  Do you have
>> anything else in mind?
>
> Nope.

Applied, then.  Thanks!
--
Michael

Bonjour Michaël,

>> Sure. I meant that the feature would make sense to write benchmark scripts
>> which would use aset and be able to act on the success or not of this aset,
>> not to resurrect it for a hidden coverage test.
>
> This could always be discussed for v14.  We'll see.

Or v15, or never, who knows? :-)

The use case I have in mind for such a feature is to be able to have a 
flow of DELETE transactions in a multi-script benchmark without breaking 
concurrent SELECT/UPDATE transactions. For that, the ability of extracting 
data easily and testing whether it was non empty would help.

> Applied, then.  Thanks!

Thanks to you!

-- 
Fabien.