Обсуждение: postgres.h included from relcache.h - but removing it breaks pg_upgrade

Hi,

I noticed that postgres.h is included from relcache.h (starting in [1]) and
wanted to fix that - it violates our usual policy against including postgres.h
from within headers.

But then I noticed that that causes pg_upgrade/file.c to fail to compile:

In file included from /home/andres/src/postgresql/src/include/access/visibilitymap.h:20,
                 from /home/andres/src/postgresql/src/bin/pg_upgrade/file.c:22:
/home/andres/src/postgresql/src/include/utils/relcache.h:53:8: error: unknown type name ‘Datum’
   53 | extern Datum *RelationGetIndexRawAttOptions(Relation relation);

Which is presumably why the postgres.h include was added in [1]. The only
reason this didn't fail before is because there wasn't any other reference to
Datum (or any of the other postgres.h types) in relcache.h before this commit.


I guess the best solution is to add include the "full" postgres.h explicitly
from file.c like several other places, like e.g. src/bin/pg_controldata/pg_controldata.c
do:

/*
 * We have to use postgres.h not postgres_fe.h here, because there's so much
 * backend-only stuff in the XLOG include files we need.  But we need a
 * frontend-ish environment otherwise.  Hence this ugly hack.
 */
#define FRONTEND 1


I was also wondering if we should put something in c.h and postgres.h to avoid
redundant includes? Currently there's a few .c files that "use" the header
guards, all scanners that we include into the parsers.


Greetings,

Andres Freund


[1] commit 911e70207703799605f5a0e8aad9f06cff067c63
Author: Alexander Korotkov <akorotkov@postgresql.org>
Date:   2020-03-30 19:17:11 +0300

    Implement operator class parameters

Andres Freund <andres@anarazel.de> writes:
> I noticed that postgres.h is included from relcache.h (starting in [1]) and
> wanted to fix that - it violates our usual policy against including postgres.h
> from within headers.

Ugh, yeah, that's entirely against policy.

As for the fix ... what in the world is pg_upgrade doing including
relcache.h?  It seems like there's a more fundamental problem here:
either relcache.h is declaring something that needs to be elsewhere,
or pg_upgrade is doing something it should not.

> I was also wondering if we should put something in c.h and postgres.h to avoid
> redundant includes?

No.  If anything, I'd want to throw an error for "redundant" includes
of these files, because it's a pretty good red flag about
poorly-thought-out header modularization.

            regards, tom lane

Hi,

On 2021-09-13 22:40:19 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I noticed that postgres.h is included from relcache.h (starting in [1]) and
> > wanted to fix that - it violates our usual policy against including postgres.h
> > from within headers.
> 
> Ugh, yeah, that's entirely against policy.
> 
> As for the fix ... what in the world is pg_upgrade doing including
> relcache.h?  It seems like there's a more fundamental problem here:
> either relcache.h is declaring something that needs to be elsewhere,
> or pg_upgrade is doing something it should not.

It's not directly including relcache. pg_upgrade/file.c needs a few symbols
from visibilitymap.h, for the upgrade of the visibilitymap from old to new
format. And visibilitymap needs relcache.h because several of its routines
take Relation params.

We could split visibilitymap.h into two, or we could forward-declare Relation
and not include relcache...


> > I was also wondering if we should put something in c.h and postgres.h to avoid
> > redundant includes?
> 
> No.  If anything, I'd want to throw an error for "redundant" includes
> of these files, because it's a pretty good red flag about
> poorly-thought-out header modularization.

I think we might be thinking of the same. What I meant with "avoid" was to
raise a warning or error. If we were to do that, it's probably worth doing the
build system ugliness to do this only when building postgres code, rather than
extensions...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2021-09-13 22:40:19 -0400, Tom Lane wrote:
>> As for the fix ... what in the world is pg_upgrade doing including
>> relcache.h?  It seems like there's a more fundamental problem here:
>> either relcache.h is declaring something that needs to be elsewhere,
>> or pg_upgrade is doing something it should not.

> We could split visibilitymap.h into two, or we could forward-declare Relation
> and not include relcache...

Without having looked at the details, I think using a forward-declare
to avoid including relcache.h in visibilitymap.h might be a reasonably
non-painful fix.  OTOH, in the long run it might be worth the effort
to split visibilitymap.h to separate useful file-contents knowledge
from backend function declarations.

>> No.  If anything, I'd want to throw an error for "redundant" includes
>> of these files, because it's a pretty good red flag about
>> poorly-thought-out header modularization.

> I think we might be thinking of the same. What I meant with "avoid" was to
> raise a warning or error.

Ah, we are on the same page then.  I misunderstood what you wrote.

> If we were to do that, it's probably worth doing the
> build system ugliness to do this only when building postgres code, rather than
> extensions...

As long as we do this in HEAD only, I'm not sure why extensions
need an exception.  Perhaps it will result in somebody pointing out
additional poorly-thought-out header contents, but I don't think
that's bad.

            regards, tom lane

On Tue, Sep 14, 2021 at 5:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I noticed that postgres.h is included from relcache.h (starting in [1]) and
> > wanted to fix that - it violates our usual policy against including postgres.h
> > from within headers.
>
> Ugh, yeah, that's entirely against policy.

I see. This is my oversight, sorry for that.

------
Regards,
Alexander Korotkov

On Tue, Sep 14, 2021 at 6:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2021-09-13 22:40:19 -0400, Tom Lane wrote:
> >> As for the fix ... what in the world is pg_upgrade doing including
> >> relcache.h?  It seems like there's a more fundamental problem here:
> >> either relcache.h is declaring something that needs to be elsewhere,
> >> or pg_upgrade is doing something it should not.
>
> > We could split visibilitymap.h into two, or we could forward-declare Relation
> > and not include relcache...
>
> Without having looked at the details, I think using a forward-declare
> to avoid including relcache.h in visibilitymap.h might be a reasonably
> non-painful fix.

I like that idea, but I didn't find an appropriate existing header for
forward-declaration of Relation.  relation.h isn't suitable, because
it includes primnodes.h.  A separate header for just
forward-definition of Relation seems too much.

> TOH, in the long run it might be worth the effort
> to split visibilitymap.h to separate useful file-contents knowledge
> from backend function declarations.

I've drafted a patch splitting visibilitymap_maros.h from
visibilitymap.h.  What do you think?

------
Regards,
Alexander Korotkov

Hi,

On 2021-09-18 02:51:09 +0300, Alexander Korotkov wrote:
> On Tue, Sep 14, 2021 at 6:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Without having looked at the details, I think using a forward-declare
> > to avoid including relcache.h in visibilitymap.h might be a reasonably
> > non-painful fix.
> 
> I like that idea, but I didn't find an appropriate existing header for
> forward-declaration of Relation.  relation.h isn't suitable, because
> it includes primnodes.h.  A separate header for just
> forward-definition of Relation seems too much.

I was just thinking of doing something like the attached.


> > TOH, in the long run it might be worth the effort
> > to split visibilitymap.h to separate useful file-contents knowledge
> > from backend function declarations.
> 
> I've drafted a patch splitting visibilitymap_maros.h from
> visibilitymap.h.  What do you think?

I'd name it visibilitymapdefs.h or such, mostly because that's what other
headers are named like...

Greetings,

Andres Freund

Hi,

On Sat, Sep 18, 2021 at 3:06 AM Andres Freund <andres@anarazel.de> wrote:
> On 2021-09-18 02:51:09 +0300, Alexander Korotkov wrote:
> > On Tue, Sep 14, 2021 at 6:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > Without having looked at the details, I think using a forward-declare
> > > to avoid including relcache.h in visibilitymap.h might be a reasonably
> > > non-painful fix.
> >
> > I like that idea, but I didn't find an appropriate existing header for
> > forward-declaration of Relation.  relation.h isn't suitable, because
> > it includes primnodes.h.  A separate header for just
> > forward-definition of Relation seems too much.
>
> I was just thinking of doing something like the attached.

I see now.  I think I'm rather favoring splitting visibilitymap.h.

> > > TOH, in the long run it might be worth the effort
> > > to split visibilitymap.h to separate useful file-contents knowledge
> > > from backend function declarations.
> >
> > I've drafted a patch splitting visibilitymap_maros.h from
> > visibilitymap.h.  What do you think?
>
> I'd name it visibilitymapdefs.h or such, mostly because that's what other
> headers are named like...

Good point.  The revised patch is attached.

------
Regards,
Alexander Korotkov

On 2021-Sep-18, Alexander Korotkov wrote:

> I see now.  I think I'm rather favoring splitting visibilitymap.h.

Agreed, this looks sane to me.  However, I think the
VM_ALL_{VISIBLE,FROZEN} macros should remain in visibilitymap.h, since
they depend on the visibilitymap_get_status function (and pg_upgrade
doesn't use them).

There's a typo "maros" for "macros" in the new header file.  (Also, why
does the copyright line say "portions" if no portion under another
copyright?  I think we don't say "portions" when there is only one
copyright statement line.)

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/

On Sat, Sep 18, 2021 at 11:35 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2021-Sep-18, Alexander Korotkov wrote:
>
> > I see now.  I think I'm rather favoring splitting visibilitymap.h.
>
> Agreed, this looks sane to me.  However, I think the
> VM_ALL_{VISIBLE,FROZEN} macros should remain in visibilitymap.h, since
> they depend on the visibilitymap_get_status function (and pg_upgrade
> doesn't use them).
>
> There's a typo "maros" for "macros" in the new header file.  (Also, why
> does the copyright line say "portions" if no portion under another
> copyright?  I think we don't say "portions" when there is only one
> copyright statement line.)

Thank you for the feedback.  All changes are accepted.

Any objections to pushing this?

------
Regards,
Alexander Korotkov

On 2021-09-19 18:45:45 +0300, Alexander Korotkov wrote:
> Any objections to pushing this?

lgtm

I assume you're planning to backpatch this?

On Mon, Sep 20, 2021 at 10:48 PM Andres Freund <andres@anarazel.de> wrote:
> On 2021-09-19 18:45:45 +0300, Alexander Korotkov wrote:
> > Any objections to pushing this?
>
> lgtm

Thanks!

> I assume you're planning to backpatch this?

Yes.

------
Regards,
Alexander Korotkov

Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Mon, Sep 20, 2021 at 10:48 PM Andres Freund <andres@anarazel.de> wrote:
>> I assume you're planning to backpatch this?

> Yes.

Probably good to wait 24 hours until 14rc1 has been tagged.

            regards, tom lane

On Tue, Sep 21, 2021 at 2:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > On Mon, Sep 20, 2021 at 10:48 PM Andres Freund <andres@anarazel.de> wrote:
> >> I assume you're planning to backpatch this?
>
> > Yes.
>
> Probably good to wait 24 hours until 14rc1 has been tagged.

OK, NP!

------
Regards,
Alexander Korotkov