theory / pgtap

PostgreSQL Unit Testing Suite
https://pgtap.org
984 stars 92 forks source link

Run qualified typmodin function in format_type_string() #332

Closed ewie closed 8 months ago

ewie commented 8 months ago

Formatting typmodin_func with %I causes a qualified function name to be formatted as a identifier when the namespace is not on the search path. For example, foo.bar() will be formatted as "foo.bar"() when foo is not on the search_path. Calling "foo.bar"() triggers the exception handler in most cases either because the function doesn't exist or because it's not a typmodin function. format_type_string() will then return NULL which in turn results in col_type_is() to fail with an error that the wanted type does not exist.

Fix this by formatting the function name with %s. This works because %s emits qualified names and quoted identifiers if necessary for regproc (or any other OID).


Hi David! The fun never stops :sweat_smile:

Just got pgTAP 1.3.2 in our CI pipeline via Debian Bookworm and we instantly got failures such as

./test/core/tables.sql ........ 
# Failed test 16: "Column core.ag_fl_af.geom should be type postgis.geometry(MultiPolygon,25832)"
#     Type postgis.geometry(MultiPolygon,25832) does not exist

We run those tests only with pgTAP on the search path. Adding postgis to the search path as well is a quick workaround. But fixing format_type_string() is quite easy. Let me know if I missed something.

I also wanted to add tests in test/sql/coltap.sql to cover a custom type with typmodin function in namespace hidden. But that doesn't work with a simple SQL function because this function has to be implemented in C:

Base Types

[...]

The support functions input_function and output_function are required, while the functions receive_function, send_function, type_modifier_input_function, type_modifier_output_function, analyze_function, and subscript_function are optional. Generally these functions have to be coded in C or another low-level language.

https://www.postgresql.org/docs/current/sql-createtype.html

I was also looking for some builtin extension that we could install in namespace hidden, but there's no such extension providing types with typmods.

At least, here's a reproducer using PostGIS.

\pset null '<NULL>'

begin;

create schema pgtap;
create extension pgtap version '1.3.2' schema pgtap;

-- Install postgis in a separate schema that is not necessarily on the search_path.
-- PostGIS defines type geometry which has a custom typmodin function.
create schema postgis;
create extension postgis schema postgis;

\dx

-- Current behavior:

set local search_path = pgtap;
show search_path;

select format_type_string('postgis.geometry(point,4326)'),
       format_type_string('geometry(point,4326)');

-- Works when placing postgis on the search_path:

set local search_path = pgtap, postgis;
show search_path;

select format_type_string('postgis.geometry(point,4326)'),
       format_type_string('geometry(point,4326)');

-- Patching format_type_string() to use %s instead of %I:

CREATE OR REPLACE FUNCTION pgtap.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 %s(%L)', typmodin_func, typmodin_arg) INTO typmod;
    RETURN format_type(want_type::regtype, typmod);
EXCEPTION WHEN OTHERS THEN RETURN NULL;
END;
$$ LANGUAGE PLPGSQL STABLE;

set local search_path = pgtap;  -- postgis no longer on search_path
show search_path;

select format_type_string('postgis.geometry(point,4326)'),
       format_type_string('geometry(point,4326)');

rollback;

Output:

Null display is "<NULL>".
BEGIN
CREATE SCHEMA
CREATE EXTENSION
CREATE SCHEMA
CREATE EXTENSION
                                List of installed extensions
  Name   | Version |   Schema   |                        Description                         
---------+---------+------------+------------------------------------------------------------
 pgtap   | 1.3.2   | pgtap      | Unit testing for PostgreSQL
 plpgsql | 1.0     | pg_catalog | PL/pgSQL procedural language
 postgis | 3.4.1   | postgis    | PostGIS geometry and geography spatial types and functions
(3 rows)

SET
 search_path 
-------------
 pgtap
(1 row)

 format_type_string | format_type_string 
--------------------+--------------------
 <NULL>             | <NULL>
(1 row)

SET
  search_path   
----------------
 pgtap, postgis
(1 row)

  format_type_string  |  format_type_string  
----------------------+----------------------
 geometry(Point,4326) | geometry(Point,4326)
(1 row)

CREATE FUNCTION
SET
 search_path 
-------------
 pgtap
(1 row)

      format_type_string      | format_type_string 
------------------------------+--------------------
 postgis.geometry(Point,4326) | <NULL>
(1 row)

ROLLBACK
theory commented 8 months ago

Super thorough @ewie, thank you! Some things just aren't testable in core, of course, so this will have to do. Hopefully the parse_type() patch will be accepted for 17.0 and we can start to switch over to it.