pg_checkpointer is not a verb or verb phrase
Hi,
I was just looking at the list of predefined roles that we have, and
pg_checkpointer is conspicuously not like the others:
rhaas=# select rolname from pg_authid where oid!=10;
rolname
---------------------------
pg_database_owner
pg_read_all_data
pg_write_all_data
pg_monitor
pg_read_all_settings
pg_read_all_stats
pg_stat_scan_tables
pg_read_server_files
pg_write_server_files
pg_execute_server_program
pg_signal_backend
pg_checkpointer
(12 rows)
Almost all of these are verbs or verb phrases: having this role gives
you the ability to read all data, or write all data, or read all
settings, or whatever. But you can't say that having this role gives
you the ability to checkpointer. It gives you the ability to
checkpoint, or to perform a checkpoint.
So I think the name is inconsistent with our usual convention, and we
ought to fix it.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Thu, 30 Jun 2022 at 08:48, Robert Haas <robertmhaas@gmail.com> wrote:
Almost all of these are verbs or verb phrases: having this role gives
you the ability to read all data, or write all data, or read all
settings, or whatever. But you can't say that having this role gives
you the ability to checkpointer. It gives you the ability to
checkpoint, or to perform a checkpoint.So I think the name is inconsistent with our usual convention, and we
ought to fix it.
I was going to point out that pg_database_owner is the same way, but it is
fundamentally different in that it has no special allowed access and is
meant to be the target of permission grants rather than being granted to
other roles.
+1 to rename it to pg_checkpoint or to some similar name.
On Thu, Jun 30, 2022 at 08:57:04AM -0400, Isaac Morland wrote:
I was going to point out that pg_database_owner is the same way, but it is
fundamentally different in that it has no special allowed access and is
meant to be the target of permission grants rather than being granted to
other roles.+1 to rename it to pg_checkpoint or to some similar name.
We are still in beta, so, FWIW, I am fine to adjust this name even if
it means an extra catversion bump.
"checkpoint" is not a verb (right?), so would something like
"pg_perform_checkpoint" rather than "pg_checkpoint" fit better in the
larger picture?
--
Michael
On Thu, 30 Jun 2022 at 21:22, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Jun 30, 2022 at 08:57:04AM -0400, Isaac Morland wrote:
I was going to point out that pg_database_owner is the same way, but it
is
fundamentally different in that it has no special allowed access and is
meant to be the target of permission grants rather than being granted to
other roles.+1 to rename it to pg_checkpoint or to some similar name.
We are still in beta, so, FWIW, I am fine to adjust this name even if
it means an extra catversion bump."checkpoint" is not a verb (right?), so would something like
"pg_perform_checkpoint" rather than "pg_checkpoint" fit better in the
larger picture?
I would argue it’s OK. In the Postgres context, I can imagine someone
saying they’re going to checkpoint the database, and the actual command is
just CHECKPOINT. Changing from checkpointer to checkpoint means that we’re
describing the action rather than what a role member is.
If we are going to put a more standard verb in there, I would use execute
rather than perform, because that is what the documentation says members of
this role can do — “Allow executing the CHECKPOINT command”. Zooming out a
little, I think we normally talk about executing commands rather than
performing them, so this is consistent with those other uses; otherwise we
should reconsider what the documentation itself says to match
other commands that we talk about running.
OK, I think I’ve bikeshedded enough. I’m just happy to have all these new
roles to avoid handing out full superuser access routinely.
On Fri, Jul 01, 2022 at 10:22:16AM +0900, Michael Paquier wrote:
On Thu, Jun 30, 2022 at 08:57:04AM -0400, Isaac Morland wrote:
I was going to point out that pg_database_owner is the same way, but it is
fundamentally different in that it has no special allowed access and is
meant to be the target of permission grants rather than being granted to
other roles.+1 to rename it to pg_checkpoint or to some similar name.
We are still in beta, so, FWIW, I am fine to adjust this name even if
it means an extra catversion bump."checkpoint" is not a verb (right?), so would something like
Why not ? There's a *command* called "checkpoint".
It is also a noun.
--
Justin
On Thu, Jun 30, 2022 at 9:22 PM Michael Paquier <michael@paquier.xyz> wrote:
"checkpoint" is not a verb (right?), so would something like
"pg_perform_checkpoint" rather than "pg_checkpoint" fit better in the
larger picture?
It's true that the dictionary describes checkpoint as a noun, but I
think in a database context it is often used as a verb. One example is
the CHECKPOINT command itself: command names are all verbs, and
CHECKPOINT is a command name, so we're using it as a verb. Similarly I
think you can talk about needing to checkpoint the database. Therefore
I think pg_checkpoint is OK, and it has the advantage of being short.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Fri, Jul 1, 2022 at 3:18 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jun 30, 2022 at 9:22 PM Michael Paquier <michael@paquier.xyz>
wrote:"checkpoint" is not a verb (right?), so would something like
"pg_perform_checkpoint" rather than "pg_checkpoint" fit better in the
larger picture?It's true that the dictionary describes checkpoint as a noun, but I
think in a database context it is often used as a verb. One example is
the CHECKPOINT command itself: command names are all verbs, and
CHECKPOINT is a command name, so we're using it as a verb. Similarly I
think you can talk about needing to checkpoint the database. Therefore
I think pg_checkpoint is OK, and it has the advantage of being short.
+1 for pg_checkpoint on that -- let's not make it longer than necessary.
And yes, +1 for actually changing it. It's a lot cheaper to change it now
than it will be in the future. Yes, it would've been even cheaper to have
already changed it, but we can't go back in time...
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
On Fri, Jul 01, 2022 at 03:36:48PM +0200, Magnus Hagander wrote:
+1 for pg_checkpoint on that -- let's not make it longer than necessary.
And yes, +1 for actually changing it. It's a lot cheaper to change it now
than it will be in the future. Yes, it would've been even cheaper to have
already changed it, but we can't go back in time...
Yeah, pg_checkpoint seems like the obvious alternative to pg_checkpointer.
I didn't see a patch in this thread yet, so I've put one together. I did
not include the catversion bump.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Rename-pg_checkpointer-to-pg_checkpoint.patchtext/x-diff; charset=utf-8Download
From 2a514b16238a55b9929029ce8f641e51178077f1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Fri, 1 Jul 2022 14:44:15 -0700
Subject: [PATCH v1 1/1] Rename pg_checkpointer to pg_checkpoint.
---
doc/src/sgml/ref/checkpoint.sgml | 2 +-
doc/src/sgml/user-manag.sgml | 2 +-
src/backend/po/de.po | 2 +-
src/backend/po/ja.po | 4 ++--
src/backend/po/sv.po | 2 +-
src/backend/tcop/utility.c | 4 ++--
src/include/catalog/pg_authid.dat | 4 ++--
7 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/doc/src/sgml/ref/checkpoint.sgml b/doc/src/sgml/ref/checkpoint.sgml
index 1cebc03d15..28a1d717b8 100644
--- a/doc/src/sgml/ref/checkpoint.sgml
+++ b/doc/src/sgml/ref/checkpoint.sgml
@@ -53,7 +53,7 @@ CHECKPOINT
<para>
Only superusers or users with the privileges of
- the <link linkend="predefined-roles-table"><literal>pg_checkpointer</literal></link>
+ the <link linkend="predefined-roles-table"><literal>pg_checkpoint</literal></link>
role can call <command>CHECKPOINT</command>.
</para>
</refsect1>
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 9067be1d9c..6eaaaa36b8 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -583,7 +583,7 @@ DROP ROLE doomed_role;
COPY and other functions which allow executing a server-side program.</entry>
</row>
<row>
- <entry>pg_checkpointer</entry>
+ <entry>pg_checkpoint</entry>
<entry>Allow executing
the <link linkend="sql-checkpoint"><command>CHECKPOINT</command></link>
command.</entry>
diff --git a/src/backend/po/de.po b/src/backend/po/de.po
index f665817458..999f2c2839 100644
--- a/src/backend/po/de.po
+++ b/src/backend/po/de.po
@@ -22711,7 +22711,7 @@ msgstr "%s kann nicht in einem Hintergrundprozess ausgeführt werden"
#: tcop/utility.c:953
#, fuzzy, c-format
#| msgid "must be superuser to do CHECKPOINT"
-msgid "must be superuser or have privileges of pg_checkpointer to do CHECKPOINT"
+msgid "must be superuser or have privileges of pg_checkpoint to do CHECKPOINT"
msgstr "nur Superuser können CHECKPOINT ausführen"
#: tsearch/dict_ispell.c:52 tsearch/dict_thesaurus.c:615
diff --git a/src/backend/po/ja.po b/src/backend/po/ja.po
index 0925465d22..8eca82d8cb 100644
--- a/src/backend/po/ja.po
+++ b/src/backend/po/ja.po
@@ -22010,8 +22010,8 @@ msgstr "ããã¯ã°ã©ã¦ã³ãããã»ã¹å
ã§%sãå®è¡ãããã¨ã¯ã§
#: tcop/utility.c:953
#, c-format
-msgid "must be superuser or have privileges of pg_checkpointer to do CHECKPOINT"
-msgstr "CHECKPOINTãå®è¡ããã«ã¯ã¹ã¼ãã¼ã¦ã¼ã¶ã¼ã§ããããã¾ãã¯pg_checkpointerã®æ¨©éãæã¤å¿
è¦ãããã¾ã"
+msgid "must be superuser or have privileges of pg_checkpoint to do CHECKPOINT"
+msgstr "CHECKPOINTãå®è¡ããã«ã¯ã¹ã¼ãã¼ã¦ã¼ã¶ã¼ã§ããããã¾ãã¯pg_checkpointã®æ¨©éãæã¤å¿
è¦ãããã¾ã"
#: tsearch/dict_ispell.c:52 tsearch/dict_thesaurus.c:615
#, c-format
diff --git a/src/backend/po/sv.po b/src/backend/po/sv.po
index 05f8184506..095eb4e4ea 100644
--- a/src/backend/po/sv.po
+++ b/src/backend/po/sv.po
@@ -22794,7 +22794,7 @@ msgstr "kan inte köra %s i en bakgrundsprocess"
#: tcop/utility.c:953
#, fuzzy, c-format
#| msgid "must be superuser to do CHECKPOINT"
-msgid "must be superuser or have privileges of pg_checkpointer to do CHECKPOINT"
+msgid "must be superuser or have privileges of pg_checkpoint to do CHECKPOINT"
msgstr "måste vara superuser för att göra CHECKPOINT"
#: tsearch/dict_ispell.c:52 tsearch/dict_thesaurus.c:615
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 6a5bcded55..6b0a865262 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -947,10 +947,10 @@ standard_ProcessUtility(PlannedStmt *pstmt,
break;
case T_CheckPointStmt:
- if (!has_privs_of_role(GetUserId(), ROLE_PG_CHECKPOINTER))
+ if (!has_privs_of_role(GetUserId(), ROLE_PG_CHECKPOINT))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or have privileges of pg_checkpointer to do CHECKPOINT")));
+ errmsg("must be superuser or have privileges of pg_checkpoint to do CHECKPOINT")));
RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT |
(RecoveryInProgress() ? 0 : CHECKPOINT_FORCE));
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 6c28119fa1..3343a69ddb 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -79,8 +79,8 @@
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
rolpassword => '_null_', rolvaliduntil => '_null_' },
-{ oid => '4544', oid_symbol => 'ROLE_PG_CHECKPOINTER',
- rolname => 'pg_checkpointer', rolsuper => 'f', rolinherit => 't',
+{ oid => '4544', oid_symbol => 'ROLE_PG_CHECKPOINT',
+ rolname => 'pg_checkpoint', rolsuper => 'f', rolinherit => 't',
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
rolpassword => '_null_', rolvaliduntil => '_null_' },
--
2.25.1
On Fri, Jul 1, 2022 at 5:50 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
On Fri, Jul 01, 2022 at 03:36:48PM +0200, Magnus Hagander wrote:
+1 for pg_checkpoint on that -- let's not make it longer than necessary.
And yes, +1 for actually changing it. It's a lot cheaper to change it now
than it will be in the future. Yes, it would've been even cheaper to have
already changed it, but we can't go back in time...Yeah, pg_checkpoint seems like the obvious alternative to pg_checkpointer.
I didn't see a patch in this thread yet, so I've put one together. I did
not include the catversion bump.
Hearing several votes in favor and none opposed, committed and
back-patched to v15. I added the catversion bump, but left out the .po
file changes, figuring it was better to let those files get updated
via the normal process.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Tue, Jul 05, 2022 at 01:38:43PM -0400, Robert Haas wrote:
Hearing several votes in favor and none opposed, committed and
back-patched to v15.
Thanks.
I added the catversion bump, but left out the .po
file changes, figuring it was better to let those files get updated
via the normal process.
I'll keep this in mind for future patches. The changes looked pretty
obvious, so I wasn't sure whether to include it.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Jul 5, 2022 at 3:42 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
On Tue, Jul 05, 2022 at 01:38:43PM -0400, Robert Haas wrote:
Hearing several votes in favor and none opposed, committed and
back-patched to v15.Thanks.
I added the catversion bump, but left out the .po
file changes, figuring it was better to let those files get updated
via the normal process.I'll keep this in mind for future patches. The changes looked pretty
obvious, so I wasn't sure whether to include it.
I believe Peter periodically runs a script that bulk copies everything
over from the translation repository.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Jul 5, 2022 at 3:42 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
On Tue, Jul 05, 2022 at 01:38:43PM -0400, Robert Haas wrote:
I added the catversion bump, but left out the .po
file changes, figuring it was better to let those files get updated
via the normal process.
I'll keep this in mind for future patches. The changes looked pretty
obvious, so I wasn't sure whether to include it.
I believe Peter periodically runs a script that bulk copies everything
over from the translation repository.
Indeed. If we did commit anything, it would just be wiped out in the
next bulk update. The authoritative versions of the .po files are in
the pgtranslation repo, not gitmaster.
regards, tom lane