Обсуждение: Proposal to suppress errors thrown by to_reg*()

Hi, hackers,

According to the document, "to_reg* functions return null rather than
throwing an error if the name is not found", but this is not the case
if the arguments to those functions are schema qualified and the
caller does not have access permission of the schema even if the table
(or other object) does exist -- we get an error.

For example, to_regclass() throws an error if its argument is
'schema_name.table_name'' (i.e. contains schema name) and caller's
role doesn't have access permission of the schema. Same thing can be
said to Other functions,

I get complain from Pgpool-II users because it uses to_regclass()
internally to confirm table's existence but in the case above it's
not useful because the error aborts user's transaction.

To be more consistent with the doc and to make those functions more
useful, I propose to change current implementation so that they do not
throw an error if the name space cannot be accessible by the caller.

Attached patch implements this. Comments and suggestions are welcome.

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

Hi Takuma,

On Thu, 14 Mar 2019 13:37:00 +0900
Takuma Hoshiai <hoshiai@sraoss.co.jp> wrote:

> Hi, hackers,
> 
> According to the document, "to_reg* functions return null rather than
> throwing an error if the name is not found", but this is not the case
> if the arguments to those functions are schema qualified and the
> caller does not have access permission of the schema even if the table
> (or other object) does exist -- we get an error.
> 
> For example, to_regclass() throws an error if its argument is
> 'schema_name.table_name'' (i.e. contains schema name) and caller's
> role doesn't have access permission of the schema. Same thing can be
> said to Other functions,
> 
> I get complain from Pgpool-II users because it uses to_regclass()
> internally to confirm table's existence but in the case above it's
> not useful because the error aborts user's transaction.
> 
> To be more consistent with the doc and to make those functions more
> useful, I propose to change current implementation so that they do not
> throw an error if the name space cannot be accessible by the caller.
> 
> Attached patch implements this. Comments and suggestions are welcome.

I reviewed the patch. Here are some comments:

 /*
+ * RangeVarCheckNamespaceAccessNoError
+ *         Returns true if given relation's namespace can be accessable by the
+ *         current user. If no namespace is given in the relation, just returns
+ *         true.
+ */
+bool
+RangeVarCheckNamespaceAccessNoError(RangeVar *relation)

Although it might be trivial, the new function's name 'RangeVar...' seems a bit
weird to me because this is used for not only to_regclass but also to_regproc,
to_regtype, and so on, that is, the argument "relation" is not always a relation. 

This function is used always with makeRangeVarFromNameList() as

  if (!RangeVarCheckNamespaceAccessNoError(makeRangeVarFromNameList(names)))

, so how about merging the two function as below, for example:

 /*
  * CheckNamespaceAccessNoError
  *         Returns true if the namespace in given qualified-name can be accessable
  *         by the current user. If no namespace is given in the names, just returns
  *         true.
  */
 bool
 CheckNamespaceAccessNoError(List *names);


BTW, this patch supresses also "Cross-database references is not allowed" error in
addition to the namespace ACL error.  Is this an intentional change?  If this error
can be allowed, you can use DeconstructQualifiedName() instead of
makeRangeVarFromNameList().


In the regression test, you are using \gset and \connect psql meta-commands to test
the user privilege to a namespace, but I think we can make this more simpler 
by using SET SESSION AUTHORIZATION and RESET AUTHORIZATION.

Regards,

-- 
Yugo Nagata <nagata@sraoss.co.jp>

Hello.

At Thu, 14 Mar 2019 13:37:00 +0900, Takuma Hoshiai <hoshiai@sraoss.co.jp> wrote in
<20190314133700.c271429ddc00ddab3aac2619@sraoss.co.jp>
> Hi, hackers,
> 
> According to the document, "to_reg* functions return null rather than
> throwing an error if the name is not found", but this is not the case
> if the arguments to those functions are schema qualified and the
> caller does not have access permission of the schema even if the table
> (or other object) does exist -- we get an error.

You explicitly specified the namespace and I think that it is not
the case of not-found. It is right that the error happens since
you explicitly tried to access a unprivileged schema.

> For example, to_regclass() throws an error if its argument is
> 'schema_name.table_name'' (i.e. contains schema name) and caller's
> role doesn't have access permission of the schema. Same thing can be
> said to Other functions,
> 
> I get complain from Pgpool-II users because it uses to_regclass()
> internally to confirm table's existence but in the case above it's
> not useful because the error aborts user's transaction.

I'm not sure how such unaccessible table names are given to the
function there, but it is also natural that any user trying to
access unprivileged objects gets an error.

> To be more consistent with the doc and to make those functions more
> useful, I propose to change current implementation so that they do not
> throw an error if the name space cannot be accessible by the caller.

Since it doesn't seem a bug, I think that changing the existing
behavior is not acceptable. Maybe we can live with another
signature of the function like to_regproc(name text, noerror
bool).

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

>> According to the document, "to_reg* functions return null rather than
>> throwing an error if the name is not found", but this is not the case
>> if the arguments to those functions are schema qualified and the
>> caller does not have access permission of the schema even if the table
>> (or other object) does exist -- we get an error.
> 
> You explicitly specified the namespace and I think that it is not
> the case of not-found. It is right that the error happens since
> you explicitly tried to access a unprivileged schema.
> 
>> For example, to_regclass() throws an error if its argument is
>> 'schema_name.table_name'' (i.e. contains schema name) and caller's
>> role doesn't have access permission of the schema. Same thing can be
>> said to Other functions,
>> 
>> I get complain from Pgpool-II users because it uses to_regclass()
>> internally to confirm table's existence but in the case above it's
>> not useful because the error aborts user's transaction.
> 
> I'm not sure how such unaccessible table names are given to the
> function there, but it is also natural that any user trying to
> access unprivileged objects gets an error.

You misunderstand the functionality of to_regclass(). Even if a user
does not have an access privilege of certain table, to_regclass() does
not raise an error.

test=> select * from t1;
ERROR:  permission denied for table t1

test=> select to_regclass('t1')::oid;
 to_regclass 
-------------
     1647238
(1 row)

So why can't we do the same thing for schema? For me, that way seems
to be more consistent.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

At Tue, 19 Mar 2019 16:35:32 +0900 (JST), Tatsuo Ishii <ishii@sraoss.co.jp> wrote in
<20190319.163532.529526338176696856.t-ishii@sraoss.co.jp>
> >> According to the document, "to_reg* functions return null rather than
> >> throwing an error if the name is not found", but this is not the case
> >> if the arguments to those functions are schema qualified and the
> >> caller does not have access permission of the schema even if the table
> >> (or other object) does exist -- we get an error.
> > 
> > You explicitly specified the namespace and I think that it is not
> > the case of not-found. It is right that the error happens since
> > you explicitly tried to access a unprivileged schema.
> > 
> >> For example, to_regclass() throws an error if its argument is
> >> 'schema_name.table_name'' (i.e. contains schema name) and caller's
> >> role doesn't have access permission of the schema. Same thing can be
> >> said to Other functions,
> >> 
> >> I get complain from Pgpool-II users because it uses to_regclass()
> >> internally to confirm table's existence but in the case above it's
> >> not useful because the error aborts user's transaction.
> > 
> > I'm not sure how such unaccessible table names are given to the
> > function there, but it is also natural that any user trying to
> > access unprivileged objects gets an error.
> 
> You misunderstand the functionality of to_regclass(). Even if a user
> does not have an access privilege of certain table, to_regclass() does
> not raise an error.
>
> test=> select * from t1;
> ERROR:  permission denied for table t1
> 
> test=> select to_regclass('t1')::oid;
>  to_regclass 
> -------------
>      1647238
> (1 row)
> 
> So why can't we do the same thing for schema? For me, that way seems
> to be more consistent.

It seems to be a different thing. The oid 1647239 would be a
table in public schema or any schema that the user has access
to. If search_path contained only unprivileged schemas, the
function silently ignores such schemas.

=> set search_path to s1;       -- the user doesn't have access to this schema.
=> select to_regclass('t1')::oid; -- the table is really exists.
> to_regclass 
> -------------
>  
> (1 row)

Superuser gets the exepcted result.

=#  set search_path to s1;
=# select to_regclass('t1')::oid; -- superuser has access to s1.
>  to_regclass 
> -------------
>        87612
> (1 row)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

>> You misunderstand the functionality of to_regclass(). Even if a user
>> does not have an access privilege of certain table, to_regclass() does
>> not raise an error.
>>
>> test=> select * from t1;
>> ERROR:  permission denied for table t1
>> 
>> test=> select to_regclass('t1')::oid;
>>  to_regclass 
>> -------------
>>      1647238
>> (1 row)
>> 
>> So why can't we do the same thing for schema? For me, that way seems
>> to be more consistent.
> 
> It seems to be a different thing. The oid 1647239 would be a
> table in public schema or any schema that the user has access
> to. If search_path contained only unprivileged schemas, the
> function silently ignores such schemas.
> 
> => set search_path to s1;       -- the user doesn't have access to this schema.
> => select to_regclass('t1')::oid; -- the table is really exists.
>> to_regclass 
>> -------------
>>  
>> (1 row)

I (and Hoshiai-san) concern about following case:

# revoke usage on schema s1 from foo;
REVOKE
:
[connect as foo]
test=> select to_regclass('s1.t1')::oid;
ERROR:  permission denied for schema s1

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

At Tue, 19 Mar 2019 17:54:01 +0900 (JST), Tatsuo Ishii <ishii@sraoss.co.jp> wrote in
<20190319.175401.646838939186238443.t-ishii@sraoss.co.jp>
> > It seems to be a different thing. The oid 1647239 would be a
> > table in public schema or any schema that the user has access
> > to. If search_path contained only unprivileged schemas, the
> > function silently ignores such schemas.
> > 
> > => set search_path to s1;       -- the user doesn't have access to this schema.
> > => select to_regclass('t1')::oid; -- the table is really exists.
> >> to_regclass 
> >> -------------
> >>  
> >> (1 row)
> 
> I (and Hoshiai-san) concern about following case:
> 
> # revoke usage on schema s1 from foo;
> REVOKE
> :
> [connect as foo]
> test=> select to_regclass('s1.t1')::oid;
> ERROR:  permission denied for schema s1

That works in a transaction. It looks right that the actually
revoked schema cannot be accessed.

S1:foo: begin;
S2:su : revoke usage on schema s1 from foo;
S1:foo: select to_regclass('s1.t1')::oid;
>  to_regclass 
> -------------
>        16418
S2:foo: commit;
S2:foo: select to_regclass('s1.t1')::oid;
> ERROR:  permission denied for schema s1

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

>> I (and Hoshiai-san) concern about following case:
>> 
>> # revoke usage on schema s1 from foo;
>> REVOKE
>> :
>> [connect as foo]
>> test=> select to_regclass('s1.t1')::oid;
>> ERROR:  permission denied for schema s1
> 
> That works in a transaction. It looks right that the actually
> revoked schema cannot be accessed.
> 
> S1:foo: begin;
> S2:su : revoke usage on schema s1 from foo;
> S1:foo: select to_regclass('s1.t1')::oid;
>>  to_regclass 
>> -------------
>>        16418
> S2:foo: commit;
> S2:foo: select to_regclass('s1.t1')::oid;
>> ERROR:  permission denied for schema s1

I'm confused. How is an explicit transaction related to the topic?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

At Tue, 19 Mar 2019 19:09:59 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20190319.190959.25783254.horiguchi.kyotaro@lab.ntt.co.jp>
> That works in a transaction. It looks right that the actually
> revoked schema cannot be accessed.

From another viewpoint, the behavior really doesn't protect nothing. The unprivileged user still can do that as the
follows.

=> select to_regclass('s1.t1')::oid;
ERROR:  permission denied for schema s1
=> select c.oid from pg_class c join pg_namespace n on c.relnamespace = n.oid where n.nspname = 's1' and c.relname =
't1';
  oid  
-------
 16418
(1 row)

So, couldn't we just ignore the privilege there?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Wed, 20 Mar 2019 07:13:28 +0900 (JST), Tatsuo Ishii <ishii@sraoss.co.jp> wrote in
<20190320.071328.485760446856666486.t-ishii@sraoss.co.jp>
> >> I (and Hoshiai-san) concern about following case:
> >> 
> >> # revoke usage on schema s1 from foo;
> >> REVOKE
> >> :
> >> [connect as foo]
> >> test=> select to_regclass('s1.t1')::oid;
> >> ERROR:  permission denied for schema s1
> > 
> > That works in a transaction. It looks right that the actually
> > revoked schema cannot be accessed.
> > 
> > S1:foo: begin;
> > S2:su : revoke usage on schema s1 from foo;
> > S1:foo: select to_regclass('s1.t1')::oid;
> >>  to_regclass 
> >> -------------
> >>        16418
> > S2:foo: commit;
> > S2:foo: select to_regclass('s1.t1')::oid;
> >> ERROR:  permission denied for schema s1
> 
> I'm confused. How is an explicit transaction related to the topic?

Since your example revokes the privilege just before (or
simultaneously with) "using" the unprivileged object. If the
given object name is obtained before the revokation, it can be
protected by beginning a transaction before obtaining the
name. If not, it is right to emit an error.

As another discussion, as I wrote just before, can be raised that
the behavior really doesn't protect nothing. We can lookup the
oid of an unprivileged objects through the system catalogs.

So I think it is reasonable that we just ignore privileges in the
commands. Maybe regclassin and friends also should be changed in
the same way.

If we protect system catalogs later, the commands naturally will
follow the change.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, 20 Mar 2019 09:48:59 +0900 (Tokyo Standard Time)
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:

> At Wed, 20 Mar 2019 07:13:28 +0900 (JST), Tatsuo Ishii <ishii@sraoss.co.jp> wrote in
<20190320.071328.485760446856666486.t-ishii@sraoss.co.jp>
> > >> I (and Hoshiai-san) concern about following case:
> > >> 
> > >> # revoke usage on schema s1 from foo;
> > >> REVOKE
> > >> :
> > >> [connect as foo]
> > >> test=> select to_regclass('s1.t1')::oid;
> > >> ERROR:  permission denied for schema s1
> > > 
> > > That works in a transaction. It looks right that the actually
> > > revoked schema cannot be accessed.
> > > 
> > > S1:foo: begin;
> > > S2:su : revoke usage on schema s1 from foo;
> > > S1:foo: select to_regclass('s1.t1')::oid;
> > >>  to_regclass 
> > >> -------------
> > >>        16418
> > > S2:foo: commit;
> > > S2:foo: select to_regclass('s1.t1')::oid;
> > >> ERROR:  permission denied for schema s1
> > 
> > I'm confused. How is an explicit transaction related to the topic?
> 
> Since your example revokes the privilege just before (or
> simultaneously with) "using" the unprivileged object. If the
> given object name is obtained before the revokation, it can be
> protected by beginning a transaction before obtaining the
> name. If not, it is right to emit an error.

What we want to say below is 'foo' has no privilege. not important to execute REVOKE.
> # revoke usage on schema s1 from foo;
> REVOKE
> :
> [connect as foo]
> test=> select to_regclass('s1.t1')::oid;
> ERROR:  permission denied for schema s1

> As another discussion, as I wrote just before, can be raised that
> the behavior really doesn't protect nothing. We can lookup the
> oid of an unprivileged objects through the system catalogs.
> 
> So I think it is reasonable that we just ignore privileges in the
> commands. Maybe regclassin and friends also should be changed in
> the same way.

Yes, I think so too. 
But their functions may be used for confirming a obejct visibility, so this time 
I want to supress errors only. 
And if want to raise  an error about "permission denied for schema xx", 
would use regclass() function.


best regards,

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

> If we protect system catalogs later, the commands naturally will
> follow the change.
> 
> regards.
> 
> -- 
> Kyotaro Horiguchi
> NTT Open Source Software Center
> 
> 

Hi Nagata-san,

sorry for te late reply.
Thank you for your comments, I have attached a patch that reflected it.

On Tue, 19 Mar 2019 15:13:04 +0900
Yugo Nagata <nagata@sraoss.co.jp> wrote:

> I reviewed the patch. Here are some comments:
> 
>  /*
> + * RangeVarCheckNamespaceAccessNoError
> + *         Returns true if given relation's namespace can be accessable by the
> + *         current user. If no namespace is given in the relation, just returns
> + *         true.
> + */
> +bool
> +RangeVarCheckNamespaceAccessNoError(RangeVar *relation)
> 
> Although it might be trivial, the new function's name 'RangeVar...' seems a bit
> weird to me because this is used for not only to_regclass but also to_regproc,
> to_regtype, and so on, that is, the argument "relation" is not always a relation. 
> 
> This function is used always with makeRangeVarFromNameList() as
> 
>   if (!RangeVarCheckNamespaceAccessNoError(makeRangeVarFromNameList(names)))
> 
> , so how about merging the two function as below, for example:
> 
>  /*
>   * CheckNamespaceAccessNoError
>   *         Returns true if the namespace in given qualified-name can be accessable
>   *         by the current user. If no namespace is given in the names, just returns
>   *         true.
>   */
>  bool
>  CheckNamespaceAccessNoError(List *names);
> 
> 
> BTW, this patch supresses also "Cross-database references is not allowed" error in
> addition to the namespace ACL error.  Is this an intentional change?  If this error
> can be allowed, you can use DeconstructQualifiedName() instead of
> makeRangeVarFromNameList().

I think it is enough to supress napesapace ACL error only. so I will use its function.

> In the regression test, you are using \gset and \connect psql meta-commands to test
> the user privilege to a namespace, but I think we can make this more simpler 
> by using SET SESSION AUTHORIZATION and RESET AUTHORIZATION.

I forgot this SQL, I fixed to use it.

Best regards,

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

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

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>

On Wed, Jul 31, 2019 at 4:24 AM 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:

...

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

Hello Hoshiai-san,

Based on the above review, I have set this to 'Returned with
feedback'.  If you plan to produce a new patch in time for the next
Commitfest in September, please let me know very soon and I'll change
it to 'Moved to next CF', but otherwise please feel free to create a
new entry when you are ready.

Thanks!

-- 
Thomas Munro
https://enterprisedb.com

On Thu, 1 Aug 2019 20:21:57 +1200
Thomas Munro <thomas.munro@gmail.com> wrote:

> On Wed, Jul 31, 2019 at 4:24 AM 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:
> 
> ...
> 
> > So I think you ought to drop this implementation and fix it properly
> > by adjusting the behaviors of the functions cited above.
> 
> Hello Hoshiai-san,
> 
> Based on the above review, I have set this to 'Returned with
> feedback'.  If you plan to produce a new patch in time for the next
> Commitfest in September, please let me know very soon and I'll change
> it to 'Moved to next CF', but otherwise please feel free to create a
> new entry when you are ready.

Yes, I plan to create next patch, Would you change it to 'Moved to next CF'?

Best Regards,

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


> Thanks!
> 
> -- 
> Thomas Munro
> https://enterprisedb.com
> 

On Thu, Aug 1, 2019 at 8:41 PM Takuma Hoshiai <hoshiai@sraoss.co.jp> wrote:
> On Thu, 1 Aug 2019 20:21:57 +1200
> Thomas Munro <thomas.munro@gmail.com> wrote:
> > Based on the above review, I have set this to 'Returned with
> > feedback'.  If you plan to produce a new patch in time for the next
> > Commitfest in September, please let me know very soon and I'll change
> > it to 'Moved to next CF', but otherwise please feel free to create a
> > new entry when you are ready.
>
> Yes, I plan to create next patch, Would you change it to 'Moved to next CF'?

Done!  Thanks.

-- 
Thomas Munro
https://enterprisedb.com

Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Aug 1, 2019 at 8:41 PM Takuma Hoshiai <hoshiai@sraoss.co.jp> wrote:
>> On Thu, 1 Aug 2019 20:21:57 +1200
>> Thomas Munro <thomas.munro@gmail.com> wrote:
>>> Based on the above review, I have set this to 'Returned with
>>> feedback'.  If you plan to produce a new patch in time for the next
>>> Commitfest in September, please let me know very soon and I'll change
>>> it to 'Moved to next CF', but otherwise please feel free to create a
>>> new entry when you are ready.

>> Yes, I plan to create next patch, Would you change it to 'Moved to next CF'?

> Done!  Thanks.

No new patch has appeared, so I'm not sure why this was in "Needs review"
state.  I've set it to "Waiting on Author".

            regards, tom lane