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.
From 3410ac02bc34b60b79c99ad96505dabe4362570f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Thu, 18 Mar 2021 21:07:42 +0100
Subject: [PATCH v4] Add result_format_auto_binary_types setting
The current way binary results are requested in the extended query
protocol is too cumbersome for some practical uses. Some clients, for
example the JDBC driver, have built-in support for handling specific
data types in binary. Such a client would always have to request a
result row description (Describe statement) before sending a Bind
message, in order to be able to pick out the result columns it 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 solution is to allow a client to register for a session which
types it wants to always get in binary. This is done by a new GUC
setting. For example, to get int2, int4, int8 in binary by default,
you could set
SET result_format_auto_binary_types = 21,23,20;
To request result formats based on this setting, send format code
-1 (instead of 0 or 1) in the Bind message.
Discussion:
https://www.postgresql.org/message-id/flat/40cbb35d-774f-23ed-3079-03f938aac...@2ndquadrant.com
---
doc/src/sgml/config.sgml | 40 ++++++++
doc/src/sgml/libpq.sgml | 10 +-
doc/src/sgml/protocol.sgml | 7 +-
src/backend/access/common/printtup.c | 94 +++++++++++++++++++
src/backend/utils/misc/guc.c | 12 +++
src/include/access/printtup.h | 5 +
src/test/modules/Makefile | 1 +
src/test/modules/libpq_extended/.gitignore | 6 ++
src/test/modules/libpq_extended/Makefile | 20 ++++
src/test/modules/libpq_extended/README | 1 +
.../libpq_extended/t/001_result_format.pl | 33 +++++++
11 files changed, 222 insertions(+), 7 deletions(-)
create mode 100644 src/test/modules/libpq_extended/.gitignore
create mode 100644 src/test/modules/libpq_extended/Makefile
create mode 100644 src/test/modules/libpq_extended/README
create mode 100644 src/test/modules/libpq_extended/t/001_result_format.pl
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8603cf3f94..0fed8abfd2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8671,6 +8671,46 @@ <title>Statement Behavior</title>
</listitem>
</varlistentry>
+ <varlistentry id="guc-result-format-auto-binary-types"
xreflabel="result_format_auto_binary_types">
+ <term><varname>result_format_auto_binary_types</varname>
(<type>string</type>)
+ <indexterm>
+ <primary><varname>result_format_auto_binary_types</varname></primary>
+ <secondary>configuration parameter</secondary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ This parameter specifies which data types are to be sent from the
+ server in binary format for rows returned in the extended query
+ protocol when result format code -1 is specified in the <link
+ linkend="protocol-message-Bind">Bind message</link>. It is intended
+ to be used by client libraries that prefer to handle specific, but not
+ all, data types in binary format. The typical usage would be that the
+ client library sets this value when it starts a connection. (A client
+ library that wants to handle <emphasis>all</emphasis> types in binary
+ doesn't need to use this because it can just specify the format code
+ for all types at once in the protocol message.)
+ </para>
+
+ <para>
+ The value is a comma-separated list of type OIDs. For example, if you
+ want to automatically get values of the types <type>int2</type>,
+ <type>int4</type>, and <type>int8</type> in binary while leaving the
+ rest in text, an appropriate setting would be
+ <literal>21,23,20</literal>. A non-existing type OIDs are ignored.
+ This allows using the same value for different databases that might
+ have different extensions installed.
+ </para>
+
+ <para>
+ This setting applies only to result rows from the extended query
+ protocol, so it does not affect usage of
+ <application>psql</application> or <application>pg_dump</application>
+ for example. Also, it does not affect the format of query parameters.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</sect2>
<sect2 id="runtime-config-client-format">
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index be674fbaa9..ef55f09487 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2834,10 +2834,12 @@ <title>Main Functions</title>
<term><parameter>resultFormat</parameter></term>
<listitem>
<para>
- Specify zero to obtain results in text format, or one to obtain
- results in binary format. (There is not currently a provision
- to obtain different result columns in different formats,
- although that is possible in the underlying protocol.)
+ Specify 0 to obtain results in text format, or 1 to obtain results
+ in binary format, or -1 to have the setting of <xref
+ linkend="guc-result-format-auto-binary-types"/> be applied by the
+ server. (There is not currently a provision to obtain different
+ result columns in different formats, although that is possible in
+ the underlying protocol.)
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 43092fe62a..e3a614e80e 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -3587,7 +3587,7 @@ <title>Message Formats</title>
</varlistentry>
-<varlistentry>
+<varlistentry id="protocol-message-Bind">
<term>
Bind (F)
</term>
@@ -3729,8 +3729,9 @@ <title>Message Formats</title>
</term>
<listitem>
<para>
- The result-column format codes. Each must presently be
- zero (text) or one (binary).
+ The result-column format codes. Each must be 0 for text, or 1
+ for binary, or -1 to apply the setting of <xref
+ linkend="guc-result-format-auto-binary-types"/>.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/access/common/printtup.c
b/src/backend/access/common/printtup.c
index 54b539f6fb..0754ec7e9b 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -22,6 +22,7 @@
#include "utils/lsyscache.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
+#include "utils/varlena.h"
static void printtup_startup(DestReceiver *self, int operation,
@@ -30,6 +31,13 @@ static bool printtup(TupleTableSlot *slot, DestReceiver
*self);
static void printtup_shutdown(DestReceiver *self);
static void printtup_destroy(DestReceiver *self);
+/* GUC variable */
+char *result_format_auto_binary_types;
+
+/* internal parsed representation */
+static List *result_format_auto_binary_types_internal = NIL; /* type OID
list */
+
+
/* ----------------------------------------------------------------
* printtup / debugtup support
* ----------------------------------------------------------------
@@ -231,6 +239,9 @@ SendRowDescriptionMessage(StringInfo buf, TupleDesc
typeinfo,
else
format = 0;
+ if (format == -1)
+ format =
list_member_oid(result_format_auto_binary_types_internal, atttypid) ? 1 : 0;
+
pq_writestring(buf, NameStr(att->attname));
pq_writeint32(buf, resorigtbl);
pq_writeint16(buf, resorigcol);
@@ -271,6 +282,9 @@ printtup_prepare_info(DR_printtup *myState, TupleDesc
typeinfo, int numAttrs)
int16 format = (formats ? formats[i] : 0);
Form_pg_attribute attr = TupleDescAttr(typeinfo, i);
+ if (format == -1)
+ format =
list_member_oid(result_format_auto_binary_types_internal, attr->atttypid) ? 1 :
0;
+
thisState->format = format;
if (format == 0)
{
@@ -483,3 +497,83 @@ debugtup(TupleTableSlot *slot, DestReceiver *self)
return true;
}
+
+
+/*
+ * Support for result_format_auto_binary_types setting
+ */
+
+bool
+check_result_format_auto_binary_types(char **newval, void **extra, GucSource
source)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *lc;
+
+ rawstring = pstrdup(*newval);
+ if (!SplitGUCList(rawstring, ',', &elemlist))
+ {
+ GUC_check_errdetail("List syntax is invalid.");
+ pfree(rawstring);
+ list_free(elemlist);
+ return false;
+ }
+
+ foreach(lc, elemlist)
+ {
+ char *str = lfirst(lc);
+
+ if (atooid(str) == 0)
+ {
+ GUC_check_errdetail("Invalid list entry: %s", str);
+ pfree(rawstring);
+ list_free(elemlist);
+ return false;
+ }
+ }
+
+ pfree(rawstring);
+ list_free(elemlist);
+
+ return true;
+}
+
+void
+assign_result_format_auto_binary_types(const char *newval, void *extra)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *lc;
+
+ rawstring = pstrdup(newval);
+ if (!SplitGUCList(rawstring, ',', &elemlist))
+ {
+ pfree(rawstring);
+ list_free(elemlist);
+ return;
+ }
+
+ list_free(result_format_auto_binary_types_internal);
+ result_format_auto_binary_types_internal = NIL;
+
+ foreach(lc, elemlist)
+ {
+ char *str = lfirst(lc);
+ Oid oid;
+ MemoryContext oldcontext;
+
+ if ((oid = atooid(str)) == 0)
+ {
+ pfree(rawstring);
+ list_free(elemlist);
+ return;
+ }
+
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+ result_format_auto_binary_types_internal =
list_append_unique_oid(result_format_auto_binary_types_internal, oid);
+ MemoryContextSwitchTo(oldcontext);
+ }
+
+ pfree(rawstring);
+ list_free(elemlist);
+}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index b263e3493b..4d5447b09a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -31,6 +31,7 @@
#include "access/commit_ts.h"
#include "access/gin.h"
+#include "access/printtup.h"
#include "access/rmgr.h"
#include "access/tableam.h"
#include "access/transam.h"
@@ -4514,6 +4515,17 @@ static struct config_string ConfigureNamesString[] =
check_backtrace_functions, assign_backtrace_functions, NULL
},
+ {
+ {"result_format_auto_binary_types", PGC_USERSET,
CLIENT_CONN_STATEMENT,
+ gettext_noop("Which data types to send in binary for
format -1 in the extended query protocol."),
+ NULL,
+ GUC_LIST_INPUT | GUC_NOT_IN_SAMPLE |
GUC_DISALLOW_IN_FILE
+ },
+ &result_format_auto_binary_types,
+ "",
+ check_result_format_auto_binary_types,
assign_result_format_auto_binary_types, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, NULL, NULL, NULL, NULL
diff --git a/src/include/access/printtup.h b/src/include/access/printtup.h
index c9b37538a0..9ceaf4cd82 100644
--- a/src/include/access/printtup.h
+++ b/src/include/access/printtup.h
@@ -14,6 +14,7 @@
#ifndef PRINTTUP_H
#define PRINTTUP_H
+#include "utils/guc.h"
#include "utils/portal.h"
extern DestReceiver *printtup_create_DR(CommandDest dest);
@@ -27,6 +28,10 @@ extern void debugStartup(DestReceiver *self, int operation,
TupleDesc typeinfo);
extern bool debugtup(TupleTableSlot *slot, DestReceiver *self);
+extern char *result_format_auto_binary_types;
+extern bool check_result_format_auto_binary_types(char **newval, void **extra,
GucSource source);
+extern void assign_result_format_auto_binary_types(const char *newval, void
*extra);
+
/* XXX these are really in executor/spi.c */
extern void spi_dest_startup(DestReceiver *self, int operation,
TupleDesc typeinfo);
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 93e7829c67..65dcbd2a09 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -10,6 +10,7 @@ SUBDIRS = \
delay_execution \
dummy_index_am \
dummy_seclabel \
+ libpq_extended \
libpq_pipeline \
plsample \
snapshot_too_old \
diff --git a/src/test/modules/libpq_extended/.gitignore
b/src/test/modules/libpq_extended/.gitignore
new file mode 100644
index 0000000000..6fb386d93b
--- /dev/null
+++ b/src/test/modules/libpq_extended/.gitignore
@@ -0,0 +1,6 @@
+/test-*
+
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/libpq_extended/Makefile
b/src/test/modules/libpq_extended/Makefile
new file mode 100644
index 0000000000..eb6f308206
--- /dev/null
+++ b/src/test/modules/libpq_extended/Makefile
@@ -0,0 +1,20 @@
+# src/test/modules/libpq_extended/Makefile
+
+PROGRAM = test-result-format
+OBJS = test-result-format.o
+
+PG_CPPFLAGS = -I$(libpq_srcdir)
+PG_LIBS_INTERNAL += $(libpq_pgport)
+
+TAP_TESTS = 1
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/libpq_pipeline
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/libpq_extended/README
b/src/test/modules/libpq_extended/README
new file mode 100644
index 0000000000..842c02d4f0
--- /dev/null
+++ b/src/test/modules/libpq_extended/README
@@ -0,0 +1 @@
+Tests for libpq extended query protocol support
diff --git a/src/test/modules/libpq_extended/t/001_result_format.pl
b/src/test/modules/libpq_extended/t/001_result_format.pl
new file mode 100644
index 0000000000..0ceea44bcc
--- /dev/null
+++ b/src/test/modules/libpq_extended/t/001_result_format.pl
@@ -0,0 +1,33 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More tests => 4;
+use Cwd;
+
+my $node = get_new_node('main');
+$node->init;
+$node->start;
+
+$ENV{PATH} = "$ENV{PATH}:" . getcwd();
+
+# int2,int4,int8
+$ENV{PGOPTIONS} = '-c result_format_auto_binary_types=21,23,20,20000';
+$node->command_like([
+ 'test-result-format',
+ $node->connstr('postgres'),
+ "SELECT 1::int4, 2::int8, 'abc'::text",
+ ],
+ qr/0->1 1->1 2->0/,
+ "binary format is applied, nonexisting OID ignored");
+
+$ENV{PGOPTIONS} = '-c result_format_auto_binary_types=foo,bar';
+$node->command_fails([
+ 'test-result-format',
+ $node->connstr('postgres'),
+ "SELECT 1::int4, 2::int8, 'abc'::text",
+ ],
+ "invalid setting is an error");
+
+$node->stop('fast');
base-commit: ed62d3737c1b823f796d974060b1d0295a3dd831
--
2.30.2
