Thread: Re: [COMMITTERS] pgsql: Properly install gram.h on MSVC builds
Magnus Hagander <magnus@hagander.net> writes:
> Properly install gram.h on MSVC builds
> This file is now needed by pgAdmin builds, which started
> failing since it was missing in the installer builds.
I'd like to protest this patch as misguided. AFAICS it is a *seriously*
bad idea for pgAdmin to be including gram.h --- we don't even want most
of the backend to include that, let alone external pieces of code. It's
not stable enough. See the note in gramparse.h.
To my way of thinking, not installing that file is a fine idea.
What we really need to be asking is why the pgAdmin folks think
they should be including it.
regards, tom lane
On Sun, Jan 9, 2011 at 17:31, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Properly install gram.h on MSVC builds >> This file is now needed by pgAdmin builds, which started >> failing since it was missing in the installer builds. > > I'd like to protest this patch as misguided. AFAICS it is a *seriously* Uh, we install the file on Unix, so we should do the same on Windows. > bad idea for pgAdmin to be including gram.h --- we don't even want most > of the backend to include that, let alone external pieces of code. It's > not stable enough. See the note in gramparse.h. Whether that's misguided is another thing :-) > To my way of thinking, not installing that file is a fine idea. > What we really need to be asking is why the pgAdmin folks think > they should be including it. It is required in order to pull kwlist.h, which is used to determine which keywords to highlight in the SQL editor (and other places that do syntax highlighting). It never actually uses the values, but it needs the file for kwlist.h to compile. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> On Sun, Jan 9, 2011 at 17:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'd like to protest this patch as misguided. �AFAICS it is a *seriously*
> Uh, we install the file on Unix, so we should do the same on Windows.
Well, my idea of how to fix that would be the other way 'round.
>> What we really need to be asking is why the pgAdmin folks think
>> they should be including it.
> It is required in order to pull kwlist.h,
No, it is not required. What they should be doing is #define'ing
PG_KEYWORD() in a way that ignores its second argument. See pg_dump's
keywords.c for an example of safe usage.
If we allow this to stand then we are going to find people complaining
that we've broken ABI anytime we make a minor change in the grammar.
I repeat: it's a SERIOUSLY bad idea.
regards, tom lane
On Sun, Jan 9, 2011 at 17:49, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On Sun, Jan 9, 2011 at 17:31, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I'd like to protest this patch as misguided. AFAICS it is a *seriously* > >> Uh, we install the file on Unix, so we should do the same on Windows. > > Well, my idea of how to fix that would be the other way 'round. Sure, then it's at least consistent... >>> What we really need to be asking is why the pgAdmin folks think >>> they should be including it. > >> It is required in order to pull kwlist.h, > > No, it is not required. What they should be doing is #define'ing > PG_KEYWORD() in a way that ignores its second argument. See pg_dump's > keywords.c for an example of safe usage. Ahh, good point. And yes, that seems to work for pgadmin. I'll commit a patch there as soon as I've finished testing, at which point it won't be required anymore. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/