Thread: Re: Clarification on Role Access Rights to Table Indexes
> As it stands, a superuser can prewarm an index (because she bypasses all
> privilege checks including this one), but nobody else can.
> privilege checks including this one), but nobody else can.
That's not fully true. Any role can prewarm an index if the role has the correct privileges.
postgres=# GRANT CREATE ON SCHEMA PUBLIC TO alpha;
GRANTpostgres=# SET ROLE alpha;
SET
postgres=> CREATE TABLE tab(id INT);
CREATE TABLE
postgres=> CREATE INDEX tab_idx ON tab(id);
CREATE INDEX
postgres=> SELECT pg_prewarm('tab_idx');
pg_prewarm
------------
1
(1 row)
Don't know what stopped it from prewarming the catalog table index.
Maybe it's checking who is the owner of the table. Although in code
it's just checking the ACL_SELECT [1]. I will be debugging more here
and maybe create a patch for the same.
it's just checking the ACL_SELECT [1]. I will be debugging more here
and maybe create a patch for the same.
postgres=# RESET ROLE;
RESET
postgres=# CREATE TABLE superuser_tab(id INT);
CREATE TABLE
postgres=# CREATE INDEX idx_superuser_tab ON superuser_tab(id);
CREATE INDEX
postgres=# GRANT SELECT ON superuser_tab TO alpha;
GRANT
postgres=# SET ROLE alpha;
SET
postgres=> SELECT pg_prewarm('superuser_tab');
pg_prewarm
------------
0
(1 row)
RESET
postgres=# CREATE TABLE superuser_tab(id INT);
CREATE TABLE
postgres=# CREATE INDEX idx_superuser_tab ON superuser_tab(id);
CREATE INDEX
postgres=# GRANT SELECT ON superuser_tab TO alpha;
GRANT
postgres=# SET ROLE alpha;
SET
postgres=> SELECT pg_prewarm('superuser_tab');
pg_prewarm
------------
0
(1 row)
postgres=> SELECT pg_prewarm('idx_superuser_tab');
ERROR: permission denied for index idx_superuser_tab
ERROR: permission denied for index idx_superuser_tab
postgres=> RESET ROLE;
RESET
postgres=# ALTER TABLE superuser_tab OWNER TO alpha;
ALTER TABLE
postgres=# SET ROLE alpha;
SET
postgres=> SELECT pg_prewarm('idx_superuser_tab');
pg_prewarm
------------
1
(1 row)
RESET
postgres=# ALTER TABLE superuser_tab OWNER TO alpha;
ALTER TABLE
postgres=# SET ROLE alpha;
SET
postgres=> SELECT pg_prewarm('idx_superuser_tab');
pg_prewarm
------------
1
(1 row)
But I agree we should just check the table privileges even in the case of
indexes to decide whether to prewarm or not, as indexes don't have any privileges
of their own.
> I think -hackers would be the appropriate location for that.
I am shifting this to -hackers mailing list instead of general.
[1] https://github.com/postgres/postgres/blob/master/contrib/pg_prewarm/pg_prewarm.c#L108-L110
Regards,
Ayush Vatsa
SDE AWS
indexes to decide whether to prewarm or not, as indexes don't have any privileges
of their own.
> I think -hackers would be the appropriate location for that.
I am shifting this to -hackers mailing list instead of general.
[1] https://github.com/postgres/postgres/blob/master/contrib/pg_prewarm/pg_prewarm.c#L108-L110
Regards,
Ayush Vatsa
SDE AWS
Ayush Vatsa <ayushvatsa1810@gmail.com> writes:
>> As it stands, a superuser can prewarm an index (because she bypasses all
>> privilege checks including this one), but nobody else can.
> That's not fully true. Any role can prewarm an index if the role has the
> correct privileges.
Ah, right. An index will have null pg_class.relacl, which'll be
interpreted as "owner has all rights", so it will work for the
table owner too. Likely this explains the lack of prior complaints.
It's still a poor design IMO.
regards, tom lane
On Mon, Feb 17, 2025 at 2:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Ayush Vatsa <ayushvatsa1810@gmail.com> writes: > >> As it stands, a superuser can prewarm an index (because she bypasses all > >> privilege checks including this one), but nobody else can. > > > That's not fully true. Any role can prewarm an index if the role has the > > correct privileges. > > Ah, right. An index will have null pg_class.relacl, which'll be > interpreted as "owner has all rights", so it will work for the > table owner too. Likely this explains the lack of prior complaints. > It's still a poor design IMO. I'm not sure if I'd call that a "design". Sounds like I just made a mistake here. -- Robert Haas EDB: http://www.enterprisedb.com
> I'm not sure if I'd call that a "design". Sounds like I just made a
> mistake here.
> mistake here.
Thanks Robert for confirming, let me submit a patch to fix the same.
Regards
Ayush Vatsa
ADE AWS
Ayush Vatsa <ayushvatsa1810@gmail.com> writes:
> Thanks Robert for confirming, let me submit a patch to fix the same.
Well, the first thing you need is consensus on what the behavior
should be instead.
I have a very vague recollection that we concluded that SELECT
privilege was a reasonable check because if you have that you
could manually prewarm by reading the table. That would lead
to the conclusion that the minimal fix is to look at the owning
table's privileges instead of the index's own privileges.
Or we could switch to using ownership, which'd keep the code
simple but some users might complain it's too restrictive.
While I mentioned built-in roles earlier, I now think those mostly
carry more privilege than should be required here, given the analogy
to SELECT.
regards, tom lane
On Mon, Feb 17, 2025 at 3:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ayush Vatsa <ayushvatsa1810@gmail.com> writes:
> Thanks Robert for confirming, let me submit a patch to fix the same.
Well, the first thing you need is consensus on what the behavior
should be instead.
I have a very vague recollection that we concluded that SELECT
privilege was a reasonable check because if you have that you
could manually prewarm by reading the table. That would lead
to the conclusion that the minimal fix is to look at the owning
table's privileges instead of the index's own privileges.
I feel like if you can blow up the cache by loading an entire table into memory with just select privilege on the table we should be ok with allowing the same person to name an index on the same table and load it into the cache too.
David J.
On Mon, Feb 17, 2025 at 5:18 PM David G. Johnston <david.g.johnston@gmail.com> wrote: >> I have a very vague recollection that we concluded that SELECT >> privilege was a reasonable check because if you have that you >> could manually prewarm by reading the table. That would lead >> to the conclusion that the minimal fix is to look at the owning >> table's privileges instead of the index's own privileges. > > I feel like if you can blow up the cache by loading an entire table into memory with just select privilege on the tablewe should be ok with allowing the same person to name an index on the same table and load it into the cache too. +1. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Feb 17, 2025 at 5:18 PM David G. Johnston
> <david.g.johnston@gmail.com> wrote:
>>> I have a very vague recollection that we concluded that SELECT
>>> privilege was a reasonable check because if you have that you
>>> could manually prewarm by reading the table. That would lead
>>> to the conclusion that the minimal fix is to look at the owning
>>> table's privileges instead of the index's own privileges.
>> I feel like if you can blow up the cache by loading an entire table into memory with just select privilege on the
tablewe should be ok with allowing the same person to name an index on the same table and load it into the cache too.
> +1.
Is that a +1 for the specific design of "check SELECT on the index's
table", or just a +1 for changing something here?
regards, tom lane
On Tue, Feb 18, 2025 at 11:02 AM Tom Lane <tgl@sss.pgh.pa.us> 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
Robert Haas <robertmhaas@gmail.com> 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
On Tue, Feb 18, 2025 at 11:30 AM Tom Lane <tgl@sss.pgh.pa.us> 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
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
Attachment
Added the CF entry for the same - https://commitfest.postgresql.org/patch/5583/
Regards,
Ayush Vatsa
Ayush Vatsa
SDE AWS
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
> 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.
> 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
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
> From a quick test and skim of the relevant
> code, I think your patch is fine, though
Thanks for reviewing.
> (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.
> 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.
Maybe we can move ahead with the patch if we can see no other concerns.
Thanks,
Ayush Vatsa
SDE AWS
Ayush Vatsa
SDE AWS
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
Attachment
Nathan Bossart <nathandbossart@gmail.com> 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
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
Nathan Bossart <nathandbossart@gmail.com> 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
On Sun, Mar 09, 2025 at 11:48:05AM -0400, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> 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
On Sun, 9 Mar 2025 at 03:27, Nathan Bossart <nathandbossart@gmail.com> 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