Debug in dest.h

Started by Alvaro Herreraover 20 years ago8 messageshackers
Jump to latest
#1Alvaro Herrera
alvherre@2ndquadrant.com

Hackers,

The "Debug" destreceiver has a pretty bad name. In fact for PL/php it
collides with a PHP symbol. Does anyone mind if I rename it to, say,
DestDebug?

--
Alvaro Herrera http://www.advogato.org/person/alvherre
"El sabio habla porque tiene algo que decir;
el tonto, porque tiene que decir algo" (Platon).

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#1)
Re: Debug in dest.h

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

The "Debug" destreceiver has a pretty bad name. In fact for PL/php it
collides with a PHP symbol. Does anyone mind if I rename it to, say,
DestDebug?

If you do that, you should rename all the values of the enum
(DestNone, etc). For consistency, and because the same charge
could be leveled at None and Remote.

regards, tom lane

#3Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#2)
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)

Attachments:

includes.patchtext/plain; charset=us-asciiDownload+3-6
portalforw.htext/x-chdr; charset=us-asciiDownload
#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#3)
Re: Limit usage of tcop/dest.h

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

#5Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#4)
Re: Limit usage of tcop/dest.h

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
#6Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Bruce Momjian (#5)
Re: Limit usage of tcop/dest.h

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)

#7Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#4)
Re: Limit usage of tcop/dest.h

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)

Attachments:

dest-rename.patchtext/plain; charset=us-asciiDownload+195-195
#8Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#7)
Re: Limit usage of tcop/dest.h

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