Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run qualified typmodin function in format_type_string() #332

Merged
merged 2 commits into from
Feb 17, 2024

Conversation

ewie
Copy link
Contributor

@ewie ewie commented Feb 10, 2024

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 😅

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

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).
@theory theory self-assigned this Feb 17, 2024
@theory theory added the bug label Feb 17, 2024
@theory
Copy link
Owner

theory commented Feb 17, 2024

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.

@theory theory merged commit 02bc769 into theory:main Feb 17, 2024
15 checks passed
halostatue added a commit to KineticCafe/docker-sqitch-pgtap that referenced this pull request Feb 20, 2024
- Upgrade pgTAP to 1.3.3 at
  theory/pgtap@02bc769. This fixes a bug
  introduced where the column type validation is reporting invalid values.

  Incorporates the fix theory/pgtap#332.
halostatue added a commit to KineticCafe/docker-sqitch-pgtap that referenced this pull request Feb 20, 2024
- Upgrade pgTAP to 1.3.3 at
  theory/pgtap@02bc769. This fixes a bug
  introduced where the column type validation is reporting invalid values.

  Incorporates the fix theory/pgtap#332.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants