Обсуждение: 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.

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