Thread: On the warpath again about ill-considered inclusion nests

I noticed that the recent custom-path commit completely ignored my
advice about not including executor headers into planner headers or
vice versa.  On the way to fixing that, I was dismayed to discover
that the RLS patch has utterly bollixed all semblance of modularization
of the headers.  src/include/rewrite/rowsecurity.h, which one would
reasonably think to be a rewriter header (nevermind its header comment
to the contrary), nonetheless includes execnodes.h (executor stuff)
and relation.h (planner stuff), neither of which a rewriter header
has any business including.  And if that weren't bad enough, it's
been included into utils/rel.h (relcache), which is close enough
to guaranteeing that all planner and executor symbols are visible
in every darn module we've got.  Might as well just put everything
we have in postgres.h and abandon all pretense of modularity.

This needs to be cleaned up.  If you don't want me doing it with
an axe, better put some suggestions forward.
        regards, tom lane

Tom Lane wrote:
> I noticed that the recent custom-path commit completely ignored my
> advice about not including executor headers into planner headers or
> vice versa.  On the way to fixing that, I was dismayed to discover
> that the RLS patch has utterly bollixed all semblance of modularization
> of the headers.  src/include/rewrite/rowsecurity.h, which one would
> reasonably think to be a rewriter header (nevermind its header comment
> to the contrary), nonetheless includes execnodes.h (executor stuff)
> and relation.h (planner stuff), neither of which a rewriter header
> has any business including.  And if that weren't bad enough, it's
> been included into utils/rel.h (relcache), which is close enough
> to guaranteeing that all planner and executor symbols are visible
> in every darn module we've got.  Might as well just put everything
> we have in postgres.h and abandon all pretense of modularity.

I noticed the RLS side of things a week ago as well, and wasn't very
pleased about it.  I don't know about an axe, but we do need some
serious cleanup.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Tom Lane wrote:
> > I noticed that the recent custom-path commit completely ignored my
> > advice about not including executor headers into planner headers or
> > vice versa.  On the way to fixing that, I was dismayed to discover
> > that the RLS patch has utterly bollixed all semblance of modularization
> > of the headers.  src/include/rewrite/rowsecurity.h, which one would
> > reasonably think to be a rewriter header (nevermind its header comment
> > to the contrary), nonetheless includes execnodes.h (executor stuff)
> > and relation.h (planner stuff), neither of which a rewriter header
> > has any business including.  And if that weren't bad enough, it's
> > been included into utils/rel.h (relcache), which is close enough
> > to guaranteeing that all planner and executor symbols are visible
> > in every darn module we've got.  Might as well just put everything
> > we have in postgres.h and abandon all pretense of modularity.
>
> I noticed the RLS side of things a week ago as well, and wasn't very
> pleased about it.  I don't know about an axe, but we do need some
> serious cleanup.

Alright- I'll be looking into this.  I've been in the weeds with the
renaming previously suggested but may just address this first.
Thanks,
    Stephen

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> src/include/rewrite/rowsecurity.h, which one would
> reasonably think to be a rewriter header (nevermind its header comment
> to the contrary), nonetheless includes execnodes.h (executor stuff)

I'll fix the header comment.  The include of execnodes.h was a not used
leftover from prior versions.

> and relation.h (planner stuff), neither of which a rewriter header

relation.h was included only for the hook definition, and only for
Relation's definition at that, which should have been coming from
utils/relcache.h instead, will fix that.

> has any business including.  And if that weren't bad enough, it's
> been included into utils/rel.h (relcache),

This is for the definition of RowSecurityDesc.  I'm happy to move that
to a utils/rowsecurity.h instead, following how TriggerDesc is handled.

> This needs to be cleaned up.  If you don't want me doing it with
> an axe, better put some suggestions forward.

If the above is agreeable, I'll get it done tomorrow (err, later today,
at this point).
Thanks!
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> has any business including.  And if that weren't bad enough, it's
>> been included into utils/rel.h (relcache),

> This is for the definition of RowSecurityDesc.  I'm happy to move that
> to a utils/rowsecurity.h instead, following how TriggerDesc is handled.

Well, if you *only* move RowSecurityDesc and not RowSecurityPolicy,
okay, but that seems a bit useless/inconsistent if I'm reading it
right that RowSecurityDesc contains a List of RowSecurityPolicy structs.
If you move both, I'm still gonna be on your case because rel.h has no
business depending on parsenodes.h or array.h.

What seems possibly saner is to just remove the header inclusion in rel.h
and declare the new Relation field similarly to the way we handle
rd_fdwroutine and some other fields there:
/* use "struct" here to avoid needing to include rowsecurity.h: */struct RowSecurityDesc *rsdesc;    /* Row-security
policy,or NULL */
 

And while you are at it, how about renaming "rsdesc" to "rd_rsdesc"?
The fact that whoever put in trigdesc didn't get the memo about the
naming convention for Relation fields doesn't excuse you from following
it.
        regards, tom lane

PS: The comments for struct RowSecurityPolicy could stand to be improved.

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Well, if you *only* move RowSecurityDesc and not RowSecurityPolicy,
> okay, but that seems a bit useless/inconsistent if I'm reading it
> right that RowSecurityDesc contains a List of RowSecurityPolicy structs.

Yes, good point.

> What seems possibly saner is to just remove the header inclusion in rel.h
> and declare the new Relation field similarly to the way we handle
> rd_fdwroutine and some other fields there:
>
>     /* use "struct" here to avoid needing to include rowsecurity.h: */
>     struct RowSecurityDesc *rsdesc;    /* Row-security policy, or NULL */

Makes sense to me.

> And while you are at it, how about renaming "rsdesc" to "rd_rsdesc"?
> The fact that whoever put in trigdesc didn't get the memo about the
> naming convention for Relation fields doesn't excuse you from following
> it.

Ok.  I tend to be bad and mistakenly consider existing code 'gospel'.
Will fix.

> PS: The comments for struct RowSecurityPolicy could stand to be improved.

Understood, will do so.
Thanks!
    Stephen

All,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > Well, if you *only* move RowSecurityDesc and not RowSecurityPolicy,
> > okay, but that seems a bit useless/inconsistent if I'm reading it
> > right that RowSecurityDesc contains a List of RowSecurityPolicy structs.
>
> Yes, good point.
>
> > What seems possibly saner is to just remove the header inclusion in rel.h
> > and declare the new Relation field similarly to the way we handle
> > rd_fdwroutine and some other fields there:
> >
> >     /* use "struct" here to avoid needing to include rowsecurity.h: */
> >     struct RowSecurityDesc *rsdesc;    /* Row-security policy, or NULL */
>
> Makes sense to me.
>
> > And while you are at it, how about renaming "rsdesc" to "rd_rsdesc"?
> > The fact that whoever put in trigdesc didn't get the memo about the
> > naming convention for Relation fields doesn't excuse you from following
> > it.
>
> Ok.  I tend to be bad and mistakenly consider existing code 'gospel'.
> Will fix.
>
> > PS: The comments for struct RowSecurityPolicy could stand to be improved.
>
> Understood, will do so.

And, done.
Thanks!
    Stephen