Subject: remove unnecessary include in src/backend/commands/policy.c

From: jian he <[email-masked]>
Date: Sun, 14 Sep 2025 14:37:28 +0800

hi.

in src/backend/commands/policy.c, i found that
#include "access/htup.h"
#include "access/htup_details.h"
#include "catalog/catalog.h"
#include "nodes/pg_list.h"
#include "parser/parse_node.h"
#include "utils/array.h"

is not necessary "include", so I removed it.

we can also remove
#include "access/relation.h"
replace relation_open to table_open, since we already did relkind check in
RangeVarCallbackForPolicy.
From 5ba1586d0844ec40674338ad920b12a61dd646ed Mon Sep 17 00:00:00 2001
From: jian he <[email-masked]>
Date: Sun, 14 Sep 2025 14:35:33 +0800
Subject: [PATCH v1 1/1] remove unnecessary include in policy.c

discussion: https://postgr.es/m/
---
 src/backend/commands/policy.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 83056960fe4..ce3eb10ae86 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -13,12 +13,8 @@
 #include "postgres.h"
 
 #include "access/genam.h"
-#include "access/htup.h"
-#include "access/htup_details.h"
-#include "access/relation.h"
 #include "access/table.h"
 #include "access/xact.h"
-#include "catalog/catalog.h"
 #include "catalog/dependency.h"
 #include "catalog/indexing.h"
 #include "catalog/namespace.h"
@@ -28,15 +24,12 @@
 #include "catalog/pg_type.h"
 #include "commands/policy.h"
 #include "miscadmin.h"
-#include "nodes/pg_list.h"
 #include "parser/parse_clause.h"
 #include "parser/parse_collate.h"
-#include "parser/parse_node.h"
 #include "parser/parse_relation.h"
 #include "rewrite/rewriteManip.h"
 #include "rewrite/rowsecurity.h"
 #include "utils/acl.h"
-#include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
 #include "utils/inval.h"
@@ -630,7 +623,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
 										stmt);
 
 	/* Open target_table to build quals. No additional lock is necessary. */
-	target_table = relation_open(table_id, NoLock);
+	target_table = table_open(table_id, NoLock);
 
 	/* Add for the regular security quals */
 	nsitem = addRangeTableEntryForRelation(qual_pstate, target_table,
@@ -752,7 +745,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
 	free_parsestate(qual_pstate);
 	free_parsestate(with_check_pstate);
 	systable_endscan(sscan);
-	relation_close(target_table, NoLock);
+	table_close(target_table, NoLock);
 	table_close(pg_policy_rel, RowExclusiveLock);
 
 	return myself;
@@ -805,7 +798,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
 										RangeVarCallbackForPolicy,
 										stmt);
 
-	target_table = relation_open(table_id, NoLock);
+	target_table = table_open(table_id, NoLock);
 
 	/* Parse the using policy clause */
 	if (stmt->qual)
@@ -1082,7 +1075,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
 
 	/* Clean up. */
 	systable_endscan(sscan);
-	relation_close(target_table, NoLock);
+	table_close(target_table, NoLock);
 	table_close(pg_policy_rel, RowExclusiveLock);
 
 	return myself;
@@ -1110,7 +1103,7 @@ rename_policy(RenameStmt *stmt)
 										RangeVarCallbackForPolicy,
 										stmt);
 
-	target_table = relation_open(table_id, NoLock);
+	target_table = table_open(table_id, NoLock);
 
 	pg_policy_rel = table_open(PolicyRelationId, RowExclusiveLock);
 
@@ -1189,7 +1182,7 @@ rename_policy(RenameStmt *stmt)
 	/* Clean up. */
 	systable_endscan(sscan);
 	table_close(pg_policy_rel, RowExclusiveLock);
-	relation_close(target_table, NoLock);
+	table_close(target_table, NoLock);
 
 	return address;
 }
-- 
2.34.1

---

From: Chao Li <[email-masked]>
Date: Mon, 15 Sep 2025 08:18:52 +0800

> On Sep 14, 2025, at 14:37, jian he <[email-masked]> wrote:
> 
> hi.
> 
> <v1-0001-remove-unnecessary-include-in-policy.c.patch>

LGTM. I built the patch on MacOS M4, and build passed without warning.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Shinya Kato <[email-masked]>
Date: Tue, 30 Sep 2025 22:00:13 +0900

Hi,

> RangeVarCallbackForPolicy.

Thanks for the patch!
I can confirm that it builds successfully and passes the regression tests.

However, the changes make policy.c rely on transitive includes. For
example, policy.c uses GETSTRUCT(), which is defined in
access/htup_details.h. Instead of being included directly, that header
is currently pulled in via a fairly long chain:
catalog/indexing.h -> nodes/execnodes.h -> access/tupconvert.h ->
executor/tuptable.h -> access/htup_details.h

While this works for now, the dependency is fragile and could break if
header files are rearranged in the future. I'm not sure this is a good
practice, and although I couldn't find a specific rule against it in
PostgreSQL's coding conventions, it seems risky.

-- 
Best regards,
Shinya Kato
NTT OSS Center

---

From: Álvaro Herrera <[email-masked]>
Date: Sun, 12 Oct 2025 10:31:50 +0200

> PostgreSQL's coding conventions, it seems risky.

Yeah -- I'm not very worried about the fragility being introduced, since
if such a problem ever occurs it's very obvious and easy to fix.
However, removing these include lines is just churn with no benefit,
because those includes are still being processed via the indirect
pathways.  We haven't saved anything.

Just look at all the crossed wires here
https://doxygen.postgresql.org/policy_8c.html
Clearly the cross-inclusion of headers in headers is a mess.  Fixing
that mess is going to cause *more* explicit inclusion of headers in .c
files.  Removing a few explicit ones so that they become implicit, only
to have to resurrect the explicit inclusion when we remove some of that
cross-header inclusion is pointless.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem."  (Tom Lane)

---

From: Shinya Kato <[email-masked]>
Date: Wed, 15 Oct 2025 14:43:33 +0900

> cross-header inclusion is pointless.

Thank you, I agree with Álvaro. So, I think it is better to leave them
as they are, except for access/relation.h. And, replacing
relation_open to table_open looks good to me.

-- 
Best regards,
Shinya Kato
NTT OSS Center

---

From: jian he <[email-masked]>
Date: Tue, 21 Oct 2025 12:00:28 +0800

> relation_open to table_open looks good to me.

ok.

The attached patch only replaces relation_open to table_open.
RangeVarCallbackForPolicy already checks that a policy can only be created on a
table or a partitioned table.

so the replacement should be ok.
From 766951113b6df59e5fa393b57487e3d2e42dc027 Mon Sep 17 00:00:00 2001
From: jian he <[email-masked]>
Date: Tue, 21 Oct 2025 11:59:36 +0800
Subject: [PATCH v2 1/1] rename relation_open/close to table_open/close in
 policy.c

RangeVarCallbackForPolicy already checks that a policy can only be created on a
table or a partitioned table.

discussion: https://postgr.es/m/[email-masked]
---
 src/backend/commands/policy.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 83056960fe4..9fec50422c4 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -630,7 +630,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
 										stmt);

@@ -752,7 +752,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
 
 	return myself;
@@ -805,7 +805,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
 										RangeVarCallbackForPolicy,
 										stmt);

 	/* Parse the using policy clause */
 	if (stmt->qual)
@@ -1082,7 +1082,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
 
 	/* Clean up. */
 	systable_endscan(sscan);
 
 	return myself;
@@ -1110,7 +1110,7 @@ rename_policy(RenameStmt *stmt)
 										RangeVarCallbackForPolicy,
 										stmt);

 
@@ -1189,7 +1189,7 @@ rename_policy(RenameStmt *stmt)
 	/* Clean up. */
 	systable_endscan(sscan);
 	table_close(pg_policy_rel, RowExclusiveLock);
-	relation_close(target_table, NoLock);
+	table_close(target_table, NoLock);
 
 	return address;
 }
-- 
2.34.1

---

From: Shinya Kato <[email-masked]>
Date: Wed, 22 Oct 2025 15:30:28 +0900

> so the replacement should be ok.

Thank you for updating the patch.
But I said "except for access/relation.h". I think access/relation.h
is not necessary if you replace relation_open to table_open.

-- 
Best regards,
Shinya Kato
NTT OSS Center

---

From: Shinya Kato <[email-masked]>
Date: Fri, 26 Dec 2025 16:30:59 +0900

> is not necessary if you replace relation_open to table_open.

I've fixed it, and marked the patch as Ready for Committer.

-- 
Best regards,
Shinya Kato
NTT OSS Center

---

From: Japin Li <[email-masked]>
Date: Fri, 26 Dec 2025 17:27:25 +0800

Hi, Shinya

> I've fixed it, and marked the patch as Ready for Committer.

Found a typo in the commit message.

> RangeVarCallbackForPolicy already ensures policies apply only to tables
> or partitioned tables, so table_* isthe appropriate API for
> opening/closing the target relation.

s/isthe/is the/

-- 
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.