Re: Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend

On 2013-01-08 15:55:24 -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > Andres Freund wrote:
> >> Sorry, misremembered the problem somewhat. The problem is that code that
> >> includes postgres.h atm ends up with ExceptionalCondition() et
> >> al. declared even if FRONTEND is defined. So if anything uses an assert
> >> you need to provide wrappers for those which seems nasty. If they are
> >> provided centrally and check for FRONTEND that problem doesn't exist.
>
> > I think the right fix here is to fix things so that postgres.h is not
> > necessary. How hard is that? Maybe it just requires some more
> > reshuffling of xlog headers.
>
> That would definitely be the ideal fix. In general, #include'ing
> postgres.h into code that's not backend code opens all kinds of hazards
> that are likely to bite us sooner or later; the incompatibility of the
> Assert definitions is just the tip of that iceberg. (In the past we've
> had issues around <stdio.h> providing different definitions in the two
> environments, for example.)

I agree that going in that direction is a good thing, but imo that doesn't
really has any bearing on whether this patch is a good/bad idea...

> But having said that, I'm also now remembering that we have files in
> src/port/ and probably elsewhere that try to work in both environments
> by just #include'ing c.h directly (relying on the Makefile to supply
> -DFRONTEND or not). If we wanted to support Assert use in such files,
> we would have to move at least the Assert() macro definitions into c.h
> as Andres is proposing. Now, I've always thought that #include'ing c.h
> directly was kind of an ugly hack, but I don't have a better design than
> that to offer ATM.

Imo having some common dumping ground for things that concern both environments
is a good idea. I don't think c.h would be the best place for it when
redesigning from ground up, but were not doing that so imo its acceptable as
long as we take care not to let it grow too much.

Here's a trivially updated patch which also defines AssertArg() for
FRONTEND-ish environments since Alvaro added one in xlogreader.c.

Do we agree this is the way forward, at least for now?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services