Message-ID: <6781cc79580c464a63fc0a1343637ed2b2b0cf09.camel@j-davis.com>
Subject: Re: Fix search_path for all maintenance commands
From: Jeff Davis <pgsql@j-davis.com>
To: Isaac Morland <isaac.morland@gmail.com>
Cc: pgsql-hackers@postgresql.org, Nathan Bossart <nathandbossart@gmail.com>,
  Robert Haas <robertmhaas@gmail.com>, Noah Misch <noah@leadboat.com>,
 "David G. Johnston" <david.g.johnston@gmail.com>, Greg Stark
 <stark@mit.edu>, Tom Lane <tgl@sss.pgh.pa.us>, GurjeetSingh
 <gurjeet@singh.im>
Date: Fri, 07 Jul 2023 08:42:01 -0700
In-Reply-To: <CAMsGm5cpsaCEvx-kYHLE28GFGMSZfewsAcHbR2KvtTmpM_9o0g@mail.gmail.com>
References: <e44327179e5c9015c8dda67351c04da552066017.camel@j-davis.com>
	 <b701501541a424111ffe47be055e0ccefefd7745.camel@j-davis.com>
	 <CAMsGm5cpsaCEvx-kYHLE28GFGMSZfewsAcHbR2KvtTmpM_9o0g@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu1 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/6781cc79580c464a63fc0a1343637ed2b2b0cf09.camel%40j-davis.com>
Precedence: bulk

Hi,

On Thu, 2023-07-06 at 23:22 -0400, Isaac Morland wrote:
> Maybe pg_upgrade could apply "SET search_path TO pg_catalog, pg_temp"
> to any function used in a functional index that doesn't have a
> search_path setting of its own?

I don't think we want to go down the road of trying to solve this at
upgrade time.

> Now I'm doing more reading and I'm worried about SET TIME ZONE (or
> more precisely, its absence) and maybe some other ones.

That's a good point that it's not limited to search_path, but
search_path is by far the biggest problem.

For one thing, functions affected by TimeZone or other GUCs are
typically marked STABLE, and can't be used in index expressions. Also,
search_path affects a lot more functions.
=C2=A0
> Change it so by default each function gets handled as if "SET
> search_path FROM CURRENT" was applied to it?

Yes, that's one idea, along with some syntax to get the old behavior
(inherit search_path at runtime) if you want.

It feels weird to make search_path too special in the syntax though. If
we want a general solution, we could do something like:

  CREATE FUNCTION ...
    [DEPENDS ON CONFIGURATION {NONE|{some_guc}[, ...]}]
    [CONFIGURATION IS {STATIC|DYNAMIC}]

where STATIC means "all of the GUC dependencies are SET FROM CURRENT
unless specified otherwise" and DYNAMIC means "all of the GUC
dependencies come from the session at runtime unless specified
otherwise".

The default would be "DEPENDS CONFIGURATION search_path CONFIGURATION
IS STATIC".

That would make search_path special only because, by default, every
function would depend on it. Which I think summarizes the reason
search_path really is special.

That also opens up opportunities to do other things we might want to
do:

  * have a compatibility GUC to set the default back to DYNAMIC
  * track other dependencies of functions better ("DEPENDS ON TABLE
...")
  * provide better error messages, like "can't use function xyz in
index expression because it depends on configuration parameter foo"
  * be more consistent about using STABLE to mean that the function
depends on a snapshot, rather than overloading it for GUC dependencies

The question is, given that the acute problem is search_path, do we
want to invent all of the syntax above? Are there other use cases for
it, or should we just focus on search_path?

>  That's what I do for all my functions (maybe hurting performance?).

It doesn't look cheap, although I think we could optimize it.

> If a view calls a function, shouldn't it be called in the context of
> the view's definer/owner?

Yeah, there are a bunch of problems along those lines. I don't know if
we can solve them all in one release.

> Is the fundamental problem that we now find ourselves wanting to do
> things that require different defaults to work smoothly? On some
> level I suspect we want lexical scoping, which is what most of us
> have in our programming languages, in the database; but the database
> has many elements of dynamic scoping,=C2=A0and changing that is both a
> compatibility break and requires significant changes in the way the
> database=C2=A0is designed.

Does that suggest another approach?

Regards,
	Jeff Davis