On Tue, 30 Jul 2019 12:24:13 -0400 Tom Lane <t...@sss.pgh.pa.us> wrote:
> Takuma Hoshiai <hosh...@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 <hosh...@sraoss.co.jp>