pgbench - extend initialization phase control

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|

pgbench - extend initialization phase control

Fabien COELHO-3

Hello devs,

the attached patch adds some more control on the initialization phase.
In particular, ( and ) allow to begin/commit explicitely, and G generates
the data server-side instead of client side, which might be a good idea
depending on the available bandwidth.

Together with the previously submitted patch about getting stats on the
initialization phase, the idea is to possibly improve this phase, or use
it as a benchmark tool in itself.

--
Fabien.

pgbench-init-extended-1.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Ibrar Ahmed-4
Does both client/server side data generation in a single command make sense?
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fabien COELHO-3

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.


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Ibrar Ahmed-4
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
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fabien COELHO-3

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.

pgbench-init-extended-2.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

btendouan
Hello Fabien,

> ---------- Forwarded message ---------
> From: Fabien COELHO <[hidden email]>
> Date: Tue, Jul 16, 2019 at 4:58 PM
> Subject: Re: pgbench - extend initialization phase control
> To: Ibrar Ahmed <[hidden email]>
> Cc: PostgreSQL Developers <[hidden email]>
>
>
>
> 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


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fabien COELHO-3

>> Attached v2 is a rebase after ce8f9467.

Here is rebase v3.

--
Fabien.

pgbench-init-extended-3.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

btendouan

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


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

btendouan
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


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fabien COELHO-3
In reply to this post by btendouan

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.

pgbench-init-extended-4.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

btendouan

> 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


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fujii Masao-2
In reply to this post by Fabien COELHO-3
On Thu, Oct 17, 2019 at 8:09 PM Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fabien COELHO-3

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.

pgbench-init-extended-5.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fujii Masao-2
On Thu, Oct 24, 2019 at 9:16 PM Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fabien COELHO-3

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.


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fujii Masao-2
On Fri, Oct 25, 2019 at 12:06 AM Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fabien COELHO-3

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.

pgbench-init-extended-6.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fujii Masao-2
On Mon, Oct 28, 2019 at 10:36 PM Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fabien COELHO-3

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.

pgbench-init-extended-7.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - extend initialization phase control

Fabien COELHO-4

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:

   https://www.postgresql.org/message-id/flat/alpine.DEB.2.21.1910311939430.27369@lancre
   https://commitfest.postgresql.org/25/2336/

--
Fabien.


12