Thread: Re: [COMMITTERS] pgsql: Remove "fmgr.h" include in cube contrib --- caused crash on a Ge

Excerpts from Bruce Momjian's message of vie sep 02 12:20:50 -0300 2011:

> Wow, that is interesting.  So the problem is the inclusion of
> replication/walsender.h in xlog.h.  Hard to see how that could cause the
> cube regression tests to fail, but of course, it is.

Hmm, so you included walsender.h into xlog.h?  That seems a bit funny
considering that walsender.h already includes xlog.h.  It seems the
reason for this is only the AllowCascadeReplication() definition.  Maybe
that should go elsewhere instead, for example walsender.h?

I wonder if there should be a new header, something like
walsender_internal.h, for stuff like WalSnd and WalSndCtlData struct
defs.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Excerpts from Bruce Momjian's message of vie sep 02 12:20:50 -0300 2011:
> 
> > Wow, that is interesting.  So the problem is the inclusion of
> > replication/walsender.h in xlog.h.  Hard to see how that could cause the
> > cube regression tests to fail, but of course, it is.
> 
> Hmm, so you included walsender.h into xlog.h?  That seems a bit funny
> considering that walsender.h already includes xlog.h.  It seems the
> reason for this is only the AllowCascadeReplication() definition.  Maybe
> that should go elsewhere instead, for example walsender.h?

Moved to walsender.h. Thanks.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +

Bruce Momjian <bruce@momjian.us> writes:
> Alvaro Herrera wrote:
>> Hmm, so you included walsender.h into xlog.h?  That seems a bit funny
>> considering that walsender.h already includes xlog.h.  It seems the
>> reason for this is only the AllowCascadeReplication() definition.  Maybe
>> that should go elsewhere instead, for example walsender.h?

> Moved to walsender.h. Thanks.

You seem to have entirely missed the point of Alvaro's remark, which is
that you've got xlog.h including walsender.h (still) as well as
walsender.h including xlog.h.  That's broken.
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Alvaro Herrera wrote:
> >> Hmm, so you included walsender.h into xlog.h?  That seems a bit funny
> >> considering that walsender.h already includes xlog.h.  It seems the
> >> reason for this is only the AllowCascadeReplication() definition.  Maybe
> >> that should go elsewhere instead, for example walsender.h?
> 
> > Moved to walsender.h. Thanks.
> 
> You seem to have entirely missed the point of Alvaro's remark, which is
> that you've got xlog.h including walsender.h (still) as well as
> walsender.h including xlog.h.  That's broken.

Oh, OK, done.  xlog.h removed from walsender.h and tested.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> You seem to have entirely missed the point of Alvaro's remark, which is
>> that you've got xlog.h including walsender.h (still) as well as
>> walsender.h including xlog.h.  That's broken.

> Oh, OK, done.  xlog.h removed from walsender.h and tested.

I would have gone the other way on that one, if possible.  Seems like
xlog.h ought to be the lower-level file.

Some quick mechanical analysis shows that's not the only circular
inclusion path in our headers, either: we have storage/proc.h including
replication/syncrep.h which includes storage/proc.h.  That one appears
to be Simon's fault not yours though.
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> You seem to have entirely missed the point of Alvaro's remark, which is
> >> that you've got xlog.h including walsender.h (still) as well as
> >> walsender.h including xlog.h.  That's broken.
> 
> > Oh, OK, done.  xlog.h removed from walsender.h and tested.
> 
> I would have gone the other way on that one, if possible.  Seems like
> xlog.h ought to be the lower-level file.

Agreed.  Let me work on that.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> I would have gone the other way on that one, if possible.  Seems like
>> xlog.h ought to be the lower-level file.

> Agreed.  Let me work on that.

Uh, I just did it.  Painful.  It would have been a lot easier before
the pgrminclude run, because that baked-in a whole lot of bad decisions.
        regards, tom lane

Excerpts from Alvaro Herrera's message of vie sep 02 13:53:12 -0300 2011:

> I wonder if there should be a new header, something like
> walsender_internal.h, for stuff like WalSnd and WalSndCtlData struct
> defs.

... as in the attached patch.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support