public inbox for [email protected]
help / color / mirror / Atom feedRe: Clarification on Role Access Rights to Table Indexes
39+ messages / 7 participants
[nested] [flat]
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-02-18 16:21 Robert Haas <[email protected]>
0 siblings, 1 reply; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-02-18 16:30 Tom Lane <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-02-18 18:16 Robert Haas <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-02-19 10:23 Ayush Vatsa <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 2 replies; 39+ 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]>; pgsql-hackers
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, ../../CACX+KaNAbOzePn710EtzH9F5xiUdBC+u59=UMab=Wr8jgDKQtw@mail.gmail.com/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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-02-19 10:31 Ayush Vatsa <[email protected]>
parent: Ayush Vatsa <[email protected]>
1 sibling, 0 replies; 39+ messages in thread
From: Ayush Vatsa @ 2025-02-19 10:31 UTC (permalink / raw)
To: pgsql-hackers; +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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-03-04 20:01 Nathan Bossart <[email protected]>
parent: Ayush Vatsa <[email protected]>
1 sibling, 1 reply; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-03-08 15:04 Ayush Vatsa <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ 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]>; pgsql-hackers
> 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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-03-08 21:08 Nathan Bossart <[email protected]>
parent: Ayush Vatsa <[email protected]>
0 siblings, 1 reply; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-03-08 21:31 Ayush Vatsa <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ 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]>; pgsql-hackers
> 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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-03-08 21:57 Nathan Bossart <[email protected]>
parent: Ayush Vatsa <[email protected]>
0 siblings, 2 replies; 39+ 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]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-03-08 22:17 Tom Lane <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 1 reply; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-03-09 01:35 Nathan Bossart <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-03-09 15:48 Tom Lane <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-03-10 15:15 Nathan Bossart <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-03-16 13:00 vignesh C <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 0 replies; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-09-24 15:58 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ 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]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-09-24 16:13 Tom Lane <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 2 replies; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-09-24 16:52 Nathan Bossart <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 2 replies; 39+ messages in thread
From: Nathan Bossart @ 2025-09-24 16:52 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Wed, Sep 24, 2025 at 12:13:34PM -0400, Tom Lane wrote:
> 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?
Ah, right. I switched it back in v4.
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-02 17:37 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 0 replies; 39+ messages in thread
From: Nathan Bossart @ 2025-10-02 17:37 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Wed, Sep 24, 2025 at 11:52:09AM -0500, Nathan Bossart wrote:
> Ah, right. I switched it back in v4.
Unless more feedback materializes, I'm planning to commit these sometime
next week.
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-09 03:06 Jeff Davis <[email protected]>
parent: Nathan Bossart <[email protected]>
1 sibling, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2025-10-09 03:06 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Wed, 2025-09-24 at 11:52 -0500, Nathan Bossart wrote:
> On Wed, Sep 24, 2025 at 12:13:34PM -0400, Tom Lane wrote:
> > 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?
>
> Ah, right. I switched it back in v4.
v4-0001 looks good to me.
Just to make sure I understand: the actual problem would only happen
with OID wraparound, right?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-09 03:28 Jeff Davis <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2025-10-09 03:28 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Wed, 2025-10-08 at 20:06 -0700, Jeff Davis wrote:
> On Wed, 2025-09-24 at 11:52 -0500, Nathan Bossart wrote:
> > On Wed, Sep 24, 2025 at 12:13:34PM -0400, Tom Lane wrote:
> > > 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?
> >
> > Ah, right. I switched it back in v4.
>
> v4-0001 looks good to me.
Actually, now I'm unsure. v4-0001 is taking a lock on the table before
checking privileges, whereas v4-0002 is going to some effort to avoid
that. Is that because the latter is taking a ShareLock?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-09 15:39 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2025-10-09 15:39 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Wed, Oct 08, 2025 at 08:28:01PM -0700, Jeff Davis wrote:
> Actually, now I'm unsure. v4-0001 is taking a lock on the table before
> checking privileges, whereas v4-0002 is going to some effort to avoid
> that. Is that because the latter is taking a ShareLock?
I was confused by this, too. We seem to go to great lengths to avoid
taking a lock before checking permissions in RangeVarGetRelidExtended(),
but in pg_prewarm() and this stats code, we are taking the lock first.
pg_prewarm() can't use RangeVarGetRelid because you give it the OID, but
I'm not seeing why stat_utils.c can't use it. We should probably fix this.
I wouldn't be surprised if there are other examples.
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-09 21:18 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2025-10-09 21:18 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Thu, Oct 09, 2025 at 10:39:32AM -0500, Nathan Bossart wrote:
> On Wed, Oct 08, 2025 at 08:28:01PM -0700, Jeff Davis wrote:
>> Actually, now I'm unsure. v4-0001 is taking a lock on the table before
>> checking privileges, whereas v4-0002 is going to some effort to avoid
>> that. Is that because the latter is taking a ShareLock?
>
> I was confused by this, too. We seem to go to great lengths to avoid
> taking a lock before checking permissions in RangeVarGetRelidExtended(),
> but in pg_prewarm() and this stats code, we are taking the lock first.
> pg_prewarm() can't use RangeVarGetRelid because you give it the OID, but
> I'm not seeing why stat_utils.c can't use it. We should probably fix this.
> I wouldn't be surprised if there are other examples.
I spent some time trying to change pg_prewarm() to check permissions before
locking and came up with the attached. There are certainly issues with the
patch, but this at least demonstrates the complexity required. I'm tempted
to say that this is more trouble than it's worth, but it does feel a little
weird to leave it as-is.
There's a similar pattern in get_rel_from_relname() in dblink.c, which also
seems to only be used with an AccessShareLock (like pg_prewarm). My best
guess from reading lots of code, commit messages, and old e-mails in the
archives is that the original check-privileges-before-locking work was
never completed.
I'm currently leaning towards continuing with v4 of the patch set. 0001
and 0003 are a little weird in that a concurrent change could lead to a
"could not find parent table" ERROR, but IIUC that is an extremely remote
possibility.
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-10 16:26 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2025-10-10 16:26 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Thu, Oct 09, 2025 at 04:18:03PM -0500, Nathan Bossart wrote:
> There's a similar pattern in get_rel_from_relname() in dblink.c, which also
> seems to only be used with an AccessShareLock (like pg_prewarm). My best
> guess from reading lots of code, commit messages, and old e-mails in the
> archives is that the original check-privileges-before-locking work was
> never completed.
I added an 0004 that changes dblink to use RangeVarGetRelidExtended().
> I'm currently leaning towards continuing with v4 of the patch set. 0001
> and 0003 are a little weird in that a concurrent change could lead to a
> "could not find parent table" ERROR, but IIUC that is an extremely remote
> possibility.
After sleeping on it, I still think this is the right call. In any case,
I've spent way too much time on this stuff, so I plan to commit the
attached soon.
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-10 18:31 Jeff Davis <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2025-10-10 18:31 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Fri, 2025-10-10 at 11:26 -0500, Nathan Bossart wrote:
> On Thu, Oct 09, 2025 at 04:18:03PM -0500, Nathan Bossart wrote:
> > There's a similar pattern in get_rel_from_relname() in dblink.c,
> > which also
> > seems to only be used with an AccessShareLock (like pg_prewarm).
> > My best
> > guess from reading lots of code, commit messages, and old e-mails
> > in the
> > archives is that the original check-privileges-before-locking work
> > was
> > never completed.
Interesting, thank you for the analysis.
> > I'm currently leaning towards continuing with v4 of the patch set.
> > 0001
> > and 0003 are a little weird in that a concurrent change could lead
> > to a
> > "could not find parent table" ERROR, but IIUC that is an extremely
> > remote
> > possibility.
>
> After sleeping on it, I still think this is the right call. In any
> case,
> I've spent way too much time on this stuff, so I plan to commit the
> attached soon.
I'm OK with that. v5-0001 is an improvement over the current situation.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-13 18:16 Jeff Davis <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 1 reply; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-13 19:30 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2025-10-13 19:30 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Fri, Oct 10, 2025 at 11:31:03AM -0700, Jeff Davis wrote:
> On Fri, 2025-10-10 at 11:26 -0500, Nathan Bossart wrote:
>> After sleeping on it, I still think this is the right call. In any
>> case, I've spent way too much time on this stuff, so I plan to commit
>> the attached soon.
>
> I'm OK with that. v5-0001 is an improvement over the current situation.
Okay, I lied. I spent even more time on these patches and came up with the
attached. Here's a summary of what's going on:
* 0001 moves the code for stats clearing/setting to use
RangeVarGetRelidExtended(). The existing code looks up the relation, locks
it, and then checks permissions. There's no guarantee that the relation
you looked up didn't concurrently change before locking, and locking before
privilege checks is troublesome from a DOS perspective. One downside of
using RangeVarGetRelidExtended() is that we can't use AccessShareLock for
regular indexes, but I'm not sure that's really a problem since we're
already using ShareUpdateExclusiveLock for everything else. The
RangeVarGetRelidExtended() callback is similar to the one modified by 0002.
This should be back-patched to v18.
* 0002 fixes the RangeVarGetRelidExtended() callback for REINDEX INDEX to
handle unlikely scenarios involving OID reuse (e.g., lookup returns the
same index OID for a different table). I did confirm there was a bug here
by concurrently re-creating an index with the same OID for a heap with a
different OID (via the pg_upgrade support functions). In previous versions
of this patch, I tried to fix this by unconditionally unlocking the heap at
the beginning of the callback, but upon further inspection, I noticed that
creates deadlock hazards because we might've already locked the index. (We
need to lock the heap first.) In v6, I've just added an ERROR for these
extremely unlikely scenarios. I've also replaced all early returns in this
function with ERRORs (except for the invalid relId case). AFAICT the extra
checks are unecessary, and even if they were necessary, I think they break
some of the code related to heap locking in subtle ways. Some callbacks do
these extra checks, and others do not, and AFAIK there haven't been any
reported problems either way. 0002 should be back-patched to v13, but it
will look a little different on v16 and newer, i.e., before MAINTAIN was
added.
* 0003 fixes the privilege checks in pg_prewarm by using a similar approach
to amcheck_lock_relation_and_check(). This seems correct to me, but it
does add more locking. This should be back-patched to v13.
* 0004 is a small patch to teach dblink to use RangeVarGetRelidExtended().
I believe this code predates that function. I don't intend to back-patch
this one.
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-13 21:21 Tom Lane <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 39+ 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]>; pgsql-hackers
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] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-13 22:11 Jeff Davis <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2025-10-13 22:11 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Mon, 2025-10-13 at 17:21 -0400, Tom Lane wrote:
> 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.
I tried imagining how that could be a problem, but couldn't come up
with anything. If the privilege check is right after the lock, then
either:
(a) The malicious AccessShareLock is granted, then is quickly released
when the privilege check fails and the transaction aborts; or
(b) The malicious AccessShareLock is queued behind a legitimate
AccessExclusiveLock, in which case every other lock would be queued up
as well. As soon as the AccessExclusiveLock is released, the
AccessShareLock would be granted, but quickly released when the
privilege check fails.
For it to be a problem, the malicious lock needs to be strong enough to
conflict with a lock level weaker than itself, i.e. ShareLock or
stronger.
I'm not sure we save anything by being lazier for weaker lock levels,
so perhaps the point is irrelevant. But if I'm missing something,
please let me know.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-14 02:23 Jeff Davis <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Jeff Davis @ 2025-10-14 02:23 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Corey Huinker <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Mon, 2025-10-13 at 14:30 -0500, Nathan Bossart wrote:
> * 0001 moves the code for stats clearing/setting to use
> RangeVarGetRelidExtended(). The existing code looks up the relation,
> locks
> it, and then checks permissions. There's no guarantee that the
> relation
> you looked up didn't concurrently change before locking, and locking
> before
> privilege checks is troublesome from a DOS perspective. One downside
> of
> using RangeVarGetRelidExtended() is that we can't use AccessShareLock
> for
> regular indexes, but I'm not sure that's really a problem since we're
> already using ShareUpdateExclusiveLock for everything else. The
> RangeVarGetRelidExtended() callback is similar to the one modified by
> 0002.
> This should be back-patched to v18.
We tried to match the locking behavior for analyze. Originally, that's
because we were using in-place updates, which required those specific
kinds of locks. Now that the in-place code is gone, then I think it's
OK to use ShareUpdateExclusiveLock for indexes, too, but it is a
notable difference in behavior.
Including Corey in case he has comments.
As for the patch itself, it looks good to me. Stylistically I might
have kept the "index_oid" variable, which makes some of the tests a bit
clearer, but I don't have a strong opinion.
The unlikely scenarios are a bit confusing. I'd probably error for
either case. Also, the error message on the second scenario is wrong if
the previous lookup was a table, I think.
> * 0002 fixes the RangeVarGetRelidExtended() callback for REINDEX
> INDEX to
> handle unlikely scenarios involving OID reuse (e.g., lookup returns
> the
> same index OID for a different table). I did confirm there was a bug
> here
> by concurrently re-creating an index with the same OID for a heap
> with a
> different OID (via the pg_upgrade support functions). In previous
> versions
> of this patch, I tried to fix this by unconditionally unlocking the
> heap at
> the beginning of the callback, but upon further inspection, I noticed
> that
> creates deadlock hazards because we might've already locked the
> index. (We
> need to lock the heap first.) In v6, I've just added an ERROR for
> these
> extremely unlikely scenarios. I've also replaced all early returns
> in this
> function with ERRORs (except for the invalid relId case).
+1 for throwing errors when we have race conditions combined with name
reuse. Looks fine to me.
>
> * 0003 fixes the privilege checks in pg_prewarm by using a similar
> approach
> to amcheck_lock_relation_and_check(). This seems correct to me, but
> it
> does add more locking. This should be back-patched to v13.
IIUC this is locking before the privilege check. Is there a reason why
we think this is OK here (and in amcheck_lock_relation_and_check()) but
not for the stats?
> * 0004 is a small patch to teach dblink to use
> RangeVarGetRelidExtended().
> I believe this code predates that function. I don't intend to back-
> patch
> this one.
Looks good.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-14 16:05 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2025-10-14 16:05 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
Thanks for reviewing.
On Mon, Oct 13, 2025 at 07:23:36PM -0700, Jeff Davis wrote:
> The unlikely scenarios are a bit confusing. I'd probably error for
> either case. Also, the error message on the second scenario is wrong if
> the previous lookup was a table, I think.
Yeah, I think that's a better idea.
> IIUC this is locking before the privilege check. Is there a reason why
> we think this is OK here (and in amcheck_lock_relation_and_check()) but
> not for the stats?
For amcheck, AFAICT there aren't actually any ACL checks within the code
because the function is restricted to superuser by default. For
pg_prewarm, I don't know. You do have to install the extension before
using it, but once installed, it's available to everyone by default. My
guess is that it just hasn't been a problem in the field.
Regardless, fixing the lock-before-privilege-checks behavior doesn't strike
me as a bug, so I think we ought to proceed with something like 0003 for
back-patching purposes and then to rework it further for v19. Does that
sound okay to you?
>> * 0004 is a small patch to teach dblink to use
>> RangeVarGetRelidExtended(). I believe this code predates that
>> function. I don't intend to back-patch this one.
>
> Looks good.
I'm going to go commit this one now to get it out of the way.
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-14 16:30 Tom Lane <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 0 replies; 39+ messages in thread
From: Tom Lane @ 2025-10-14 16:30 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]>; pgsql-hackers
Jeff Davis <[email protected]> writes:
> On Mon, 2025-10-13 at 17:21 -0400, Tom Lane wrote:
>> 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.
> I tried imagining how that could be a problem, but couldn't come up
> with anything. If the privilege check is right after the lock, then
> either:
> (a) The malicious AccessShareLock is granted, then is quickly released
> when the privilege check fails and the transaction aborts; or
> (b) The malicious AccessShareLock is queued behind a legitimate
> AccessExclusiveLock, in which case every other lock would be queued up
> as well. As soon as the AccessExclusiveLock is released, the
> AccessShareLock would be granted, but quickly released when the
> privilege check fails.
> For it to be a problem, the malicious lock needs to be strong enough to
> conflict with a lock level weaker than itself, i.e. ShareLock or
> stronger.
Robert might remember better, but I think the assumption behind
the current design of RangeVarGetRelidExtended is that it's not
okay to take a lock in the first place if you don't have the
privilege to do so. Your analysis here supposes that it's okay
to take a lock without privileges so long as you can't block someone
else for very long, where "very long" is not tightly defined but
hopefully isn't controllable by the malicious user. So that's
moving the goalposts somewhat, but you might get people to sign
onto it with more careful analysis of the worst-case delay.
(The thing I'd worry about is whether it's possible to block
execution of the privilege check, or even just make it slow.)
Given that definition, I think you're right that it's possible to
identify cases where lock-then-check can't cause meaningful DoS.
RangeVarGetRelidExtended has to cope with the general case, so
that's not an argument for simplifying it, but we might not need
equivalent complexity everywhere.
regards, tom lane
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-14 17:01 Jeff Davis <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 2 replies; 39+ messages in thread
From: Jeff Davis @ 2025-10-14 17:01 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Tue, 2025-10-14 at 11:05 -0500, Nathan Bossart wrote:
> For
> pg_prewarm, I don't know. You do have to install the extension
> before
> using it, but once installed, it's available to everyone by default.
> My
> guess is that it just hasn't been a problem in the field.
If we start with an OID, what's the right way to do these kinds of
checks? Could we do an ACL check, then lock it, then do an ACL check
again to catch OID wraparound?
Last-minute suggestions on 0003:
* Add a comment around the privOid check to explain that, if the
object is an index, we must check the privileges on the table instead.
* Clarify in the comment that the race against index drop/recreation
involves OID wraparound.
+1 to the patch and backpatch.
As a separate thought, I'm wondering if we should do more to enforce
the idea that we check the privileges and owner of an index's table,
and never the index itself. That's for another discussion, though.
> Regardless, fixing the lock-before-privilege-checks behavior doesn't
> strike
> me as a bug, so I think we ought to proceed with something like 0003
> for
> back-patching purposes and then to rework it further for v19. Does
> that
> sound okay to you?
According to the current rules[1], it does seem to technically be a
bug, but as far as I can tell, not one of much consequence.
Regards,
Jeff Davis
[1]
https://www.postgresql.org/message-id/[email protected]
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-14 21:32 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
1 sibling, 0 replies; 39+ messages in thread
From: Nathan Bossart @ 2025-10-14 21:32 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
I've committed 0004. Here is an updated patch set.
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-15 15:39 Nathan Bossart <[email protected]>
parent: Jeff Davis <[email protected]>
1 sibling, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2025-10-15 15:39 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Tue, Oct 14, 2025 at 10:01:37AM -0700, Jeff Davis wrote:
> If we start with an OID, what's the right way to do these kinds of
> checks? Could we do an ACL check, then lock it, then do an ACL check
> again to catch OID wraparound?
I tried something like this upthread [0]. My feeling was that this was a
lot of complexity for not a lot of gain. Perhaps it's still worth doing,
though.
[0] https://postgr.es/m/aOgmi6avE6qMw_6t%40nathan
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-15 17:54 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2025-10-15 17:54 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
I just pushed 0001, and longfin and sifaka have very quickly reminded me
that we don't require C11 on v18. Will fix shortly...
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-15 21:38 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 39+ messages in thread
From: Nathan Bossart @ 2025-10-15 21:38 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
I've committed everything except for 0003, which I probably won't get to
until Friday. Note that I decided against back-patching 0002 because of
the presumed rarity of and lack of reports for the bug.
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-10-17 16:56 Nathan Bossart <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 0 replies; 39+ messages in thread
From: Nathan Bossart @ 2025-10-17 16:56 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On Wed, Oct 15, 2025 at 04:38:05PM -0500, Nathan Bossart wrote:
> I've committed everything except for 0003, which I probably won't get to
> until Friday. Note that I decided against back-patching 0002 because of
> the presumed rarity of and lack of reports for the bug.
Everything tracked in this thread is now committed.
--
nathan
^ permalink raw reply [nested|flat] 39+ messages in thread
* [PATCH v13 2/2] handle relation statistics correctly during rewrites
@ 2025-11-04 13:52 Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 39+ messages in thread
From: Bertrand Drouvot @ 2025-11-04 13:52 UTC (permalink / raw)
Now that PGSTAT_KIND_RELATION is keyed by refilenode, we need to handle rewrites.
To do so, this patch:
- Adds PgStat_PendingRewrite, a new struct to track rewrite operations within
a transaction, storing the old locator, new locator, and original locator (for
rewrite chains). This allows stats to be copied from the original location to
the final location at commit time.
- Adds a new function, pgstat_mark_rewrite(), called when a table rewrite begins.
It records the rewrite operation in a local list and detects rewrite chains by
checking if the old_locator matches any existing new_locator, preserving the
chain's original_locator.
- Modifies pgstat_copy_relation_stats(), to accept RelFileLocators instead of
Relations, with a new increment parameter to accumulate stats (needed for rewrite
chains with DML between rewrites).
- Ensures that AtEOXact_PgStat_Relations(), AtPrepare_PgStat_Relations(),
pgstat_twophase_postcommit()/postabort() pgstat_drop_relation() handle the
PgStat_PendingRewrite list correctly.
Note that due to the new flush call in pgstat_twophase_postcommit() we can not
call GetCurrentTransactionStopTimestamp() in pgstat_relation_flush_cb(). So,
adding a check to handle this special case and call GetCurrentTimestamp() instead.
Note that we'd call GetCurrentTimestamp() only if there is a rewrite, so that
the GetCurrentTimestamp() extra cost should be negligible. Another solution
could be to trigger the flush from FinishPreparedTransaction() but that's not
worth the extra complexity.
The new pending_rewrites list is traversed in multiple places. The overhead
should be negligible in comparison to a rewrite and the list should not contain
a lot of rewrites in practice.
The pending_rewrites list is traversed in multiple places. In typical usage,
the list will contain only a few entries so the traversal cost is negligible (
furthermore in comparison to a rewrite).
---
src/backend/catalog/index.c | 2 +-
src/backend/commands/cluster.c | 5 +
src/backend/commands/tablecmds.c | 6 +
src/backend/utils/activity/pgstat_relation.c | 391 ++++++++++++++++++-
src/backend/utils/activity/pgstat_xact.c | 25 +-
src/backend/utils/cache/relcache.c | 6 +
src/include/pgstat.h | 5 +-
src/tools/pgindent/typedefs.list | 1 +
8 files changed, 424 insertions(+), 17 deletions(-)
92.8% src/backend/utils/activity/
4.9% src/backend/
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8b3c60d91f9..7c7d0fb8e7a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1796,7 +1796,7 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
/* copy over statistics from old to new index */
- pgstat_copy_relation_stats(newClassRel, oldClassRel);
+ pgstat_copy_relation_stats(newClassRel->rd_locator, oldClassRel->rd_locator, false);
/* Copy data of pg_statistic from the old index to the new one */
CopyStatistics(oldIndexId, newIndexId);
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 09066db0956..c491daceb0f 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1206,6 +1206,11 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
rel1 = relation_open(r1, NoLock);
rel2 = relation_open(r2, NoLock);
+
+ /* Mark that a rewrite happened */
+ if (RELKIND_HAS_STORAGE(rel1->rd_rel->relkind))
+ pgstat_mark_rewrite(rel1->rd_locator, rel2->rd_locator);
+
rel2->rd_createSubid = rel1->rd_createSubid;
rel2->rd_newRelfilelocatorSubid = rel1->rd_newRelfilelocatorSubid;
rel2->rd_firstRelfilelocatorSubid = rel1->rd_firstRelfilelocatorSubid;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 67e42e5df29..1715af2c0d8 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -17099,6 +17099,7 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
Oid reltoastrelid;
RelFileNumber newrelfilenumber;
RelFileLocator newrlocator;
+ RelFileLocator oldrlocator;
List *reltoastidxids = NIL;
ListCell *lc;
@@ -17137,6 +17138,7 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
newrlocator = rel->rd_locator;
newrlocator.relNumber = newrelfilenumber;
newrlocator.spcOid = newTableSpace;
+ oldrlocator = rel->rd_locator;
/* hand off to AM to actually create new rel storage and copy the data */
if (rel->rd_rel->relkind == RELKIND_INDEX)
@@ -17149,6 +17151,10 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
table_relation_copy_data(rel, &newrlocator);
}
+ /* mark that a rewrite happened */
+ if (RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
+ pgstat_mark_rewrite(oldrlocator, newrlocator);
+
/*
* Update the pg_class row.
*
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 89bf0cbed56..4d929972fab 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -30,6 +30,19 @@
#include "utils/syscache.h"
#include "utils/timestamp.h"
+/* Pending rewrite operations for stats copying */
+typedef struct PgStat_PendingRewrite
+{
+ RelFileLocator old_locator;
+ RelFileLocator new_locator;
+ RelFileLocator original_locator;
+ int nest_level; /* Transaction nesting level where rewrite
+ * occurred */
+ struct PgStat_PendingRewrite *next;
+} PgStat_PendingRewrite;
+
+/* The pending rewrites list for current transaction */
+static PgStat_PendingRewrite *pending_rewrites = NULL;
/* Record that's written to 2PC state file when pgstat state is persisted */
typedef struct TwoPhasePgStatRecord
@@ -43,6 +56,8 @@ typedef struct TwoPhasePgStatRecord
PgStat_Counter deleted_pre_truncdrop;
RelFileLocator locator; /* table's rd_locator */
bool truncdropped; /* was the relation truncated/dropped? */
+ RelFileLocator rewrite_old_locator;
+ int rewrite_nest_level;
} TwoPhasePgStatRecord;
@@ -54,27 +69,70 @@ static void restore_truncdrop_counters(PgStat_TableXactStatus *trans);
/*
- * Copy stats between relations. This is used for things like REINDEX
+ * Copy stats between RelFileLocator. This is used for things like REINDEX
* CONCURRENTLY.
*/
void
-pgstat_copy_relation_stats(Relation dst, Relation src)
+pgstat_copy_relation_stats(RelFileLocator dst, RelFileLocator src, bool increment)
{
PgStat_StatTabEntry *srcstats;
PgStatShared_Relation *dstshstats;
PgStat_EntryRef *dst_ref;
- srcstats = pgstat_fetch_stat_tabentry_ext(RelationGetRelid(src));
+ srcstats = (PgStat_StatTabEntry *) pgstat_fetch_entry(PGSTAT_KIND_RELATION,
+ src.dbOid,
+ RelFileLocatorToPgStatObjid(src));
if (!srcstats)
return;
dst_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_RELATION,
- dst->rd_rel->relisshared ? InvalidOid : MyDatabaseId,
- RelationGetRelid(dst),
+ dst.dbOid,
+ RelFileLocatorToPgStatObjid(dst),
false);
dstshstats = (PgStatShared_Relation *) dst_ref->shared_stats;
- dstshstats->stats = *srcstats;
+
+ if (!increment)
+ dstshstats->stats = *srcstats;
+ else
+ {
+ /* Increment those statistics */
+#define RELFSTAT_ACC(fld, stats_to_add) \
+ (dstshstats->stats.fld += stats_to_add->fld)
+ RELFSTAT_ACC(numscans, srcstats);
+ RELFSTAT_ACC(tuples_returned, srcstats);
+ RELFSTAT_ACC(tuples_fetched, srcstats);
+ RELFSTAT_ACC(tuples_inserted, srcstats);
+ RELFSTAT_ACC(tuples_updated, srcstats);
+ RELFSTAT_ACC(tuples_deleted, srcstats);
+ RELFSTAT_ACC(tuples_hot_updated, srcstats);
+ RELFSTAT_ACC(tuples_newpage_updated, srcstats);
+ RELFSTAT_ACC(live_tuples, srcstats);
+ RELFSTAT_ACC(dead_tuples, srcstats);
+ RELFSTAT_ACC(mod_since_analyze, srcstats);
+ RELFSTAT_ACC(ins_since_vacuum, srcstats);
+ RELFSTAT_ACC(blocks_fetched, srcstats);
+ RELFSTAT_ACC(blocks_hit, srcstats);
+ RELFSTAT_ACC(vacuum_count, srcstats);
+ RELFSTAT_ACC(autovacuum_count, srcstats);
+ RELFSTAT_ACC(analyze_count, srcstats);
+ RELFSTAT_ACC(autoanalyze_count, srcstats);
+ RELFSTAT_ACC(total_vacuum_time, srcstats);
+ RELFSTAT_ACC(total_autovacuum_time, srcstats);
+ RELFSTAT_ACC(total_analyze_time, srcstats);
+ RELFSTAT_ACC(total_autoanalyze_time, srcstats);
+#undef RELFSTAT_ACC
+
+ /* Replace those statistics */
+#define RELFSTAT_REP(fld, stats_to_rep) \
+ (dstshstats->stats.fld = stats_to_rep->fld)
+ RELFSTAT_REP(lastscan, srcstats);
+ RELFSTAT_REP(last_vacuum_time, srcstats);
+ RELFSTAT_REP(last_autovacuum_time, srcstats);
+ RELFSTAT_REP(last_analyze_time, srcstats);
+ RELFSTAT_REP(last_autoanalyze_time, srcstats);
+#undef RELFSTAT_REP
+ }
pgstat_unlock_entry(dst_ref);
}
@@ -136,6 +194,7 @@ void
pgstat_assoc_relation(Relation rel)
{
RelFileLocator locator;
+ PgStat_TableStatus *pgstat_info;
Assert(rel->pgstat_enabled);
Assert(rel->pgstat_info == NULL);
@@ -164,14 +223,54 @@ pgstat_assoc_relation(Relation rel)
locator.relNumber = rel->rd_id;
}
+ /*
+ * If this relation was rewritten during the current transaction we may be
+ * reopening it with its new RelFileLocator. In that case, continue using
+ * the stats entry associated with the old locator rather than creating a
+ * new one. This ensures all stats from before and after the rewrite are
+ * tracked in a single entry which will be properly copied to the new
+ * locator at transaction commit.
+ */
+ if (pending_rewrites != NULL)
+ {
+ PgStat_PendingRewrite *rewrite;
+
+ for (rewrite = pending_rewrites; rewrite != NULL; rewrite = rewrite->next)
+ {
+ if (locator.dbOid == rewrite->new_locator.dbOid &&
+ locator.spcOid == rewrite->new_locator.spcOid &&
+ locator.relNumber == rewrite->new_locator.relNumber)
+ {
+ pgstat_info = pgstat_prep_relation_pending(rewrite->old_locator);
+ goto found_entry;
+ }
+ }
+ }
+
/* Else find or make the PgStat_TableStatus entry, and update link */
- rel->pgstat_info = pgstat_prep_relation_pending(locator);
+ pgstat_info = pgstat_prep_relation_pending(locator);
+
+found_entry:
+ rel->pgstat_info = pgstat_info;
+
+ /*
+ * For relations stats, we key by physical file location, not by relation
+ * OID. This means during operations like ALTER TYPE it's possible that
+ * the relation OID changes but the relfilenode stays the same (no actual
+ * rewrite needed). Unlink the old relation first.
+ */
+ if (pgstat_info->relation != NULL &&
+ pgstat_info->relation != rel)
+ {
+ pgstat_info->relation->pgstat_info = NULL;
+ pgstat_info->relation = NULL;
+ }
/* don't allow link a stats to multiple relcache entries */
- Assert(rel->pgstat_info->relation == NULL);
+ Assert(pgstat_info->relation == NULL);
/* mark this relation as the owner */
- rel->pgstat_info->relation = rel;
+ pgstat_info->relation = rel;
}
/*
@@ -214,14 +313,37 @@ pgstat_drop_relation(Relation rel)
{
int nest_level = GetCurrentTransactionNestLevel();
PgStat_TableStatus *pgstat_info;
+ bool skip_transactional_drop = false;
/* don't track stats for relations without storage */
if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
return;
- pgstat_drop_transactional(PGSTAT_KIND_RELATION,
- rel->rd_locator.dbOid,
- RelFileLocatorToPgStatObjid(rel->rd_locator));
+ /* Check if this drop is part of a pending rewrite */
+ if (pending_rewrites != NULL)
+ {
+ PgStat_PendingRewrite *rewrite;
+
+ for (rewrite = pending_rewrites; rewrite != NULL; rewrite = rewrite->next)
+ {
+ if (rel->rd_locator.dbOid == rewrite->old_locator.dbOid &&
+ rel->rd_locator.spcOid == rewrite->old_locator.spcOid &&
+ rel->rd_locator.relNumber == rewrite->old_locator.relNumber)
+ {
+ skip_transactional_drop = true;
+ break;
+ }
+ }
+ }
+
+ /*
+ * If it is part of a rewrite, drop its stats later, for example in
+ * AtEOXact_PgStat_Relations(), so skip it here.
+ */
+ if (!skip_transactional_drop)
+ pgstat_drop_transactional(PGSTAT_KIND_RELATION,
+ rel->rd_locator.dbOid,
+ RelFileLocatorToPgStatObjid(rel->rd_locator));
if (!pgstat_should_count_relation(rel))
return;
@@ -666,6 +788,48 @@ AtEOXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isCommit)
}
tabstat->trans = NULL;
}
+
+ /* preserve the stats in case of rewrite */
+ if (isCommit && pending_rewrites != NULL)
+ {
+ PgStat_PendingRewrite *rewrite;
+ PgStat_PendingRewrite *prev = NULL;
+ PgStat_PendingRewrite *current = pending_rewrites;
+ PgStat_PendingRewrite *next;
+
+ /* reverse the rewrites list to process in chronological order */
+ while (current != NULL)
+ {
+ next = current->next;
+ current->next = prev;
+ prev = current;
+ current = next;
+ }
+
+ /* now process rewrites in chronological order */
+ for (rewrite = prev; rewrite != NULL; rewrite = rewrite->next)
+ {
+ PgStat_EntryRef *old_entry_ref;
+
+ old_entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION,
+ rewrite->old_locator.dbOid,
+ RelFileLocatorToPgStatObjid(rewrite->old_locator));
+
+ if (old_entry_ref && old_entry_ref->pending)
+ pgstat_relation_flush_cb(old_entry_ref, false);
+
+ pgstat_copy_relation_stats(rewrite->new_locator,
+ rewrite->old_locator, true);
+
+ /* drop old locator's stats */
+ if (!pgstat_drop_entry(PGSTAT_KIND_RELATION,
+ rewrite->old_locator.dbOid,
+ RelFileLocatorToPgStatObjid(rewrite->old_locator)))
+ pgstat_request_entry_refs_gc();
+ }
+ }
+
+ pending_rewrites = NULL;
}
/*
@@ -681,6 +845,30 @@ AtEOSubXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isCommit, in
PgStat_TableXactStatus *trans;
PgStat_TableXactStatus *next_trans;
+ /*
+ * If we don't commit then remove the associated rewrites if any, to keep
+ * the rewrite chain in sync with what will be eventually committed.
+ */
+ if (!isCommit)
+ {
+ PgStat_PendingRewrite **rewrite_ptr = &pending_rewrites;
+
+ while (*rewrite_ptr != NULL)
+ {
+ if ((*rewrite_ptr)->nest_level >= nestDepth)
+ {
+ PgStat_PendingRewrite *to_remove = *rewrite_ptr;
+
+ *rewrite_ptr = (*rewrite_ptr)->next;
+ pfree(to_remove);
+ }
+ else
+ {
+ rewrite_ptr = &((*rewrite_ptr)->next);
+ }
+ }
+ }
+
for (trans = xact_state->first; trans != NULL; trans = next_trans)
{
PgStat_TableStatus *tabstat;
@@ -760,11 +948,19 @@ void
AtPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state)
{
PgStat_TableXactStatus *trans;
+ PgStat_PendingRewrite *rewrite;
+ /*
+ * For each tabstat, find its matching rewrite and remove it from the
+ * pending rewrites list. This way, after processing all tabstats, pending
+ * rewrites will only contain rewrite only transactions.
+ */
for (trans = xact_state->first; trans != NULL; trans = trans->next)
{
PgStat_TableStatus *tabstat PG_USED_FOR_ASSERTS_ONLY;
TwoPhasePgStatRecord record;
+ PgStat_PendingRewrite **rewrite_ptr;
+ bool found_rewrite = false;
Assert(trans->nest_level == 1);
Assert(trans->upper == NULL);
@@ -784,10 +980,83 @@ AtPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state)
record.locator = tabstat->locator;
record.truncdropped = trans->truncdropped;
+ record.rewrite_nest_level = 0;
+
+ /*
+ * Look for a matching rewrite and remove it from pending rewrites. We
+ * check three possible matches:
+ *
+ * The new_locator when stats have been added after the rewrite. The
+ * old_locator when stats have been added before the rewrite but not
+ * after. The original_locator when this tabstat is part of a rewrite
+ * chain.
+ */
+ rewrite_ptr = &pending_rewrites;
+ while (*rewrite_ptr != NULL)
+ {
+ rewrite = *rewrite_ptr;
+
+ if ((record.locator.dbOid == rewrite->new_locator.dbOid &&
+ record.locator.spcOid == rewrite->new_locator.spcOid &&
+ record.locator.relNumber == rewrite->new_locator.relNumber) ||
+ (tabstat->locator.dbOid == rewrite->old_locator.dbOid &&
+ tabstat->locator.spcOid == rewrite->old_locator.spcOid &&
+ tabstat->locator.relNumber == rewrite->old_locator.relNumber) ||
+ (tabstat->locator.dbOid == rewrite->original_locator.dbOid &&
+ tabstat->locator.spcOid == rewrite->original_locator.spcOid &&
+ tabstat->locator.relNumber == rewrite->original_locator.relNumber))
+ {
+ /*
+ * Found matching rewrite. Record the rewrite information and
+ * remove this rewrite from the list since it's now handled.
+ */
+ record.rewrite_old_locator = rewrite->original_locator;
+ record.rewrite_nest_level = rewrite->nest_level;
+ record.locator = rewrite->new_locator;
+ found_rewrite = true;
+
+ /* Remove from pending_rewrites list */
+ *rewrite_ptr = rewrite->next;
+ pfree(rewrite);
+ break;
+ }
+ else
+ {
+ /* Move to next rewrite in the list */
+ rewrite_ptr = &(rewrite->next);
+ }
+ }
+
+ /* If no rewrite found, clear the rewrite fields */
+ if (!found_rewrite)
+ {
+ memset(&record.rewrite_old_locator, 0, sizeof(RelFileLocator));
+ }
+
+ RegisterTwoPhaseRecord(TWOPHASE_RM_PGSTAT_ID, 0,
+ &record, sizeof(TwoPhasePgStatRecord));
+ }
+
+ /*
+ * Now process any rewrites still pending. These are rewrite only
+ * transactions. We need to preserve their stats even though there's no
+ * tabstat entry for them.
+ */
+ for (rewrite = pending_rewrites; rewrite != NULL; rewrite = rewrite->next)
+ {
+ TwoPhasePgStatRecord record;
+
+ memset(&record, 0, sizeof(TwoPhasePgStatRecord));
+ record.locator = rewrite->new_locator;
+ record.rewrite_old_locator = rewrite->original_locator;
+ record.rewrite_nest_level = rewrite->nest_level;
+ record.truncdropped = false;
RegisterTwoPhaseRecord(TWOPHASE_RM_PGSTAT_ID, 0,
&record, sizeof(TwoPhasePgStatRecord));
}
+
+ pending_rewrites = NULL;
}
/*
@@ -810,6 +1079,8 @@ PostPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state)
tabstat = trans->parent;
tabstat->trans = NULL;
}
+
+ pending_rewrites = NULL;
}
/*
@@ -845,6 +1116,29 @@ pgstat_twophase_postcommit(FullTransactionId fxid, uint16 info,
pgstat_info->counts.changed_tuples +=
rec->tuples_inserted + rec->tuples_updated +
rec->tuples_deleted;
+
+ if (rec->rewrite_nest_level > 0)
+ {
+ PgStat_EntryRef *old_entry_ref;
+
+ /* Flush any pending stats for old locator first */
+ old_entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION,
+ rec->rewrite_old_locator.dbOid,
+ RelFileLocatorToPgStatObjid(rec->rewrite_old_locator));
+
+ if (old_entry_ref && old_entry_ref->pending)
+ pgstat_relation_flush_cb(old_entry_ref, false);
+
+ /* Copy stats from old to new locator */
+ pgstat_copy_relation_stats(rec->locator, rec->rewrite_old_locator,
+ true);
+
+ /* Drop old locator's stats */
+ if (!pgstat_drop_entry(PGSTAT_KIND_RELATION,
+ rec->rewrite_old_locator.dbOid,
+ RelFileLocatorToPgStatObjid(rec->rewrite_old_locator)))
+ pgstat_request_entry_refs_gc();
+ }
}
/*
@@ -859,9 +1153,26 @@ pgstat_twophase_postabort(FullTransactionId fxid, uint16 info,
{
TwoPhasePgStatRecord *rec = (TwoPhasePgStatRecord *) recdata;
PgStat_TableStatus *pgstat_info;
+ RelFileLocator target_locator;
+
+ /*
+ * For aborted transactions with rewrites (like TRUNCATE), we need to
+ * restore stats to the old locator, not the new one. The new locator
+ * should be dropped since the rewrite is being rolled back.
+ */
+ if (rec->rewrite_nest_level > 0)
+ {
+ /* Use the old locator */
+ target_locator = rec->rewrite_old_locator;
+ }
+ else
+ {
+ /* No rewrite, use the original locator */
+ target_locator = rec->locator;
+ }
/* Find or create a tabstat entry for the target locator */
- pgstat_info = pgstat_prep_relation_pending(rec->locator);
+ pgstat_info = pgstat_prep_relation_pending(target_locator);
/* Same math as in AtEOXact_PgStat, abort case */
if (rec->truncdropped)
@@ -916,7 +1227,17 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
tabentry->numscans += lstats->counts.numscans;
if (lstats->counts.numscans)
{
- TimestampTz t = GetCurrentTransactionStopTimestamp();
+ TimestampTz t;
+
+ /*
+ * Checking the transaction state due to the flush call in
+ * pgstat_twophase_postcommit() that would break the assertion on the
+ * state in GetCurrentTransactionStopTimestamp().
+ */
+ if (!IsTransactionState())
+ t = GetCurrentTransactionStopTimestamp();
+ else
+ t = GetCurrentTimestamp();
if (t > tabentry->lastscan)
tabentry->lastscan = t;
@@ -1167,3 +1488,45 @@ pgstat_reloid_to_relfilelocator(Oid reloid, RelFileLocator *locator)
ReleaseSysCache(tuple);
return result;
}
+
+/*
+ * Mark that a relation rewrite has occurred, preserving the original locator
+ * so stats can be copied at transaction commit.
+ */
+void
+pgstat_mark_rewrite(RelFileLocator old_locator, RelFileLocator new_locator)
+{
+ PgStat_PendingRewrite *rewrite;
+ PgStat_PendingRewrite *existing;
+ RelFileLocator original_locator = old_locator;
+
+ for (existing = pending_rewrites; existing != NULL; existing = existing->next)
+ {
+ if (old_locator.dbOid == existing->new_locator.dbOid &&
+ old_locator.spcOid == existing->new_locator.spcOid &&
+ old_locator.relNumber == existing->new_locator.relNumber)
+ {
+ original_locator = existing->original_locator;
+ break;
+ }
+ }
+
+ /* Allocate in TopTransactionContext memory context */
+ rewrite = MemoryContextAlloc(TopTransactionContext,
+ sizeof(PgStat_PendingRewrite));
+
+ rewrite->old_locator = old_locator;
+ rewrite->new_locator = new_locator;
+ rewrite->original_locator = original_locator;
+ rewrite->nest_level = GetCurrentTransactionNestLevel();
+
+ /* Add to the list */
+ rewrite->next = pending_rewrites;
+ pending_rewrites = rewrite;
+}
+
+void
+pgstat_clear_rewrite(void)
+{
+ pending_rewrites = NULL;
+}
diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/activity/pgstat_xact.c
index 5e2d69e6297..8ed8f5317f3 100644
--- a/src/backend/utils/activity/pgstat_xact.c
+++ b/src/backend/utils/activity/pgstat_xact.c
@@ -55,6 +55,8 @@ AtEOXact_PgStat(bool isCommit, bool parallel)
}
pgStatXactStack = NULL;
+ pgstat_clear_rewrite();
+
/* Make sure any stats snapshot is thrown away */
pgstat_clear_snapshot();
}
@@ -360,8 +362,29 @@ create_drop_transactional_internal(PgStat_Kind kind, Oid dboid, uint64 objid, bo
void
pgstat_create_transactional(PgStat_Kind kind, Oid dboid, uint64 objid)
{
- if (pgstat_get_entry_ref(kind, dboid, objid, false, NULL))
+ PgStat_EntryRef *entry_ref;
+
+ entry_ref = pgstat_get_entry_ref(kind, dboid, objid, false, NULL);
+
+ if (entry_ref)
{
+ /*
+ * For relations stats, we key by physical file location, not by
+ * relation OID. This means during operations like ALTER TYPE where
+ * the relation OID changes but the relfilenode stays the same (no
+ * actual rewrite needed), we'll find an existing entry.
+ *
+ * This is expected behavior, we want to preserve stats across the
+ * catalog change. Simply reset and recreate the entry for the new
+ * relation OID without warning.
+ */
+ if (kind == PGSTAT_KIND_RELATION)
+ {
+ pgstat_reset(kind, dboid, objid);
+ create_drop_transactional_internal(kind, dboid, objid, true);
+ return;
+ }
+
ereport(WARNING,
errmsg("resetting existing statistics for kind %s, db=%u, oid=%" PRIu64,
(pgstat_get_kind_info(kind))->name, dboid,
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 3a4f19e8d58..60491962668 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -86,6 +86,7 @@
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
+#include "utils/pgstat_internal.h"
#include "utils/relmapper.h"
#include "utils/resowner.h"
#include "utils/snapmgr.h"
@@ -3771,6 +3772,7 @@ RelationSetNewRelfilenumber(Relation relation, char persistence)
MultiXactId minmulti = InvalidMultiXactId;
TransactionId freezeXid = InvalidTransactionId;
RelFileLocator newrlocator;
+ RelFileLocator oldrlocator = relation->rd_locator;
if (!IsBinaryUpgrade)
{
@@ -3942,6 +3944,10 @@ RelationSetNewRelfilenumber(Relation relation, char persistence)
table_close(pg_class, RowExclusiveLock);
+ /* Mark that a rewrite happened */
+ if (RELKIND_HAS_STORAGE(relation->rd_rel->relkind))
+ pgstat_mark_rewrite(oldrlocator, newrlocator);
+
/*
* Make the pg_class row change or relation map change visible. This will
* cause the relcache entry to get updated, too.
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 689c624f373..d0d5473f8a5 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -673,7 +673,7 @@ extern PgStat_FunctionCounts *find_funcstat_entry(Oid func_id);
extern void pgstat_create_relation(Relation rel);
extern void pgstat_drop_relation(Relation rel);
-extern void pgstat_copy_relation_stats(Relation dst, Relation src);
+extern void pgstat_copy_relation_stats(RelFileLocator dst, RelFileLocator src, bool increment);
extern void pgstat_init_relation(Relation rel);
extern void pgstat_assoc_relation(Relation rel);
@@ -685,6 +685,9 @@ extern void pgstat_report_vacuum(Relation rel, PgStat_Counter livetuples,
extern void pgstat_report_analyze(Relation rel,
PgStat_Counter livetuples, PgStat_Counter deadtuples,
bool resetcounter, TimestampTz starttime);
+extern void pgstat_mark_rewrite(RelFileLocator old_locator,
+ RelFileLocator new_locator);
+extern void pgstat_clear_rewrite(void);
/*
* If stats are enabled, but pending data hasn't been prepared yet, call
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 52f8603a7be..8b84e3a0bcf 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2298,6 +2298,7 @@ PgStat_KindInfo
PgStat_LocalState
PgStat_PendingDroppedStatsItem
PgStat_PendingIO
+PgStat_PendingRewrite
PgStat_SLRUStats
PgStat_ShmemControl
PgStat_Snapshot
--
2.34.1
--USlF2Vr25us5UpvY--
^ permalink raw reply [nested|flat] 39+ messages in thread
end of thread, other threads:[~2025-11-04 13:52 UTC | newest]
Thread overview: 39+ 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-09-24 16:52 ` Nathan Bossart <[email protected]>
2025-10-02 17:37 ` Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Jeff Davis <[email protected]>
2025-10-09 03:28 ` Jeff Davis <[email protected]>
2025-10-09 15:39 ` Nathan Bossart <[email protected]>
2025-10-09 21:18 ` Nathan Bossart <[email protected]>
2025-10-10 16:26 ` Nathan Bossart <[email protected]>
2025-10-10 18:31 ` Jeff Davis <[email protected]>
2025-10-13 19:30 ` Nathan Bossart <[email protected]>
2025-10-14 02:23 ` Jeff Davis <[email protected]>
2025-10-14 16:05 ` Nathan Bossart <[email protected]>
2025-10-14 17:01 ` Jeff Davis <[email protected]>
2025-10-14 21:32 ` Nathan Bossart <[email protected]>
2025-10-15 15:39 ` Nathan Bossart <[email protected]>
2025-10-15 17:54 ` Nathan Bossart <[email protected]>
2025-10-15 21:38 ` Nathan Bossart <[email protected]>
2025-10-17 16:56 ` Nathan Bossart <[email protected]>
2025-10-13 18:16 ` Jeff Davis <[email protected]>
2025-10-13 21:21 ` Tom Lane <[email protected]>
2025-10-13 22:11 ` Jeff Davis <[email protected]>
2025-10-14 16:30 ` Tom Lane <[email protected]>
2025-03-16 13:00 ` vignesh C <[email protected]>
2025-11-04 13:52 [PATCH v13 2/2] handle relation statistics correctly during rewrites Bertrand Drouvot <[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