Horton

Re: Possibility to disable `ALTER SYSTEM`

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Jelte Fennema-Nio <postgres@jeltef.nl>
Cc: "David G. Johnston" <david.g.johnston@gmail.com>, Bruce Momjian <bruce@momjian.us>, Maciek Sakrejda <m.sakrejda@gmail.com>, Isaac Morland <isaac.morland@gmail.com>, Greg Sabino Mullane <htamfids@gmail.com>, Andrew Dunstan <andrew@dunslane.net>, Tom Lane <tgl@sss.pgh.pa.us>, Heikki Linnakangas <hlinnaka@iki.fi>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Daniel Gustafsson <daniel@yesql.se>, Joel Jacobson <joel@compiler.org>, Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>, Magnus Hagander <magnus.hagander@redpill-linpro.com>
Date: 2024-03-28T11:57:24Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add allow_alter_system GUC.

  2. Rename COMPAT_OPTIONS_CLIENT to COMPAT_OPTIONS_OTHER.

  3. Remove support for version-0 calling conventions.

On Thu, Mar 28, 2024 at 5:42 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> On Thu, 28 Mar 2024 at 10:24, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > I addressed them all I think. Mostly the small changes that were
> > suggested, but I rewrote the sentence with "might be discarded". And I
> > added references to the new GUC in both places suggested by David.
>
> Changed the error hint to use "external tool" too. And removed a
> duplicate header definition of AllowAlterSystem (I moved it to guc.h
> so it was together with other definitions a few patches ago, but
> apparently forgot to remove it from guc_tables.h)

I disagree with a lot of these changes. I think the old version was
mostly better. But I can live with a lot of it if it makes other
people happy. However:

+        Which might result in unintended behavior, such as the external tool
+        discarding the change at some later point in time when it updates the
+        configuration.

This is not OK from a grammatical point of view. You can't just start
a sentence with "which" like this. You could replace "Which" with
"This", though.

+ if (!AllowAlterSystem)
+ {
+
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ALTER SYSTEM is not allowed in this environment"),
+ errhint("Global configuration changes should be made using an
external tool, not by using ALTER SYSTEM.")));
+ }

The extra blank line should go. The brackets should go. And I think
the errhint should go, too, because the errhint implies that we know
why the user chose to set allow_alter_system=false. There's no reason
for this message to be opinionated about that.

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