Date: Fri, 23 Feb 2024 15:30:55 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: pgsql-hackers@postgresql.org, Joe Conway <mail@joeconway.com>,
	Robert Haas <robertmhaas@gmail.com>
Subject: Re: MAINTAIN privilege -- what do we need to un-revert it?
Message-ID: <20240223213055.GA1805646@nathanxps13>
References: <d4ccaf3658cb3c281ec88c851a09733cd9482f22.camel@j-davis.com>
 <20240214190226.GA373350@nathanxps13>
 <4d497d82e4516037dc1082d5a62d199ea00d62e7.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4d497d82e4516037dc1082d5a62d199ea00d62e7.camel@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20240223213055.GA1805646%40nathanxps13>
Precedence: bulk

(Apologies in advance for anything I'm bringing up that we've already
covered somewhere else.)

On Fri, Feb 16, 2024 at 04:03:55PM -0800, Jeff Davis wrote:
> Note the changes in amcheck. It's creating functions and calling those
> functions from the comparators, and so the comparators need to set the
> search_path. I don't think that's terribly common, but does represent a
> behavior change and could break something.

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?  Or is amcheck similarly affected by search_path tricks?

>  void
>  InitializeSearchPath(void)
>  {
> +	/* Make the context we'll keep search path cache hashtable in */
> +	SearchPathCacheContext = AllocSetContextCreate(TopMemoryContext,
> +												   "search_path processing cache",
> +												   ALLOCSET_DEFAULT_SIZES);
> +
>  	if (IsBootstrapProcessingMode())
>  	{
>  		/*
> @@ -4739,11 +4744,6 @@ InitializeSearchPath(void)
>  	}
>  	else
>  	{
> -		/* Make the context we'll keep search path cache hashtable in */
> -		SearchPathCacheContext = AllocSetContextCreate(TopMemoryContext,
> -													   "search_path processing cache",
> -													   ALLOCSET_DEFAULT_SIZES);
> -

What is the purpose of this change?

> +	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.

> +/*
> + * Safe search path when executing code as the table owner, such as during
> + * maintenance operations.
> + */
> +#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.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com