Skip to content

Commit

Permalink
Replace C parse_type() function with PL/pgSQL
Browse files Browse the repository at this point in the history
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.

Make adjustments to the patches to eliminate hunk offset messages due to
the changes to the SQL code, and upgrade `actions/checkout` to v4 to
eliminate warnings in the workflows.

  [secure design principals]: https://www.postgresql.org/docs/current/extend-extensions.html#EXTEND-EXTENSIONS-SECURITY
  • Loading branch information
theory committed Feb 3, 2024
1 parent c47f862 commit 1ace153
Show file tree
Hide file tree
Showing 12 changed files with 92 additions and 137 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
PGXN_PASSWORD: ${{ secrets.PGXN_PASSWORD }}
steps:
- name: Check out the repo
uses: actions/checkout@v3
uses: actions/checkout@v4
- name: Bundle the Release
id: bundle
run: pgxn-bundle
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ jobs:
UPDATE_FROM: "${{ matrix.update_from }}"
steps:
- run: pg-start ${{ matrix.version }}
- uses: actions/checkout@v3
- uses: actions/checkout@v4

# Basic regression test.
- run: pg-build-test
Expand Down
8 changes: 8 additions & 0 deletions Changes
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions compat/install-9.1.patch
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
--- sql/pgtap.sql
+++ sql/pgtap.sql
@@ -786,10 +786,6 @@
@@ -781,10 +781,6 @@
RETURN ok( TRUE, descr );
EXCEPTION WHEN OTHERS THEN
-- There should have been no exception.
Expand All @@ -11,7 +11,7 @@
RETURN ok( FALSE, descr ) || E'\n' || diag(
' died: ' || _error_diag(SQLSTATE, SQLERRM, detail, hint, context, schname, tabname, colname, chkname, typname)
);
@@ -6667,10 +6663,6 @@
@@ -6688,10 +6684,6 @@
-- Something went wrong. Record that fact.
errstate := SQLSTATE;
errmsg := SQLERRM;
Expand All @@ -22,15 +22,15 @@
END;

-- Always raise an exception to rollback any changes.
@@ -7138,7 +7130,6 @@
@@ -7159,7 +7151,6 @@
RETURN ok( true, $3 );
EXCEPTION
WHEN datatype_mismatch THEN
- GET STACKED DIAGNOSTICS err_msg = MESSAGE_TEXT;
RETURN ok( false, $3 ) || E'\n' || diag(
E' Number of columns or their types differ between the queries' ||
CASE WHEN have_rec::TEXT = want_rec::text THEN '' ELSE E':\n' ||
@@ -7292,7 +7283,6 @@
@@ -7313,7 +7304,6 @@
RETURN ok( false, $3 );
EXCEPTION
WHEN datatype_mismatch THEN
Expand Down
4 changes: 2 additions & 2 deletions compat/install-9.2.patch
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
--- sql/pgtap.sql
+++ sql/pgtap.sql
@@ -789,12 +789,7 @@
@@ -784,12 +784,7 @@
GET STACKED DIAGNOSTICS
detail = PG_EXCEPTION_DETAIL,
hint = PG_EXCEPTION_HINT,
Expand All @@ -14,7 +14,7 @@
RETURN ok( FALSE, descr ) || E'\n' || diag(
' died: ' || _error_diag(SQLSTATE, SQLERRM, detail, hint, context, schname, tabname, colname, chkname, typname)
);
@@ -6675,12 +6670,7 @@
@@ -6696,12 +6691,7 @@
GET STACKED DIAGNOSTICS
detail = PG_EXCEPTION_DETAIL,
hint = PG_EXCEPTION_HINT,
Expand Down
6 changes: 3 additions & 3 deletions compat/install-9.4.patch
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
--- sql/pgtap.sql
+++ sql/pgtap.sql
@@ -680,7 +680,7 @@
@@ -675,7 +675,7 @@
' caught: no exception' ||
E'\n wanted: ' || COALESCE( errcode, 'an exception' )
);
Expand All @@ -9,7 +9,7 @@
IF (errcode IS NULL OR SQLSTATE = errcode)
AND ( errmsg IS NULL OR SQLERRM = errmsg)
THEN
@@ -784,7 +784,7 @@
@@ -779,7 +779,7 @@
BEGIN
EXECUTE code;
RETURN ok( TRUE, descr );
Expand All @@ -18,7 +18,7 @@
-- There should have been no exception.
GET STACKED DIAGNOSTICS
detail = PG_EXCEPTION_DETAIL,
@@ -10219,233 +10219,6 @@
@@ -10240,233 +10240,6 @@
), $2);
$$ LANGUAGE SQL immutable;

Expand Down
2 changes: 1 addition & 1 deletion compat/install-9.6.patch
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
--- sql/pgtap.sql
+++ sql/pgtap.sql
@@ -10201,136 +10201,6 @@
@@ -10222,136 +10222,6 @@
);
$$ LANGUAGE sql;

Expand Down
39 changes: 11 additions & 28 deletions doc/pgtap.mmd
Original file line number Diff line number Diff line change
Expand Up @@ -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"' );
Expand Down Expand Up @@ -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 );
Expand All @@ -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
Expand Down
34 changes: 34 additions & 0 deletions sql/pgtap--1.3.1--1.3.2.sql
Original file line number Diff line number Diff line change
@@ -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;
33 changes: 27 additions & 6 deletions sql/pgtap.sql.in
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
90 changes: 0 additions & 90 deletions src/pgtap.c

This file was deleted.

0 comments on commit 1ace153

Please sign in to comment.