Thread: default result formats setting

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

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.


[0]: 
https://www.postgresql.org/message-id/flat/6e747f98-835f-2e05-cde5-86ee444a7140%402ndquadrant.com

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

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

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 24, 2021 at 10:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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 can't say that I'm 100% comfortable with it either, but the alternative
seems quite unpleasant, precisely because the client side might have
multiple layers involved.  If we make it a wire-protocol thing then
a whole lot of client API thrashing is going to ensue to transmit the
desired setting up and down the stack.  As an example, libpq doesn't
really give a darn which data format is returned: it is the application
using libpq that would want to be able to set this option.  If libpq
has to be involved in transmitting the option to the backend, then we
need a new libpq API call to tell it to do that.  Rinse and repeat
for anything that wraps libpq.  And, in the end, it's not real clear
which client-side layer *should* have control of this.  In some cases
you might want the decision to be taken quite high up, because which
format is really more efficient will depend on the total usage picture
for a given application, which low-level code like libpq wouldn't know.
Having a library decide that "this buck stops with me" is likely to be
the wrong thing.

I do not understand the structure of the client stack for JDBC, but
I wonder whether there won't be similar issues there.

As you say, DateStyle and the like are precedents for things that
*could* break application stacks, and in another universe maybe we'd
have managed them differently.  In the end though, they've been like
that for a long time and we've not heard many complaints about them.
So I'm inclined to think that that precedent says this is OK too.

BTW, I thought briefly about whether we could try to lock things down
a bit by marking the GUC as PGC_BACKEND, which would effectively mean
that clients would have to send it in the startup packet.  However,
that would verge on making it unusable for non-built-in datatypes,
for which you need to look up the OID first.  So I don't think that'd
be an improvement.

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

I don't think I buy the premise that there are exactly two levels
on the client side.

            regards, tom lane

On Wed, Mar 24, 2021 at 12:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I don't think I buy the premise that there are exactly two levels
> on the client side.

Thanks for sharing your thoughts on this. I agree it's a complex
issue, and the idea that there are possibly more than two logical
levels is, for me, maybe your most interesting observation.

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