Re: MAINTAIN privilege -- what do we need to un-revert it?

New version attached.

Do we need a documentation update here? If so, where would be a good
place?

On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote:
> Why is this change needed?  Is the idea to make amcheck follow the
> same
> rules as maintenance commands to encourage folks to set up index
> functions
> correctly?

amcheck is calling functions it defined, so in order to find those
functions it needs to set the right search path.

>
> What is the purpose of this [bootstrap-related] change?

DefineIndex() is called during bootstrap, and it's also a maintenance
command. I tried to handle the bootstrapping case, but I think it's
best to just guard it with a conditional. Done.

I also added Assert(!IsBootstrapProcessingMode()) in
assign_search_path().

> > +       SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH,
> > PGC_USERSET,
> > +                                       PGC_S_SESSION);
>
> I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new
> value
> for these.
> >

Did you have a particular concern about PGC_S_SESSION?

If it's less than PGC_S_SESSION, it won't work, because the caller's
SET command will override it, and the same manipulation is possible.

And I don't think we want it higher than PGC_S_SESSION, otherwise the
function can't set its own search_path, if needed.

> > +#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"
>
> Is including pg_temp actually safe?  I worry that a user could use
> their
> temporary schema to inject objects that would take the place of
> non-schema-qualified stuff in functions.

pg_temp cannot (currently) be excluded. If it is omitted from the
string, it will be placed *first* in the search_path, which is more
dangerous.

pg_temp does not take part in function or operator resolution, which
makes it safer than it first appears. There are potentially some risks
around tables, but it's not typical to access a table in a function
called as part of an index expression.

If we determine that pg_temp is actually unsafe to include, we need to
do something like what I proposed here:

https://www.postgresql.org/message-id/a6865db287596c9c6ea12bdd9de87216cb5e7902.camel@j-davis.com

before this change.

Regards,
Jeff Davis