Re: pg_checkpointer is not a verb or verb phrase

[Tom Lane]

Robert Haas  writes:
> On Tue, Jul 5, 2022 at 3:42 PM Nathan Bossart  
> 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

Re: pg_checkpointer is not a verb or verb phrase

[Robert Haas]

On Tue, Jul 5, 2022 at 3:42 PM Nathan Bossart  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

Re: pg_checkpointer is not a verb or verb phrase

[Nathan Bossart]

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

Re: pg_checkpointer is not a verb or verb phrase

[Robert Haas]

On Fri, Jul 1, 2022 at 5:50 PM Nathan Bossart  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

Re: pg_checkpointer is not a verb or verb phrase

[Nathan Bossart]

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
>From 2a514b16238a55b9929029ce8f641e51178077f1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
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
 
   
Only superusers or users with the privileges of
-   the pg_checkpointer
+   the pg_checkpoint
role can call CHECKPOINT.
   
  
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 9067be1d9c..6e36b8 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.
   
   
-   pg_checkpointer
+   pg_checkpoint
Allow executing
the CHECKPOINT
command.
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',
-  

Re: pg_checkpointer is not a verb or verb phrase

[Magnus Hagander]

On Fri, Jul 1, 2022 at 3:18 PM Robert Haas  wrote:

> On Thu, Jun 30, 2022 at 9:22 PM Michael Paquier 
> 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/ 
 Work: https://www.redpill-linpro.com/ 

Re: pg_checkpointer is not a verb or verb phrase

[Robert Haas]

On Thu, Jun 30, 2022 at 9:22 PM Michael Paquier  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

Re: pg_checkpointer is not a verb or verb phrase

[Justin Pryzby]

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

Re: pg_checkpointer is not a verb or verb phrase

[Isaac Morland]

On Thu, 30 Jun 2022 at 21:22, 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
> "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.

Re: pg_checkpointer is not a verb or verb phrase

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_checkpointer is not a verb or verb phrase

[Isaac Morland]

On Thu, 30 Jun 2022 at 08:48, Robert Haas  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.

pg_checkpointer is not a verb or verb phrase

[Robert Haas]

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