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

On Tue, 30 Jul 2019 12:24:13 -0400
Tom Lane <tgl@sss.pgh.pa.us> wrote:

> 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.

Thank you for watching.
Sorry, I overlooked an access permission error of argument.

behavior of 'missing_ok = true' is changed  have an impact on
DROP TABLE IF EXISTS for example. So, I will consider to add an additonal
flag like schema_access_violation_ok, instead of checking ACL twice.

>             regards, tom lane
> 

Best Regards,

-- 
Takuma Hoshiai <hoshiai@sraoss.co.jp>