Fix pg_dump dependency on postgres.h

Lists: pgsql-hackerspgsql-patches
From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Fix pg_dump dependency on postgres.h
Date: 2007-11-06 13:22:09
Message-ID: 47306A81.60402@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Attached patch removes pg_dump dependency on postgres.h. The main reason
for that was discussed there:

http://archives.postgresql.org/pgsql-hackers/2007-10/msg01261.php

This fix contains several steps:

1) I removed sugar word from postgres.h and put them closer to consumer
:-). I created include/catalog/genbki.h which contains sugar words -
macros for correct catalog data processing. All catalogs file now
include this header.

2) I moved SEQ_MAXVALUE and SEQ_MINVALUE macros from sequence.h to
postgres_config_manual.h

3) I created two new headers pg_type_fn.h and pg_proc_fn.h and I moved
all extern function definition from related headers into them. Second
possible solution could be let function definition into headers and
fence them by #ifndef FRONTED.

Let me know your comments.

Thanks
Zdenek

Attachment Content-Type Size
pg_dump.patch.gz application/x-gzip 6.1 KB

From: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
To: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
Cc: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-06 16:58:05
Message-ID: 20071106165805.GF2694@alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Zdenek Kotala wrote:
> Attached patch removes pg_dump dependency on postgres.h. The main reason
> for that was discussed there:
>
> http://archives.postgresql.org/pgsql-hackers/2007-10/msg01261.php
>
> This fix contains several steps:
>
> 1) I removed sugar word from postgres.h and put them closer to consumer
> :-). I created include/catalog/genbki.h which contains sugar words - macros
> for correct catalog data processing. All catalogs file now include this
> header.

What's the point of this? I don't see what difference it makes from the
current situation. In particular I don't see it being included in any
new place.

The other two changes seem to be what was discussed:

> 2) I moved SEQ_MAXVALUE and SEQ_MINVALUE macros from sequence.h to
> postgres_config_manual.h
>
> 3) I created two new headers pg_type_fn.h and pg_proc_fn.h and I moved all
> extern function definition from related headers into them.

--
Alvaro Herrera http://www.amazon.com/gp/registry/5ZYLFMCVHXC
One man's impedance mismatch is another man's layer of abstraction.
(Lincoln Yeoh)


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-06 17:43:30
Message-ID: 4730A7C2.5030702@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Alvaro Herrera wrote:
> Zdenek Kotala wrote:
>> Attached patch removes pg_dump dependency on postgres.h. The main reason
>> for that was discussed there:
>>
>> http://archives.postgresql.org/pgsql-hackers/2007-10/msg01261.php
>>
>> This fix contains several steps:
>>
>> 1) I removed sugar word from postgres.h and put them closer to consumer
>> :-). I created include/catalog/genbki.h which contains sugar words - macros
>> for correct catalog data processing. All catalogs file now include this
>> header.
>
> What's the point of this? I don't see what difference it makes from the
> current situation. In particular I don't see it being included in any
> new place.

The problem is that postgres.h include palloc.h which contains extern
declaration of MemoryContext global variable. It is not correct for
tools as pg_dump is. Because it does not use palloc. When I enabled
inline functions and start compiling postgres with sunstudio, linking
phase failed on pg_dump because MemoryContext is not allocated. It is
background of problem.

pg_dump.c needs some macros which are defined into catalogs header. But
it should not include postgres.h where is defined "sugar words" for
catalog data. It is the reason why I moved these macros to the separate
header and include this header from all catalog header.

It was discussed there:

http://archives.postgresql.org/pgsql-hackers/2007-10/msg01277.php

Zdenek


From: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
To: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
Cc: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-06 23:49:06
Message-ID: 20071106234906.GK8635@alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Zdenek Kotala wrote:
> Alvaro Herrera wrote:

>>> 1) I removed sugar word from postgres.h and put them closer to consumer
>>> :-). I created include/catalog/genbki.h which contains sugar words -
>>> macros for correct catalog data processing. All catalogs file now include
>>> this header.
>> What's the point of this? I don't see what difference it makes from the
>> current situation. In particular I don't see it being included in any
>> new place.

> pg_dump.c needs some macros which are defined into catalogs header. But it
> should not include postgres.h where is defined "sugar words" for catalog
> data. It is the reason why I moved these macros to the separate header and
> include this header from all catalog header.

Ah, the part that I was missing was that the catalog headers were being
included by pg_dump.c.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Cc: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-09 17:02:27
Message-ID: 473492A3.9070900@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Zdenek Kotala wrote:
> Attached patch removes pg_dump dependency on postgres.h. The main reason
> for that was discussed there:
>
> http://archives.postgresql.org/pgsql-hackers/2007-10/msg01261.php
>

I found two problems there. One is that I forgot postgres.h include in
common.c. it is easy to fix. However second problem is more complicated.
dumputils.c calls ScandKeywordLookup function which is defined in
keyword.c. :(

I currently see two possible variant:

1) Put list of RESERVED keyword into dumputils and use bsearch for
lookup. It is easy to implement but it will be difficult to keep
synchronize these two list together.

2) Modify gram.y to generate parse.h which will be friendly for backend
and can be used in keyword.c. Probably add some ifdef ...

3) Put following fake into keyword.c before include "parse.h" line. It
is easiest way.

#define TYPE_IS_DECLARED 1
#define YYLTYPE_IS_DECLARED 1
#define YYLTYPE void*
#define YYSTYPE void*

Comments or any ideas?

Zdenek


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To:
Cc: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-14 14:14:45
Message-ID: 473B02D5.8020200@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Zdenek Kotala wrote:
> Zdenek Kotala wrote:
>> Attached patch removes pg_dump dependency on postgres.h. The main
>> reason for that was discussed there:
>>
>> http://archives.postgresql.org/pgsql-hackers/2007-10/msg01261.php
>>
>
> I found two problems there. One is that I forgot postgres.h include in
> common.c. it is easy to fix. However second problem is more complicated.
> dumputils.c calls ScandKeywordLookup function which is defined in
> keyword.c. :(
>

<snip>

> 3) Put following fake into keyword.c before include "parse.h" line. It
> is easiest way.
>
> #define TYPE_IS_DECLARED 1
> #define YYLTYPE_IS_DECLARED 1
> #define YYLTYPE void*
> #define YYSTYPE void*
>

New version of patch is attached. I selected variant 3 as a best
solution. Patch also fix some other postgres.h dependencyin another
tools such as pg_controldata, pg_config. The last unfixed tool is
pg_resetxlog which deserves own patch.

With regards Zdenek

Attachment Content-Type Size
pg_dump_02.patch text/x-patch 53.4 KB

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
Cc: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-14 14:33:03
Message-ID: 20071114143303.GQ19014@alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Zdenek Kotala wrote:
> Zdenek Kotala wrote:
>> Zdenek Kotala wrote:
>>> Attached patch removes pg_dump dependency on postgres.h. The main reason
>>> for that was discussed there:
>>>
>>> http://archives.postgresql.org/pgsql-hackers/2007-10/msg01261.php
>>>
>> I found two problems there. One is that I forgot postgres.h include in
>> common.c. it is easy to fix. However second problem is more complicated.
>> dumputils.c calls ScandKeywordLookup function which is defined in
>> keyword.c. :(
>
> <snip>
>
>> 3) Put following fake into keyword.c before include "parse.h" line. It is
>> easiest way.
>> #define TYPE_IS_DECLARED 1
>> #define YYLTYPE_IS_DECLARED 1
>> #define YYLTYPE void*
>> #define YYSTYPE void*
>
> New version of patch is attached. I selected variant 3 as a best solution.
> Patch also fix some other postgres.h dependencyin another tools such as
> pg_controldata, pg_config. The last unfixed tool is pg_resetxlog which
> deserves own patch.

Humm, but YYLTYPE is defined in gramparse.h (and as a different type)
... Also, I see that if you define YYLTYPE then you don't need
YYLTYPE_IS_DECLARED as well. Also I don't see any TYPE_IS_DECLARED
here. What I'm thinking is that this patch is not very portable :-(

--
Alvaro Herrera Valdivia, Chile ICBM: S 39º 49' 18.1", W 73º 13' 56.4"
"Llegará una época en la que una investigación diligente y prolongada sacará
a la luz cosas que hoy están ocultas" (Séneca, siglo I)


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-14 15:13:19
Message-ID: 473B108F.40809@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Alvaro Herrera wrote:
> Zdenek Kotala wrote:
>> Zdenek Kotala wrote:
>>> Zdenek Kotala wrote:
>>>> Attached patch removes pg_dump dependency on postgres.h. The main reason
>>>> for that was discussed there:
>>>>
>>>> http://archives.postgresql.org/pgsql-hackers/2007-10/msg01261.php
>>>>
>>> I found two problems there. One is that I forgot postgres.h include in
>>> common.c. it is easy to fix. However second problem is more complicated.
>>> dumputils.c calls ScandKeywordLookup function which is defined in
>>> keyword.c. :(
>> <snip>
>>
>>> 3) Put following fake into keyword.c before include "parse.h" line. It is
>>> easiest way.
>>> #define TYPE_IS_DECLARED 1
>>> #define YYLTYPE_IS_DECLARED 1
>>> #define YYLTYPE void*
>>> #define YYSTYPE void*
>> New version of patch is attached. I selected variant 3 as a best solution.
>> Patch also fix some other postgres.h dependencyin another tools such as
>> pg_controldata, pg_config. The last unfixed tool is pg_resetxlog which
>> deserves own patch.
>
> Humm, but YYLTYPE is defined in gramparse.h (and as a different type)
> ... Also, I see that if you define YYLTYPE then you don't need
> YYLTYPE_IS_DECLARED as well. Also I don't see any TYPE_IS_DECLARED
> here. What I'm thinking is that this patch is not very portable :-(

Thanks you for your comments.

You are right, define YYLTYPE and YYSTYPE is enough to skip
union/structure definition. I think, data type is not important for
this purpose, but use int instead of void* seem good idea. It will be
synchronized with gramparse.h.

What do you mean "not very portable"? What could be problem there?

Zdenek


From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
Cc: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-14 15:21:28
Message-ID: 20071114152128.GR19014@alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Zdenek Kotala wrote:
> Alvaro Herrera wrote:

>>>> 3) Put following fake into keyword.c before include "parse.h" line. It
>>>> is easiest way.
>>>> #define TYPE_IS_DECLARED 1
>>>> #define YYLTYPE_IS_DECLARED 1
>>>> #define YYLTYPE void*
>>>> #define YYSTYPE void*
>>> New version of patch is attached. I selected variant 3 as a best
>>> solution. Patch also fix some other postgres.h dependencyin another tools
>>> such as pg_controldata, pg_config. The last unfixed tool is pg_resetxlog
>>> which deserves own patch.
>> Humm, but YYLTYPE is defined in gramparse.h (and as a different type)
>> ... Also, I see that if you define YYLTYPE then you don't need
>> YYLTYPE_IS_DECLARED as well. Also I don't see any TYPE_IS_DECLARED
>> here. What I'm thinking is that this patch is not very portable :-(
>
> Thanks you for your comments.
>
> You are right, define YYLTYPE and YYSTYPE is enough to skip union/structure
> definition. I think, data type is not important for this purpose, but use
> int instead of void* seem good idea. It will be synchronized with
> gramparse.h.
>
> What do you mean "not very portable"? What could be problem there?

I'm not sure. My point is that it seems your parse.h requires
TYPE_IS_DECLARED, but mine doesn't. What else could be lurking in there
that requires a specific fix? In order to avoid that, it would be
better if there was a solution to the problem along the lines of #2 you
proposed.

--
Alvaro Herrera Developer, http://www.PostgreSQL.org/
"No renuncies a nada. No te aferres a nada."


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-14 15:29:44
Message-ID: 473B1468.6000704@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Alvaro Herrera wrote:
> Zdenek Kotala wrote:
>> Alvaro Herrera wrote:
>
>>>>> 3) Put following fake into keyword.c before include "parse.h" line. It
>>>>> is easiest way.
>>>>> #define TYPE_IS_DECLARED 1
>>>>> #define YYLTYPE_IS_DECLARED 1
>>>>> #define YYLTYPE void*
>>>>> #define YYSTYPE void*
>>>> New version of patch is attached. I selected variant 3 as a best
>>>> solution. Patch also fix some other postgres.h dependencyin another tools
>>>> such as pg_controldata, pg_config. The last unfixed tool is pg_resetxlog
>>>> which deserves own patch.
>>> Humm, but YYLTYPE is defined in gramparse.h (and as a different type)
>>> ... Also, I see that if you define YYLTYPE then you don't need
>>> YYLTYPE_IS_DECLARED as well. Also I don't see any TYPE_IS_DECLARED
>>> here. What I'm thinking is that this patch is not very portable :-(
>> Thanks you for your comments.
>>
>> You are right, define YYLTYPE and YYSTYPE is enough to skip union/structure
>> definition. I think, data type is not important for this purpose, but use
>> int instead of void* seem good idea. It will be synchronized with
>> gramparse.h.
>>
>> What do you mean "not very portable"? What could be problem there?
>
> I'm not sure. My point is that it seems your parse.h requires
> TYPE_IS_DECLARED, but mine doesn't. What else could be lurking in there
> that requires a specific fix? In order to avoid that, it would be
> better if there was a solution to the problem along the lines of #2 you
> proposed.
>

TYPE_IS_DECLARED was my mistake. It should be YYSTYPE_IS_DECLARED. It
works because YYSTYPE is also defined and #ifdef checks both. Copy and
paste :( error. Sorry for confusion. I'm going to send new version.

Zdenek


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-14 15:54:03
Message-ID: 24322.1195055643@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> Zdenek Kotala wrote:
>> What do you mean "not very portable"? What could be problem there?

> I'm not sure. My point is that it seems your parse.h requires
> TYPE_IS_DECLARED, but mine doesn't. What else could be lurking in there
> that requires a specific fix?

The "portability" axis of concern here is portability across different
versions of bison. ATM I believe we work with everything from 1.875 up,
and I'd be loath to give that up.

I concur with Alvaro that this feels like a kluge rather than a
permanent solution.

regards, tom lane


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-14 16:47:38
Message-ID: 473B26AA.3020908@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
>> Zdenek Kotala wrote:
>>> What do you mean "not very portable"? What could be problem there?
>
>> I'm not sure. My point is that it seems your parse.h requires
>> TYPE_IS_DECLARED, but mine doesn't. What else could be lurking in there
>> that requires a specific fix?
>
> The "portability" axis of concern here is portability across different
> versions of bison. ATM I believe we work with everything from 1.875 up,
> and I'd be loath to give that up.
>
> I concur with Alvaro that this feels like a kluge rather than a
> permanent solution.
>

But, if you look into gramparse.h it also redefine YYLTYPE for own
purpose. It is similar mechanism. However YYSTYPE definition generates
different storage for base_yylval. It could be problem with some
compiler/linker. :(

Unfortunately, I don't see any solution with modification gram.y (I'm
not bison hunter).

Another solution is what ecpg does. It has own modified copy of
keyword.c. But it will require to keep it synchronized. Or if we put
parentheses around all columns we don't need keyword.c. It also fix a
problem with old dump file when we introduce new keyword in the future.

Zdenek


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-14 16:50:55
Message-ID: 25156.1195059055@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> writes:
> TYPE_IS_DECLARED was my mistake. It should be YYSTYPE_IS_DECLARED. It
> works because YYSTYPE is also defined and #ifdef checks both. Copy and
> paste :( error. Sorry for confusion. I'm going to send new version.

[ after further review... ]

It looks to me like the intended use of YYSTYPE_IS_DECLARED is to
denote the case where the user has supplied a typedef, not macro,
definition of YYSTYPE. So defining YYSTYPE as a macro and setting
YYSTYPE_IS_DECLARED as well really seems incorrect.

The real question with all this is that while the Bison manual clearly
says that you can #define YYSTYPE, there is not anything suggesting
that you can or should do that when using %union; they are presented as
two different ways of defining the type. So I find it a bit surprising
that the #if is there at all in parse.h. Nonetheless it is there in
all versions from 1.875 to 2.3, and in the 2.3 manual it says

Unless `YYSTYPE' is already defined as a macro, the output header
declares `YYSTYPE'.

So apparently the Bison guys do intend to carry that behavior forward.

AFAICT, therefore, the proposed patch should only define YYSTYPE and
not anything else (there's no need to be afraid of YYLTYPE at present).

regards, tom lane


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-14 20:17:10
Message-ID: 473B57C6.4070805@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Tom Lane wrote:

<snip>
>
> AFAICT, therefore, the proposed patch should only define YYSTYPE and
> not anything else (there's no need to be afraid of YYLTYPE at present).

Thank your for Your and Alvaro's comments. I attached updated patch
version only with YYSTYPE definition.

Thanks Zdenek

Attachment Content-Type Size
pg_dump_03.patch text/x-patch 53.3 KB

From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-21 01:38:48
Message-ID: 200711210138.lAL1cmE29851@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches


This has been saved for the 8.4 release:

http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Zdenek Kotala wrote:
> Tom Lane wrote:
>
> <snip>
> >
> > AFAICT, therefore, the proposed patch should only define YYSTYPE and
> > not anything else (there's no need to be afraid of YYLTYPE at present).
>
> Thank your for Your and Alvaro's comments. I attached updated patch
> version only with YYSTYPE definition.
>
> Thanks Zdenek

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +


From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-21 01:58:20
Message-ID: 20071121015820.GI11249@alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Bruce Momjian wrote:
>
> This has been saved for the 8.4 release:
>
> http://momjian.postgresql.org/cgi-bin/pgpatches_hold

Huh, I think the point is to be able to build 8.3 at all on certain
Solaris releases. I think it qualifies as a portability fix.

> ---------------------------------------------------------------------------
>
> Zdenek Kotala wrote:
> > Tom Lane wrote:
> >
> > <snip>
> > >
> > > AFAICT, therefore, the proposed patch should only define YYSTYPE and
> > > not anything else (there's no need to be afraid of YYLTYPE at present).
> >
> > Thank your for Your and Alvaro's comments. I attached updated patch
> > version only with YYSTYPE definition.

--
Alvaro Herrera Valdivia, Chile ICBM: S 39º 49' 18.1", W 73º 13' 56.4"
"No es bueno caminar con un hombre muerto"


From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-21 02:04:08
Message-ID: 200711210204.lAL249S15546@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Alvaro Herrera wrote:
> Bruce Momjian wrote:
> >
> > This has been saved for the 8.4 release:
> >
> > http://momjian.postgresql.org/cgi-bin/pgpatches_hold
>
> Huh, I think the point is to be able to build 8.3 at all on certain
> Solaris releases. I think it qualifies as a portability fix.

I thought this was just cleanup. Why does Solaris need it? I don't
remember it has been so long. I see it referenced here:

http://archives.postgresql.org/pgsql-hackers/2007-10/msg01261.php

Is this some Sun bug or an optimization that is causing the problem.

The fact that this patch is not in beta3 has me worried about putting it
in with no beta before RC1.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-21 02:17:13
Message-ID: 21193.1195611433@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> Huh, I think the point is to be able to build 8.3 at all on certain
> Solaris releases. I think it qualifies as a portability fix.

(a) it wasn't "to build at all", it was to allow "inline" to be enabled
on Sun Studio's compiler, which apparently is too dumb to not generate
copies of an unreferenced inline function.

(b) portability fix or not, this ain't going into 8.3. It's *far* too
invasive for this late stage of the cycle.

regards, tom lane


From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-21 03:08:16
Message-ID: 200711210308.lAL38GH15899@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> > Huh, I think the point is to be able to build 8.3 at all on certain
> > Solaris releases. I think it qualifies as a portability fix.
>
> (a) it wasn't "to build at all", it was to allow "inline" to be enabled
> on Sun Studio's compiler, which apparently is too dumb to not generate
> copies of an unreferenced inline function.
>
> (b) portability fix or not, this ain't going into 8.3. It's *far* too
> invasive for this late stage of the cycle.

Yea, that was my analysis too.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +


From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-21 12:47:35
Message-ID: 20071121124735.GE4918@alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Bruce Momjian wrote:
> Tom Lane wrote:
> > Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> > > Huh, I think the point is to be able to build 8.3 at all on certain
> > > Solaris releases. I think it qualifies as a portability fix.
> >
> > (a) it wasn't "to build at all", it was to allow "inline" to be enabled
> > on Sun Studio's compiler, which apparently is too dumb to not generate
> > copies of an unreferenced inline function.
> >
> > (b) portability fix or not, this ain't going into 8.3. It's *far* too
> > invasive for this late stage of the cycle.
>
> Yea, that was my analysis too.

Oh, right, I misremembered that it was only to allow inline.

--
Alvaro Herrera http://www.amazon.com/gp/registry/DXLWNGRJD34J
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-21 13:39:30
Message-ID: 47443512.7090308@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Bruce Momjian wrote:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
>>> Huh, I think the point is to be able to build 8.3 at all on certain
>>> Solaris releases. I think it qualifies as a portability fix.
>> (a) it wasn't "to build at all", it was to allow "inline" to be enabled
>> on Sun Studio's compiler, which apparently is too dumb to not generate
>> copies of an unreferenced inline function.
>>
>> (b) portability fix or not, this ain't going into 8.3. It's *far* too
>> invasive for this late stage of the cycle.
>
> Yea, that was my analysis too.
>

Yes, I agree with Bruce and Tom. It is more invasive then I expected.
And specially in context when pg_resetxlog needs to be also fixed and
this fix seems more complicated then pg_dump fix :(.

Zdenek


From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-21 22:45:56
Message-ID: 200711212245.lALMjuS21402@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Zdenek Kotala wrote:
> Bruce Momjian wrote:
> > Tom Lane wrote:
> >> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> >>> Huh, I think the point is to be able to build 8.3 at all on certain
> >>> Solaris releases. I think it qualifies as a portability fix.
> >> (a) it wasn't "to build at all", it was to allow "inline" to be enabled
> >> on Sun Studio's compiler, which apparently is too dumb to not generate
> >> copies of an unreferenced inline function.
> >>
> >> (b) portability fix or not, this ain't going into 8.3. It's *far* too
> >> invasive for this late stage of the cycle.
> >
> > Yea, that was my analysis too.
> >
>
> Yes, I agree with Bruce and Tom. It is more invasive then I expected.
> And specially in context when pg_resetxlog needs to be also fixed and
> this fix seems more complicated then pg_dump fix :(.

The good news is that we will have it ready for 8.4.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Bruce Momjian <bruce(at)momjian(dot)us>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2007-11-22 15:59:09
Message-ID: 4745A74D.6040504@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Tom Lane napsal(a):

> (a) it wasn't "to build at all", it was to allow "inline" to be enabled
> on Sun Studio's compiler, which apparently is too dumb to not generate
> copies of an unreferenced inline function.

I don't think that you have right. inline is just optimizer hint and Sun Studio
performs inline optimization from level 3 (-xO3). In lower level it generates
it as local function. And on other side GCC (3.4.3) performs inconsistent
optimization on -O0 level which is by my opinion also for discussion see
following case:

---------------------------------
#include <stdio.h>

static inline int test_1(int a)
{
return a*a;
}

static inline int test_2(int p)
{
return p*p;
}

static int test_3(int p)
{
return p*p;
}

int main()
{

int a = 3;
printf("%i\n", test_1(a));
return 0;
}

bash-3.2$ nm ./a.out | grep test
[58] | 134548680| 15|FUNC |LOCL |0 |13 |test_1
[57] | 134548592| 15|FUNC |LOCL |0 |13 |test_3

---------------------------------

Why test_3 is not removed as unused function, but test_2 is? By my opinion it is
inconsistent. GCC removes test_3 on -O2 level. And why -fkeep-inline-functions
does not work? It seems to me that somebody modify gcc to fix similar problems
as postgreSQL has in the source code.

Zdenek


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
Cc: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2008-03-27 04:03:05
Message-ID: 845.1206590585@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> writes:
> Thank your for Your and Alvaro's comments. I attached updated patch
> version only with YYSTYPE definition.

Applied with minor revisions. I fixed pg_conversion.h as well; the only
remaining special inclusions in the catalog header files are of
pg_list.h. For the moment that header doesn't seem to be a problem
for client-side code to include, but someday we may want to do more
cleanup here.

The immediate remaining problem is that pg_resetxlog.c still includes
a bunch of backend-only headers, which as far as I understand still
blocks your build requirements. I poked at this a little bit, but
didn't see any easy solution ...

regards, tom lane


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Fix pg_dump dependency on postgres.h
Date: 2008-03-27 12:35:16
Message-ID: 47EB9484.3050306@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-patches

Tom Lane napsal(a):
> Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> writes:
>> Thank your for Your and Alvaro's comments. I attached updated patch
>> version only with YYSTYPE definition.
>
> Applied with minor revisions. I fixed pg_conversion.h as well; the only
> remaining special inclusions in the catalog header files are of
> pg_list.h. For the moment that header doesn't seem to be a problem
> for client-side code to include, but someday we may want to do more
> cleanup here.

Thanks.

> The immediate remaining problem is that pg_resetxlog.c still includes
> a bunch of backend-only headers, which as far as I understand still
> blocks your build requirements. I poked at this a little bit, but
> didn't see any easy solution ...

Yes, I know about it. When I tried to solved it in November, I found some
unnecessary includes and also there are some not well placed structures and so
on. I also sent one fix which probably lost in black hole of patch queue:

See http://archives.postgresql.org/pgsql-patches/2007-10/msg00197.php

There are big work on pg_resetxlog.c and I will look on it. It probably will
require a cleanup in headers files.

Zdenek