default result formats setting

po 26. 10. 2020 v 9:31 odesílatel Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> napsal:

During the discussion on dynamic result sets[0], it became apparent that
the current way binary results are requested in the extended query
protocol is too cumbersome for some practical uses, and keeping that
style around would also make the proposed protocol extensions very
complicated.

The premise here is that a client library has hard-coded knowledge on
how to deal with binary format for certain, but not all, data types.
(Most client libraries process everything in text, and some client
libraries process everything in binary. Neither of these extremes are
of concern here.) Such a client always has to request a result row
description (Describe statement) before sending a Bind message, in order
to be able to pick out the result columns in should request in binary.
The feedback was that this extra round trip is often not worth it in
terms of performance, and so it is not done and binary format is not
used when it could be.

The conceptual solution is to allow a client to register for a session
which types it wants to always get in binary, unless it says otherwise.
In the discussion in [0], I pondered a new protocol message for that,
but after further thought, a GUC setting would do just as well.

The attached patch implements this. For example, to get int2, int4,
int8 in binary by default, you could set

SET default_result_formats = '21=1,23=1,20=1';

Using SET statement for this case looks very obscure :/

This is a protocol related issue, and should be solved by protocol
extending. I don't think so SQL level is good for that.

More, this format is not practical for custom types, and the list can be
pretty long.

This is a list of oid=format pairs.

I think this format satisfies the current requirements of the JDBC
driver. But the format could also be extended in the future to allow
type names to be listed or some other ways of identifying the types.

In order to be able to test this via libpq, I had to add a little hack.
Currently, PQexecParams() and similar functions can only pass exactly
one result format code, which per protocol is then applied to all result
columns. There is no support for sending zero result format codes to
make the session default apply. I enabled this by allowing -1 to be
passed as the format code. I'm not sure if we want to make this part of
the official API, but it would be useful to have something like this
somehow.

+1 to this feature, but -1 for design. It should be solved on protocol
level.

Regards

Pavel

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

The conceptual solution is to allow a client to register for a session
which types it wants to always get in binary, unless it says otherwise.

OK.

In the discussion in [0], I pondered a new protocol message for that,
but after further thought, a GUC setting would do just as well.

I think a GUC is conceptually the wrong level ...

In order to be able to test this via libpq, I had to add a little hack.

... which is part of the reason why you have to kluge this. I'm not
entirely certain which levels of the client stack need to know about
this, but surely libpq is one.

I'm also quite worried about failures (maybe even security problems)
arising from the "wrong level" of the client stack setting the GUC.

Independently of that, how would you implement "says otherwise" here,
ie do a single-query override of the session's prevailing setting?
Maybe the right thing for that is to define -1 all the way down to the
protocol level as meaning "use the session's per-type default", and
then if you don't want that you can pass 0 or 1. An advantage of that
is that you couldn't accidentally break an application that wasn't
ready for this feature, because it would not be the default to use it.

regards, tom lane

On 2020-10-26 09:45, Pavel Stehule wrote:

The attached patch implements this.  For example, to get int2, int4,
int8 in binary by default, you could set

SET default_result_formats = '21=1,23=1,20=1';

Using SET statement for this case looks very obscure :/

This is a protocol related issue, and should be solved by protocol
extending. I don't think so SQL level is good for that.

We could also make it a protocol message, but it would essentially
implement the same thing, just again separately. And then you'd have no
support to inspect the current setting, test out different settings
interactively, etc. That seems pretty wasteful and complicated for no
real gain.

More, this format is not practical for custom types, and the list can
be pretty long.

The list is what the list is. I don't see how you can make it any
shorter. You have to list the data types that you're interested in
somehow. Any other ideas?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-10-26 15:35, Tom Lane wrote:

In the discussion in [0], I pondered a new protocol message for that,
but after further thought, a GUC setting would do just as well.

I think a GUC is conceptually the wrong level ...

It does feel that way, but it gets the job done well and you can use all
the functionality already existing, such as being able to inspect
settings, temporarily change settings, etc. Otherwise we'd have to
implement a lot of things like that again. That would turn this 200
line patch into a 2000 line patch without any real additional benefit.

In order to be able to test this via libpq, I had to add a little hack.

... which is part of the reason why you have to kluge this. I'm not
entirely certain which levels of the client stack need to know about
this, but surely libpq is one.

I'm also quite worried about failures (maybe even security problems)
arising from the "wrong level" of the client stack setting the GUC.

I don't think libpq needs to know about this very deeply. The protocol
provides format information with the result set. Libpq programs can
query that with PQfformat() and act accordingly. Nothing else is needed.

The real consumer of this would be the JDBC driver, which has built-in
knowledge of the binary formats of some data types. Libpq doesn't, so
it wouldn't use this facility anyway. (Not saying someone couldn't
write a higher-level C library that does this, but it doesn't exist now.
... hmm ... ecpg ...)

Independently of that, how would you implement "says otherwise" here,
ie do a single-query override of the session's prevailing setting?
Maybe the right thing for that is to define -1 all the way down to the
protocol level as meaning "use the session's per-type default", and
then if you don't want that you can pass 0 or 1. An advantage of that
is that you couldn't accidentally break an application that wasn't
ready for this feature, because it would not be the default to use it.

Yeah, that sounds a lot better. I'll look into that.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

čt 5. 11. 2020 v 21:48 odesílatel Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> napsal:

On 2020-10-26 09:45, Pavel Stehule wrote:

The attached patch implements this. For example, to get int2, int4,
int8 in binary by default, you could set

SET default_result_formats = '21=1,23=1,20=1';

Using SET statement for this case looks very obscure :/

This is a protocol related issue, and should be solved by protocol
extending. I don't think so SQL level is good for that.

We could also make it a protocol message, but it would essentially
implement the same thing, just again separately. And then you'd have no
support to inspect the current setting, test out different settings
interactively, etc. That seems pretty wasteful and complicated for no
real gain.

If you need a debug API, then it can be better implemented with functions.
But why do you need it on SQL level?

This is a protocol related thing.

On 2020-11-05 22:03, Peter Eisentraut wrote:

Independently of that, how would you implement "says otherwise" here,
ie do a single-query override of the session's prevailing setting?
Maybe the right thing for that is to define -1 all the way down to the
protocol level as meaning "use the session's per-type default", and
then if you don't want that you can pass 0 or 1. An advantage of that
is that you couldn't accidentally break an application that wasn't
ready for this feature, because it would not be the default to use it.

Yeah, that sounds a lot better. I'll look into that.

Here is a new patch updated to work that way. Feels better now.

--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

On 11/9/20 5:10 AM, Peter Eisentraut wrote:

On 2020-11-05 22:03, Peter Eisentraut wrote:

Independently of that, how would you implement "says otherwise" here,
ie do a single-query override of the session's prevailing setting?
Maybe the right thing for that is to define -1 all the way down to the
protocol level as meaning "use the session's per-type default", and
then if you don't want that you can pass 0 or 1.  An advantage of that
is that you couldn't accidentally break an application that wasn't
ready for this feature, because it would not be the default to use it.

Yeah, that sounds a lot better.  I'll look into that.

Here is a new patch updated to work that way.  Feels better now.

I think this is conceptually OK, although it feels a bit odd.

Might it be better to have the values as typename={binary,text} pairs
instead of oid={0,1} pairs, which are fairly opaque? That might make
things easier for things like UDTs where the oid might not be known or
constant.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2020-11-16 16:15, Andrew Dunstan wrote:

I think this is conceptually OK, although it feels a bit odd.

Might it be better to have the values as typename={binary,text} pairs
instead of oid={0,1} pairs, which are fairly opaque? That might make
things easier for things like UDTs where the oid might not be known or
constant.

Yes, type names would be better. I was hesitant because of all the
parsing work involved, but I bit the bullet and did it in the new patch.

To simplify the format, I changed the parameter so it's just a list of
types that you want in binary, rather than type=value pairs. If we ever
want to add another format, we would revisit this, but it seems unlikely
in the near future.

Also, I have changed the naming of the parameter since this is no longer
the "default" but something you choose explicitly. I'm thinking in the
direction of "auto" mode for the naming. Obviously, the name is easy to
tweak in any case.

--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

On 11/25/20 2:06 AM, Peter Eisentraut wrote:

On 2020-11-16 16:15, Andrew Dunstan wrote:

I think this is conceptually OK, although it feels a bit odd.

Might it be better to have the values as typename={binary,text} pairs
instead of oid={0,1} pairs, which are fairly opaque? That might make
things easier for things like UDTs where the oid might not be known or
constant.

Yes, type names would be better.  I was hesitant because of all the
parsing work involved, but I bit the bullet and did it in the new patch.

To simplify the format, I changed the parameter so it's just a list of
types that you want in binary, rather than type=value pairs.  If we ever
want to add another format, we would revisit this, but it seems unlikely
in the near future.

Also, I have changed the naming of the parameter since this is no longer
the "default" but something you choose explicitly.  I'm thinking in the
direction of "auto" mode for the naming.  Obviously, the name is easy to
tweak in any case.

Andrew, Tom, does the latest patch address your concerns?

Regards,
--
-David
david@pgmasters.net

David Steele <david@pgmasters.net> writes:

Andrew, Tom, does the latest patch address your concerns?

[ reads patch quickly... ] I think the definition is fine now,
modulo possible bikeshedding on the GUC name. (I have no
great suggestion on that right now, but the current proposal
seems mighty verbose.)

The implementation feels weird though, mainly in that I don't like
Peter's choices for where to put the code. pquery.c is not where
I would have expected to find the support for this, and I do not
have any confidence that applying the format conversion while
filling portal->formats[] is enough to cover all cases. I'd have
thought that access/common/printtup.c or somewhere near there
would be where to do it.

Likewise, the code associated with caching the results of the type
OID lookups seems like it should be someplace where you'd be more
likely to find (a) type name lookup and (b) caching logic. I'm
not quite sure about the best place for that, but we could do
worse than put it in parse_type.c. (As I recall, the parser
already has some caching related to operator lookup, so doing
part (b) there isn't too much of a stretch.)

Also, if we need YA string-splitting function, please put it
beside the ones that already exist (SplitIdentifierString etc in
varlena.c). That way (a) it's available if some other code needs
it, and (b) when somebody gets around to refactoring all the
splitters, they won't have to dig into nooks and crannies to find
them.

Having said that, I wonder if we should define the parameter's
contents this way, i.e. as things that parseTypeString will
accept. At best that's overspecification, e.g. should people
expect that varchar(7) and varchar(9) are different things
(and, perhaps, that such entries *don't* match varchars of other
lengths?) I think a case could be made for requiring the entries
to be identifiers exactly matching pg_type.typname, possibly with
schema qualification. This'd allow tighter verification of the
GUC value's format in the GUC check hook.

Or we could drop all of that and go back to having it be a list
of type OIDs, which would remove a *whole lot* of the complexity,
and I'm not sure that it's materially less friendly. Applications
have had to deal with type OIDs in the protocol since forever.

BTW, I wonder whether we still need to restrict the GUC to not
be settable from postgresql.conf. The fact that the client has
to explicitly pass -1 seems to reduce any security issues quite
a bit.

regards, tom lane

On 09.03.21 19:04, Tom Lane wrote:

The implementation feels weird though, mainly in that I don't like
Peter's choices for where to put the code. pquery.c is not where
I would have expected to find the support for this, and I do not
have any confidence that applying the format conversion while
filling portal->formats[] is enough to cover all cases. I'd have
thought that access/common/printtup.c or somewhere near there
would be where to do it.

done

Or we could drop all of that and go back to having it be a list
of type OIDs, which would remove a *whole lot* of the complexity,
and I'm not sure that it's materially less friendly. Applications
have had to deal with type OIDs in the protocol since forever.

also done

The client driver needs to be able to interpret the OIDs that the
RowDescription sends back, so it really needs to be able to deal in
OIDs, and having the option to specify type names won't help it right now.

BTW, I wonder whether we still need to restrict the GUC to not
be settable from postgresql.conf. The fact that the client has
to explicitly pass -1 seems to reduce any security issues quite
a bit.

There was no security concern, but I don't think it's useful. The
driver would specify "send int4 in binary, I know how to handle that".
There doesn't seem to be a point in specifying that sort of thing globally.

I applied the patch, tried running the test and got the following:

rm -rf '/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended'/tmp_check
/bin/sh ../../../../config/install-sh -c -d '/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended'/tmp_check
cd . && TESTDIR='/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended' PATH="/Users/hasegeli/Developer/postgres/tmp_install/Users/hasegeli/.local/pgsql/bin:$PATH" DYLD_LIBRARY_PATH="/Users/hasegeli/Developer/postgres/tmp_install/Users/hasegeli/.local/pgsql/lib" PGPORT='65432' PG_REGRESS='/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended/../../../../src/test/regress/pg_regress' REGRESS_SHLIB='/Users/hasegeli/Developer/postgres/src/test/regress/regress.so' /usr/bin/prove -I ../../../../src/test/perl/ -I . t/*.pl
t/001_result_format.pl .. # Looks like your test exited with 2 before it could output anything.
t/001_result_format.pl .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 4/4 subtests

Test Summary Report
-------------------
t/001_result_format.pl (Wstat: 512 Tests: 0 Failed: 0)
Non-zero exit status: 2
Parse errors: Bad plan. You planned 4 tests but ran 0.

On 19.03.21 15:55, Emre Hasegeli wrote:

I applied the patch, tried running the test and got the following:

rm -rf '/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended'/tmp_check
/bin/sh ../../../../config/install-sh -c -d '/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended'/tmp_check
cd . && TESTDIR='/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended' PATH="/Users/hasegeli/Developer/postgres/tmp_install/Users/hasegeli/.local/pgsql/bin:$PATH" DYLD_LIBRARY_PATH="/Users/hasegeli/Developer/postgres/tmp_install/Users/hasegeli/.local/pgsql/lib" PGPORT='65432' PG_REGRESS='/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended/../../../../src/test/regress/pg_regress' REGRESS_SHLIB='/Users/hasegeli/Developer/postgres/src/test/regress/regress.so' /usr/bin/prove -I ../../../../src/test/perl/ -I . t/*.pl
t/001_result_format.pl .. # Looks like your test exited with 2 before it could output anything.
t/001_result_format.pl .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 4/4 subtests

Test Summary Report
-------------------
t/001_result_format.pl (Wstat: 512 Tests: 0 Failed: 0)
Non-zero exit status: 2
Parse errors: Bad plan. You planned 4 tests but ran 0.

Could you look into the log files in that test directory what is going
on? The test setup is closely modeled after
src/test/modules/libpq_pipeline/. Does that one run ok?

Could you look into the log files in that test directory what is going
on?

Command 'test-result-format' not found in
/Users/hasegeli/Developer/postgres/tmp_install/Users/hasegeli/.local/pgsql/bin,
/Users/hasegeli/.local/bin, /opt/homebrew/bin, /usr/local/bin,
/usr/bin, /bin, /usr/sbin, /sbin,
/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended at
/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended/../../../../src/test/perl/TestLib.pm
line 818.

Maybe you forgot to commit the file in the test?

The test setup is closely modeled after
src/test/modules/libpq_pipeline/. Does that one run ok?

Yes

On 21.03.21 20:18, Emre Hasegeli wrote:

Could you look into the log files in that test directory what is going
on?

Command 'test-result-format' not found in
/Users/hasegeli/Developer/postgres/tmp_install/Users/hasegeli/.local/pgsql/bin,
/Users/hasegeli/.local/bin, /opt/homebrew/bin, /usr/local/bin,
/usr/bin, /bin, /usr/sbin, /sbin,
/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended at
/Users/hasegeli/Developer/postgres/src/test/modules/libpq_extended/../../../../src/test/perl/TestLib.pm
line 818.

Maybe you forgot to commit the file in the test?

Indeed. Here is an updated patch.

I think this is a good feature that would be useful to JDBC and more.

I don't know the surrounding code very well, but the patch looks good to me.

I agree with Tom Lane that the name of the variable is too verbose.
Maybe "auto_binary_types" is enough. Do we gain much by prefixing
"result_format_"? Wouldn't we use the same variable, if we support
binary inputs one day?

It is nice that the patch comes with the test module. The name
"libpq_extended" sounds a bit vague to me. Maybe it's a better idea
to call it "libpq_result_format" and test "format=1" in it as well.

My last nitpicking about the names is the "test-result-format"
command. All the rest of the test modules name the commands with
underscores. It would be nicer if this one complies.

There is one place that needs to be updated on the Makefile of the test:

+subdir = src/test/modules/libpq_pipeline

s/pipeline/extended/

Then the test runs successfully.

On Thu, Nov 5, 2020 at 3:49 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

We could also make it a protocol message, but it would essentially
implement the same thing, just again separately. And then you'd have no
support to inspect the current setting, test out different settings
interactively, etc. That seems pretty wasteful and complicated for no
real gain.

But ... if it's just a GUC, it can be set by code on the server side
that the client knows nothing about, breaking the client. That seems
pretty bad to me.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

But ... if it's just a GUC, it can be set by code on the server side
that the client knows nothing about, breaking the client. That seems
pretty bad to me.

It's impossible for the proposed patch to break *existing* clients,
because they all send requested format 0 or 1, and that is exactly
what they'll get back.

A client that is sending -1 and assuming that it will get back
a particular format could get broken if the GUC doesn't have the
value it thinks, true. But I'd argue that such code is unreasonably
non-robust. Can't we solve this by recommending that clients using
this feature always double-check which format they actually got?
ISTM that the use-cases for the feature involve checking what data
type you got anyway, so that's not an unreasonable added requirement.

regards, tom lane

On Wed, Mar 24, 2021 at 10:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

But ... if it's just a GUC, it can be set by code on the server side
that the client knows nothing about, breaking the client. That seems
pretty bad to me.

It's impossible for the proposed patch to break *existing* clients,
because they all send requested format 0 or 1, and that is exactly
what they'll get back.

OK.

A client that is sending -1 and assuming that it will get back
a particular format could get broken if the GUC doesn't have the
value it thinks, true. But I'd argue that such code is unreasonably
non-robust. Can't we solve this by recommending that clients using
this feature always double-check which format they actually got?
ISTM that the use-cases for the feature involve checking what data
type you got anyway, so that's not an unreasonable added requirement.

I suppose that's a fair idea, but to me it still feels a bit like a
round peg in the square hole. Suppose for example that there's a
client application which wants to talk to a connection pooler which in
turn wants to talk to the server. Let's also suppose that connection
pooler isn't just a pass-through, but wants to redirect client
connections to various servers or even intercept queries and result
sets and make changes as the data passes by. It can do that by parsing
SQL and solving the halting problem, whereas if this were a
protocol-level option it would be completely doable. Now you could say
"well, by that argument, DateStyle ought to be a protocol-level
option, too," and that's pretty a pretty fair criticism of what I'm
saying here. On the other hand, I'm not too sure that wouldn't have
been the right call. Using SQL to tailor the wire protocol format
feels like some kind of layering inversion to me. I think we should be
working toward a state where it's more clear which things are "owned"
at the wire protocol level and which things are "owned" at the SQL
level, and this seems to be going in exactly the opposite direction,
and in fact probably taking things further in that direction than
we've ever gone before.

--
Robert Haas
EDB: http://www.enterprisedb.com