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

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.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.

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.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.

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.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.

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.

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

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

On Sun, Jan 4, 2026 at 4:47 AM Kirill Reshke <reshkekirill@gmail.com> wrote:

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]).

Thanks for the review. I've updated the commit message.

--
Best regards,
Shinya Kato
NTT OSS Center

On 12.02.26 03:05, Shinya Kato wrote:

On Sun, Jan 4, 2026 at 4:47 AM Kirill Reshke <reshkekirill@gmail.com> wrote:

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]).

Thanks for the review. I've updated the commit message.

I don't understand the point of this patch. It changes some
relation_open() calls to table_open(). The only difference of
table_open() is that it checks the relkind. But in these cases, the
relkind was already checked earlier via RangeVarCallbackForPolicy. So
this change has no effect, but it adds extra overhead.