Обсуждение: [17] Special search_path names "!pg_temp" and "!pg_catalog"

The attached patch adds some special names to prevent pg_temp and/or
pg_catalog from being included implicitly.

This is a useful safety feature for functions that don't have any need
to search pg_temp.

The current (v16) recommendation is to include pg_temp last, which does
add to the safety, but it's confusing to *include* a namespace when
your intention is actually to *exclude* it, and it's also not
completely excluding pg_temp.

Although the syntax in the attached patch is not much friendlier, at
least it's clear that the intent is to exclude pg_temp. Furthermore, it
will be friendlier if we adopt the SEARCH SYSTEM syntax proposed in
another thread[1].

Additionally, this patch adds a WARNING when creating a schema that
uses one of these special names. Previously, there was no warning when
creating a schema with the name "$user", which could cause confusion.

[1]
https://www.postgresql.org/message-id/flat/2710f56add351a1ed553efb677408e51b060e67c.camel@j-davis.com


--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Sat, 2023-08-19 at 07:18 +0200, Pavel Stehule wrote:
> cannot be better special syntax
>
> CREATE OR REPLACE FUNCTION xxx()
> RETURNS yyy AS $$ ... $$$
> SET SEARCH_PATH DISABLE
>
> with possible next modification
>
> SET SEARCH_PATH CATALOG .. only for pg_catalog
> SET SEARCH_PATH MINIMAL .. pg_catalog, pg_temp

I agree that we should consider new syntax, and there's a related
discussion here:

https://www.postgresql.org/message-id/flat/2710f56add351a1ed553efb677408e51b060e67c.camel@j-davis.com

Regardless, even with syntax changes, we need something to print when
someone does a "SHOW search_path", i.e. some representation that
indicates pg_temp is excluded. That way it can also be saved and
restored.

> I question if we should block search path settings when this setting
> is used. Although I set search_path, the search_path can be
> overwritten in function of inside some nesting calls

If so, that should be a separate feature. For the purposes of this
thread, we just need a way to represent a search path that excludes
pg_temp and/or pg_catalog.

Regards,
    Jeff Davis

On Fri, Aug 18, 2023 at 02:44:31PM -0700, Jeff Davis wrote:
> +    SET search_path = admin, "!pg_temp";

I think it's unfortunate that these new identifiers must be quoted.  I
wonder if we could call these something like "no_pg_temp".  *shrug*

> +     * Add any implicitly-searched namespaces to the list unless the markers
> +     * "!pg_catalog" or "!pg_temp" are present.  Note these go on the front,
> +     * not the back; also notice that we do not check USAGE permissions for
> +     * these.
>       */
> -    if (!list_member_oid(oidlist, PG_CATALOG_NAMESPACE))
> +    if (implicit_pg_catalog &&
> +        !list_member_oid(oidlist, PG_CATALOG_NAMESPACE))
>          oidlist = lcons_oid(PG_CATALOG_NAMESPACE, oidlist);
>  
> -    if (OidIsValid(myTempNamespace) &&
> +    if (implicit_pg_temp &&
> +        OidIsValid(myTempNamespace) &&
>          !list_member_oid(oidlist, myTempNamespace))
>          oidlist = lcons_oid(myTempNamespace, oidlist);

Should we disallow including both !pg_temp and pg_temp at the same time?  I
worry that could be a source of confusion.  IIUC your patches effectively
ignore !pg_temp if pg_temp is explicitly listed elsewhere in the list.

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

On Thu, 2023-10-26 at 16:28 -0500, Nathan Bossart wrote:
> On Fri, Aug 18, 2023 at 02:44:31PM -0700, Jeff Davis wrote:
> > +    SET search_path = admin, "!pg_temp";
>
> I think it's unfortunate that these new identifiers must be quoted. 
> I
> wonder if we could call these something like "no_pg_temp".  *shrug*

Do you, overall, find this feature useful?

Most functions don't need pg_temp, so it feels cleaner to exclude it.
But pg_temp is ignored for function/op lookup anyway, so functions
won't be exposed to search_path risks related to pg_temp unless they
are accessing tables.

If my proposal for the SEARCH clause got more support, I'd be more
excited about this feature because it could be set implicitly as part
of a safe search_path. Without the SEARCH clause, the only way to set
"!pg_temp" is by typing it out, and I'm not sure a lot of people will
actually do that.

Regards,
    Jeff Davis

On Fri, Oct 27, 2023 at 12:58:47PM -0700, Jeff Davis wrote:
> Do you, overall, find this feature useful?
> 
> Most functions don't need pg_temp, so it feels cleaner to exclude it.
> But pg_temp is ignored for function/op lookup anyway, so functions
> won't be exposed to search_path risks related to pg_temp unless they
> are accessing tables.
> 
> If my proposal for the SEARCH clause got more support, I'd be more
> excited about this feature because it could be set implicitly as part
> of a safe search_path. Without the SEARCH clause, the only way to set
> "!pg_temp" is by typing it out, and I'm not sure a lot of people will
> actually do that.

I thought it sounded generally useful, but if we're not going to proceed
with the primary use-case for this feature, then perhaps it's not worth
going through this particular one-way door at this time.

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