pgbench - refactor init functions with buffers

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

pgbench - refactor init functions with buffers

Fabien COELHO-3

Hello,

While developing pgbench to allow partitioned tabled, I reproduced the
string management style used in the corresponding functions, but was
pretty unhappy with that kind of pattern:

  snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), ...)

However adding a feature is not the place for refactoring.

This patch refactors initialization functions so as to use PQExpBuffer
where appropriate to simplify and clarify the code. SQL commands are
generated by accumulating parts into a buffer in order, before executing
it. I also added a more generic function to execute a statement and fail
if the result is unexpected.

--
Fabien.

pgbench-buffer-1.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Dilip Kumar-2
On Tue, Oct 22, 2019 at 12:03 PM Fabien COELHO <[hidden email]> wrote:

>
>
> Hello,
>
> While developing pgbench to allow partitioned tabled, I reproduced the
> string management style used in the corresponding functions, but was
> pretty unhappy with that kind of pattern:
>
>         snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), ...)
>
> However adding a feature is not the place for refactoring.
>
> This patch refactors initialization functions so as to use PQExpBuffer
> where appropriate to simplify and clarify the code. SQL commands are
> generated by accumulating parts into a buffer in order, before executing
> it. I also added a more generic function to execute a statement and fail
> if the result is unexpected.
>

- for (i = 0; i < nbranches * scale; i++)
+ for (int i = 0; i < nbranches * scale; i++)
 ...
- for (i = 0; i < ntellers * scale; i++)
+ for (int i = 0; i < ntellers * scale; i++)
  {

I haven't read the complete patch.  But, I have noticed that many
places you changed the variable declaration from c to c++ style (i.e
moved the declaration in the for loop).  IMHO, generally in PG, we
don't follow this convention.  Is there any specific reason to do
this?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Jeevan Ladhe

I haven't read the complete patch.  But, I have noticed that many
places you changed the variable declaration from c to c++ style (i.e
moved the declaration in the for loop).  IMHO, generally in PG, we
don't follow this convention.  Is there any specific reason to do
this?

+1.

The patch does not apply on master, needs rebase.
Also, I got some whitespace errors.

I think you can also refactor the function tryExecuteStatement(), and
call your newly added function executeStatementExpect() by passing
an additional flag something like "errorOK".

Regards,
Jeevan Ladhe
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Fabien COELHO-3
In reply to this post by Dilip Kumar-2

Hello Dilip,

> - for (i = 0; i < nbranches * scale; i++)
> + for (int i = 0; i < nbranches * scale; i++)
> ...
> - for (i = 0; i < ntellers * scale; i++)
> + for (int i = 0; i < ntellers * scale; i++)
>  {
>
> I haven't read the complete patch.  But, I have noticed that many
> places you changed the variable declaration from c to c++ style (i.e
> moved the declaration in the for loop).  IMHO, generally in PG, we
> don't follow this convention.  Is there any specific reason to do
> this?

There are many places where it is used now in pg (120 occurrences in
master, 7 in pgbench). I had a bug recently because of a stupidly reused
index variable, so I tend to use this now it is admissible, moreover here
I'm actually doing a refactoring patch, so it seems ok to include that.

--
Fabien.


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Fabien COELHO-3
In reply to this post by Jeevan Ladhe

Hello Jeevan,

>> I haven't read the complete patch.  But, I have noticed that many
>> places you changed the variable declaration from c to c++ style (i.e
>> moved the declaration in the for loop).  IMHO, generally in PG, we
>> don't follow this convention.  Is there any specific reason to do
>> this?
>
> +1.

As I said, this C99 feature is already used extensively in pg sources, so
it makes sense to use it when refactoring something and if appropriate,
which IMO is the case here.

> The patch does not apply on master, needs rebase.

Hmmm. "git apply pgbench-buffer-1.patch" works for me on current master.

> Also, I got some whitespace errors.

It possible, but I cannot see any. Could you be more specific?

Many mailers do not conform to MIME and mess-up newlines when attachements
are typed text/*, because MIME requires the mailer to convert those to
crnl eol when sending and back to system eol when receiving, but few
actually do it. Maybe the issue is really there.

> I think you can also refactor the function tryExecuteStatement(), and
> call your newly added function executeStatementExpect() by passing
> an additional flag something like "errorOK".

Indeed, good point.

--
Fabien.

pgbench-buffer-2.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Dilip Kumar-2
In reply to this post by Fabien COELHO-3
On Tue, Oct 22, 2019 at 3:30 PM Fabien COELHO <[hidden email]> wrote:

>
>
> Hello Dilip,
>
> > - for (i = 0; i < nbranches * scale; i++)
> > + for (int i = 0; i < nbranches * scale; i++)
> > ...
> > - for (i = 0; i < ntellers * scale; i++)
> > + for (int i = 0; i < ntellers * scale; i++)
> >  {
> >
> > I haven't read the complete patch.  But, I have noticed that many
> > places you changed the variable declaration from c to c++ style (i.e
> > moved the declaration in the for loop).  IMHO, generally in PG, we
> > don't follow this convention.  Is there any specific reason to do
> > this?
>
> There are many places where it is used now in pg (120 occurrences in
> master, 7 in pgbench). I had a bug recently because of a stupidly reused
> index variable, so I tend to use this now it is admissible, moreover here
> I'm actually doing a refactoring patch, so it seems ok to include that.
>
I see.  I was under impression that we don't use this style in PG.
But, since we are already using this style other places so no
objection from my side for this particular point.
Sorry for the noise.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Jeevan Ladhe
In reply to this post by Fabien COELHO-3


On Tue, Oct 22, 2019 at 4:36 PM Fabien COELHO <[hidden email]> wrote:

Hello Jeevan,

>> I haven't read the complete patch.  But, I have noticed that many
>> places you changed the variable declaration from c to c++ style (i.e
>> moved the declaration in the for loop).  IMHO, generally in PG, we
>> don't follow this convention.  Is there any specific reason to do
>> this?
>
> +1.

As I said, this C99 feature is already used extensively in pg sources, so
it makes sense to use it when refactoring something and if appropriate,
which IMO is the case here.

Ok, no problem.
 
 
> The patch does not apply on master, needs rebase.

Hmmm. "git apply pgbench-buffer-1.patch" works for me on current master.

> Also, I got some whitespace errors.

It possible, but I cannot see any. Could you be more specific?

For me it failing, see below:

$ git log -1
commit ad4b7aeb84434c958e2df76fa69b68493a889e4a
Author: Peter Eisentraut <[hidden email]>
Date:   Tue Oct 22 10:35:54 2019 +0200

    Make command order in test more sensible
    
    Through several updates, the CREATE USER command has been separated
    from where the user is actually used in the test.

$ git apply pgbench-buffer-1.patch
pgbench-buffer-1.patch:10: trailing whitespace.
static void append_fillfactor(PQExpBuffer query);
pgbench-buffer-1.patch:18: trailing whitespace.
executeStatementExpect(PGconn *con, const char *sql, const ExecStatusType expected)
pgbench-buffer-1.patch:19: trailing whitespace.
{
pgbench-buffer-1.patch:20: trailing whitespace.
        PGresult   *res;
pgbench-buffer-1.patch:21: trailing whitespace.

error: patch failed: src/bin/pgbench/pgbench.c:599
error: src/bin/pgbench/pgbench.c: patch does not apply

$  

Regards,
Jeevan Ladhe
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Fabien COELHO-3

>> The patch does not apply on master, needs rebase.
>>
>> Hmmm. "git apply pgbench-buffer-1.patch" works for me on current master.
>>
>>> Also, I got some whitespace errors.
>>
>> It possible, but I cannot see any. Could you be more specific?
>
> For me it failing, see below:
>
> $ git log -1
> commit ad4b7aeb84434c958e2df76fa69b68493a889e4a

Same for me, but it works:

   Switched to a new branch 'test'
   sh> git apply ~/pgbench-buffer-2.patch
   sh> git st
    On branch test
    Changes not staged for commit: ...
         modified:   src/bin/pgbench/pgbench.c

   sh> file ~/pgbench-buffer-2.patch
   .../pgbench-buffer-2.patch: unified diff output, ASCII text

   sh> sha1sum ~/pgbench-buffer-2.patch
   eab8167ef3ec5eca814c44b30e07ee5631914f07 ...

I suspect that your mailer did or did not do something with the
attachment. Maybe try with "patch -p1 < foo.patch" at the root.

--
Fabien.


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Jeevan Ladhe
I am able to apply the v2 patch with "patch -p1 "

-----

+static void
+executeStatementExpect(PGconn *con, const char *sql, const ExecStatusType expected, bool errorOK)
+{

I think some instances like this need 80 column alignment?

-----

in initCreatePKeys():
+ for (int i = 0; i < lengthof(DDLINDEXes); i++)
+ {
+ resetPQExpBuffer(&query);
+ appendPQExpBufferStr(&query, DDLINDEXes[i]);
 
I think you can simply use printfPQExpBuffer() for the first append, similar to
what you have used in createPartitions(), which is a combination of both reset
and append.

-----

The pgbench tap tests are also running fine.

Regards,
Jeevan Ladhe

On Tue, Oct 22, 2019 at 8:57 PM Fabien COELHO <[hidden email]> wrote:

>> The patch does not apply on master, needs rebase.
>>
>> Hmmm. "git apply pgbench-buffer-1.patch" works for me on current master.
>>
>>> Also, I got some whitespace errors.
>>
>> It possible, but I cannot see any. Could you be more specific?
>
> For me it failing, see below:
>
> $ git log -1
> commit ad4b7aeb84434c958e2df76fa69b68493a889e4a

Same for me, but it works:

   Switched to a new branch 'test'
   sh> git apply ~/pgbench-buffer-2.patch
   sh> git st
    On branch test
    Changes not staged for commit: ...
         modified:   src/bin/pgbench/pgbench.c

   sh> file ~/pgbench-buffer-2.patch
   .../pgbench-buffer-2.patch: unified diff output, ASCII text

   sh> sha1sum ~/pgbench-buffer-2.patch
   eab8167ef3ec5eca814c44b30e07ee5631914f07 ...

I suspect that your mailer did or did not do something with the
attachment. Maybe try with "patch -p1 < foo.patch" at the root.

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Fabien COELHO-3

Hello Jeevan,

> +static void
> +executeStatementExpect(PGconn *con, const char *sql, const ExecStatusType
> expected, bool errorOK)
> +{
>
> I think some instances like this need 80 column alignment?

Yep. Applying the pgindent is kind-of a pain, so I tend to do a reasonable
job by hand and rely on the next global pgindent to fix such things. I
shorten the line anyway.

> + resetPQExpBuffer(&query);
> + appendPQExpBufferStr(&query, DDLINDEXes[i]);
>
> I think you can simply use printfPQExpBuffer() for the first append,
> similar to what you have used in createPartitions(), which is a
> combination of both reset and append.

It could, but it would mean switching to using a format which is not very
useful here as it uses the simpler append*Str variant.

While looking at it, I noticed the repeated tablespace addition just
afterwards, so I factored it out as well in a function.

Attached v3 shorten some lines and adds "append_tablespace".

--
Fabien.

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

Re: pgbench - refactor init functions with buffers

Andres Freund
Hi,

On 2019-10-24 08:33:06 +0200, Fabien COELHO wrote:
> Attached v3 shorten some lines and adds "append_tablespace".

I'd prefer not to expand the use of pqexpbuffer in more places, and
instead rather see this use StringInfo, now that's also available to
frontend programs.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Fabien COELHO-3

Hello Andres,

>> Attached v3 shorten some lines and adds "append_tablespace".

A v4 which just extends the patch to newly added 'G'.

> I'd prefer not to expand the use of pqexpbuffer in more places, and
> instead rather see this use StringInfo, now that's also available to
> frontend programs.

Franckly, one or the other does not matter much to me.

However, pgbench already uses PQExpBuffer, it uses PsqlScanState which
also uses PQExpBuffer, and it intrinsically depends on libpq which
provides PQExpBuffer: ISTM that it makes sense to keep going there, unless
PQExpBuffer support is to be dropped.

Switching all usages would involve a significant effort and having both
PQExpBuffer and string_info used in the same file for the same purpose
would be confusing.

--
Fabien.

pgbench-buffer-4.patch (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Fabien COELHO-3

>>> Attached v3 shorten some lines and adds "append_tablespace".
>
> A v4 which just extends the patch to newly added 'G'.

v5 is a rebase after 30a3e772.

--
Fabien.

pgbench-buffer-5.patch (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

David Steele
In reply to this post by Fabien COELHO-3
On 11/6/19 12:48 AM, Fabien COELHO wrote:

>
> Hello Andres,
>
>>> Attached v3 shorten some lines and adds "append_tablespace".
>
> A v4 which just extends the patch to newly added 'G'.
>
>> I'd prefer not to expand the use of pqexpbuffer in more places, and
>> instead rather see this use StringInfo, now that's also available to
>> frontend programs.
>
> Franckly, one or the other does not matter much to me.

FWIW, I agree with Andres with regard to using StringInfo.

Also, the changes to executeStatementExpect() and adding
executeStatement() do not seem to fit in with the purpose of this patch.

Regards,
--
-David
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Fabien COELHO-3

Hello David,

>>> I'd prefer not to expand the use of pqexpbuffer in more places, and
>>> instead rather see this use StringInfo, now that's also available to
>>> frontend programs.
>>
>> Franckly, one or the other does not matter much to me.
>
> FWIW, I agree with Andres with regard to using StringInfo.

Ok. I find it strange to mix PQExpBuffer & StringInfo in the same file.

> Also, the changes to executeStatementExpect() and adding executeStatement()
> do not seem to fit in with the purpose of this patch.

Yep, that was in passing.

Attached a v6 which uses StringInfo, and the small refactoring as a
separate patch.

--
Fabien.

pgbench-buffer-6.patch (17K) Download Attachment
pgbench-refactor-exec-1.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

David Steele
On 3/27/20 6:13 PM, Fabien COELHO wrote:

>
> Hello David,
>
>>>> I'd prefer not to expand the use of pqexpbuffer in more places, and
>>>> instead rather see this use StringInfo, now that's also available to
>>>> frontend programs.
>>>
>>> Franckly, one or the other does not matter much to me.
>>
>> FWIW, I agree with Andres with regard to using StringInfo.
>
> Ok. I find it strange to mix PQExpBuffer & StringInfo in the same file.

Agreed, but we'd rather use StringInfo going forward.  However, I don't
think that puts you on the hook for updating all the PQExpBuffer references.

Unless you want to...

>> Also, the changes to executeStatementExpect() and adding
>> executeStatement() do not seem to fit in with the purpose of this patch.
>
> Yep, that was in passing.
>
> Attached a v6 which uses StringInfo, and the small refactoring as a
> separate patch.

I think that's better, thanks.

Regards,
--
-David
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Fabien COELHO-3

>> Ok. I find it strange to mix PQExpBuffer & StringInfo in the same file.
>
> Agreed, but we'd rather use StringInfo going forward.  However, I don't think
> that puts you on the hook for updating all the PQExpBuffer references.
>
> Unless you want to...

I cannot say that I "want" to fix something which already works the same
way, because it is against my coding principles.

However there may be some fun in writing a little script to replace one
with the other automatically. I counted nearly 3500 calls under src/bin.

--
Fabien.


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Tom Lane-2
Fabien COELHO <[hidden email]> writes:
>>> Ok. I find it strange to mix PQExpBuffer & StringInfo in the same file.

>> Agreed, but we'd rather use StringInfo going forward.  However, I don't think
>> that puts you on the hook for updating all the PQExpBuffer references.
>> Unless you want to...

> I cannot say that I "want" to fix something which already works the same
> way, because it is against my coding principles.
> However there may be some fun in writing a little script to replace one
> with the other automatically. I counted nearly 3500 calls under src/bin.

Yeah, that's the problem.  If someone does come forward with a patch to do
that, I think it'd be summarily rejected, at least in high-traffic code
like pg_dump.  The pain it'd cause for back-patching would outweigh the
value.

That being the case, I'd think a better design principle is "make your
new code look like the code around it", which would tend to weigh against
introducing StringInfo uses into pgbench when there's none there now and
a bunch of PQExpBuffer instead.  So I can't help thinking the advice
you're being given here is suspect.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Alvaro Herrera-9
On 2020-Mar-27, Tom Lane wrote:

> That being the case, I'd think a better design principle is "make your
> new code look like the code around it", which would tend to weigh against
> introducing StringInfo uses into pgbench when there's none there now and
> a bunch of PQExpBuffer instead.  So I can't help thinking the advice
> you're being given here is suspect.

+1 for keeping it PQExpBuffer-only, until such a time when you need a
StringInfo feature that's not in PQExpBuffer -- and even at that point,
I think you'd switch just that one thing to StringInfo, not the whole
program.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

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

Hello Tom,

>> I cannot say that I "want" to fix something which already works the same
>> way, because it is against my coding principles. [...]
>> I counted nearly 3500 calls under src/bin.
>
> Yeah, that's the problem.  If someone does come forward with a patch to do
> that, I think it'd be summarily rejected, at least in high-traffic code
> like pg_dump.  The pain it'd cause for back-patching would outweigh the
> value.

What about "typedef StringInfoData PQExpBufferData" and replacing
PQExpBuffer by StringInfo internally, just keeping the old interface
around because it is there? That would remove a few hundreds clocs.

ISTM that with inline and varargs macro the substition can be managed
reasonably lightly, depending on what level of compatibility is required
for libpq: should it be linkability, or requiring a recompilation is ok?

A clear benefit is that there are quite a few utils for PQExpBuffer in
"fe_utils/string_utils.c" which would become available for StringInfo,
which would help using StringInfo without duplicating them.

> That being the case, I'd think a better design principle is "make your
> new code look like the code around it",

Yep.

> which would tend to weigh against introducing StringInfo uses into
> pgbench when there's none there now and a bunch of PQExpBuffer instead.
> So I can't help thinking the advice you're being given here is suspect.

Well, that is what I was saying, but at 2 against 1, I fold.

--
Fabien.


12