Re: Proposal to suppress errors thrown by to_reg*()

On Tue, 30 Jul 2019 12:24:13 -0400
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Takuma Hoshiai <hoshiai(at)sraoss(dot)co(dot)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(at)sraoss(dot)co(dot)jp>