Обсуждение: 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