ACL_MAINTAIN, Lack of comment content

Started by btsugieyuusukeover 1 year ago7 messages

btsugieyuusuke@oss.nttdata.com

over 1 year ago

Hi hackers,
I found a flaw in the ACL_MAINTAIN comment.

Commands such as VACUUM are listed as commands that are allowed to be
executed by the MAINTAIN privilege.
However, LOCK TABLE is missing from the comment.

/*
* Check if ACL_MAINTAIN is being checked and, if so, and not already
set
* as part of the result, then check if the user is a member of the
* pg_maintain role, which allows VACUUM, ANALYZE, CLUSTER, REFRESH
* MATERIALIZED VIEW, and REINDEX on all relations.
*/

Therefore, shouldn't LOCK TABLE be added to the comment?

Best regards,
Yusuke Sugie

Daniel Gustafsson

daniel@yesql.se

over 1 year ago

In reply to: btsugieyuusuke (#1)

1 attachment(s)

Re: ACL_MAINTAIN, Lack of comment content

On 30 Sep 2024, at 10:29, btsugieyuusuke <btsugieyuusuke@oss.nttdata.com> wrote:

Hi hackers,
I found a flaw in the ACL_MAINTAIN comment.

Commands such as VACUUM are listed as commands that are allowed to be executed by the MAINTAIN privilege.
However, LOCK TABLE is missing from the comment.

/*
* Check if ACL_MAINTAIN is being checked and, if so, and not already set
* as part of the result, then check if the user is a member of the
* pg_maintain role, which allows VACUUM, ANALYZE, CLUSTER, REFRESH
* MATERIALIZED VIEW, and REINDEX on all relations.
*/

Therefore, shouldn't LOCK TABLE be added to the comment?

That's correct, for the list to be complete LOCK TABLE should be added as per
the attached.

--
Daniel Gustafsson

Attachments:

acl_maintain_comment.diffapplication/octet-stream; name=acl_maintain_comment.diff; x-unix-mode=0644Download

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 819045203d..d9c02614d1 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3449,7 +3449,7 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
 	 * Check if ACL_MAINTAIN is being checked and, if so, and not already set
 	 * as part of the result, then check if the user is a member of the
 	 * pg_maintain role, which allows VACUUM, ANALYZE, CLUSTER, REFRESH
-	 * MATERIALIZED VIEW, and REINDEX on all relations.
+	 * MATERIALIZED VIEW, REINDEX and LOCK TABLE on all relations.
 	 */
 	if (mask & ACL_MAINTAIN &&
 		!(result & ACL_MAINTAIN) &&

Yugo Nagata

nagata@sraoss.co.jp

over 1 year ago

In reply to: Daniel Gustafsson (#2)

Re: ACL_MAINTAIN, Lack of comment content

On Mon, 30 Sep 2024 11:40:29 +0200
Daniel Gustafsson <daniel@yesql.se> wrote:

-	 * MATERIALIZED VIEW, and REINDEX on all relations.
+	 * MATERIALIZED VIEW, REINDEX and LOCK TABLE on all relations.

Should we put a comma between REINDEX and "and" as following?

"... MATERIALIZED VIEW, REINDEX, and LOCK TABLE on all relations."

Regards,
Yugo Nagata

--
Yugo Nagata <nagata@sraoss.co.jp>

Daniel Gustafsson

daniel@yesql.se

over 1 year ago

In reply to: Yugo Nagata (#3)

Re: ACL_MAINTAIN, Lack of comment content

On 30 Sep 2024, at 12:38, Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Mon, 30 Sep 2024 11:40:29 +0200
Daniel Gustafsson <daniel@yesql.se> wrote:
- * MATERIALIZED VIEW, and REINDEX on all relations.
+ * MATERIALIZED VIEW, REINDEX and LOCK TABLE on all relations.
Should we put a comma between REINDEX and "and" as following?

"... MATERIALIZED VIEW, REINDEX, and LOCK TABLE on all relations."

I'm not a native speaker so I'm not sure which is right, but grepping for other
lists of items shows that the last "and" item is often preceded by a comma so
I'll do that.

--
Daniel Gustafsson

Nathan Bossart

nathandbossart@gmail.com

over 1 year ago

In reply to: Daniel Gustafsson (#4)

Re: ACL_MAINTAIN, Lack of comment content

On Mon, Sep 30, 2024 at 04:13:55PM +0200, Daniel Gustafsson wrote:

On 30 Sep 2024, at 12:38, Yugo Nagata <nagata@sraoss.co.jp> wrote:

Should we put a comma between REINDEX and "and" as following?

"... MATERIALIZED VIEW, REINDEX, and LOCK TABLE on all relations."

I'm not a native speaker so I'm not sure which is right, but grepping for other
lists of items shows that the last "and" item is often preceded by a comma so
I'll do that.

I'm not aware of a project policy around the Oxford comma [0]https://en.wikipedia.org/wiki/Serial_comma, but I tend
to include one.

[0]: https://en.wikipedia.org/wiki/Serial_comma

--
nathan

Tom Lane

tgl@sss.pgh.pa.us

over 1 year ago

In reply to: Nathan Bossart (#5)

Re: ACL_MAINTAIN, Lack of comment content

Nathan Bossart <nathandbossart@gmail.com> writes:

On Mon, Sep 30, 2024 at 04:13:55PM +0200, Daniel Gustafsson wrote:

I'm not a native speaker so I'm not sure which is right, but grepping for other
lists of items shows that the last "and" item is often preceded by a comma so
I'll do that.

I'm not aware of a project policy around the Oxford comma [0], but I tend
to include one.

Yeah, as that wikipedia article suggests, you can find support for
either choice. I'd say do what looks best in context.

regards, tom lane

Daniel Gustafsson

daniel@yesql.se

over 1 year ago

In reply to: Tom Lane (#6)

Re: ACL_MAINTAIN, Lack of comment content

On 30 Sep 2024, at 17:43, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

On Mon, Sep 30, 2024 at 04:13:55PM +0200, Daniel Gustafsson wrote:

I'm not a native speaker so I'm not sure which is right, but grepping for other
lists of items shows that the last "and" item is often preceded by a comma so
I'll do that.

I'm not aware of a project policy around the Oxford comma [0], but I tend
to include one.

Yeah, as that wikipedia article suggests, you can find support for
either choice. I'd say do what looks best in context.

Thanks for input, I ended up keeping the comma.

--
Daniel Gustafsson