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

Hi Takuma,

On Thu, 14 Mar 2019 13:37:00 +0900
Takuma Hoshiai <hoshiai(at)sraoss(dot)co(dot)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(at)sraoss(dot)co(dot)jp>