Re: Proposal to suppress errors thrown by to_reg*() - Mailing list pgsql-hackers

Takuma Hoshiai <hoshiai@sraoss.co.jp> writes:
> [ fix_to_reg_v2.patch ]

I took a quick look through this patch.  I'm on board with the goal
of not having schema-access violations throw an error in these
functions, but the implementation feels pretty ugly and bolted-on.
Nobody who had designed the code to do this from the beginning
would have chosen to parse the names twice, much less check the
ACLs twice which is what's going to happen in the success path.

But a worse problem is that this only fixes the issue for the
object name proper.  to_regprocedure and to_regoperator also
have type name(s) to think about, and this doesn't fix the
problem for those:

regression=# create schema s1;
CREATE SCHEMA
regression=# create type s1.d1 as enum('a','b');
CREATE TYPE
regression=# create function f1(s1.d1) returns s1.d1 language sql as
regression-# 'select $1';
CREATE FUNCTION
regression=# select to_regprocedure('f1(s1.d1)');
 to_regprocedure 
-----------------
 f1(s1.d1)
(1 row)

regression=# create user joe;
CREATE ROLE
regression=# \c - joe
You are now connected to database "regression" as user "joe".
regression=> select to_regprocedure('f1(s1.d1)');
ERROR:  permission denied for schema s1


A closely related issue that's been complained of before is that
while these functions properly return NULL when the main object
name includes a nonexistent schema:

regression=> select to_regprocedure('q1.f1(int)');
 to_regprocedure 
-----------------
 
(1 row)

it doesn't work for a nonexistent schema in a type name:

regression=> select to_regprocedure('f1(q1.d1)');
ERROR:  schema "q1" does not exist


Looking at the back-traces for these failures,

#0  errfinish (dummy=0) at elog.c:411
#1  0x0000000000553626 in aclcheck_error (aclerr=<value optimized out>, 
    objtype=OBJECT_SCHEMA, objectname=<value optimized out>) at aclchk.c:3623
#2  0x000000000055028f in LookupExplicitNamespace (
    nspname=<value optimized out>, missing_ok=false) at namespace.c:2928
#3  0x00000000005b3433 in LookupTypeName (pstate=0x0, typeName=0x20d87a0, 
    typmod_p=0x7fff94c3ee38, missing_ok=<value optimized out>)
    at parse_type.c:162
#4  0x00000000005b3b29 in parseTypeString (str=<value optimized out>, 
    typeid_p=0x7fff94c3ee3c, typmod_p=0x7fff94c3ee38, missing_ok=false)
    at parse_type.c:822
#5  0x000000000086fe21 in parseNameAndArgTypes (string=<value optimized out>, 
    allowNone=false, names=<value optimized out>, nargs=0x7fff94c3f01c, 
    argtypes=0x7fff94c3ee80) at regproc.c:1874
#6  0x0000000000870b2d in to_regprocedure (fcinfo=0x2134900) at regproc.c:305

#0  errfinish (dummy=0) at elog.c:411
#1  0x000000000054dc7b in get_namespace_oid (nspname=<value optimized out>, 
    missing_ok=false) at namespace.c:3061
#2  0x0000000000550230 in LookupExplicitNamespace (
    nspname=<value optimized out>, missing_ok=false) at namespace.c:2922
#3  0x00000000005b3433 in LookupTypeName (pstate=0x0, typeName=0x216bd20, 
    typmod_p=0x7fff94c3ee38, missing_ok=<value optimized out>)
    at parse_type.c:162
#4  0x00000000005b3b29 in parseTypeString (str=<value optimized out>, 
    typeid_p=0x7fff94c3ee3c, typmod_p=0x7fff94c3ee38, missing_ok=false)
    at parse_type.c:822
#5  0x000000000086fe21 in parseNameAndArgTypes (string=<value optimized out>, 
    allowNone=false, names=<value optimized out>, nargs=0x7fff94c3f01c, 
    argtypes=0x7fff94c3ee80) at regproc.c:1874
#6  0x0000000000870b2d in to_regprocedure (fcinfo=0x2170f50) at regproc.c:305

it's not *that* far from where we know we need no-error behavior to
where it's failing to happen.  parseNameAndArgTypes isn't even global,
so certainly changing its API is not problematic.  For the functions
below it, we'd have to decide whether it's okay to consider that
missing_ok=true also enables treating a schema access failure as
"missing", or whether we should add an additional flag argument
to decide that.  It seems like it might be more flexible to use a
separate flag, but that decision could propagate to a lot of places,
especially if we conclude that all the namespace.c functions that
expose missing_ok also need to expose schema_access_violation_ok.

So I think you ought to drop this implementation and fix it properly
by adjusting the behaviors of the functions cited above.

            regards, tom lane