On 2017-09-18 02:53:03 -0700, Andres Freund wrote: > On 2017-09-13 23:39:21 -0400, Tom Lane wrote: > > The real problem in this area, to my mind, is that we're not testing that > > code --- either end of it --- in any systematic way. If it's broken it > > could take us quite a while to notice. > > Independent of the thrust of my question - why aren't we adding a > 'force-v2' option to libpq? A test that basically does something like > postgres[22923][1]=# \setenv PGFORCEV2 1 > postgres[22923][1]=# \c > You are now connected to database "postgres" as user "andres". > postgres[22924][1]=> > seems easy enough to add, in fact I tested the above. > > And the protocol coverage of the v2 protocol seems small enough that a > single not too large file ought to cover most if it quite easily.
Here's what I roughly was thinking of. I don't quite like the name, and the way the version is specified for libpq (basically just the "raw" integer). Not sure if others have an opinion on that. I personally would lean towards not documenting this option... There's a few things that I couldn't find easy ways to test: - the v2 specific binary protocol - I don't quite see how we could test that without writing C - version error checks - pg_regress/psql errors out in non-interactive mode if a connection fails to be established. This we could verify with a s simple tap test. Coverage of the relevant files is a good bit higher afterwards. Although our libpq coverage is generally pretty damn awful. Regards, Andres
>From d4b9fb485110a5a27d9f85eff23a8ffca550eaf2 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Wed, 20 Sep 2017 01:12:32 -0700 Subject: [PATCH 1/3] Add ability to force libpq to negotiate a specific version of the protocol. Author: Andres Freund Discussion: https://postgr.es/m/20170918095303.g766pdnvviqle...@alap3.anarazel.de --- src/interfaces/libpq/fe-connect.c | 13 ++++++++++++- src/interfaces/libpq/libpq-int.h | 3 +++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index c580d91135..a9dd14d48b 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -323,6 +323,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = { "Target-Session-Attrs", "", 11, /* sizeof("read-write") = 11 */ offsetof(struct pg_conn, target_session_attrs)}, + {"forced_protocol_version", "PGFORCEPROTOCOLVERSION", NULL, NULL, + "Forced-version", "D", sizeof("Forced-version"), + offsetof(struct pg_conn, forced_protocol_version)}, + /* Terminating entry --- MUST BE LAST */ {NULL, NULL, NULL, NULL, NULL, NULL, 0} @@ -1803,7 +1807,14 @@ connectDBStart(PGconn *conn) */ conn->whichhost = 0; conn->addr_cur = conn->connhost[0].addrlist; - conn->pversion = PG_PROTOCOL(3, 0); + if (conn->forced_protocol_version != NULL) + { + conn->pversion = atoi(conn->forced_protocol_version); + } + else + { + conn->pversion = PG_PROTOCOL(3, 0); + } conn->send_appname = true; conn->status = CONNECTION_NEEDED; diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 42913604e3..7bfc09ec71 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -364,6 +364,9 @@ struct pg_conn /* Type of connection to make. Possible values: any, read-write. */ char *target_session_attrs; + char *forced_protocol_version; /* for testing: force protocol to a + * specific version */ + /* Optional file to write trace info to */ FILE *Pfdebug; -- 2.14.1.536.g6867272d5b.dirty
>From da43188afcf150213212c6c419c5c87b003feb65 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Wed, 20 Sep 2017 01:10:17 -0700 Subject: [PATCH 2/3] Add minimal v2 protocol regression tests. Author: Andres Freund Discussion: https://postgr.es/m/20170918095303.g766pdnvviqle...@alap3.anarazel.de --- src/test/regress/expected/protocol.out | 252 +++++++++++++++++++++++++++++++++ src/test/regress/parallel_schedule | 2 +- src/test/regress/serial_schedule | 1 + src/test/regress/sql/protocol.sql | 124 ++++++++++++++++ 4 files changed, 378 insertions(+), 1 deletion(-) create mode 100644 src/test/regress/expected/protocol.out create mode 100644 src/test/regress/sql/protocol.sql diff --git a/src/test/regress/expected/protocol.out b/src/test/regress/expected/protocol.out new file mode 100644 index 0000000000..75e183ba78 --- /dev/null +++ b/src/test/regress/expected/protocol.out @@ -0,0 +1,252 @@ +\set ON_ERROR_STOP 0 +-- Function that returns the protocol version, as required for the protocol +CREATE OR REPLACE FUNCTION pg_protocol(major int, minor int) +RETURNS int +LANGUAGE SQL +IMMUTABLE +AS $$ + SELECT $1 << 16 | $2; +$$; +-- +-- first a few error checks about acceptance of various protocol versions +-- +-- Can't execute failing tests these via psql -f / pg_regress, as they +-- exit after a connection failure, therefore those are commented out. +-- +-- -- too old protocol version +-- SELECT pg_protocol(major => 1, minor => 0) \gset +-- \setenv PGFORCEPROTOCOLVERSION :pg_protocol +-- \c +-- +-- -- check too low major, with high minor +-- SELECT pg_protocol(major => 1, minor => 30) \gset +-- \setenv PGFORCEPROTOCOLVERSION :pg_protocol +-- \c +-- +-- check 2.0 protocol, the earliest accepted +SELECT pg_protocol(major => 2, minor => 0) \gset +\setenv PGFORCEPROTOCOLVERSION :pg_protocol +\c +-- check 2.10 protocol, an unknown minor +SELECT pg_protocol(major => 2, minor => 10) \gset +\setenv PGFORCEPROTOCOLVERSION :pg_protocol +\c +-- check 3.0 protocol, the default version +SELECT pg_protocol(major => 3, minor => 0) \gset +\setenv PGFORCEPROTOCOLVERSION :pg_protocol +\c +-- -- check 3.10 protocol, an unknown minor, will error out +-- SELECT pg_protocol(major => 3, minor => 10) \gset +-- \setenv PGFORCEPROTOCOLVERSION :pg_protocol +-- \c +-- +-- ensure some coverage for v2 protocol, which is otherwise untested +-- +SELECT pg_protocol(major => 2, minor => 0) \gset +\setenv PGFORCEPROTOCOLVERSION :pg_protocol +\c +-- check that NOTICEs in the middle of a statement work +CREATE OR REPLACE FUNCTION pg_temp.raise_notice(p_text text) +RETURNS text +LANGUAGE plpgsql AS $$ +BEGIN + RAISE NOTICE '%', p_text; + RETURN p_text; +END$$; +-- check that NOTICE in the middle of a normal statement works +SELECT d, COALESCE(d, pg_temp.raise_notice('isnull')) +FROM (VALUES ('notnull'), (NULL), ('notnullagain')) AS d(d); +NOTICE: isnull + d | coalesce +--------------+-------------- + notnull | notnull + | isnull + notnullagain | notnullagain +(3 rows) + +-- check that NOTICE in the middle of a COPY works +CREATE TEMPORARY TABLE bleat_on_null(d text CHECK(COALESCE(d, pg_temp.raise_notice('isnull')) IS NOT NULL)); +\copy bleat_on_null from stdin +NOTICE: isnull +NOTICE: isnull +-- while at it, also test COPY OUT +\copy bleat_on_null TO stdout +notnull +\N +\N +notnullagain +COPY BLEAT_ON_NULL TO stdout; +notnull +\N +\N +notnullagain +-- no columns +SELECT; +-- +(1 row) + +-- anonymous columns +SELECT 1; + ?column? +---------- + 1 +(1 row) + +-- many columns with NULLs (for NULL bitmap) +SELECT 1 c1, 2 c2, 3 c3, NULL::int c4, 5 c5, 6 c6, 7 c7, 8 c8, NULL::text c9, 10 c10; + c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8 | c9 | c10 +----+----+----+----+----+----+----+----+----+----- + 1 | 2 | 3 | | 5 | 6 | 7 | 8 | | 10 +(1 row) + +-- send two sql queries in one protocol message +SELECT 1\;SELECT 2; + ?column? +---------- + 2 +(1 row) + +-- a few more rows +SELECT a, b +FROM generate_series(1, 10) a, generate_series(1, 10) b; + a | b +----+---- + 1 | 1 + 1 | 2 + 1 | 3 + 1 | 4 + 1 | 5 + 1 | 6 + 1 | 7 + 1 | 8 + 1 | 9 + 1 | 10 + 2 | 1 + 2 | 2 + 2 | 3 + 2 | 4 + 2 | 5 + 2 | 6 + 2 | 7 + 2 | 8 + 2 | 9 + 2 | 10 + 3 | 1 + 3 | 2 + 3 | 3 + 3 | 4 + 3 | 5 + 3 | 6 + 3 | 7 + 3 | 8 + 3 | 9 + 3 | 10 + 4 | 1 + 4 | 2 + 4 | 3 + 4 | 4 + 4 | 5 + 4 | 6 + 4 | 7 + 4 | 8 + 4 | 9 + 4 | 10 + 5 | 1 + 5 | 2 + 5 | 3 + 5 | 4 + 5 | 5 + 5 | 6 + 5 | 7 + 5 | 8 + 5 | 9 + 5 | 10 + 6 | 1 + 6 | 2 + 6 | 3 + 6 | 4 + 6 | 5 + 6 | 6 + 6 | 7 + 6 | 8 + 6 | 9 + 6 | 10 + 7 | 1 + 7 | 2 + 7 | 3 + 7 | 4 + 7 | 5 + 7 | 6 + 7 | 7 + 7 | 8 + 7 | 9 + 7 | 10 + 8 | 1 + 8 | 2 + 8 | 3 + 8 | 4 + 8 | 5 + 8 | 6 + 8 | 7 + 8 | 8 + 8 | 9 + 8 | 10 + 9 | 1 + 9 | 2 + 9 | 3 + 9 | 4 + 9 | 5 + 9 | 6 + 9 | 7 + 9 | 8 + 9 | 9 + 9 | 10 + 10 | 1 + 10 | 2 + 10 | 3 + 10 | 4 + 10 | 5 + 10 | 6 + 10 | 7 + 10 | 8 + 10 | 9 + 10 | 10 +(100 rows) + +-- check error responses +SELECT "nonexistant-column"; +ERROR: column "nonexistant-column" does not exist at character 8 +DO $$ +BEGIN + RAISE 'I am an error, what is your name?'; +END; +$$; +ERROR: I am an error, what is your name? +-- minimal largeobject test, also verifies PQfn() +BEGIN; +SELECT lo_creat(1) AS lo_oid \gset +SELECT lo_open(:lo_oid, CAST(x'20000' | x'40000' AS integer)) as fd \gset +SELECT lowrite(:fd, 'just some text'); + lowrite +--------- + 14 +(1 row) + +SELECT loread(:fd, '100'); + loread +-------- + \x +(1 row) + +SELECT lo_lseek(:fd, 0, 2); + lo_lseek +---------- + 14 +(1 row) + +COMMIT; +\lo_unlink :lo_oid +-- +-- Cleanup +-- +DROP FUNCTION pg_protocol(int, int); diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule index 2fd3f2b1b1..5db1a89b30 100644 --- a/src/test/regress/parallel_schedule +++ b/src/test/regress/parallel_schedule @@ -89,7 +89,7 @@ test: brin gin gist spgist privileges init_privs security_label collate matview # ---------- # Another group of parallel tests # ---------- -test: alter_generic alter_operator misc psql async dbsize misc_functions sysviews tsrf tidscan stats_ext +test: alter_generic alter_operator misc psql async dbsize misc_functions sysviews tsrf tidscan stats_ext protocol # rules cannot run concurrently with any test that creates a view test: rules psql_crosstab amutils diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule index 76b0de30a7..0452943162 100644 --- a/src/test/regress/serial_schedule +++ b/src/test/regress/serial_schedule @@ -131,6 +131,7 @@ test: sysviews test: tsrf test: tidscan test: stats_ext +test: protocol test: rules test: psql_crosstab test: select_parallel diff --git a/src/test/regress/sql/protocol.sql b/src/test/regress/sql/protocol.sql new file mode 100644 index 0000000000..e2f71dba25 --- /dev/null +++ b/src/test/regress/sql/protocol.sql @@ -0,0 +1,124 @@ +\set ON_ERROR_STOP 0 + +-- Function that returns the protocol version, as required for the protocol +CREATE OR REPLACE FUNCTION pg_protocol(major int, minor int) +RETURNS int +LANGUAGE SQL +IMMUTABLE +AS $$ + SELECT $1 << 16 | $2; +$$; + +-- +-- first a few error checks about acceptance of various protocol versions +-- +-- Can't execute failing tests these via psql -f / pg_regress, as they +-- exit after a connection failure, therefore those are commented out. +-- + +-- -- too old protocol version +-- SELECT pg_protocol(major => 1, minor => 0) \gset +-- \setenv PGFORCEPROTOCOLVERSION :pg_protocol +-- \c +-- +-- -- check too low major, with high minor +-- SELECT pg_protocol(major => 1, minor => 30) \gset +-- \setenv PGFORCEPROTOCOLVERSION :pg_protocol +-- \c +-- + +-- check 2.0 protocol, the earliest accepted +SELECT pg_protocol(major => 2, minor => 0) \gset +\setenv PGFORCEPROTOCOLVERSION :pg_protocol +\c + +-- check 2.10 protocol, an unknown minor +SELECT pg_protocol(major => 2, minor => 10) \gset +\setenv PGFORCEPROTOCOLVERSION :pg_protocol +\c + +-- check 3.0 protocol, the default version +SELECT pg_protocol(major => 3, minor => 0) \gset +\setenv PGFORCEPROTOCOLVERSION :pg_protocol +\c + +-- -- check 3.10 protocol, an unknown minor, will error out +-- SELECT pg_protocol(major => 3, minor => 10) \gset +-- \setenv PGFORCEPROTOCOLVERSION :pg_protocol +-- \c + + +-- +-- ensure some coverage for v2 protocol, which is otherwise untested +-- + +SELECT pg_protocol(major => 2, minor => 0) \gset +\setenv PGFORCEPROTOCOLVERSION :pg_protocol +\c + + +-- check that NOTICEs in the middle of a statement work +CREATE OR REPLACE FUNCTION pg_temp.raise_notice(p_text text) +RETURNS text +LANGUAGE plpgsql AS $$ +BEGIN + RAISE NOTICE '%', p_text; + RETURN p_text; +END$$; + +-- check that NOTICE in the middle of a normal statement works +SELECT d, COALESCE(d, pg_temp.raise_notice('isnull')) +FROM (VALUES ('notnull'), (NULL), ('notnullagain')) AS d(d); + +-- check that NOTICE in the middle of a COPY works +CREATE TEMPORARY TABLE bleat_on_null(d text CHECK(COALESCE(d, pg_temp.raise_notice('isnull')) IS NOT NULL)); +\copy bleat_on_null from stdin +notnull +\N +\N +notnullagain +\. +-- while at it, also test COPY OUT +\copy bleat_on_null TO stdout +COPY BLEAT_ON_NULL TO stdout; + +-- no columns +SELECT; + +-- anonymous columns +SELECT 1; + +-- many columns with NULLs (for NULL bitmap) +SELECT 1 c1, 2 c2, 3 c3, NULL::int c4, 5 c5, 6 c6, 7 c7, 8 c8, NULL::text c9, 10 c10; + +-- send two sql queries in one protocol message +SELECT 1\;SELECT 2; + +-- a few more rows +SELECT a, b +FROM generate_series(1, 10) a, generate_series(1, 10) b; + +-- check error responses +SELECT "nonexistant-column"; + +DO $$ +BEGIN + RAISE 'I am an error, what is your name?'; +END; +$$; + + +-- minimal largeobject test, also verifies PQfn() +BEGIN; +SELECT lo_creat(1) AS lo_oid \gset +SELECT lo_open(:lo_oid, CAST(x'20000' | x'40000' AS integer)) as fd \gset +SELECT lowrite(:fd, 'just some text'); +SELECT loread(:fd, '100'); +SELECT lo_lseek(:fd, 0, 2); +COMMIT; +\lo_unlink :lo_oid + +-- +-- Cleanup +-- +DROP FUNCTION pg_protocol(int, int); -- 2.14.1.536.g6867272d5b.dirty
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
