Thread: Limit usage of tcop/dest.h
Hi, I just found out that tcop/dest.h is included in executor/spi.h, and it contains many things that aren't needed for compiling SPI programs/ libraries. By way of example I compiled the whole of contrib with the attached patch and it works fine. Notice that the only thing I'm doing is taking the forward declaration of Portal into a separate file, portalforw.h -- that's the only definition that's really needed by SPI programs. (It also allows PL/php to compile without having to patch PHP nor PostgreSQL sources). Note that since tcop/dest.h now includes portalforw.h, anybody who currently needs the Portal definition is still getting it. The only thing I'm doing is un-export the rest of tcop/dest.h from executor/spi.h. So instead of changing the names of the CommandDest enum, I'm hiding it from external view. Note that executor/spi.h does not follow the convention that #includes should be alphabetically ordered. I did not change that in this patch in order to show that this change is really minimal. Does anybody object to committing this patch to current CVS HEAD? (Comments about a better position/name for the new file are welcome.) -- Alvaro Herrera http://www.amazon.com/gp/registry/DXLWNGRJD34J "The only difference is that Saddam would kill you on private, where the Americans will kill you in public" (Mohammad Saleh, 39, a building contractor)
Attachment
Alvaro Herrera said: > Hi, > > (It also allows PL/php to compile without having to patch > PHP nor PostgreSQL sources). > That will make some people I know happy ;-) cheers andrew
Andrew Dunstan wrote: > Alvaro Herrera said: > > Hi, > > > > (It also allows PL/php to compile without having to patch > > PHP nor PostgreSQL sources). > > That will make some people I know happy ;-) Yeah -- the current PL/php build system is a crock (not sure what that is, but it sounds nice and it appears on Pg sources) :-) so I'm currently modifying it to work using PGXS. They won't need Pg sources at all really (and conversely, only PHP headers and the shared lib, not the whole sources). -- Alvaro Herrera http://www.amazon.com/gp/registry/CTMLCN8V17R4 "La realidad se compone de muchos sueños, todos ellos diferentes, pero en cierto aspecto, parecidos..." (Yo, hablando de sueños eróticos)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> So instead of changing the names of the CommandDest enum, I'm hiding it
> from external view.
I thought renaming them was a better idea, actually. A whole separate
include file to have one forward typedef seems pretty silly. Nor am I
convinced that you won't break some people's code by removing the rest
of dest.h from spi.h. Finally, for anyone who *does* need to include
dest.h, this doesn't address the underlying problem of risk of conflict
of names.
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > So instead of changing the names of the CommandDest enum, I'm hiding it > > from external view. > > I thought renaming them was a better idea, actually. A whole separate > include file to have one forward typedef seems pretty silly. Nor am I > convinced that you won't break some people's code by removing the rest > of dest.h from spi.h. Finally, for anyone who *does* need to include > dest.h, this doesn't address the underlying problem of risk of conflict > of names. Does the change make building PL/PHP easier? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian wrote: > Tom Lane wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > So instead of changing the names of the CommandDest enum, I'm hiding it > > > from external view. > > > > I thought renaming them was a better idea, actually. A whole separate > > include file to have one forward typedef seems pretty silly. Nor am I > > convinced that you won't break some people's code by removing the rest > > of dest.h from spi.h. Finally, for anyone who *does* need to include > > dest.h, this doesn't address the underlying problem of risk of conflict > > of names. > > Does the change make building PL/PHP easier? Yes, the point of these changes is to make PL/php much easier. Either one will do -- renaming the enum elements is what I'm doing now, so we don't have to change include file. (Mind you, I still believe that that particular declaration does not belong in that file, but that's a different discussion.) (We will still need some hack in order to build PL/php against 8.0, but that's another problem.) -- Alvaro Herrera http://www.amazon.com/gp/registry/DXLWNGRJD34J "Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)
Tom Lane wrote: > I thought renaming them was a better idea, actually. Here is a patch for that. I will apply this to HEAD later today. -- Alvaro Herrera Valdivia, Chile ICBM: S 39º 49' 17.7", W 73º 14' 26.8" "The eagle never lost so much time, as when he submitted to learn of the crow." (William Blake)
Attachment
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Tom Lane wrote:
>> I thought renaming them was a better idea, actually.
> Here is a patch for that. I will apply this to HEAD later today.
Looks ok in a quick eyeball pass.
regards, tom lane
Merlin Moncure wrote: > > Tom Lane wrote: > > > > > I thought renaming them was a better idea, actually. > > > > Here is a patch for that. I will apply this to HEAD later today. > > I'm getting compiler error (win32) which I think is related to this > patch: Oops, certainly. Sorry, fixed. I looked for other cases of symbols in EXEC_BACKEND and didn't find one. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
> Tom Lane wrote: > > > I thought renaming them was a better idea, actually. > > Here is a patch for that. I will apply this to HEAD later today. > > -- I'm getting compiler error (win32) which I think is related to this patch: postmaster.c: In function `SubPostmasterMain': postmaster.c:3189: error: `None' undeclared (first use in this function) postmaster.c:3189: error: (Each undeclared identifier is reported only once postmaster.c:3189: error: for each function it appears in.) make[2]: *** [postmaster.o] Error 1 make[2]: Leaving directory `/postgres/pgsql/src/backend/postmaster' make[1]: *** [postmaster-recursive] Error 2 Merlin