Re: Fixes for missing schema qualifications

On Wed, Mar 14, 2018 at 10:50:38AM +0900, Michael Paquier wrote:
> On Sat, Mar 10, 2018 at 08:36:34AM +0000, Noah Misch wrote:
> > This qualifies some functions, but it leaves plenty of unqualified operators.
>
> Yeah, I know that, and i don't have a perfect reply to offer to you.
> There are a couple of methods that we could use to tackle that:
> 1) For functions, enforce search_path with a SET search_path =
> 'pg_catalog' command. However this has a performance impact.
> 2) Enforce operators qualification with operator(pg_catalog.foo). This
> has no impact on performance, but repeating that all over the place is
> rather ugly, particularly for psql's describe.c and tab-completion.c.
> 3) Tweak dynamically search_path before running a query:
> - Save the existing search_path value by issuing SHOW search_path.
> - Use ALWAYS_SECURE_SEARCH_PATH_SQL to enforce the path.
> - Set back search_path based on the previous value.
> This logic can happen in a dedicated wrapper, but this impacts
> performance as it requires extra round trips to the server.
>
> For information_schema.sql, we are talking about tweaking 12 functions.
> So I think that we could live with 2). To simplify user's life, we
> could also recommend just to users to issue a ALTER FUNCTION SET
> search_path to fix the problem for all functions, that's easier to
> digest.

For information_schema, I'd pick (1). Performance is not very important
there, and reading or editing code like this is painful:

(($2 OPERATOR(pg_catalog.-) 4) OPERATOR(pg_catalog.>>) 16) OPERATOR(pg_catalog.&) 65535

(If performance becomes important, one could implement a way to automatically
translate sql-language function source to fully-qualified SQL at CREATE
FUNCTION time or at plan time.)

> For the rest, which basically concerns psql, I have been thinking that
> actually using 2) would be the most painful approach, still something
> which does not impact the user experience, while 3) is easier to
> back-patch by minimizing the code footprint and avoids also any kind of
> future problems.

Dozens of psql queries call pg_*_is_visible functions, which need the
search_path pertinent for user-entered queries. By itself, (3) doesn't work
for such queries. Even if you implemented (2), using psql with a hostile
search_path would remain approximately hopeless. It's too hard for psql users
to write safe input. Thus, I'd be -1 on accepting (2) or a similarly-ugly
change in psql. Any proposal for schema qualification in psql faces stiff
competition from the alternative of doing nothing.

For src/test, I would change nothing. If tests malfunction in a hostile
database, that is not important. Keeping tests easy to add, modify and review
is more important.

nm