From 8fab510672e9fb940008c117590d045e13105e33 Mon Sep 17 00:00:00 2001 From: "David E. Wheeler" Date: Sat, 3 Feb 2024 17:01:53 -0500 Subject: [PATCH] Replace C parse_type() function with PL/pgSQL The C code cause a number of challenges, including the requirement for a superuser to install the extension, and would require a fair bit of refactoring to abide by [secure design principals]. It also makes installation difficult on Windows, and would likely be rejected by organizations like AWS that tend to balk at C code. So delete the `parse_type()` function and all the C code added in 4ec32e7 and rewrite the `format_type_string()` function based on the prototype that @ewie developed in the comments of #315. By looking up the type output format function in the catalog, we're able to correctly output the data type with its full SQL name as before. The exception is interval types, which require the PostgreSQL grammar to parse the interval fields (`second`, `interval minute`, etc.) into a bitmask for the `typmod`. So require that they be specified exactly as PostgreSQL outputs them. [secure design principals]: https://www.postgresql.org/docs/current/extend-extensions.html#EXTEND-EXTENSIONS-SECURITY --- Changes | 8 ++++ Makefile | 1 - doc/pgtap.mmd | 39 +++++----------- sql/pgtap--1.3.1--1.3.2.sql | 34 ++++++++++++++ sql/pgtap.sql.in | 33 +++++++++++--- src/pgtap.c | 90 ------------------------------------- 6 files changed, 80 insertions(+), 125 deletions(-) create mode 100644 sql/pgtap--1.3.1--1.3.2.sql delete mode 100644 src/pgtap.c diff --git a/Changes b/Changes index c60b8b13..cd457972 100644 --- a/Changes +++ b/Changes @@ -4,6 +4,14 @@ Revision history for pgTAP 1.3.2 -------------------------- +* Replaced the C `parse_type()` function added in v1.3.1 with a PL/pgSQL + function, eliminating the need for the the installing use to be a superuser. + Aliases still work for all data types passed to `col_type_is()` except for + intervals, which must be specified exactly as rendered by Postgres itself, + without aliasing. Thanks to @spencerbryson for the report (#328) and to + Erik Wienhold for the PL/pgSQL technique necessary to properly format + type strings (#315). + 1.3.1 2023-09-24T15:29:42Z -------------------------- * Revamped the handling of data type declarations in `col_type_is()` to always diff --git a/Makefile b/Makefile index a8b4eaed..f255fe6a 100644 --- a/Makefile +++ b/Makefile @@ -13,7 +13,6 @@ EXTRA_CLEAN = $(VERSION_FILES) sql/pgtap.sql sql/uninstall_pgtap.sql sql/pgtap- EXTRA_CLEAN += $(wildcard sql/*.orig) # These are files left behind by patch DOCS = doc/pgtap.mmd PG_CONFIG ?= pg_config -MODULES = src/pgtap # # Test configuration. This must be done BEFORE including PGXS diff --git a/doc/pgtap.mmd b/doc/pgtap.mmd index 75cecde3..bce5cb9e 100644 --- a/doc/pgtap.mmd +++ b/doc/pgtap.mmd @@ -4524,6 +4524,11 @@ pass the type as either "varchar(64)" or "character varying(64)". Example: SELECT col_type_is( 'myschema', 'sometable', 'somecolumn', 'timespantz(3)' ); +The exception to this rule is interval types, which must be specified as +rendered by PostgreSQL itself: + + SELECT col_type_is( 'myschema', 'sometable', 'somecolumn', 'interval second(3)' ); + Types with case-sensitive names or special characters must be double-quoted: SELECT col_type_is( 'myschema', 'sometable', 'somecolumn', '"myType"' ); @@ -8293,31 +8298,6 @@ included in the display. For example: Used internally by pgTAP to compare operators, but may be more generally useful. -### `parse_type()` ### - - SELECT * FROM parse_type( :text ); - -**Parameters** - -`:text` -: An SQL type declaration, optionally schema-qualified. - -Parses a string representing an SQL type declaration as used in a `CREATE TABLE` -statement, optionally schema-qualified. Returns a record with two fields, -`typid` and `typmod`, representing the OID and modifier for the type. These are -the underlying values that define column data types, and which can be passed to -the PostgreSQL core -[`format_type()`](https://www.postgresql.org/docs/current/functions-info.html#FUNCTIONS-INFO-CATALOG) -function to display the normalized string representation of the data type. -Raises an error if the specify type does not exist or cannot be found in the -search path. - - try=% SELECT format_type(p.typid, p.typmod) - try-% FROM parse_type('timestamp(4)') p; - format_type - -------------------------------- - timestamp(4) without time zone - ### `format_type_string()` ### SELECT format_type_string( :text ); @@ -8329,9 +8309,12 @@ search path. This function normalizes data type declarations for accurate comparison to table columns by `col_type_is()`. It's effectively the identical to -the calling `format_type()` with the values returned by `parse_type()`, -but returns a `NULL` on an invalid or missing type, rather than raising -an error. +the calling `format_type()` with the type OID and type modifier that define +the column, but returns a `NULL` on an invalid or missing type, rather than +raising an error. Types can be defined by their canonical names or their +aliases, e.g., `character varying` or `varchar`. The exception is `interval` +types, which must be specified exactly as Postgres renders them internally, +e.g., `'interval(0)`, `interval second(0)`, or `interval day to second(4)`. try=# SELECT format_type_string('timestamp(3)'); format_type_string diff --git a/sql/pgtap--1.3.1--1.3.2.sql b/sql/pgtap--1.3.1--1.3.2.sql new file mode 100644 index 00000000..d0dbd80c --- /dev/null +++ b/sql/pgtap--1.3.1--1.3.2.sql @@ -0,0 +1,34 @@ +DROP FUNCTION parse_type(type text, OUT typid oid, OUT typmod int4); + +CREATE OR REPLACE FUNCTION format_type_string ( TEXT ) +RETURNS TEXT AS $$ +DECLARE + want_type TEXT := $1; + typmodin_arg cstring[]; + typmodin_func regproc; + typmod int; +BEGIN + IF want_type::regtype = 'interval'::regtype THEN + -- RAISE NOTICE 'cannot resolve: %', want_type; -- TODO + RETURN want_type; + END IF; + + -- Extract type modifier from type declaration and format as cstring[] literal. + typmodin_arg := translate(substring(want_type FROM '[(][^")]+[)]'), '()', '{}'); + + -- Find typmodin function for want_type. + SELECT typmodin INTO typmodin_func + FROM pg_catalog.pg_type + WHERE oid = want_type::regtype; + + IF typmodin_func = 0 THEN + -- Easy: types without typemods. + RETURN format_type(want_type::regtype, null); + END IF; + + -- Get typemod via type-specific typmodin function. + EXECUTE format('SELECT %I(%L)', typmodin_func, typmodin_arg) INTO typmod; + RETURN format_type(want_type::regtype, typmod); +EXCEPTION WHEN OTHERS THEN RETURN NULL; +END; +$$ LANGUAGE PLPGSQL STABLE; diff --git a/sql/pgtap.sql.in b/sql/pgtap.sql.in index 0e873948..e245cbfe 100644 --- a/sql/pgtap.sql.in +++ b/sql/pgtap.sql.in @@ -22,11 +22,6 @@ CREATE OR REPLACE FUNCTION pgtap_version() RETURNS NUMERIC AS 'SELECT __VERSION__;' LANGUAGE SQL IMMUTABLE; -CREATE FUNCTION parse_type(type text, OUT typid oid, OUT typmod int4) -RETURNS RECORD -AS '$libdir/pgtap' -LANGUAGE C STABLE STRICT; - CREATE OR REPLACE FUNCTION plan( integer ) RETURNS TEXT AS $$ DECLARE @@ -1468,7 +1463,33 @@ $$ LANGUAGE PLPGSQL STABLE; CREATE OR REPLACE FUNCTION format_type_string ( TEXT ) RETURNS TEXT AS $$ -BEGIN RETURN format_type(p.typid, p.typmod) from parse_type($1) p; +DECLARE + want_type TEXT := $1; + typmodin_arg cstring[]; + typmodin_func regproc; + typmod int; +BEGIN + IF want_type::regtype = 'interval'::regtype THEN + -- RAISE NOTICE 'cannot resolve: %', want_type; -- TODO + RETURN want_type; + END IF; + + -- Extract type modifier from type declaration and format as cstring[] literal. + typmodin_arg := translate(substring(want_type FROM '[(][^")]+[)]'), '()', '{}'); + + -- Find typmodin function for want_type. + SELECT typmodin INTO typmodin_func + FROM pg_catalog.pg_type + WHERE oid = want_type::regtype; + + IF typmodin_func = 0 THEN + -- Easy: types without typemods. + RETURN format_type(want_type::regtype, null); + END IF; + + -- Get typemod via type-specific typmodin function. + EXECUTE format('SELECT %I(%L)', typmodin_func, typmodin_arg) INTO typmod; + RETURN format_type(want_type::regtype, typmod); EXCEPTION WHEN OTHERS THEN RETURN NULL; END; $$ LANGUAGE PLPGSQL STABLE; diff --git a/src/pgtap.c b/src/pgtap.c deleted file mode 100644 index aa92d5ba..00000000 --- a/src/pgtap.c +++ /dev/null @@ -1,90 +0,0 @@ -/* - * PostgreSQL utility functions for pgTAP. - */ - -#include "postgres.h" -#include "fmgr.h" -#include "funcapi.h" /* for returning composite type */ -#include "utils/builtins.h" /* text_to_cstring() */ -#include "parser/parse_type.h" /* parseTypeString() */ - - -#if PG_VERSION_NUM < 90300 -#include "access/htup.h" /* heap_form_tuple() */ -#elif PG_VERSION_NUM < 130000 -#include "access/htup_details.h" /* heap_form_tuple() */ -#endif - -#ifdef PG_MODULE_MAGIC -PG_MODULE_MAGIC; -#endif - - -/* - * Given a string that is supposed to be a SQL-compatible type declaration, - * such as "int4" or "integer" or "character varying(32)", parse - * the string and convert it to a type OID and type modifier. - * - * Raises an error on an invalid type. - */ - -/* - * parse_type() is the inverse of pg_catalog.format_type(): it takes a string - * representing an SQL-compatible type declaration, such as "int4" or "integer" - * or "character varying(32)", parses it, and returns the OID and type modifier. - * Returns NULL for an invalid type. - * - * Internally it relies on the Postgres core parseTypeString() function defined - * in src/backend/parser/parse_type.c. - */ -Datum parse_type(PG_FUNCTION_ARGS); - -PG_FUNCTION_INFO_V1(parse_type); - -Datum -parse_type(PG_FUNCTION_ARGS) -{ -#define PARSE_TYPE_STRING_COLS 2 /* Returns two columns. */ - const char *type; /* the type string we want to resolve */ - Oid typid; /* the resolved type oid */ - int32 typmod; /* the resolved type modifier */ - TupleDesc tupdesc; - HeapTuple rettuple; - Datum values[PARSE_TYPE_STRING_COLS] = {0}; - bool nulls[PARSE_TYPE_STRING_COLS] = {0}; - - type = text_to_cstring(PG_GETARG_TEXT_PP(0)); - - /* - * Build a tuple descriptor for our result type; return an error if not - * called in a context that expects a record. - */ - if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) { - ereport( - ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("function returning record called in context that cannot accept type record")) - ); - } - - BlessTupleDesc(tupdesc); - - /* - * Parse type-name argument to obtain type OID and encoded typmod. We don't - * need to check for parseTypeString failure, but just let the error be - * raised. The 0 arg works both as the `Node *escontext` arg in Postgres 16 - * and the `bool missing_ok` arg in 9.4-15. - */ -#if PG_VERSION_NUM < 90400 - (void) parseTypeString(type, &typid, &typmod); -#else - (void) parseTypeString(type, &typid, &typmod, 0); -#endif - - /* Create and return tuple. */ - values[0] = typid; - values[1] = typmod; - rettuple = heap_form_tuple(tupdesc, values, nulls); - return HeapTupleGetDatum(rettuple); -#undef PARSE_TYPE_STRING_COLS -}