public inbox for [email protected]
help / color / mirror / Atom feedRe: Clarification on Role Access Rights to Table Indexes
19+ messages / 6 participants
[nested] [flat]
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-02-18 16:21 Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Robert Haas @ 2025-02-18 16:21 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: David G. Johnston <[email protected]>; Ayush Vatsa <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Feb 18, 2025 at 11:02 AM Tom Lane <[email protected]> wrote:
> Is that a +1 for the specific design of "check SELECT on the index's
> table", or just a +1 for changing something here?
That is a +1 for the specific design of "check SELECT on the index's
table". I don't want to be closed-minded: if you have some strong
reason for believing that's the wrong thing to do, I'm all ears.
However, I'm presently of the view that it is exactly the right thing
to do, to the point where I don't currently understand why there's
anything to think about here.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
@ 2025-02-18 16:30 ` Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Tom Lane @ 2025-02-18 16:30 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: David G. Johnston <[email protected]>; Ayush Vatsa <[email protected]>; PostgreSQL Hackers <[email protected]>
Robert Haas <[email protected]> writes:
> That is a +1 for the specific design of "check SELECT on the index's
> table". I don't want to be closed-minded: if you have some strong
> reason for believing that's the wrong thing to do, I'm all ears.
> However, I'm presently of the view that it is exactly the right thing
> to do, to the point where I don't currently understand why there's
> anything to think about here.
I have no objection to it, but I wasn't as entirely convinced
as you are that it's the only plausible answer.
One specific thing I'm slightly worried about is that a naive
implementation would probably cause this function to lock the
table after the index, risking deadlock against queries that
take the locks in the more conventional order. I don't recall
what if anything we've done about that in other places
(-ENOCAFFEINE).
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
@ 2025-02-18 18:16 ` Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Robert Haas @ 2025-02-18 18:16 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: David G. Johnston <[email protected]>; Ayush Vatsa <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Feb 18, 2025 at 11:30 AM Tom Lane <[email protected]> wrote:
> I have no objection to it, but I wasn't as entirely convinced
> as you are that it's the only plausible answer.
Hmm, OK.
> One specific thing I'm slightly worried about is that a naive
> implementation would probably cause this function to lock the
> table after the index, risking deadlock against queries that
> take the locks in the more conventional order. I don't recall
> what if anything we've done about that in other places
> (-ENOCAFFEINE).
Yeah, that seems like a good thing to worry about from an
implementation point of view but it doesn't seem like a reason to
question the basic design choice. In general, if you can use a table,
you also get to use its indexes, so that interpretation seems natural
to me here, also. Now, if somebody finds a problem with requiring only
SELECT permission, I could see changing the requirements for both
tables and indexes, but I find it harder to imagine that we'd want
those things to work differently from each other. Of course I'm
willing to be convinced that there's a good reason for them to be
different; I just can't currently imagine what it might be.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
@ 2025-02-19 10:23 ` Ayush Vatsa <[email protected]>
2025-02-19 10:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
0 siblings, 2 replies; 19+ messages in thread
From: Ayush Vatsa @ 2025-02-19 10:23 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
Hello Everyone,
It seems there's a general consensus that we should maintain a
original design to support pg_prewarm, with a minor adjustment:
when querying indexes, we should verify the privileges of the parent table.
I’ve attached a patch for this, which includes some test cases as well.
Let me know if it needs any changes.
Regards,
Ayush Vatsa
SDE AWS
Attachments:
[application/octet-stream] v1-0001-Improve-ACL-checks-in-pg_prewarm-for-indexes.patch (4.8K, 3-v1-0001-Improve-ACL-checks-in-pg_prewarm-for-indexes.patch)
download | inline diff:
From 8f8606c007fdb58403792d36a62c6739341b0549 Mon Sep 17 00:00:00 2001
From: Ayush Vatsa <[email protected]>
Date: Mon, 17 Feb 2025 20:44:56 +0000
Subject: [PATCH v1] Improve ACL checks in pg_prewarm for indexes
When pg_prewarm is called on an index, perform the ACL check on its
underlying table instead of the index itself. Indexes do not have
independent access rights and depend on their associated table for
access control.
---
contrib/pg_prewarm/pg_prewarm.c | 29 ++++++++++++++++++++++++---
contrib/pg_prewarm/t/001_basic.pl | 33 ++++++++++++++++++++++++++++++-
2 files changed, 58 insertions(+), 4 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index a2f0ac4af0..b2846cee2e 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,6 +16,7 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
@@ -55,6 +56,7 @@ Datum
pg_prewarm(PG_FUNCTION_ARGS)
{
Oid relOid;
+ Oid tableOid;
text *forkName;
text *type;
int64 first_block;
@@ -105,9 +107,30 @@ pg_prewarm(PG_FUNCTION_ARGS)
/* Open relation and check privileges. */
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
+
+ /**
+ * Check access permissions for pg_prewarm. If the relation is an index,
+ * perform the ACL check on its underlying table since indexes do not have
+ * their own access rights.
+ */
+ if (rel->rd_rel->relkind == RELKIND_INDEX)
+ {
+ /*
+ * If it's an index, get the table OID and perform ACL check on the
+ * table.
+ */
+ tableOid = IndexGetRelation(relOid, false);
+ aclresult = pg_class_aclcheck(tableOid, GetUserId(), ACL_SELECT);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, get_relkind_objtype(RELKIND_RELATION), get_rel_name(tableOid));
+ }
+ else
+ {
+ /* If it's not an index, perform ACL check on the relation itself. */
+ aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
+ }
/* Check that the fork exists. */
if (!smgrexists(RelationGetSmgr(rel), forkNumber))
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d367..651f4ec371 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user WITH LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,35 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# grant SELECT permission on the table to test_user
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+
+# test pg_prewarm on the table/index as test_user (should succeed)
+$result =
+ $node->safe_psql("postgres", "SELECT pg_prewarm('test', 'buffer');", extra_params => [ '--username' => "test_user" ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm on table succeeds with SELECT permission on table');
+
+$result =
+ $node->safe_psql("postgres", "SELECT pg_prewarm('test_idx', 'buffer');", extra_params => [ '--username' => "test_user" ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm on index succeeds with SELECT permission on table');
+
+# revoke SELECT permission on the table from test_user
+$node->safe_psql("postgres", "REVOKE SELECT ON test FROM test_user;");
+
+# test pg_prewarm on the table/index as test_user (should fail)
+($cmdret, $stdout, $stderr) =
+ $node->psql("postgres", "SELECT pg_prewarm('test', 'buffer');", extra_params => [ '--username' => "test_user" ]);
+ok( $stderr =~ /permission denied for table test/,
+ "error message indicates user doesn't have sufficient privileges for the parent table");
+
+($cmdret, $stdout, $stderr) =
+ $node->psql("postgres", "SELECT pg_prewarm('test_idx', 'buffer');", extra_params => [ '--username' => "test_user" ]);
+ok( $stderr =~ /permission denied for table test/,
+ "error message indicates user doesn't have sufficient privileges for the parent table");
+
+# clean up: drop the test_user role
+$node->safe_psql("postgres", "DROP ROLE test_user;");
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.47.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
@ 2025-02-19 10:31 ` Ayush Vatsa <[email protected]>
1 sibling, 0 replies; 19+ messages in thread
From: Ayush Vatsa @ 2025-02-19 10:31 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; +Cc: Tom Lane <[email protected]>; David G. Johnston <[email protected]>; Robert Haas <[email protected]>
Added the CF entry for the same -
https://commitfest.postgresql.org/patch/5583/
Regards,
Ayush Vatsa
SDE AWS
>
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
@ 2025-03-04 20:01 ` Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2025-03-04 20:01 UTC (permalink / raw)
To: Ayush Vatsa <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Feb 19, 2025 at 03:53:48PM +0530, Ayush Vatsa wrote:
> It seems there's a general consensus that we should maintain a
> original design to support pg_prewarm, with a minor adjustment:
> when querying indexes, we should verify the privileges of the parent table.
>
> I´ve attached a patch for this, which includes some test cases as well.
> Let me know if it needs any changes.
+ tableOid = IndexGetRelation(relOid, false);
+ aclresult = pg_class_aclcheck(tableOid, GetUserId(), ACL_SELECT);
I'm wondering whether setting missing_ok to true is correct here. IIUC we
should have an AccessShareLock on the index, but I don't know if that's
enough protection. The only other similar coding pattern I'm aware of is
RangeVarCallbackForReindexIndex(), which sets missing_ok to false and
attempts to gracefully handle a missing table. Of course, maybe that's
wrong, too.
Perhaps it's all close enough in practice. If we get it wrong, you might
get a slightly less helpful error message when the table is concurrently
dropped, which isn't so bad.
--
nathan
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-03-08 15:04 ` Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Ayush Vatsa @ 2025-03-08 15:04 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
> I'm wondering whether setting missing_ok to true is correct here. IIUC we
> should have an AccessShareLock on the index, but I don't know if that's
> enough protection.
Since we are already opening the relation with rel = relation_open(relOid,
AccessShareLock);,
if relOid does not exist, it will throw an error. If it does exist, we
acquire an AccessShareLock,
preventing it from being dropped.
By the time we reach IndexGetRelation(), we can be confident that relOid
exists and is
protected by the lock. Given this, it makes sense to keep missing_ok = false
here.
Let me know if you agree or if you see any scenario where
missing_ok = true would be preferable—I can update the condition
accordingly.
Thanks!
Ayush Vatsa
SDE AWS
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
@ 2025-03-08 21:08 ` Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2025-03-08 21:08 UTC (permalink / raw)
To: Ayush Vatsa <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Mar 08, 2025 at 08:34:40PM +0530, Ayush Vatsa wrote:
>> I'm wondering whether setting missing_ok to true is correct here. IIUC we
>> should have an AccessShareLock on the index, but I don't know if that's
>> enough protection.
>
> Since we are already opening the relation with rel = relation_open(relOid,
> AccessShareLock);,
> if relOid does not exist, it will throw an error. If it does exist, we
> acquire an AccessShareLock,
> preventing it from being dropped.
>
> By the time we reach IndexGetRelation(), we can be confident that relOid
> exists and is
> protected by the lock. Given this, it makes sense to keep missing_ok = false
> here.
>
> Let me know if you agree or if you see any scenario where
> missing_ok = true would be preferable-I can update the condition
> accordingly.
Right, we will have a lock on the index, but my concern is that we won't
have a lock on its table. I was specifically concerned that a concurrent
DROP TABLE could cause IndexGetRelation() to fail, i.e., emit a gross
"cache lookup failed" error. From a quick test and skim of the relevant
code, I think your patch is fine, though. IndexGetRelation() retrieves the
table OID from pg_index, so the OID should definitely be valid. And IIUC
DROP TABLE first acquires a lock on the table and its dependent objects
(e.g., indexes) before any actual deletions, so AFAICT there's no problem
with using it in pg_class_aclcheck() and get_rel_name(), either.
--
nathan
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-03-08 21:31 ` Ayush Vatsa <[email protected]>
2025-03-08 21:57 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Ayush Vatsa @ 2025-03-08 21:31 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
> From a quick test and skim of the relevant
> code, I think your patch is fine, though
Thanks for reviewing.
> And IIUC
> DROP TABLE first acquires a lock on the table and its dependent objects
> (e.g., indexes) before any actual deletions, so AFAICT there's no problem
> with using it in pg_class_aclcheck() and get_rel_name(), either.
True, I have also verified that from [1], hence I think we are safe here.
Maybe we can move ahead with the patch if we can see no other concerns.
[1]
https://github.com/postgres/postgres/blob/master/src/backend/catalog/dependency.c#L398-L430
Thanks,
Ayush Vatsa
SDE AWS
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
@ 2025-03-08 21:57 ` Nathan Bossart <[email protected]>
2025-03-08 22:17 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-03-16 13:00 ` Re: Clarification on Role Access Rights to Table Indexes vignesh C <[email protected]>
0 siblings, 2 replies; 19+ messages in thread
From: Nathan Bossart @ 2025-03-08 21:57 UTC (permalink / raw)
To: Ayush Vatsa <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sun, Mar 09, 2025 at 03:01:41AM +0530, Ayush Vatsa wrote:
> Maybe we can move ahead with the patch if we can see no other concerns.
I think we should allow some time in case others want to review the patch.
I do see a concern upthread about increased deadlock risk [0], but your
patch doesn't lock the table, but unless I'm wrong [1] (which is always
possible), it doesn't need to lock it.
Anyway, here is a tidied up patch.
[0] https://postgr.es/m/1246906.1739896202%40sss.pgh.pa.us
[1] https://postgr.es/m/Z8yxsm9ZWVkHlPbV%40nathan
--
nathan
From 326b4bfbb67485f40d77eaf97ea78bfef49b02f3 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Sat, 8 Mar 2025 15:45:32 -0600
Subject: [PATCH v2 1/1] pg_prewarm: For indexes, check privileges on table.
Author: Ayush Vatsa <[email protected]>
Discussion: https://postgr.es/m/CACX%2BKaMz2ZoOojh0nQ6QNBYx8Ak1Dkoko%3DD4FSb80BYW%2Bo8CHQ%40mail.gmail.com
Backpatch-through: 13
---
contrib/pg_prewarm/pg_prewarm.c | 13 +++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 ++++++++++++++++++++++++++++-
2 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index a2f0ac4af0c..57bd1527a50 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,6 +16,7 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
@@ -55,6 +56,7 @@ Datum
pg_prewarm(PG_FUNCTION_ARGS)
{
Oid relOid;
+ Oid permOid;
text *forkName;
text *type;
int64 first_block;
@@ -103,9 +105,16 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkString = text_to_cstring(forkName);
forkNumber = forkname_to_number(forkString);
- /* Open relation and check privileges. */
+ /*
+ * Open relation and check privileges. Indexes don't have their own
+ * privileges, so we check privileges on the table instead in that case.
+ */
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+ if (rel->rd_rel->relkind == RELKIND_INDEX)
+ permOid = IndexGetRelation(relOid, false);
+ else
+ permOid = relOid;
+ aclresult = pg_class_aclcheck(permOid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..a77ab67d29e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,31 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
Attachments:
[text/plain] v2-0001-pg_prewarm-For-indexes-check-privileges-on-table.patch (3.8K, 2-v2-0001-pg_prewarm-For-indexes-check-privileges-on-table.patch)
download | inline diff:
From 326b4bfbb67485f40d77eaf97ea78bfef49b02f3 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Sat, 8 Mar 2025 15:45:32 -0600
Subject: [PATCH v2 1/1] pg_prewarm: For indexes, check privileges on table.
Author: Ayush Vatsa <[email protected]>
Discussion: https://postgr.es/m/CACX%2BKaMz2ZoOojh0nQ6QNBYx8Ak1Dkoko%3DD4FSb80BYW%2Bo8CHQ%40mail.gmail.com
Backpatch-through: 13
---
contrib/pg_prewarm/pg_prewarm.c | 13 +++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 ++++++++++++++++++++++++++++-
2 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index a2f0ac4af0c..57bd1527a50 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,6 +16,7 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
@@ -55,6 +56,7 @@ Datum
pg_prewarm(PG_FUNCTION_ARGS)
{
Oid relOid;
+ Oid permOid;
text *forkName;
text *type;
int64 first_block;
@@ -103,9 +105,16 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkString = text_to_cstring(forkName);
forkNumber = forkname_to_number(forkString);
- /* Open relation and check privileges. */
+ /*
+ * Open relation and check privileges. Indexes don't have their own
+ * privileges, so we check privileges on the table instead in that case.
+ */
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+ if (rel->rd_rel->relkind == RELKIND_INDEX)
+ permOid = IndexGetRelation(relOid, false);
+ else
+ permOid = relOid;
+ aclresult = pg_class_aclcheck(permOid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..a77ab67d29e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,31 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:57 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-03-08 22:17 ` Tom Lane <[email protected]>
2025-03-09 01:35 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: Tom Lane @ 2025-03-08 22:17 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
Nathan Bossart <[email protected]> writes:
> I do see a concern upthread about increased deadlock risk [0], but your
> patch doesn't lock the table, but unless I'm wrong [1] (which is always
> possible), it doesn't need to lock it.
It bothers me a bit that this proposes to do something as complicated
as pg_class_aclcheck on a table we have no lock on. As you say, the
lock we hold on the index would prevent DROP TABLE, but that doesn't
mean we won't have any issues with other DDL on the table. Still,
taking a lock would be bad because of the deadlock hazard, and I
think the potential for conflicts with concurrent DDL is nonzero in
a lot of other places. So I don't have any concrete reason to object.
ReindexIndex() faces this same problem and solves it with some
very complex code that manages to get the table's lock first.
But I see that it's also doing pg_class_aclcheck on a table
it hasn't locked yet, so I don't think that adopting its approach
would do anything useful for us here.
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:57 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 22:17 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
@ 2025-03-09 01:35 ` Nathan Bossart <[email protected]>
2025-03-09 15:48 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2025-03-09 01:35 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Mar 08, 2025 at 05:17:40PM -0500, Tom Lane wrote:
> It bothers me a bit that this proposes to do something as complicated
> as pg_class_aclcheck on a table we have no lock on. As you say, the
> lock we hold on the index would prevent DROP TABLE, but that doesn't
> mean we won't have any issues with other DDL on the table. Still,
> taking a lock would be bad because of the deadlock hazard, and I
> think the potential for conflicts with concurrent DDL is nonzero in
> a lot of other places. So I don't have any concrete reason to object.
>
> ReindexIndex() faces this same problem and solves it with some
> very complex code that manages to get the table's lock first.
> But I see that it's also doing pg_class_aclcheck on a table
> it hasn't locked yet, so I don't think that adopting its approach
> would do anything useful for us here.
I noticed that amcheck's bt_index_check_internal() handles this problem,
and I think that approach could be adapted here:
relkind = get_rel_relkind(relOid);
if (relkind == RELKIND_INDEX || relkind == RELKIND_PARTITIONED_INDEX)
{
permOid = IndexGetRelation(relOid, true);
if (OidIsValid(permOid))
LockRelationOid(permOid, AccessShareLock);
else
fail = true;
}
else
permOid = relOid;
rel = relation_open(relOid, AccessShareLock);
if (fail ||
(permOid != relOid && permOid != IndexGetRelation(relOid, false)))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_TABLE),
errmsg("could not find parent table of index \"%s\"",
RelationGetRelationName(rel))));
aclresult = pg_class_aclcheck(permOid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
if (permOid != relOid)
UnlockRelationOid(permOid, AccessShareLock);
stats_lock_check_privileges() does something similar, but it's not as
cautious about the "heapid != IndexGetRelation(indrelid, false)" race
condition. Maybe RangeVarCallbackForReindexIndex() should be smarter about
this, too. That being said, this is a fair amount of complexity to handle
something that is in theory extremely rare...
--
nathan
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:57 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 22:17 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-03-09 01:35 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-03-09 15:48 ` Tom Lane <[email protected]>
2025-03-10 15:15 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Tom Lane @ 2025-03-09 15:48 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
Nathan Bossart <[email protected]> writes:
> On Sat, Mar 08, 2025 at 05:17:40PM -0500, Tom Lane wrote:
>> ReindexIndex() faces this same problem and solves it with some
>> very complex code that manages to get the table's lock first.
> I noticed that amcheck's bt_index_check_internal() handles this problem,
> ...
> stats_lock_check_privileges() does something similar, but it's not as
> cautious about the "heapid != IndexGetRelation(indrelid, false)" race
> condition.
Egad, we've already got three inconsistent implementations of this
functionality? I think the first step must be to unify them into
a common implementation, if at all possible.
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:57 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 22:17 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-03-09 01:35 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-09 15:48 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
@ 2025-03-10 15:15 ` Nathan Bossart <[email protected]>
2025-09-24 15:58 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2025-03-10 15:15 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sun, Mar 09, 2025 at 11:48:05AM -0400, Tom Lane wrote:
> Nathan Bossart <[email protected]> writes:
>> On Sat, Mar 08, 2025 at 05:17:40PM -0500, Tom Lane wrote:
>>> ReindexIndex() faces this same problem and solves it with some
>>> very complex code that manages to get the table's lock first.
>
>> I noticed that amcheck's bt_index_check_internal() handles this problem,
>> ...
>> stats_lock_check_privileges() does something similar, but it's not as
>> cautious about the "heapid != IndexGetRelation(indrelid, false)" race
>> condition.
>
> Egad, we've already got three inconsistent implementations of this
> functionality? I think the first step must be to unify them into
> a common implementation, if at all possible.
Agreed. I worry that trying to unify each bespoke implementation into a
single function might result in an unwieldy mess, but I'll give it a
shot...
--
nathan
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:57 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 22:17 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-03-09 01:35 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-09 15:48 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-03-10 15:15 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-09-24 15:58 ` Nathan Bossart <[email protected]>
2025-09-24 16:13 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Bossart @ 2025-09-24 15:58 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 10, 2025 at 10:15:19AM -0500, Nathan Bossart wrote:
> On Sun, Mar 09, 2025 at 11:48:05AM -0400, Tom Lane wrote:
>> Nathan Bossart <[email protected]> writes:
>>> On Sat, Mar 08, 2025 at 05:17:40PM -0500, Tom Lane wrote:
>>>> ReindexIndex() faces this same problem and solves it with some
>>>> very complex code that manages to get the table's lock first.
>>
>>> I noticed that amcheck's bt_index_check_internal() handles this problem,
>>> ...
>>> stats_lock_check_privileges() does something similar, but it's not as
>>> cautious about the "heapid != IndexGetRelation(indrelid, false)" race
>>> condition.
>>
>> Egad, we've already got three inconsistent implementations of this
>> functionality? I think the first step must be to unify them into
>> a common implementation, if at all possible.
>
> Agreed. I worry that trying to unify each bespoke implementation into a
> single function might result in an unwieldy mess, but I'll give it a
> shot...
I tried to unify these, but each one seems to be just different enough to
make it not worth the trouble. Instead, I took a look at each
implementation:
* amcheck's amcheck_lock_relation_and_check() seems correct to me.
* stats_lock_check_privileges() appears to be missing the second
IndexGetRelation() check after locking the table and index, so I added
that in 0001. Since this code is new to v18, I proposed to back-patch 0001
there.
* RangeVarCallbackForReindexIndex() was checking privileges on the table
before locking it, so I reversed it in 0002. Interestingly, this caused
test errors because LockRelationOid() checks for invalidation messages, so
the pg_class_aclcheck() call started failing with unhelpful errors due to
concurrently dropped relations. To deal with that, I switched to
pg_class_aclcheck_ext() so that we can handle missing relations.
Furthermore, I noticed that this callback seems to assume that as long as
the index does not change between calls, its table won't, either. That's
probably always true in practice, but even if it's completely true, I see
no reason to rely on it. So, I simplified the code to unconditionally
unlock any previously-locked table and to lock whatever IndexGetRelation()
returns. This could probably be back-patched, but in the absence of any
reports or any reproducible bugs, I don't think we should.
* 0003 fixes pg_prewarm's privilege checks by following a similar pattern.
This probably ought to get back-patched to all supported versions.
--
nathan
From 0e70810f8ef47a0e4e22a70a5c9a2a7776611950 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:28 -0500
Subject: [PATCH v3 1/3] fix priv checks in stats code
---
src/backend/statistics/stat_utils.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c
index ef7e5168bed..8b8203a58e3 100644
--- a/src/backend/statistics/stat_utils.c
+++ b/src/backend/statistics/stat_utils.c
@@ -189,6 +189,17 @@ stats_lock_check_privileges(Oid reloid)
Assert(index->rd_index && index->rd_index->indrelid == table_oid);
+ /*
+ * Since we did the IndexGetRelation() call above without any lock,
+ * it's barely possible that a race against an index drop/recreation
+ * could have netted us the wrong table.
+ */
+ if (table_oid != IndexGetRelation(index_oid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(index))));
+
/* retain lock on index */
relation_close(index, NoLock);
}
--
2.39.5 (Apple Git-154)
From 28fa582f8372a6d73adb79f4d28fe997a08a23e0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:36 -0500
Subject: [PATCH v3 2/3] fix priv checks in index code
---
src/backend/commands/indexcmds.c | 51 +++++++++++++++-----------------
1 file changed, 24 insertions(+), 27 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca2bde62e82..e4d05b5f8e6 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2976,6 +2976,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
struct ReindexIndexCallbackState *state = arg;
LOCKMODE table_lockmode;
Oid table_oid;
+ AclResult aclresult;
+ bool is_missing = false;
/*
* Lock level here should match table lock in reindex_index() for
@@ -2985,12 +2987,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
table_lockmode = (state->params.options & REINDEXOPT_CONCURRENTLY) != 0 ?
ShareUpdateExclusiveLock : ShareLock;
- /*
- * If we previously locked some other index's heap, and the name we're
- * looking up no longer refers to that relation, release the now-useless
- * lock.
- */
- if (relId != oldRelId && OidIsValid(oldRelId))
+ /* Unlock any previously locked heap. */
+ if (OidIsValid(state->locked_table_oid))
{
UnlockRelationOid(state->locked_table_oid, table_lockmode);
state->locked_table_oid = InvalidOid;
@@ -3014,30 +3012,29 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not an index", relation->relname)));
- /* Check permissions */
+ /*
+ * If the OID isn't valid, it means the index was concurrently dropped,
+ * which is not a problem for us; just return normally.
+ */
table_oid = IndexGetRelation(relId, true);
- if (OidIsValid(table_oid))
- {
- AclResult aclresult;
-
- aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
- }
+ if (!OidIsValid(table_oid))
+ return;
/* Lock heap before index to avoid deadlock. */
- if (relId != oldRelId)
- {
- /*
- * If the OID isn't valid, it means the index was concurrently
- * dropped, which is not a problem for us; just return normally.
- */
- if (OidIsValid(table_oid))
- {
- LockRelationOid(table_oid, table_lockmode);
- state->locked_table_oid = table_oid;
- }
- }
+ LockRelationOid(table_oid, table_lockmode);
+ state->locked_table_oid = table_oid;
+
+ /*
+ * Check permissions. Since LockRelationOid() checks for invalidation
+ * messages, the table might go missing here, too. As before, this isn't
+ * our problem; just return normally.
+ */
+ aclresult = pg_class_aclcheck_ext(table_oid, GetUserId(),
+ ACL_MAINTAIN, &is_missing);
+ if (is_missing)
+ return;
+ else if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
}
/*
--
2.39.5 (Apple Git-154)
From 84159c5fffd04ea20090e3cae5b9eb783820dd06 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:47:02 -0500
Subject: [PATCH v3 3/3] fix priv checks in pg_prewarm
---
contrib/pg_prewarm/pg_prewarm.c | 34 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 +++++++++++++++++++++++++-
2 files changed, 60 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index b968933ea8b..810a291204b 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,9 +16,11 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+#include "storage/lmgr.h"
#include "storage/read_stream.h"
#include "storage/smgr.h"
#include "utils/acl.h"
@@ -71,6 +73,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
char *ttype;
PrewarmType ptype;
AclResult aclresult;
+ char relkind;
+ Oid privOid;
/* Basic sanity checking. */
if (PG_ARGISNULL(0))
@@ -107,8 +111,31 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkNumber = forkname_to_number(forkString);
/* Open relation and check privileges. */
+ relkind = get_rel_relkind(relOid);
+ if (relkind == RELKIND_INDEX ||
+ relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ privOid = IndexGetRelation(relOid, false);
+ LockRelationOid(privOid, AccessShareLock);
+ }
+ else
+ privOid = relOid;
+
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+
+ /*
+ * If we did the IndexGetRelation() call above, it's barely possible that
+ * a race against an index drop/recreation could have netted us the wrong
+ * table.
+ */
+ if (privOid != relOid &&
+ privOid != IndexGetRelation(relOid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(rel))));
+
+ aclresult = pg_class_aclcheck(privOid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
@@ -233,8 +260,11 @@ pg_prewarm(PG_FUNCTION_ARGS)
read_stream_end(stream);
}
- /* Close relation, release lock. */
+ /* Close relation, release locks. */
relation_close(rel, AccessShareLock);
+ if (privOid != relOid)
+ UnlockRelationOid(privOid, AccessShareLock);
+
PG_RETURN_INT64(blocks_done);
}
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..a77ab67d29e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,31 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
Attachments:
[text/plain] v3-0001-fix-priv-checks-in-stats-code.patch (1.1K, 2-v3-0001-fix-priv-checks-in-stats-code.patch)
download | inline diff:
From 0e70810f8ef47a0e4e22a70a5c9a2a7776611950 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:28 -0500
Subject: [PATCH v3 1/3] fix priv checks in stats code
---
src/backend/statistics/stat_utils.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c
index ef7e5168bed..8b8203a58e3 100644
--- a/src/backend/statistics/stat_utils.c
+++ b/src/backend/statistics/stat_utils.c
@@ -189,6 +189,17 @@ stats_lock_check_privileges(Oid reloid)
Assert(index->rd_index && index->rd_index->indrelid == table_oid);
+ /*
+ * Since we did the IndexGetRelation() call above without any lock,
+ * it's barely possible that a race against an index drop/recreation
+ * could have netted us the wrong table.
+ */
+ if (table_oid != IndexGetRelation(index_oid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(index))));
+
/* retain lock on index */
relation_close(index, NoLock);
}
--
2.39.5 (Apple Git-154)
[text/plain] v3-0002-fix-priv-checks-in-index-code.patch (3.0K, 3-v3-0002-fix-priv-checks-in-index-code.patch)
download | inline diff:
From 28fa582f8372a6d73adb79f4d28fe997a08a23e0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:36 -0500
Subject: [PATCH v3 2/3] fix priv checks in index code
---
src/backend/commands/indexcmds.c | 51 +++++++++++++++-----------------
1 file changed, 24 insertions(+), 27 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca2bde62e82..e4d05b5f8e6 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2976,6 +2976,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
struct ReindexIndexCallbackState *state = arg;
LOCKMODE table_lockmode;
Oid table_oid;
+ AclResult aclresult;
+ bool is_missing = false;
/*
* Lock level here should match table lock in reindex_index() for
@@ -2985,12 +2987,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
table_lockmode = (state->params.options & REINDEXOPT_CONCURRENTLY) != 0 ?
ShareUpdateExclusiveLock : ShareLock;
- /*
- * If we previously locked some other index's heap, and the name we're
- * looking up no longer refers to that relation, release the now-useless
- * lock.
- */
- if (relId != oldRelId && OidIsValid(oldRelId))
+ /* Unlock any previously locked heap. */
+ if (OidIsValid(state->locked_table_oid))
{
UnlockRelationOid(state->locked_table_oid, table_lockmode);
state->locked_table_oid = InvalidOid;
@@ -3014,30 +3012,29 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not an index", relation->relname)));
- /* Check permissions */
+ /*
+ * If the OID isn't valid, it means the index was concurrently dropped,
+ * which is not a problem for us; just return normally.
+ */
table_oid = IndexGetRelation(relId, true);
- if (OidIsValid(table_oid))
- {
- AclResult aclresult;
-
- aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
- }
+ if (!OidIsValid(table_oid))
+ return;
/* Lock heap before index to avoid deadlock. */
- if (relId != oldRelId)
- {
- /*
- * If the OID isn't valid, it means the index was concurrently
- * dropped, which is not a problem for us; just return normally.
- */
- if (OidIsValid(table_oid))
- {
- LockRelationOid(table_oid, table_lockmode);
- state->locked_table_oid = table_oid;
- }
- }
+ LockRelationOid(table_oid, table_lockmode);
+ state->locked_table_oid = table_oid;
+
+ /*
+ * Check permissions. Since LockRelationOid() checks for invalidation
+ * messages, the table might go missing here, too. As before, this isn't
+ * our problem; just return normally.
+ */
+ aclresult = pg_class_aclcheck_ext(table_oid, GetUserId(),
+ ACL_MAINTAIN, &is_missing);
+ if (is_missing)
+ return;
+ else if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
}
/*
--
2.39.5 (Apple Git-154)
[text/plain] v3-0003-fix-priv-checks-in-pg_prewarm.patch (4.4K, 4-v3-0003-fix-priv-checks-in-pg_prewarm.patch)
download | inline diff:
From 84159c5fffd04ea20090e3cae5b9eb783820dd06 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:47:02 -0500
Subject: [PATCH v3 3/3] fix priv checks in pg_prewarm
---
contrib/pg_prewarm/pg_prewarm.c | 34 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 +++++++++++++++++++++++++-
2 files changed, 60 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index b968933ea8b..810a291204b 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,9 +16,11 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+#include "storage/lmgr.h"
#include "storage/read_stream.h"
#include "storage/smgr.h"
#include "utils/acl.h"
@@ -71,6 +73,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
char *ttype;
PrewarmType ptype;
AclResult aclresult;
+ char relkind;
+ Oid privOid;
/* Basic sanity checking. */
if (PG_ARGISNULL(0))
@@ -107,8 +111,31 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkNumber = forkname_to_number(forkString);
/* Open relation and check privileges. */
+ relkind = get_rel_relkind(relOid);
+ if (relkind == RELKIND_INDEX ||
+ relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ privOid = IndexGetRelation(relOid, false);
+ LockRelationOid(privOid, AccessShareLock);
+ }
+ else
+ privOid = relOid;
+
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+
+ /*
+ * If we did the IndexGetRelation() call above, it's barely possible that
+ * a race against an index drop/recreation could have netted us the wrong
+ * table.
+ */
+ if (privOid != relOid &&
+ privOid != IndexGetRelation(relOid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(rel))));
+
+ aclresult = pg_class_aclcheck(privOid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
@@ -233,8 +260,11 @@ pg_prewarm(PG_FUNCTION_ARGS)
read_stream_end(stream);
}
- /* Close relation, release lock. */
+ /* Close relation, release locks. */
relation_close(rel, AccessShareLock);
+ if (privOid != relOid)
+ UnlockRelationOid(privOid, AccessShareLock);
+
PG_RETURN_INT64(blocks_done);
}
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..a77ab67d29e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,31 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:57 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 22:17 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-03-09 01:35 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-09 15:48 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-03-10 15:15 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-09-24 15:58 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-09-24 16:13 ` Tom Lane <[email protected]>
2025-10-13 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Tom Lane @ 2025-09-24 16:13 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
Nathan Bossart <[email protected]> writes:
> * RangeVarCallbackForReindexIndex() was checking privileges on the table
> before locking it, so I reversed it in 0002.
Don't we do that intentionally, to make sure someone can't cause DOS
on a table they have no privileges on?
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:57 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 22:17 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-03-09 01:35 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-09 15:48 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-03-10 15:15 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-09-24 15:58 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-09-24 16:13 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
@ 2025-10-13 18:16 ` Jeff Davis <[email protected]>
2025-10-13 21:21 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Jeff Davis @ 2025-10-13 18:16 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, 2025-09-24 at 12:13 -0400, Tom Lane wrote:
> Don't we do that intentionally, to make sure someone can't cause DOS
> on a table they have no privileges on?
Is this only a problem for strong locks (ShareLock or greater)?
Strong locks are a problem when you have a pattern like a long running
query that holds an AccessShareLock, and then an unprivileged user
requests an AccessExclusiveLock, forcing other queries to queue up
behind it, and the queue doesn't clear until the long running query
finishes.
But weaker locks don't seem to have that problem, right?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:57 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 22:17 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-03-09 01:35 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-09 15:48 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-03-10 15:15 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-09-24 15:58 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-09-24 16:13 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-10-13 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
@ 2025-10-13 21:21 ` Tom Lane <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Tom Lane @ 2025-10-13 21:21 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
Jeff Davis <[email protected]> writes:
> On Wed, 2025-09-24 at 12:13 -0400, Tom Lane wrote:
>> Don't we do that intentionally, to make sure someone can't cause DOS
>> on a table they have no privileges on?
> Is this only a problem for strong locks (ShareLock or greater)?
> Strong locks are a problem when you have a pattern like a long running
> query that holds an AccessShareLock, and then an unprivileged user
> requests an AccessExclusiveLock, forcing other queries to queue up
> behind it, and the queue doesn't clear until the long running query
> finishes.
> But weaker locks don't seem to have that problem, right?
I don't think so. Even AccessShareLock is enough to block another
session trying to acquire AccessExclusiveLock, and then not only
have you DoS'd that session, but everything else trying to access
the table will queue up behind the AccessExclusiveLock request.
So it's only not-a-problem if nothing anywhere in the system wants
non-sharable locks.
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Re: Clarification on Role Access Rights to Table Indexes Tom Lane <[email protected]>
2025-02-18 18:16 ` Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-19 10:23 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Re: Clarification on Role Access Rights to Table Indexes Ayush Vatsa <[email protected]>
2025-03-08 21:57 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-03-16 13:00 ` vignesh C <[email protected]>
1 sibling, 0 replies; 19+ messages in thread
From: vignesh C @ 2025-03-16 13:00 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sun, 9 Mar 2025 at 03:27, Nathan Bossart <[email protected]> wrote:
>
> On Sun, Mar 09, 2025 at 03:01:41AM +0530, Ayush Vatsa wrote:
> > Maybe we can move ahead with the patch if we can see no other concerns.
>
> I think we should allow some time in case others want to review the patch.
> I do see a concern upthread about increased deadlock risk [0], but your
> patch doesn't lock the table, but unless I'm wrong [1] (which is always
> possible), it doesn't need to lock it.
>
> Anyway, here is a tidied up patch.
I noticed that Tom Lane's comment from [1] is not addressed. I'm
changing the commitfest entry status to Waiting on Author, Please
address them and update the status to Needs Review.
[1] - https://www.postgresql.org/message-id/279947.1741535285%40sss.pgh.pa.us
Regards,
Vignesh
^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2025-10-13 21:21 UTC | newest]
Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-02-18 16:21 Re: Clarification on Role Access Rights to Table Indexes Robert Haas <[email protected]>
2025-02-18 16:30 ` Tom Lane <[email protected]>
2025-02-18 18:16 ` Robert Haas <[email protected]>
2025-02-19 10:23 ` Ayush Vatsa <[email protected]>
2025-02-19 10:31 ` Ayush Vatsa <[email protected]>
2025-03-04 20:01 ` Nathan Bossart <[email protected]>
2025-03-08 15:04 ` Ayush Vatsa <[email protected]>
2025-03-08 21:08 ` Nathan Bossart <[email protected]>
2025-03-08 21:31 ` Ayush Vatsa <[email protected]>
2025-03-08 21:57 ` Nathan Bossart <[email protected]>
2025-03-08 22:17 ` Tom Lane <[email protected]>
2025-03-09 01:35 ` Nathan Bossart <[email protected]>
2025-03-09 15:48 ` Tom Lane <[email protected]>
2025-03-10 15:15 ` Nathan Bossart <[email protected]>
2025-09-24 15:58 ` Nathan Bossart <[email protected]>
2025-09-24 16:13 ` Tom Lane <[email protected]>
2025-10-13 18:16 ` Jeff Davis <[email protected]>
2025-10-13 21:21 ` Tom Lane <[email protected]>
2025-03-16 13:00 ` vignesh C <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox