pgbench - extend initialization phase control

Does both client/server side data generation in a single command make sense?

Hello Ibrar,

Does both client/server side data generation in a single command make
sense?

I think yes, especially with the other patch which adds timing measures to
the initialization phases. It really depends what you want to test.

With client-side generation you test the libpq COPY interface and network
protocol for bulk loading.

With server-side generation you are get the final result faster when
network bandwidth is low, and somehow you are testing a different kind of
small query which generates a lot of data.

--
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

Other than that, the patch looks good to me.

The new status of this patch is: Ready for Committer

Hello Ibrar,

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

Other than that, the patch looks good to me.

The new status of this patch is: Ready for Committer

Thanks for the review.

Attached v2 is a rebase after ce8f9467.

--
Fabien.

Hello Fabien,

---------- Forwarded message ---------
From: Fabien COELHO <coelho@cri.ensmp.fr>
Date: Tue, Jul 16, 2019 at 4:58 PM
Subject: Re: pgbench - extend initialization phase control
To: Ibrar Ahmed <ibrar.ahmad@gmail.com>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>

Hello Ibrar,

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

Other than that, the patch looks good to me.

The new status of this patch is: Ready for Committer

Thanks for the review.

Attached v2 is a rebase after ce8f9467.

Thanks for your new patch.

But I failed to apply it. Please rebase it against HEAD.

Regards,

---------
Anna

Attached v2 is a rebase after ce8f9467.

Here is rebase v3.

--
Fabien.

Here is rebase v3.

Hi,

Thanks for your new patch.

Failed regression test.
It's necessary to change the first a in “allowed step characters are” to
uppercase A in the regression test of 002_pgbench_no_server.pl.

The behavior of "g" is different between v12 and the patche, and
backward compatibility is lost.
In v12, BEGIN and COMMIT are specified only by choosing "g".
It's a problem that backward compatibility is lost.

When using ( and ) with the -I, the documentation should indicate that
double quotes are required,
and "v" not be able to enclose in ( and ).

Regards,

--
Anna

Hi,

When g is specified, null is inserted in the filler column of
pgbentch_tellrs, acounts, branches.
But when G is specified, empty string is inserted.

Do you have any intention of this difference?

--
Anna

Hello,

Failed regression test. It's necessary to change the first a in “allowed
step characters are” to uppercase A in the regression test of
002_pgbench_no_server.pl.

Argh. I think I ran the test, then stupidly updated the message afterwards
to better match best practices, without rechecking:-(

The behavior of "g" is different between v12 and the patche, and
backward compatibility is lost. In v12, BEGIN and COMMIT are specified
only by choosing "g". It's a problem that backward compatibility is
lost.

Somehow yes, but I do not see this as an actual problem from a functional
point of view: it just means that if you use a 'dtgvp' with the newer
version and if the inserts were to fail, then they are not under an
explicit transaction, so previous inserts are not cleaned up. However,
this is a pretty unlikely case, and anyway the error is reported, so any
user would be expected not to go on after the initialization phase.

So basically I do not see the very small regression for an unlikely corner
case to induce any problem in practice.

The benefit of controlling where begin/end actually occur is that it may
have an impact on performance, and it allows to check that.

When using ( and ) with the -I, the documentation should indicate that double
quotes are required,

Or single quotes, or backslash, if launch from the command line. I added a
mention of escaping or protection in the doc in that case.

and "v" not be able to enclose in ( and ).

That is a postgresql limitation, which may evolve. There could be others.
I updated the doc to say that some commands may not work inside an
explicit transaction.

When g is specified, null is inserted in the filler column of
pgbentch_tellrs, acounts, branches. But when G is specified, empty
string is inserted.

Indeed there is a small diff. ISTM that the actual filling with the
initial client version is NULL for branches and tellers, and a
blank-padded string for accounts.

I fixed the patch so that the end-result is the same with both g and G.

Do you have any intention of this difference?

Yes and no.

I intended that tellers & branches filler are filled, but I did not really
notice that the client side was implicitely using NULL, although it says
so in a comment. Although I'm not happy with the fact because it cheats
with the benchmark design which requires the filler columns to be really
filled and stored as is, it is indeed the place to change this (bad)
behavior.

Attached a v4 with the updates described above.

--
Fabien.

Attached a v4 with the updates described above.

Hi,

Thanks for updating the patch.
All tests are passed. There is no problem in operation.

--
Anna

On Thu, Oct 17, 2019 at 8:09 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Hello,

Failed regression test. It's necessary to change the first a in “allowed
step characters are” to uppercase A in the regression test of
002_pgbench_no_server.pl.

Argh. I think I ran the test, then stupidly updated the message afterwards
to better match best practices, without rechecking:-(

The behavior of "g" is different between v12 and the patche, and
backward compatibility is lost. In v12, BEGIN and COMMIT are specified
only by choosing "g". It's a problem that backward compatibility is
lost.

Somehow yes, but I do not see this as an actual problem from a functional
point of view: it just means that if you use a 'dtgvp' with the newer
version and if the inserts were to fail, then they are not under an
explicit transaction, so previous inserts are not cleaned up. However,
this is a pretty unlikely case, and anyway the error is reported, so any
user would be expected not to go on after the initialization phase.

So basically I do not see the very small regression for an unlikely corner
case to induce any problem in practice.

The benefit of controlling where begin/end actually occur is that it may
have an impact on performance, and it allows to check that.

I still fail to understand the benefit of addition of () settings.
Could you clarify what case () settings are useful for? You are
thinking to execute all initialization SQL statements within
single transaction, e.g., -I (dtgp), for some reasons?

When using ( and ) with the -I, the documentation should indicate that double
quotes are required,

Or single quotes, or backslash, if launch from the command line. I added a
mention of escaping or protection in the doc in that case.

What about using, for example, b (BEGIN) and c (COMMIT) instead
to avoid such restriction?

and "v" not be able to enclose in ( and ).

That is a postgresql limitation, which may evolve. There could be others.
I updated the doc to say that some commands may not work inside an
explicit transaction.

I think that it's better to check whehter "v" is enclosed with () or not
at the beginning of pgbench, and report an error if it is. Otherwise,
if -I (dtgv) is specified, pgbench reports an error after time-consuming
data generation is performed, and of course that data generation is
rollbacked.

Regards,

--
Fujii Masao

Hello Masao-san,

The benefit of controlling where begin/end actually occur is that it may
have an impact on performance, and it allows to check that.

I still fail to understand the benefit of addition of () settings.
Could you clarify what case () settings are useful for? You are
thinking to execute all initialization SQL statements within
single transaction, e.g., -I (dtgp), for some reasons?

Yep. Or anything else, including without (), to allow checking the
performance impact or non impact of transactions on the initialization
phase.

When using ( and ) with the -I, the documentation should indicate that double
quotes are required,

Or single quotes, or backslash, if launch from the command line. I added a
mention of escaping or protection in the doc in that case.

What about using, for example, b (BEGIN) and c (COMMIT) instead
to avoid such restriction?

It is indeed possible. Using a open/close symmetric character ( (), {},
[]) looks more pleasing and allows to see easily whether everything is
properly closed. I switched to {} which does not generate the same quoting
issue in shell.

I think that it's better to check whehter "v" is enclosed with () or not
at the beginning of pgbench, and report an error if it is.

Otherwise, if -I (dtgv) is specified, pgbench reports an error after
time-consuming data generation is performed, and of course that data
generation is rollbacked.

Patch v5 attached added a check for v inside (), although I'm not keen on
putting it there, and uses {} instead of ().

--
Fabien.

On Thu, Oct 24, 2019 at 9:16 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Hello Masao-san,

The benefit of controlling where begin/end actually occur is that it may
have an impact on performance, and it allows to check that.

I still fail to understand the benefit of addition of () settings.
Could you clarify what case () settings are useful for? You are
thinking to execute all initialization SQL statements within
single transaction, e.g., -I (dtgp), for some reasons?

Yep. Or anything else, including without (), to allow checking the
performance impact or non impact of transactions on the initialization
phase.

Is there actually such performance impact? AFAIR most time-consuming part in
initialization phase is the generation of pgbench_accounts data. This part is
performed within single transaction whether () are specified or not. No?
So I'm not sure how () are useful to check performance impact in init phase.
Maybe I'm missing something...

When using ( and ) with the -I, the documentation should indicate that double
quotes are required,

Or single quotes, or backslash, if launch from the command line. I added a
mention of escaping or protection in the doc in that case.

What about using, for example, b (BEGIN) and c (COMMIT) instead
to avoid such restriction?

It is indeed possible. Using a open/close symmetric character ( (), {},
[]) looks more pleasing and allows to see easily whether everything is
properly closed. I switched to {} which does not generate the same quoting
issue in shell.

I think that it's better to check whehter "v" is enclosed with () or not
at the beginning of pgbench, and report an error if it is.

Otherwise, if -I (dtgv) is specified, pgbench reports an error after
time-consuming data generation is performed, and of course that data
generation is rollbacked.

Patch v5 attached added a check for v inside (), although I'm not keen on
putting it there, and uses {} instead of ().

Thanks for updating the patch!

Regards,

--
Fujii Masao

Hello,

Yep. Or anything else, including without (), to allow checking the
performance impact or non impact of transactions on the initialization
phase.

Is there actually such performance impact? AFAIR most time-consuming part in
initialization phase is the generation of pgbench_accounts data.

Maybe. If you cannot check, you can only guess. Probably it should be
small, but the current version does not allow to check whether it is so.

--
Fabien.

On Fri, Oct 25, 2019 at 12:06 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Hello,

Yep. Or anything else, including without (), to allow checking the
performance impact or non impact of transactions on the initialization
phase.

Is there actually such performance impact? AFAIR most time-consuming part in
initialization phase is the generation of pgbench_accounts data.

Maybe. If you cannot check, you can only guess. Probably it should be
small, but the current version does not allow to check whether it is so.

Could you elaborate what you actually want to measure the performance
impact by adding explicit begin and commit? Currently pgbench -i issues
the following queries. The data generation part is already executed within
single transaction. You want to execute not only data generation but also
drop/creation of tables within single transaction, and measure how much
performance impact happens? I'm sure that would be negligible.
Or you want to execute data generate in multiple transactions, i.e.,
execute each statement for data generation (e.g., one INSERT) in single
transaction, and then want to measure the performance impact?
But the patch doesn't enable us to do such data generation yet.

So I'm thinking that it's maybe better to commit the addtion of "G" option
first separately. And then we can discuss how much "(" and ")" options
are useful later.

------------------------------------------
drop table if exists pgbench_accounts, pgbench_branches,
pgbench_history, pgbench_tellers
create table pgbench_history(tid int,bid int,aid int,delta
int,mtime timestamp,filler char(22))
create table pgbench_tellers(tid int not null,bid int,tbalance
int,filler char(84)) with (fillfactor=100)
create table pgbench_accounts(aid int not null,bid int,abalance
int,filler char(84)) with (fillfactor=100)
create table pgbench_branches(bid int not null,bbalance int,filler
char(88)) with (fillfactor=100)
begin
truncate table pgbench_accounts, pgbench_branches, pgbench_history,
pgbench_tellers
insert into pgbench_branches(bid,bbalance) values(1,0)
insert into pgbench_tellers(tid,bid,tbalance) values (1,1,0)
insert into pgbench_tellers(tid,bid,tbalance) values (2,1,0)
insert into pgbench_tellers(tid,bid,tbalance) values (3,1,0)
insert into pgbench_tellers(tid,bid,tbalance) values (4,1,0)
insert into pgbench_tellers(tid,bid,tbalance) values (5,1,0)
insert into pgbench_tellers(tid,bid,tbalance) values (6,1,0)
insert into pgbench_tellers(tid,bid,tbalance) values (7,1,0)
insert into pgbench_tellers(tid,bid,tbalance) values (8,1,0)
insert into pgbench_tellers(tid,bid,tbalance) values (9,1,0)
insert into pgbench_tellers(tid,bid,tbalance) values (10,1,0)
copy pgbench_accounts from stdin
commit
vacuum analyze pgbench_branches
vacuum analyze pgbench_tellers
vacuum analyze pgbench_accounts
vacuum analyze pgbench_history
alter table pgbench_branches add primary key (bid)
alter table pgbench_tellers add primary key (tid)
alter table pgbench_accounts add primary key (aid)
------------------------------------------

Regards,

--
Fujii Masao

Hello Masao-san,

Maybe. If you cannot check, you can only guess. Probably it should be
small, but the current version does not allow to check whether it is so.

Could you elaborate what you actually want to measure the performance
impact by adding explicit begin and commit? Currently pgbench -i issues
the following queries. The data generation part is already executed within
single transaction. You want to execute not only data generation but also
drop/creation of tables within single transaction, and measure how much
performance impact happens? I'm sure that would be negligible.
Or you want to execute data generate in multiple transactions, i.e.,
execute each statement for data generation (e.g., one INSERT) in single
transaction, and then want to measure the performance impact?
But the patch doesn't enable us to do such data generation yet.

Indeed, you cannot do this precise thing, but you can do others.

So I'm thinking that it's maybe better to commit the addtion of "G" option
first separately. And then we can discuss how much "(" and ")" options
are useful later.

Attached patch v6 only provides G - server side data generation.

--
Fabien.

On Mon, Oct 28, 2019 at 10:36 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Hello Masao-san,

Maybe. If you cannot check, you can only guess. Probably it should be
small, but the current version does not allow to check whether it is so.

Could you elaborate what you actually want to measure the performance
impact by adding explicit begin and commit? Currently pgbench -i issues
the following queries. The data generation part is already executed within
single transaction. You want to execute not only data generation but also
drop/creation of tables within single transaction, and measure how much
performance impact happens? I'm sure that would be negligible.
Or you want to execute data generate in multiple transactions, i.e.,
execute each statement for data generation (e.g., one INSERT) in single
transaction, and then want to measure the performance impact?
But the patch doesn't enable us to do such data generation yet.

Indeed, you cannot do this precise thing, but you can do others.

So I'm thinking that it's maybe better to commit the addtion of "G" option
first separately. And then we can discuss how much "(" and ")" options
are useful later.

Attached patch v6 only provides G - server side data generation.

Thanks for the patch!

+ snprintf(sql, sizeof(sql),
+ "insert into pgbench_branches(bid,bbalance) "
+ "select bid, 0 "
+ "from generate_series(1, %d) as bid", scale);

"scale" should be "nbranches * scale".

+ snprintf(sql, sizeof(sql),
+ "insert into pgbench_accounts(aid,bid,abalance,filler) "
+ "select aid, (aid - 1) / %d + 1, 0, '' "
+ "from generate_series(1, %d) as aid", naccounts, scale * naccounts);

Like client-side data generation, INT64_FORMAT should be used here
instead of %d?

If large scale factor is specified, the query for generating pgbench_accounts
data can take a very long time. While that query is running, operators may be
likely to do Ctrl-C to cancel the data generation. In this case, IMO pgbench
should cancel the query, i.e., call PQcancel(). Otherwise, the query will keep
running to the end.

- for (step = initialize_steps; *step != '\0'; step++)
+ for (const char *step = initialize_steps; *step != '\0'; step++)

Per PostgreSQL basic coding style, ISTM that "const char *step"
should be declared separately from "for" loop, like the original.

- fprintf(stderr, "unrecognized initialization step \"%c\"\n",
+ fprintf(stderr,
+ "unrecognized initialization step \"%c\"\n"
+ "Allowed step characters are: \"" ALL_INIT_STEPS "\".\n",
  *step);
- fprintf(stderr, "allowed steps are: \"d\", \"t\", \"g\", \"v\",
\"p\", \"f\"\n");

The original message seems better to me. So what about just appending "G"
into the above latter message? That is,
"allowed steps are: \"d\", \"t\", \"g\", \"G\", \"v\", \"p\", \"f\"\n"

-          <term><literal>g</literal> (Generate data)</term>
+          <term><literal>g</literal> or <literal>G</literal>
(Generate data, client or server side)</term>

Isn't it better to explain a bit more what "client-side / server-side data
generation" is? For example, something like

When "g" (client-side data generation) is specified, data is generated
in pgbench client and sent to the server. When "G" (server-side data
generation) is specified, only queries are sent from pgbench client
and then data is generated in the server. If the network bandwidth is low
between pgbench and the server, using "G" might make the data
generation faster.

Regards,

--
Fujii Masao

Hello Masao-san,

+ snprintf(sql, sizeof(sql),
+ "insert into pgbench_branches(bid,bbalance) "
+ "select bid, 0 "
+ "from generate_series(1, %d) as bid", scale);

"scale" should be "nbranches * scale".

Yep, even if nbranches is 1, it should be there.

+ snprintf(sql, sizeof(sql),
+ "insert into pgbench_accounts(aid,bid,abalance,filler) "
+ "select aid, (aid - 1) / %d + 1, 0, '' "
+ "from generate_series(1, %d) as aid", naccounts, scale * naccounts);

Like client-side data generation, INT64_FORMAT should be used here
instead of %d?

Indeed.

If large scale factor is specified, the query for generating pgbench_accounts
data can take a very long time. While that query is running, operators may be
likely to do Ctrl-C to cancel the data generation. In this case, IMO pgbench
should cancel the query, i.e., call PQcancel(). Otherwise, the query will keep
running to the end.

Hmmm. Why not. Now the infra to do that seems to already exists twice,
once in "src/bin/psql/common.c" and once in "src/bin/scripts/common.c".

I cannot say I'm thrilled to replicate this once more. I think that the
reasonable option is to share this in fe-utils and then to reuse it from
there. However, ISTM that such a restructuring patch which not belong to
this feature.

- for (step = initialize_steps; *step != '\0'; step++)
+ for (const char *step = initialize_steps; *step != '\0'; step++)

Per PostgreSQL basic coding style,

C99 (20 years ago) is new the norm, and this style is now allowed, there
are over a hundred instances of these already. I tend to use that where
appropriate.

- fprintf(stderr, "unrecognized initialization step \"%c\"\n",
+ fprintf(stderr,
+ "unrecognized initialization step \"%c\"\n"
+ "Allowed step characters are: \"" ALL_INIT_STEPS "\".\n",
*step);
- fprintf(stderr, "allowed steps are: \"d\", \"t\", \"g\", \"v\",
\"p\", \"f\"\n");

The original message seems better to me. So what about just appending "G"
into the above latter message? That is,
"allowed steps are: \"d\", \"t\", \"g\", \"G\", \"v\", \"p\", \"f\"\n"

I needed this list in several places, so it makes sense to share the
definition, and frankly the list of half a dozen comma-separated chars
does not strike me as much better than just giving the allowed chars
directly. So the simpler the better, from my point of view.

Isn't it better to explain a bit more what "client-side / server-side data
generation" is? For example, something like

Ok.

Attached v7 does most of the above, but the list of char message and the
signal handling. The first one does not look really better to me, and the
second one belongs to a restructuring patch that I'll try to submit.

--
Fabien.

Hello Masao-san,

If large scale factor is specified, the query for generating
pgbench_accounts data can take a very long time. While that query is
running, operators may be likely to do Ctrl-C to cancel the data
generation. In this case, IMO pgbench should cancel the query, i.e.,
call PQcancel(). Otherwise, the query will keep running to the end.

Hmmm. Why not. Now the infra to do that seems to already exists twice, once
in "src/bin/psql/common.c" and once in "src/bin/scripts/common.c".

I cannot say I'm thrilled to replicate this once more. I think that the
reasonable option is to share this in fe-utils and then to reuse it from
there. However, ISTM that such a restructuring patch which not belong to this
feature. [...]

I just did a patch to share the code:

/messages/by-id/alpine.DEB.2.21.1910311939430.27369@lancre
https://commitfest.postgresql.org/25/2336/

--
Fabien.