Remove redundant comment regarding RelationBuildRowSecurity in relcache.c
Hi,
A comment in relcache.c mentions that RelationBuildRowSecurity
adds a default-deny policy when no policy exists on table. This
does not seem to be the case. The default deny policy is added
later on, inside get_row_security_policies(). Also it mentions that
there can never be zero policies for a relation which is true
but here it kind of hints that rd_rsdesc->policies can never be
null.
Added a patch to remove both of these comments.
Thanks,
Tanzeel Khan
Attachments:
0001-Remove-redundant-comment-about-RelationBuildRowSecur.patchapplication/octet-stream; name=0001-Remove-redundant-comment-about-RelationBuildRowSecur.patchDownload
From 3c3b70ba5044610f4825c8eb46008f8f75904c88 Mon Sep 17 00:00:00 2001
From: Tanzeel Khan <tzlkhan@amazon.com>
Date: Mon, 28 Apr 2025 12:59:32 +0000
Subject: [PATCH] Remove redundant comment about RelationBuildRowSecurity
RelationBuildRowSecurity does not create the default-deny
policy so remove the comment which says so.
---
src/backend/utils/cache/relcache.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 68ff67de549..2f0958b96c5 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4328,10 +4328,7 @@ RelationCacheInitializePhase3(void)
/*
* Re-load the row security policies if the relation has them, since
- * they are not preserved in the cache. Note that we can never NOT
- * have a policy while relrowsecurity is true,
- * RelationBuildRowSecurity will create a single default-deny policy
- * if there is no policy defined in pg_policy.
+ * they are not preserved in the cache.
*/
if (relation->rd_rel->relrowsecurity && relation->rd_rsdesc == NULL)
{
--
2.47.1
On Mon, 28 Apr 2025 at 16:02, Khan, Tanzeel <tzlkhan@amazon.com> wrote:
A comment in relcache.c mentions that RelationBuildRowSecurity
adds a default-deny policy when no policy exists on table. This
does not seem to be the case. The default deny policy is added
later on, inside get_row_security_policies(). Also it mentions that
there can never be zero policies for a relation which is true
but here it kind of hints that rd_rsdesc->policies can never be
null.Added a patch to remove both of these comments.
Hmm, well I agree that the comment is wrong, but I don't think that
simply deleting it is the answer.
I think that the point the comment is trying to make is that it's
perfectly valid for there to be no policies while relrowsecurity is
true, which is why the code block for policies needs to be different
from the preceding code blocks for rules and triggers.
So perhaps it should say something like this:
/*
* Re-load the row security policies if the relation has them, since
* they are not preserved in the cache. Note that relrowsecurity may
* be true even when there are no policies defined in pg_policy, in
* which case RelationBuildRowSecurity will create a RowSecurityDesc
* with an empty policy list, and rd_rsdesc will always be non-NULL.
* When the table is queried, if there are no policies granting access
* to rows in the table, a single always-false, default-deny policy
* will be applied instead, see get_row_security_policies.
*/
Regards,
Dean