MIME-Version: 1.0
References: 
 <CAGECzQQzDqDzakBkR71ZkQ1N1ffTjAaruRSqppQAKu3WF+6rNQ@mail.gmail.com>
 <dbd3cc5a5edde04941c8624dde7e4bd2a063f366.camel@j-davis.com>
 <CAGECzQTe3y6xwP9aMbFBCxKnQEEn3G3=cEDqoD5xaOwPwypd_A@mail.gmail.com>
 <CAGECzQTAgtTp3G=Em3xEY=T=uiKfyu5xLYri9By=sfNBS5C_9A@mail.gmail.com>
 <CAKFQuwaT4_n=e0YKBZAyox1CQUra2ka0cySs+3pGZR5p50pn-g@mail.gmail.com>
 <CAGECzQTOJrnnJkmMe9nems0jouiKUbFcEb1rb9kE_svsAZiGQg@mail.gmail.com>
 <585e996c-a5c6-4e61-acc4-d92b7a1458ea@vondra.me>
 <CAGECzQS02M6YPDXemo36tShO-ZYObjqnyTJyVttua1PGyN4xRw@mail.gmail.com>
 <CAFPkQKzALOTTBrhj2qDHwVxZQyjF5Xg_P9M=Tn_Dcm3vr=xdTA@mail.gmail.com>
 <DBOFQN54M5FX.1XWJE4LKMR8XH@jeltef.nl>
In-Reply-To: <DBOFQN54M5FX.1XWJE4LKMR8XH@jeltef.nl>
From: Robert Haas <robertmhaas@gmail.com>
Date: Mon, 11 Aug 2025 13:55:57 -0400
Message-ID: 
 <CA+TgmoY=NO7_L=UDuoUWj-icABF-7EP=UNUXCFBYpDNFoUZmbA@mail.gmail.com>
Subject: Re: Extension security improvement: Add support for extensions with
 an owned schema
To: Jelte Fennema-Nio <postgres@jeltef.nl>
Cc: Artem Gavrilov <artem.gavrilov@percona.com>,
 Jelte Fennema-Nio <me@jeltef.nl>,
	Tomas Vondra <tomas@vondra.me>,
 "David G. Johnston" <david.g.johnston@gmail.com>,
	Jeff Davis <pgsql@j-davis.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BTgmoY%3DNO7_L%3DUDuoUWj-icABF-7EP%3DUNUXCFBYpDNFoUZmbA%40mail.gmail.com>
Precedence: bulk

On Tue, Jul 29, 2025 at 5:35=E2=80=AFAM Jelte Fennema-Nio <postgres@jeltef.=
nl> wrote:
> On Wed Jul 23, 2025 at 7:12 PM CEST, Artem Gavrilov wrote:
> > 1) I noticed that pg_dump changes weren't covered with tests.
> >
> > 2) I assume these error messages may be confusing, especially first one=
:
>
> Attached is an updated version that addresses these issues.

I generally like the direction that this is going but there are places
where the new stuff looks too much like it was bolted onto whatever
was there already. It's important to go back and edit things so that
they look natural, as if the new feature had been present all along.

-        relocated.  The named schema must already exist.
+        relocated.  The named schema must already exist, unless
+        <literal>owned_schema</literal> is set to <literal>true</literal> =
in
+        the control file, then the schema must not exist.

This reads awkwardly, at least to me. The smallest possible edit that
would make it passable for me is to replace "the" with "in which
case," but possibly the whole sentence should be rephrased somehow.

+ * If the user is giving us the schema name, it must exist already if
+ * the extension does not want to own the schema

This could be made clearer.

+ errmsg("schema \"%s\" already exists but the extension needs to create it=
",
+ schemaName),

I don't really find this an improvement over: ERROR:  schema
"test_ext_owned_schema" already exists.

+ else if (control->owned_schema)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_SCHEMA),
+ errmsg("schema \"%s\" already exists but the extension needs to create it=
",
+ schemaName),
+ errhint("Drop schema \"%s\" or specify another schema using CREATE
EXTENSION ... SCHEMA ...", schemaName)));
+ }
+
  }
- else if (!OidIsValid(schemaOid))
+ else if (control->owned_schema)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_SCHEMA),
+ errmsg("no schema has been selected to create in")));
+ }
+ else

It certainly seems worth asking whether this if-nest should be
rephrased in some way to make it clearer. But even if it's best to
keep it as it is, I find the absence of comments hard to justify. Who
is going to read the bit that emits "no schema has been selected to
create in" and find that self-explanatory?

I would like to see some improvements in AlterExtensionNamespace. In
the context of the patch, it's possible to puzzle out what is
happening, but I think the picture is not going to be clear to later
readers. It seems to me that this either needs some restructuring to
make the logical flow clearer, or a few well-written comments.

--=20
Robert Haas
EDB: http://www.enterprisedb.com