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
|

Re: pgbench - refactor init functions with buffers

David Steele
On 3/27/20 9:52 PM, Alvaro Herrera wrote:

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

I think I need to be careful what I joke about.  It wasn't my intention
to advocate changing all the existing *PQExpBuffer() calls in bin.

But, the only prior committer to look at this patch expressed a
preference for StringInfo so in the absence of any other input I thought
it might move the patch forward if I reinforced that.  Now it seems the
consensus has moved in favor of *PQExpBuffer().

Fabien has provided a patch in each flavor, so I guess the question is:
is it committable either way?

Regards,
--
-David
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Andres Freund
In reply to this post by Tom Lane-2
Hi,

On 2020-03-27 19:57:12 -0400, Tom Lane wrote:

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

Sure, but that's not at all what was proposed.


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

I don't agree with this. This is a "fresh" usage of StringInfo. That's
different to adding one new printed line among others built with
pqexpbuffer. If we continue adding large numbers of new uses of both
pieces of infrastructure, we're just making things more confusing.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2020-03-27 19:57:12 -0400, 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.

> I don't agree with this. This is a "fresh" usage of StringInfo. That's
> different to adding one new printed line among others built with
> pqexpbuffer. If we continue adding large numbers of new uses of both
> pieces of infrastructure, we're just making things more confusing.

Why?  I'm not aware of any intention to deprecate/remove PQExpBuffer,
and I doubt it'd be a good thing to try.  It does some things that
StringInfo won't, notably cope with OOM without crashing.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Andres Freund
On 2020-03-28 14:49:31 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > On 2020-03-27 19:57:12 -0400, 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.
>
> > I don't agree with this. This is a "fresh" usage of StringInfo. That's
> > different to adding one new printed line among others built with
> > pqexpbuffer. If we continue adding large numbers of new uses of both
> > pieces of infrastructure, we're just making things more confusing.
>
> Why?  I'm not aware of any intention to deprecate/remove PQExpBuffer,
> and I doubt it'd be a good thing to try.  It does some things that
> StringInfo won't, notably cope with OOM without crashing.

- code using it cannot easily be shared between frontend/backend (no
  memory context integration etc)
- most code does *not* want to deal with the potential for OOM without
  erroring out
- it's naming is even more confusing than StringInfo
- it introduces dependencies to libpq even when not needed
- both stringinfo and pqexpbuffer are performance relevant in some uses,
  needing to optimize both is wasted effort
- we shouldn't expose everyone to both APIs except where needed - it's
  stuff one has to learn


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2020-03-28 14:49:31 -0400, Tom Lane wrote:
>> Why?  I'm not aware of any intention to deprecate/remove PQExpBuffer,
>> and I doubt it'd be a good thing to try.  It does some things that
>> StringInfo won't, notably cope with OOM without crashing.

> - code using it cannot easily be shared between frontend/backend (no
>   memory context integration etc)

True, but also pretty irrelevant for pgbench and similar code.

> - most code does *not* want to deal with the potential for OOM without
>   erroring out

Fair point.

> - it's naming is even more confusing than StringInfo

Eye of the beholder ...

> - it introduces dependencies to libpq even when not needed

Most of our FE programs do include libpq, and pgbench certainly does,
so this seems like a pretty irrelevant objection as well.

> - both stringinfo and pqexpbuffer are performance relevant in some uses,
>   needing to optimize both is wasted effort

I'm not aware that anybody is trying to micro-optimize either.  Even
if someone is, it doesn't imply that they need to change both.

> - we shouldn't expose everyone to both APIs except where needed - it's
>   stuff one has to learn

That situation is unlikely to change in the foreseeable future.
Moreover, using both APIs in one program, where we were not before,
makes it worse not better.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Andres Freund
On 2020-03-28 15:16:21 -0400, Tom Lane wrote:
> Andres Freund <[hidden email]> writes:
> > - both stringinfo and pqexpbuffer are performance relevant in some uses,
> >   needing to optimize both is wasted effort
>
> I'm not aware that anybody is trying to micro-optimize either.

https://postgr.es/m/5450.1578797036%40sss.pgh.pa.us


Reply | Threaded
Open this post in threaded view
|

Re: pgbench - refactor init functions with buffers

Fabien COELHO-3
In reply to this post by Andres Freund

Hello Andres,

>> 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.
>
> I don't agree with this. This is a "fresh" usage of StringInfo. That's
> different to adding one new printed line among others built with
> pqexpbuffer. If we continue adding large numbers of new uses of both
> pieces of infrastructure, we're just making things more confusing.
My 0.02 € :

  - I'm in favor or having one tool for one purpose, so a fe/be common
StringInfo interface is fine with me;

  - I prefer to avoid using both PQExpBuffer & StringInfo in the same file,
because they do the exact same thing and it is locally confusing;

  - I'd be fine with switching all of pgbench to StringInfo, as there are
only 31 uses;

  - But, pgbench relies on psql scanner, which uses PQExpBuffer in
PsqlScanState, so mixing is unavoidable, unless PQExpBuffer & StringInfo
are the same thing (i.e. typedef + cpp/inline/function wrappers);

  - There are 1260 uses of PQExpBuffer in psql that, although they are
trivial, I'm in no hurry to update.

--
Fabien.
12