Обсуждение: Datum should be defined outside postgres.h

I'm trying to solve one TODO item mentioned in 
src/backend/utils/mmgr/mcxt.c.

----
/* * MemoryContextSwitchTo *              Returns the current context; installs the given context. * * This is inlined
whenusing GCC. * * TODO: investigate supporting inlining for some non-GCC compilers. */
 
----

Everything works fine with Sun Studio instead of zic and ecpg 
compilation. The problem there is that palloc.h defines 
CurrentMemoryContext which is declared in mcxt.c. And palloc.h is 
included by postgres.h.

Unfortunately zic and ecpg break the rule which is mentioned on many 
places and they include postgres.h. Linker is looking for 
CurrentMemoryContext because inlined function requires it. This problem 
disappears when -xO3 is enabled and SS optimizes a code. But it cannot 
be use in general.

I fixed it for zic, but problem with ecpg is that it includes 
nodes/primnodes.h and it requires Datum type definition which is defined 
in postgres.h. :(

By my opinion Datum should be defined in separate file and all headers 
which use this type should include it. (this is problem on many places 
with another types). Another question is why ecpg needs it?

    Any comments how to fix ecpg?
        Zdenek

Am Donnerstag, 25. Oktober 2007 schrieb Zdenek Kotala:
> I fixed it for zic, but problem with ecpg is that it includes
> nodes/primnodes.h and it requires Datum type definition which is defined
> in postgres.h. :(

I don't find an inclusion of primnodes.h in ecpg.  Which file are you looking 
at?

-- 
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:
> Am Donnerstag, 25. Oktober 2007 schrieb Zdenek Kotala:
>> I fixed it for zic, but problem with ecpg is that it includes
>> nodes/primnodes.h and it requires Datum type definition which is defined
>> in postgres.h. :(
> 
> I don't find an inclusion of primnodes.h in ecpg.  Which file are you looking 
> at?
> 

It is indirectly included in parser.c from parser/gramparse.h. Now I 
probably find a solution. I created fake gramparse.h and parser.h into 
ecpg directory (same way as parse.h is faked). It looks fine. Now I'm 
testing it.
    Zdenek

Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> I fixed it for zic, but problem with ecpg is that it includes 
> nodes/primnodes.h and it requires Datum type definition which is defined 
> in postgres.h. :(

Why in the world is ecpg including either primnodes.h or postgres.h?

> By my opinion Datum should be defined in separate file and all headers 
> which use this type should include it. (this is problem on many places 
> with another types). Another question is why ecpg needs it?

Datum is a type that no frontend code has any business dealing in;
and the same goes for everything in primnodes.h.

I'd suggest trying to fix ecpg to not depend on backend-only include
files...
        regards, tom lane

Tom Lane wrote:
> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>> I fixed it for zic, but problem with ecpg is that it includes 
>> nodes/primnodes.h and it requires Datum type definition which is defined 
>> in postgres.h. :(
> 
> Why in the world is ecpg including either primnodes.h or postgres.h?

The problem is that ecpg shares parser.c source code and this code 
includes postgres.h.

>> By my opinion Datum should be defined in separate file and all headers 
>> which use this type should include it. (this is problem on many places 
>> with another types). Another question is why ecpg needs it?
> 
> Datum is a type that no frontend code has any business dealing in;
> and the same goes for everything in primnodes.h.
> 
> I'd suggest trying to fix ecpg to not depend on backend-only include
> files...

Yes, agree. I'm now testing my fix. I removed postgres.h from parser.c + 
performed some other changes around.

    Zdenek

Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> Tom Lane wrote:
>> Why in the world is ecpg including either primnodes.h or postgres.h?

> The problem is that ecpg shares parser.c source code and this code 
> includes postgres.h.

ecpg cannot do that.  It would fail if parser.c happened to use anything
that won't compile in frontend, eg elog() or palloc().  It's mere luck
that it's worked for him so far.

Considering that ecpg has its own copy of all of gram.y and scan.l,
sharing parser.c isn't much of a savings anyway.
        regards, tom lane

Zdenek Kotala wrote:
> Tom Lane wrote:
>> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:

>>> By my opinion Datum should be defined in separate file and all 
>>> headers which use this type should include it. (this is problem on 
>>> many places with another types). Another question is why ecpg needs it?
>>
>> Datum is a type that no frontend code has any business dealing in;
>> and the same goes for everything in primnodes.h.
>>
>> I'd suggest trying to fix ecpg to not depend on backend-only include
>> files...

OK the problem now is pg_dump.c. It includes postgres.h :( and it is 
described there why. It needs it for catalog header files.
Any suggestion how to fix it?

One solution should be put sugar words into separate header and include 
them directly from catalog/*.h files.
    Zdenek

Tom Lane wrote:
> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>> Tom Lane wrote:
>>> Why in the world is ecpg including either primnodes.h or postgres.h?
> 
>> The problem is that ecpg shares parser.c source code and this code 
>> includes postgres.h.
> 
> ecpg cannot do that.  It would fail if parser.c happened to use anything
> that won't compile in frontend, eg elog() or palloc().  It's mere luck
> that it's worked for him so far.
> 
> Considering that ecpg has its own copy of all of gram.y and scan.l,
> sharing parser.c isn't much of a savings anyway.

OK. I will create separate infrastructure fix for ecpg.

    Zdenek

Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> One solution should be put sugar words into separate header and include 
> them directly from catalog/*.h files.

Yeah, that would probably be a good idea.  It's unlikely that we'll
get away anytime soon from frontend code wanting to include
catalog/pg_type.h, in particular (to get the macros for type OIDs).

[ looks at code... ]  Another problem with #including those headers
without postgres.h is going to be the function declarations --- eg.
GenerateTypeDependencies() needs Node *.  I've always thought that
the function declarations lurking at the bottom of the catalog
headers were pretty out-of-place anyway.  What say we pull all
the function declarations out of the catalog/pg_xxx.h files?

Not quite sure where to put them instead, though.  We could smash
them all into one new header, but if you want to keep a separate
header per module then we'll need some new naming convention to
select the filenames to use.
        regards, tom lane

Zdenek Kotala wrote:
> Zdenek Kotala wrote:
> > Tom Lane wrote:
> >> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> 
> >>> By my opinion Datum should be defined in separate file and all 
> >>> headers which use this type should include it. (this is problem on 
> >>> many places with another types). Another question is why ecpg needs it?
> >>
> >> Datum is a type that no frontend code has any business dealing in;
> >> and the same goes for everything in primnodes.h.
> >>
> >> I'd suggest trying to fix ecpg to not depend on backend-only include
> >> files...
> 
> OK the problem now is pg_dump.c. It includes postgres.h :( and it is 
> described there why. It needs it for catalog header files.
> 
>     Any suggestion how to fix it?
> 
> One solution should be put sugar words into separate header and include 
> them directly from catalog/*.h files.

Another idea is to test the FRONTEND macro in the include file to
prevent inclusion of things you don't need.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

On Thu, Oct 25, 2007 at 11:31:15AM -0400, Tom Lane wrote:
> > The problem is that ecpg shares parser.c source code and this code 
> > includes postgres.h.
> 
> ecpg cannot do that.  It would fail if parser.c happened to use anything
> that won't compile in frontend, eg elog() or palloc().  It's mere luck
> that it's worked for him so far.

No, actually it's the first step at making ecpg use all the backend
files instead. I would prefer to get away from all those manual syncing.

> Considering that ecpg has its own copy of all of gram.y and scan.l,
> sharing parser.c isn't much of a savings anyway.

For the time being, no, you're right. 

Michael
-- 
Michael Meskes
Email: Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go SF 49ers! Go Rhein Fire! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes <meskes@postgresql.org> writes:
> On Thu, Oct 25, 2007 at 11:31:15AM -0400, Tom Lane wrote:
>> ecpg cannot do that.  It would fail if parser.c happened to use anything
>> that won't compile in frontend, eg elog() or palloc().  It's mere luck
>> that it's worked for him so far.

> No, actually it's the first step at making ecpg use all the backend
> files instead. I would prefer to get away from all those manual syncing.

Well, that's surely a good idea, but there'll have to be some
negotiation to figure out how to do that.  None of those files are
currently designed with any thought of being compilable outside the
backend environment.

The hard part really would be sharing gram.y and scan.l, especially in
view of the fact that we need different actions.  Do you have a plan
for doing that?
        regards, tom lane

On Sat, Oct 27, 2007 at 11:04:19AM -0400, Tom Lane wrote:
> Well, that's surely a good idea, but there'll have to be some
> negotiation to figure out how to do that.  None of those files are
> currently designed with any thought of being compilable outside the
> backend environment.
> 
> The hard part really would be sharing gram.y and scan.l, especially in
> view of the fact that we need different actions.  Do you have a plan
> for doing that?

Not yet, no. My only plan was to start tackling the topic by trying to
get scan.l into ecpg (seems to be easier than gram.y) and see what ecpg
needs and what not. 

Michael
-- 
Michael Meskes
Email: Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go SF 49ers! Go Rhein Fire! Use Debian GNU/Linux! Use PostgreSQL!

Tom Lane wrote:
> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>> One solution should be put sugar words into separate header and include 
>> them directly from catalog/*.h files.
> 
> Yeah, that would probably be a good idea.  It's unlikely that we'll
> get away anytime soon from frontend code wanting to include
> catalog/pg_type.h, in particular (to get the macros for type OIDs).
> 
> [ looks at code... ]  Another problem with #including those headers
> without postgres.h is going to be the function declarations --- eg.
> GenerateTypeDependencies() needs Node *.  I've always thought that
> the function declarations lurking at the bottom of the catalog
> headers were pretty out-of-place anyway.  What say we pull all
> the function declarations out of the catalog/pg_xxx.h files?


catalog directory contains mix of solutions :(. There are for example 
functions defined into pg_type.h or there are namespace and 
pg_namespace.h already.

My idea is to put functions declaration int pg_xxx.h and structure
declaration in pg_xxx_def.h. I'm not sure if split DATA into separate
header is good idea, but if yes then pg_xxx_data.h should be good name
for it (it seems that pg_dump needs only defines).

There is also problem with sequence.h  which contains SEQ_MAX and 
SEQ_MIN macros.
Comments?
    Thanks Zdenek

> Not quite sure where to put them instead, though.  We could smash
> them all into one new header, but if you want to keep a separate
> header per module then we'll need some new naming convention to
> select the filenames to use.
> 

Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> My idea is to put functions declaration int pg_xxx.h and structure
> declaration in pg_xxx_def.h. I'm not sure if split DATA into separate
> header is good idea, but if yes then pg_xxx_data.h should be good name
> for it (it seems that pg_dump needs only defines).

That seems far more invasive than is justified, as it will essentially
force touching every file that includes any catalog header.  I think
the vast majority are including for the struct definitions, and so the
structs should stay where they are.  Moving the DATA lines is a pretty
bad idea as well, since we'd also have to move the corresponding OID
macro #defines (in examples such as pg_type.h), leading to yet more
pointless #include-thrashing.
        regards, tom lane

Tom Lane wrote:
> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>> My idea is to put functions declaration int pg_xxx.h and structure
>> declaration in pg_xxx_def.h. I'm not sure if split DATA into separate
>> header is good idea, but if yes then pg_xxx_data.h should be good name
>> for it (it seems that pg_dump needs only defines).
> 
> That seems far more invasive than is justified, as it will essentially
> force touching every file that includes any catalog header.  

Not exactly pg_type could include pg_type.data, but you are right it is 
more invasive than I really need.

> I think
> the vast majority are including for the struct definitions, and so the
> structs should stay where they are.  

Ok. In this point of view better should be create e.g. pg_type_fn.h with 
function declaration and add this include file into related files.

    Zdenek