Proposal to suppress errors thrown by to_reg*()

Started by Takuma Hoshiaialmost 7 years ago19 messages

hoshiai@sraoss.co.jp

almost 7 years ago

1 attachment(s)

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>

Attachments:

fix_to_reg.patchapplication/octet-stream; name=fix_to_reg.patchDownload

diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index cdd5006..e4c9a4f 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -664,6 +664,39 @@ RangeVarAdjustRelationPersistence(RangeVar *newRelation, Oid nspid)
 }
 
 /*
+ * 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)
+{
+	Oid namespaceId;
+	AclResult	aclresult;
+
+	if (relation->catalogname)
+	{
+		if (strcmp(relation->catalogname, get_database_name(MyDatabaseId)) != 0)
+			/* Cross-database references is not allowed */
+			return false;
+	}
+	if (relation->schemaname)
+	{
+		namespaceId = get_namespace_oid(relation->schemaname, true);
+		if (!OidIsValid(namespaceId))
+			/* Namespace is invalid */
+			return false;
+		aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(), ACL_USAGE);
+		if (aclresult != ACLCHECK_OK)
+			/* Namespace ACL is not ok */
+			return false;
+	}
+
+	return true;
+}
+
+/*
  * RelnameGetRelid
  *		Try to resolve an unqualified relation name.
  *		Returns OID if relation found in search path, else InvalidOid.
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 09cf0e1..6828633 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -128,6 +128,8 @@ to_regproc(PG_FUNCTION_ARGS)
 	 * entries in the current search path.
 	 */
 	names = stringToQualifiedNameList(pro_name);
+	if (!RangeVarCheckNamespaceAccessNoError(makeRangeVarFromNameList(names)))
+		PG_RETURN_NULL();
 	clist = FuncnameGetCandidates(names, -1, NIL, false, false, true);
 
 	if (clist == NULL || clist->next != NULL)
@@ -301,6 +303,8 @@ to_regprocedure(PG_FUNCTION_ARGS)
 	 * given argument types.    (There will not be more than one match.)
 	 */
 	parseNameAndArgTypes(pro_name, false, &names, &nargs, argtypes);
+	if (!RangeVarCheckNamespaceAccessNoError(makeRangeVarFromNameList(names)))
+		PG_RETURN_NULL();
 
 	clist = FuncnameGetCandidates(names, nargs, NIL, false, false, true);
 
@@ -546,6 +550,8 @@ to_regoper(PG_FUNCTION_ARGS)
 	 * entries in the current search path.
 	 */
 	names = stringToQualifiedNameList(opr_name);
+	if (!RangeVarCheckNamespaceAccessNoError(makeRangeVarFromNameList(names)))
+		PG_RETURN_NULL();
 	clist = OpernameGetCandidates(names, '\0', true);
 
 	if (clist == NULL || clist->next != NULL)
@@ -736,6 +742,8 @@ to_regoperator(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_TOO_MANY_ARGUMENTS),
 				 errmsg("too many arguments"),
 				 errhint("Provide two argument types for operator.")));
+	if (!RangeVarCheckNamespaceAccessNoError(makeRangeVarFromNameList(names)))
+		PG_RETURN_NULL();
 
 	result = OpernameGetOprid(names, argtypes[0], argtypes[1]);
 
@@ -948,16 +956,18 @@ to_regclass(PG_FUNCTION_ARGS)
 {
 	char	   *class_name = text_to_cstring(PG_GETARG_TEXT_PP(0));
 	Oid			result;
-	List	   *names;
+	RangeVar	*names;
 
 	/*
 	 * Parse the name into components and see if it matches any pg_class
 	 * entries in the current search path.
 	 */
-	names = stringToQualifiedNameList(class_name);
+	names = makeRangeVarFromNameList(stringToQualifiedNameList(class_name));
+	if (!RangeVarCheckNamespaceAccessNoError(names))
+		PG_RETURN_NULL();
 
 	/* We might not even have permissions on this relation; don't lock it. */
-	result = RangeVarGetRelid(makeRangeVarFromNameList(names), NoLock, true);
+	result = RangeVarGetRelid(names, NoLock, true);
 
 	if (OidIsValid(result))
 		PG_RETURN_OID(result);
@@ -1104,10 +1114,14 @@ to_regtype(PG_FUNCTION_ARGS)
 	char	   *typ_name = text_to_cstring(PG_GETARG_TEXT_PP(0));
 	Oid			result;
 	int32		typmod;
+	List	   *names;
 
 	/*
 	 * Invoke the full parser to deal with special cases such as array syntax.
 	 */
+	names = stringToQualifiedNameList(typ_name);
+	if (!RangeVarCheckNamespaceAccessNoError(makeRangeVarFromNameList(names)))
+		PG_RETURN_NULL();
 	parseTypeString(typ_name, &result, &typmod, true);
 
 	if (OidIsValid(result))
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index 6741834..ff3b065 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -73,6 +73,7 @@ extern Oid RangeVarGetAndCheckCreationNamespace(RangeVar *newRelation,
 									 LOCKMODE lockmode,
 									 Oid *existing_relation_id);
 extern void RangeVarAdjustRelationPersistence(RangeVar *newRelation, Oid nspid);
+extern bool RangeVarCheckNamespaceAccessNoError(RangeVar *relation);
 extern Oid	RelnameGetRelid(const char *relname);
 extern bool RelationIsVisible(Oid relid);
 
diff --git a/src/test/regress/expected/regproc.out b/src/test/regress/expected/regproc.out
index ee4fcda..ccadae4 100644
--- a/src/test/regress/expected/regproc.out
+++ b/src/test/regress/expected/regproc.out
@@ -395,3 +395,65 @@ SELECT to_regnamespace('"Nonexistent"');
 
 SELECT to_regnamespace('foo.bar');
 ERROR:  invalid name syntax
+/* If objects exist, and user don't have USAGE privilege, return NULL with no error. */
+SELECT current_user AS username
+\gset
+CREATE USER test_usr;
+CREATE SCHEMA test_schema;
+REVOKE ALL ON SCHEMA test_schema FROM test_usr;
+CREATE OPERATOR  test_schema.+ (
+	leftarg = int8,
+	rightarg = int8,
+	procedure = int8pl
+);
+CREATE OR REPLACE FUNCTION test_schema.test_func(int)
+RETURNS INTEGER AS
+	'SELECT 1;'
+LANGUAGE sql;
+CREATE TABLE test_schema.test_tbl(id int);
+CREATE TYPE test_schema.test_type AS (a1 int,a2 int);
+\connect - test_usr
+SELECT to_regoper('test_schema.+');
+ to_regoper 
+------------
+ 
+(1 row)
+
+SELECT to_regoperator('test_schema.+(int8,int8)');
+ to_regoperator 
+----------------
+ 
+(1 row)
+
+SELECT to_regproc('test_schema.test_func');
+ to_regproc 
+------------
+ 
+(1 row)
+
+SELECT to_regprocedure('test_schema.test_func(int)');
+ to_regprocedure 
+-----------------
+ 
+(1 row)
+
+SELECT to_regclass('test_schema.test_tbl');
+ to_regclass 
+-------------
+ 
+(1 row)
+
+SELECT to_regtype('test_schema.test_type');
+ to_regtype 
+------------
+ 
+(1 row)
+
+\connect - :username
+DROP SCHEMA test_schema CASCADE;
+NOTICE:  drop cascades to 4 other objects
+DETAIL:  drop cascades to operator test_schema.+(bigint,bigint)
+drop cascades to function test_schema.test_func(integer)
+drop cascades to table test_schema.test_tbl
+drop cascades to type test_schema.test_type
+DROP ROLE test_usr;
diff --git a/src/test/regress/sql/regproc.sql b/src/test/regress/sql/regproc.sql
index a60bc28..897f181 100644
--- a/src/test/regress/sql/regproc.sql
+++ b/src/test/regress/sql/regproc.sql
@@ -113,3 +113,37 @@ SELECT to_regrole('foo.bar');
 SELECT to_regnamespace('Nonexistent');
 SELECT to_regnamespace('"Nonexistent"');
 SELECT to_regnamespace('foo.bar');
+
+/* If objects exist, and user don't have USAGE privilege, return NULL with no error. */
+
+SELECT current_user AS username
+\gset
+
+CREATE USER test_usr;
+CREATE SCHEMA test_schema;
+REVOKE ALL ON SCHEMA test_schema FROM test_usr;
+
+CREATE OPERATOR  test_schema.+ (
+	leftarg = int8,
+	rightarg = int8,
+	procedure = int8pl
+);
+CREATE OR REPLACE FUNCTION test_schema.test_func(int)
+RETURNS INTEGER AS
+	'SELECT 1;'
+LANGUAGE sql;
+CREATE TABLE test_schema.test_tbl(id int);
+CREATE TYPE test_schema.test_type AS (a1 int,a2 int);
+
+\connect - test_usr
+
+SELECT to_regoper('test_schema.+');
+SELECT to_regoperator('test_schema.+(int8,int8)');
+SELECT to_regproc('test_schema.test_func');
+SELECT to_regprocedure('test_schema.test_func(int)');
+SELECT to_regclass('test_schema.test_tbl');
+SELECT to_regtype('test_schema.test_type');
+
+\connect - :username
+DROP SCHEMA test_schema CASCADE;
+DROP ROLE test_usr;

Yugo Nagata

nagata@sraoss.co.jp

almost 7 years ago

In reply to: Takuma Hoshiai (#1)

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

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>

Kyotaro HORIGUCHI

horiguchi.kyotaro@lab.ntt.co.jp

almost 7 years ago

In reply to: Takuma Hoshiai (#1)

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

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

Tatsuo Ishii

ishii@sraoss.co.jp

almost 7 years ago

In reply to: Kyotaro HORIGUCHI (#3)

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

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

Kyotaro HORIGUCHI

horiguchi.kyotaro@lab.ntt.co.jp

almost 7 years ago

In reply to: Tatsuo Ishii (#4)

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

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

Tatsuo Ishii

ishii@sraoss.co.jp

almost 7 years ago

In reply to: Kyotaro HORIGUCHI (#5)

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

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

Kyotaro HORIGUCHI

horiguchi.kyotaro@lab.ntt.co.jp

almost 7 years ago

In reply to: Tatsuo Ishii (#6)

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

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

Tatsuo Ishii

ishii@sraoss.co.jp

almost 7 years ago

In reply to: Kyotaro HORIGUCHI (#7)

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

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

Kyotaro HORIGUCHI

horiguchi.kyotaro@lab.ntt.co.jp

almost 7 years ago

In reply to: Kyotaro HORIGUCHI (#7)

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

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

#10

Kyotaro HORIGUCHI

horiguchi.kyotaro@lab.ntt.co.jp

almost 7 years ago

In reply to: Tatsuo Ishii (#8)

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

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

#11

Takuma Hoshiai

hoshiai@sraoss.co.jp

almost 7 years ago

In reply to: Kyotaro HORIGUCHI (#10)

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

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>

Show quoted text

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

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#12

Pavel Stehule

pavel.stehule@gmail.com

almost 7 years ago

In reply to: Takuma Hoshiai (#11)

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

st 20. 3. 2019 v 5:55 odesílatel Takuma Hoshiai <hoshiai@sraoss.co.jp>
napsal:

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.

Pavel

Show quoted text

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

#13

Takuma Hoshiai

hoshiai@sraoss.co.jp

almost 7 years ago

In reply to: Yugo Nagata (#2)

1 attachment(s)

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

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>

Attachments:

fix_to_reg_v2.patchapplication/octet-stream; name=fix_to_reg_v2.patchDownload

diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index cdd5006..e2d7447 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -664,6 +664,38 @@ RangeVarAdjustRelationPersistence(RangeVar *newRelation, Oid nspid)
 }
 
 /*
+ * CheckNamespaceAccessNoError
+ * 		Returns true if the namespace in given qualified-name can be accessable
+ * 		by the current user. If no namespace is given in names, just returns
+ * 		true.
+ */
+bool
+CheckNamespaceAccessNoError(List *names)
+{
+	char *namespacename;
+	char *objectname;
+	Oid namespaceId;
+	AclResult	aclresult;
+
+	/* deconstruct the name list */
+	DeconstructQualifiedName(names, &namespacename, &objectname);
+
+	if (namespacename)
+	{
+		namespaceId = get_namespace_oid(namespacename, true);
+		if (!OidIsValid(namespaceId))
+			/* Namespace is invalid */
+			return false;
+		aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(), ACL_USAGE);
+		if (aclresult != ACLCHECK_OK)
+			/* Namespace ACL is not ok */
+			return false;
+	}
+
+	return true;
+}
+
+/*
  * RelnameGetRelid
  *		Try to resolve an unqualified relation name.
  *		Returns OID if relation found in search path, else InvalidOid.
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 09cf0e1..ab9ebad 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -128,6 +128,8 @@ to_regproc(PG_FUNCTION_ARGS)
 	 * entries in the current search path.
 	 */
 	names = stringToQualifiedNameList(pro_name);
+	if (!CheckNamespaceAccessNoError(names))
+		PG_RETURN_NULL();
 	clist = FuncnameGetCandidates(names, -1, NIL, false, false, true);
 
 	if (clist == NULL || clist->next != NULL)
@@ -301,6 +303,8 @@ to_regprocedure(PG_FUNCTION_ARGS)
 	 * given argument types.    (There will not be more than one match.)
 	 */
 	parseNameAndArgTypes(pro_name, false, &names, &nargs, argtypes);
+	if (!CheckNamespaceAccessNoError(names))
+		PG_RETURN_NULL();
 
 	clist = FuncnameGetCandidates(names, nargs, NIL, false, false, true);
 
@@ -546,6 +550,8 @@ to_regoper(PG_FUNCTION_ARGS)
 	 * entries in the current search path.
 	 */
 	names = stringToQualifiedNameList(opr_name);
+	if (!CheckNamespaceAccessNoError(names))
+		PG_RETURN_NULL();
 	clist = OpernameGetCandidates(names, '\0', true);
 
 	if (clist == NULL || clist->next != NULL)
@@ -736,6 +742,8 @@ to_regoperator(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_TOO_MANY_ARGUMENTS),
 				 errmsg("too many arguments"),
 				 errhint("Provide two argument types for operator.")));
+	if (!CheckNamespaceAccessNoError(names))
+		PG_RETURN_NULL();
 
 	result = OpernameGetOprid(names, argtypes[0], argtypes[1]);
 
@@ -948,13 +956,15 @@ to_regclass(PG_FUNCTION_ARGS)
 {
 	char	   *class_name = text_to_cstring(PG_GETARG_TEXT_PP(0));
 	Oid			result;
-	List	   *names;
+	List		*names;
 
 	/*
 	 * Parse the name into components and see if it matches any pg_class
 	 * entries in the current search path.
 	 */
 	names = stringToQualifiedNameList(class_name);
+	if (!CheckNamespaceAccessNoError(names))
+		PG_RETURN_NULL();
 
 	/* We might not even have permissions on this relation; don't lock it. */
 	result = RangeVarGetRelid(makeRangeVarFromNameList(names), NoLock, true);
@@ -1104,10 +1114,14 @@ to_regtype(PG_FUNCTION_ARGS)
 	char	   *typ_name = text_to_cstring(PG_GETARG_TEXT_PP(0));
 	Oid			result;
 	int32		typmod;
+	List	   *names;
 
 	/*
 	 * Invoke the full parser to deal with special cases such as array syntax.
 	 */
+	names = stringToQualifiedNameList(typ_name);
+	if (!CheckNamespaceAccessNoError(names))
+		PG_RETURN_NULL();
 	parseTypeString(typ_name, &result, &typmod, true);
 
 	if (OidIsValid(result))
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index 6741834..5507da3 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -73,6 +73,7 @@ extern Oid RangeVarGetAndCheckCreationNamespace(RangeVar *newRelation,
 									 LOCKMODE lockmode,
 									 Oid *existing_relation_id);
 extern void RangeVarAdjustRelationPersistence(RangeVar *newRelation, Oid nspid);
+extern bool CheckNamespaceAccessNoError(List *names);
 extern Oid	RelnameGetRelid(const char *relname);
 extern bool RelationIsVisible(Oid relid);
 
diff --git a/src/test/regress/expected/regproc.out b/src/test/regress/expected/regproc.out
index ee4fcda..bb00b31 100644
--- a/src/test/regress/expected/regproc.out
+++ b/src/test/regress/expected/regproc.out
@@ -395,3 +395,63 @@ SELECT to_regnamespace('"Nonexistent"');
 
 SELECT to_regnamespace('foo.bar');
 ERROR:  invalid name syntax
+/* If objects exist, and user don't have USAGE privilege, return NULL with no error. */
+CREATE USER test_usr;
+CREATE SCHEMA test_schema;
+REVOKE ALL ON SCHEMA test_schema FROM test_usr;
+CREATE OPERATOR  test_schema.+ (
+	leftarg = int8,
+	rightarg = int8,
+	procedure = int8pl
+);
+CREATE OR REPLACE FUNCTION test_schema.test_func(int)
+RETURNS INTEGER AS
+	'SELECT 1;'
+LANGUAGE sql;
+CREATE TABLE test_schema.test_tbl(id int);
+CREATE TYPE test_schema.test_type AS (a1 int,a2 int);
+SET SESSION AUTHORIZATION test_usr;
+SELECT to_regoper('test_schema.+');
+ to_regoper 
+------------
+ 
+(1 row)
+
+SELECT to_regoperator('test_schema.+(int8,int8)');
+ to_regoperator 
+----------------
+ 
+(1 row)
+
+SELECT to_regproc('test_schema.test_func');
+ to_regproc 
+------------
+ 
+(1 row)
+
+SELECT to_regprocedure('test_schema.test_func(int)');
+ to_regprocedure 
+-----------------
+ 
+(1 row)
+
+SELECT to_regclass('test_schema.test_tbl');
+ to_regclass 
+-------------
+ 
+(1 row)
+
+SELECT to_regtype('test_schema.test_type');
+ to_regtype 
+------------
+ 
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+DROP SCHEMA test_schema CASCADE;
+NOTICE:  drop cascades to 4 other objects
+DETAIL:  drop cascades to operator test_schema.+(bigint,bigint)
+drop cascades to function test_schema.test_func(integer)
+drop cascades to table test_schema.test_tbl
+drop cascades to type test_schema.test_type
+DROP ROLE test_usr;
diff --git a/src/test/regress/sql/regproc.sql b/src/test/regress/sql/regproc.sql
index a60bc28..53e6c36 100644
--- a/src/test/regress/sql/regproc.sql
+++ b/src/test/regress/sql/regproc.sql
@@ -113,3 +113,33 @@ SELECT to_regrole('foo.bar');
 SELECT to_regnamespace('Nonexistent');
 SELECT to_regnamespace('"Nonexistent"');
 SELECT to_regnamespace('foo.bar');
+
+/* If objects exist, and user don't have USAGE privilege, return NULL with no error. */
+
+CREATE USER test_usr;
+CREATE SCHEMA test_schema;
+REVOKE ALL ON SCHEMA test_schema FROM test_usr;
+
+CREATE OPERATOR  test_schema.+ (
+	leftarg = int8,
+	rightarg = int8,
+	procedure = int8pl
+);
+CREATE OR REPLACE FUNCTION test_schema.test_func(int)
+RETURNS INTEGER AS
+	'SELECT 1;'
+LANGUAGE sql;
+CREATE TABLE test_schema.test_tbl(id int);
+CREATE TYPE test_schema.test_type AS (a1 int,a2 int);
+
+SET SESSION AUTHORIZATION test_usr;
+SELECT to_regoper('test_schema.+');
+SELECT to_regoperator('test_schema.+(int8,int8)');
+SELECT to_regproc('test_schema.test_func');
+SELECT to_regprocedure('test_schema.test_func(int)');
+SELECT to_regclass('test_schema.test_tbl');
+SELECT to_regtype('test_schema.test_type');
+RESET SESSION AUTHORIZATION;
+
+DROP SCHEMA test_schema CASCADE;
+DROP ROLE test_usr;

#14

Tom Lane

tgl@sss.pgh.pa.us

over 6 years ago

In reply to: Takuma Hoshiai (#13)

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

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

#15

Takuma Hoshiai

hoshiai@sraoss.co.jp

over 6 years ago

In reply to: Tom Lane (#14)

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

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>

#16

Thomas Munro

thomas.munro@gmail.com

over 6 years ago

In reply to: Tom Lane (#14)

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

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

#17

Takuma Hoshiai

hoshiai@sraoss.co.jp

over 6 years ago

In reply to: Thomas Munro (#16)

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

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>

Show quoted text

Thanks!

--
Thomas Munro
https://enterprisedb.com

#18

Thomas Munro

thomas.munro@gmail.com

over 6 years ago

In reply to: Takuma Hoshiai (#17)

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

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

#19

Tom Lane

tgl@sss.pgh.pa.us

over 6 years ago

In reply to: Thomas Munro (#18)

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

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