remove unnecessary include in src/backend/commands/policy.c
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.
Attachments:
v1-0001-remove-unnecessary-include-in-policy.c.patchtext/x-patch; charset=US-ASCII; name=v1-0001-remove-unnecessary-include-in-policy.c.patchDownload
From 5ba1586d0844ec40674338ad920b12a61dd646ed Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
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
On Sep 14, 2025, at 14:37, jian he <jian.universality@gmail.com> wrote:
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.
<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/
Hi,
On Sun, Sep 14, 2025 at 3:38 PM jian he <jian.universality@gmail.com> wrote:
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.
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
On 2025-Sep-30, Shinya Kato wrote:
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.hWhile 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.
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)
On Sun, Oct 12, 2025 at 5:31 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Sep-30, Shinya Kato wrote:
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.hWhile 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.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.
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
On Wed, Oct 15, 2025 at 1:44 PM Shinya Kato <shinya11.kato@gmail.com> wrote:
On Sun, Oct 12, 2025 at 5:31 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Sep-30, Shinya Kato wrote:
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.hWhile 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.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.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.
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.
Attachments:
v2-0001-rename-relation_open-close-to-table_open-close-in-policy.c.patchtext/x-patch; charset=US-ASCII; name=v2-0001-rename-relation_open-close-to-table_open-close-in-policy.c.patchDownload
From 766951113b6df59e5fa393b57487e3d2e42dc027 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
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/CACJufxFvcqOd6g6uaQqKuKPRgcEfPwp_tLSaaxDiHFBb2snJDA@mail.gmail.com
---
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);
/* 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 +752,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 +805,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 +1082,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 +1110,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 +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
On Tue, Oct 21, 2025 at 1:01 PM jian he <jian.universality@gmail.com> wrote:
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.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.
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
On Wed, Oct 22, 2025 at 3:30 PM Shinya Kato <shinya11.kato@gmail.com> wrote:
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.
I've fixed it, and marked the patch as Ready for Committer.
--
Best regards,
Shinya Kato
NTT OSS Center
Attachments:
v3-0001-Replace-relation_-open-close-to-table_-open-close.patchapplication/octet-stream; name=v3-0001-Replace-relation_-open-close-to-table_-open-close.patchDownload
From 4b529266ebf18f95ca4e8c335d2a494812cf65bd Mon Sep 17 00:00:00 2001
From: Shinya Kato <shinya11.kato@gmail.com>
Date: Fri, 26 Dec 2025 16:19:26 +0900
Subject: [PATCH v3] Replace relation_{open,close} to table_{open,close} in
policy.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
RangeVarCallbackForPolicy already ensures policies apply only to tables
or partitioned tables, so table_* isthe appropriate API for
opening/closing the target relation.
Author: Jian He <jian.universality@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Shinya Kato <shinya11.kato@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/CACJufxFvcqOd6g6uaQqKuKPRgcEfPwp_tLSaaxDiHFBb2snJDA@mail.gmail.com
---
src/backend/commands/policy.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 5bd5f8c9968..b75592434b8 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -15,7 +15,6 @@
#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"
@@ -630,7 +629,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 +751,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 +804,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 +1081,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 +1109,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 +1188,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.47.3
Hi, Shinya
On Fri, 26 Dec 2025 at 16:30, Shinya Kato <shinya11.kato@gmail.com> wrote:
On Wed, Oct 22, 2025 at 3:30 PM Shinya Kato <shinya11.kato@gmail.com> wrote:
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.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.
On Fri, 26 Dec 2025 at 14:29, Japin Li <japinli@hotmail.com> wrote:
Hi, Shinya
On Fri, 26 Dec 2025 at 16:30, Shinya Kato <shinya11.kato@gmail.com> wrote:
On Wed, Oct 22, 2025 at 3:30 PM Shinya Kato <shinya11.kato@gmail.com> wrote:
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.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.
Hi hackers!
We also can reflect that our new coding practice is to use table/index
open over relation_open in the commit message (I guess so after[0]https://git.postgresql.org/cgit/postgresql.git/commit/?id=9d0f7996e58c2a92efe06c901c6dfe1f6ced0a1d).
Overall LGTM.
[0]: https://git.postgresql.org/cgit/postgresql.git/commit/?id=9d0f7996e58c2a92efe06c901c6dfe1f6ced0a1d
--
Best regards,
Kirill Reshke