Fix pg_dump dependency on postgres.h

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)

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

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

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

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

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)

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

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."

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

Alvaro Herrera <alvherre@alvh.no-ip.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

Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.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

Zdenek Kotala <Zdenek.Kotala@Sun.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

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

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@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

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

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"

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@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

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

Alvaro Herrera <alvherre@alvh.no-ip.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

Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.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@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

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

Bruce Momjian wrote:

Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.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)