libpq: PQfnumber overload for not null-terminated strings

Ivan Trofimov <i.trofimow@yandex.ru> writes:

Would you be willing to review a patch that adds an `PQfnumber` overload
that takes a `field_name` size as well?

I'm a little skeptical of this idea. If you need counted strings
for PQfnumber, wouldn't you need them for every single other
string-based API in libpq as well? That's not a lift that I think
we'd want to undertake.

regards, tom lane

Thanks for the quick reply.

If you need counted strings
for PQfnumber, wouldn't you need them for every single other
string-based API in libpq as well?

No, not really.

Thing is, out of all the functions listed in "34.3.2. Retrieving Query
Result Information" and "34.3.3. Retrieving Other Result Information"
PQfnumber is the only (well, technically also PQprint) one that takes a
string as an input.

As far as I know PQfnumber is the only portable way to implement "give
me the the value at this row with this 'column_name'", which is an
essential feature for any kind of client-side parsing.
Right now as a library writer in a higher-level language I'm forced to
either
* Sacrifice performance to ensure 'column_name' is null-terminated
(that's what some bindings in Rust do)
* Sacrifice interface quality by requiring a null-terminated string,
which is not necessary idiomatic (that's what we do)
* Sacrifice usability by requiring a user to guarantee that the
'string_view' provided is null-terminated (that's what libpqxx does, for
example)

I don't think it's _that_ big of a deal, but could it be QoL improvement
nice enough to be worth of a tiny addition into libpq interface?

Ivan Trofimov <i.trofimow@yandex.ru> writes:

If you need counted strings
for PQfnumber, wouldn't you need them for every single other
string-based API in libpq as well?

No, not really.

Thing is, out of all the functions listed in "34.3.2. Retrieving Query
Result Information" and "34.3.3. Retrieving Other Result Information"
PQfnumber is the only (well, technically also PQprint) one that takes a
string as an input.

I think that's a mighty myopic definition of which APIs would need
counted-string support if we were to make that a thing in libpq.
Just for starters, why are you only concerned with processing a
query result, and not with the functions needed to send the query?

Right now as a library writer in a higher-level language I'm forced to
either
* Sacrifice performance to ensure 'column_name' is null-terminated
(that's what some bindings in Rust do)

I'd go with that. You would have a very hard time convincing me that
the per-query overhead that building a null-terminated string adds
is even measurable compared to the time needed to send, process, and
receive the query.

regards, tom lane

Right now as a library writer in a higher-level language I'm forced to
either
* Sacrifice performance to ensure 'column_name' is null-terminated
(that's what some bindings in Rust do)

I'd go with that. You would have a very hard time convincing me that
the per-query overhead

I see now that I failed to express myself clearly: it's not a per-query
overhead, but rather a per-result-field one.

Given a code like this (in pseudo-code)

result = ExecuteQuery(some_query)
for (row in result):
a = row["some_column_name"]
b = row["some_other_column_name"]
...

a field-name string should be null-terminated for every field accessed.

There absolutely are ways to write the same in a more performant way and
avoid repeatedly calling PQfnumber altogether, but that I as a library
writer can't control.

In my quickly-hacked-together test just null-terminating a user-provided
string takes ~14% of total CPU time (and PQfnumber itself takes ~30%,
but oh well), please see the code and flamegraph attached.

On Tue, 27 Feb 2024 at 00:31, Ivan Trofimov <i.trofimow@yandex.ru> wrote:

I see now that I failed to express myself clearly: it's not a per-query
overhead, but rather a per-result-field one.

I'm fairly sympathetic to decreasing the overhead of any per-row
operation. And looking at the code, it doesn't surprise me that
PQfnumber shows up so big in your profile. I think it would probably
make sense to introduce a PQfnumber variant that does not do the
downcasing/quote handling (called e.g. PQfnumberRaw).

However, I do think you could convert this per-row overhead in your
case to per-query overhead by caching the result of PQfnumber for each
unique C++ string_view. Afaict that should solve your performance
problem.

However, I do think you could convert this per-row overhead in your
case to per-query overhead by caching the result of PQfnumber for each
unique C++ string_view. Afaict that should solve your performance
problem.

Absolutely, you're right.

The problem here is not that it's impossible to write it in a performant
way, but rather that it's impossible to do so in a performant _and_
clean way given the convenient abstractions wrapper-libraries provide:
here's a `Result`, which consists of `Row`s, which in turn consist of
`Field`s.
The most natural and straightforward way to iterate over a `Result`
would be in the lines of that loop, and people do write code like that
because it's what they expect to just work given the abstractions (and
it does, it's just slow).
Caching the result of PQfnumber could be done, but would result in
somewhat of a mess on a call-site.

I like your idea of 'PQfnumberRaw': initially i was only concerned about
a null-terminated string requirement affecting my interfaces (because
users complained about that to me,
https://github.com/userver-framework/userver/issues/494), but I think
PQfnumberRaw could solve both problems (PQfnumber being a bottleneck
when called repeatedly and a null-terminated string requirement)
simultaneously.

On Tue, 27 Feb 2024 at 15:49, Ivan Trofimov <i.trofimow@yandex.ru> wrote:

Caching the result of PQfnumber could be done, but would result in
somewhat of a mess on a call-site.

To be clear I meant your wrapper around libpq could internally cache
this, then the call sites of users of your wrapper would not need to
be changed. i.e. your Result could contain a cache of
columnname->columnumber mapping that you know because of previous
calls to PQfnumber on the same Result.

I like your idea of 'PQfnumberRaw': initially i was only concerned about
a null-terminated string requirement affecting my interfaces (because
users complained about that to me,
https://github.com/userver-framework/userver/issues/494), but I think
PQfnumberRaw could solve both problems (PQfnumber being a bottleneck
when called repeatedly and a null-terminated string requirement)
simultaneously.

Feel free to send a patch for this.