Thread: building pg_dump doesn't work
Hi, I noticed that if you start from a clean tree, it doesn't work to build pg_dump because gram.h has not been generated yet: make -C ../../../src/backend/parser keywords.o make[1]: Entering directory `/home/alvherre/Code/CVS/pgsql/build/00head/src/backend/parser' gcc -O0 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv-g -I/pgsql/source/00head/src/backend/parser -I../../../src/include -I/pgsql/source/00head/src/include -D_GNU_SOURCE-I/usr/include/libxml2 -c -o keywords.o /pgsql/source/00head/src/backend/parser/keywords.c -MMD -MP -MF .deps/keywords.Po /pgsql/source/00head/src/backend/parser/keywords.c:33:25: error: parser/gram.h: No such file or directory /pgsql/source/00head/src/backend/parser/keywords.c:46: error: 'ABORT_P' undeclared here (not in a function) I notice that there's a line in backend/parser that makes keywords.o depend on gram.h, but apparently it doesn't work: # Force these dependencies to be known even without dependency info built: gram.o keywords.o parser.o: $(srcdir)/gram.h The pg_dump Makefile appears to be expecting the file to be in src/include/parser. This works for src/backend/parser because it adds the current srcdir as -I to CPPFLAGS. (Actually I see, and vaguely remember, that this has been broken for a long time.) -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Hi, > > I noticed that if you start from a clean tree, it doesn't work to build > pg_dump because gram.h has not been generated yet: > > make -C ../../../src/backend/parser keywords.o > make[1]: Entering directory `/home/alvherre/Code/CVS/pgsql/build/00head/src/backend/parser' > gcc -O0 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv-g -I/pgsql/source/00head/src/backend/parser -I../../../src/include -I/pgsql/source/00head/src/include -D_GNU_SOURCE-I/usr/include/libxml2 -c -o keywords.o /pgsql/source/00head/src/backend/parser/keywords.c -MMD -MP -MF .deps/keywords.Po > /pgsql/source/00head/src/backend/parser/keywords.c:33:25: error: parser/gram.h: No such file or directory > /pgsql/source/00head/src/backend/parser/keywords.c:46: error: 'ABORT_P' undeclared here (not in a function) This patch fixes it. The fmgroids.h bits are not needed for pg_dump, but they are if you start building on the parser directory. I don't care strongly for that one, so if people don't like it I'll just go ahead with the gram.h bit, unless anyone has a better idea. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Alvaro Herrera wrote:
>> I noticed that if you start from a clean tree, it doesn't work to build
>> pg_dump because gram.h has not been generated yet:
> This patch fixes it.
I think this is probably going in the wrong direction. The reason
gram.h isn't already in the main include tree is that we don't *want*
all and sundry depending on it --- we have very carefully minimized
the number of files that depend on the grammar's symbol codes.
ISTM that pg_dump doesn't actually care about the symbol codes, it
just needs a list of known keywords. Can we refactor things so that
the frontend-side version of the keyword list doesn't include the
grammar symbols at all?
One idea that comes to mind is to replace the entries like
{"abort", ABORT_P, UNRESERVED_KEYWORD},
with macro calls
PG_KEYWORD("abort", ABORT_P, UNRESERVED_KEYWORD),
and then the frontend build of the file could define the macro
to ignore its second argument.
The way we do it now seems to have other disadvantages too: we are
incorporating a backend .o file into pg_dump as-is, which would lead
to large problems if there were differences in say the compiler flags
needed. In fact, I thought Zdenek had been working on decoupling
that sort of thing, so I'm a bit surprised it's still like this at all.
regards, tom lane
Tom Lane wrote: > I think this is probably going in the wrong direction. The reason > gram.h isn't already in the main include tree is that we don't *want* > all and sundry depending on it --- we have very carefully minimized > the number of files that depend on the grammar's symbol codes. > > ISTM that pg_dump doesn't actually care about the symbol codes, it > just needs a list of known keywords. Hmm, I had thought that pg_dump only wanted the header file, not the keywords.o object file. I now see that I was wrong. I agree that your proposed solution is a lot better. I'll see about it. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Hmm, I had thought that pg_dump only wanted the header file, not the > keywords.o object file. I now see that I was wrong. I agree that your > proposed solution is a lot better. I'll see about it. Here it is. The #ifdef parts seem a bit ugly, but I'm not sure how can this be improved, given that ECPG is already using this file. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Perhaps this could be made less ugly by only having the ScanKeywords
> array in the .c file, and #including that into other .c files in
> src/backend/parser, ecpg and pg_dump.
What I'd suggest is something similar to the design of the errcodes.h
header: create a header file containing just the list of PG_KEYWORD
macro calls, and have the different users #include it after defining
that macro appropriately. Having .c files include other .c files is
usually best avoided on least-surprise grounds.
> Not sure what to do about ScanKeywordLookup function.
Yeah, duplicating that function is a bit annoying.
regards, tom lane
Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>
>> Hmm, I had thought that pg_dump only wanted the header file, not the
>> keywords.o object file. I now see that I was wrong. I agree that your
>> proposed solution is a lot better. I'll see about it.
>
> Here it is. The #ifdef parts seem a bit ugly, but I'm not sure how can
> this be improved, given that ECPG is already using this file.
Perhaps this could be made less ugly by only having the ScanKeywords
array in the .c file, and #including that into other .c files in
src/backend/parser, ecpg and pg_dump.
So, keywords.c would look like:
#include "parser/keywords.h"
const ScanKeyword ScanKeywords[] = {/* name, value, category */PG_KEYWORD("abort", ABORT_P,
UNRESERVED_KEYWORD),PG_KEYWORD("absolute",ABSOLUTE_P, UNRESERVED_KEYWORD),PG_KEYWORD("access", ACCESS,
UNRESERVED_KEYWORD),
...
And there would be a new file in src/bin/pg_dump, say dumpkeywords.c,
that looks like this:
#include "c.h"
#define PG_KEYWORD(a,b,c) {a,b,c}
#include "src/backend/parser/keywords.c"
Not sure what to do about ScanKeywordLookup function.
> /*
> + * We don't want to include the gram.h file on frontend builds, except ECPG, so
> + * leave out the second struct member in that case.
> + */
> + #if !defined FRONTEND || defined ECPG_COMPILE
> + #define PG_KEYWORD(a,b,c) {a,b,c}
> + #else
> + #define PG_KEYWORD(a,b,c) {a,c}
> + #endif
Doesn't that put 'c' into the wrong field in ScanKeyword struct? It only
compiles because both 'value' and 'category' are int16.
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Tom Lane wrote: > Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: > > Perhaps this could be made less ugly by only having the ScanKeywords > > array in the .c file, and #including that into other .c files in > > src/backend/parser, ecpg and pg_dump. > > What I'd suggest is something similar to the design of the errcodes.h > header: create a header file containing just the list of PG_KEYWORD > macro calls, and have the different users #include it after defining > that macro appropriately. Having .c files include other .c files is > usually best avoided on least-surprise grounds. Seems doable. > > Not sure what to do about ScanKeywordLookup function. > > Yeah, duplicating that function is a bit annoying. Another new file, backend/parser/kwlookup.c perhaps? -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Heikki Linnakangas wrote:
> Alvaro Herrera wrote:
>> /*
>> + * We don't want to include the gram.h file on frontend builds, except ECPG, so
>> + * leave out the second struct member in that case.
>> + */
>> + #if !defined FRONTEND || defined ECPG_COMPILE
>> + #define PG_KEYWORD(a,b,c) {a,b,c}
>> + #else
>> + #define PG_KEYWORD(a,b,c) {a,c}
>> + #endif
>
> Doesn't that put 'c' into the wrong field in ScanKeyword struct? It only
> compiles because both 'value' and 'category' are int16.
No, because I had the header with the second field omitted too.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > Tom Lane wrote: > > What I'd suggest is something similar to the design of the errcodes.h > > header: create a header file containing just the list of PG_KEYWORD > > macro calls, and have the different users #include it after defining > > that macro appropriately. Having .c files include other .c files is > > usually best avoided on least-surprise grounds. > > Seems doable. Attached. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Attachment
Alvaro Herrera wrote: > Alvaro Herrera wrote: > > Tom Lane wrote: > > > > What I'd suggest is something similar to the design of the errcodes.h > > > header: create a header file containing just the list of PG_KEYWORD > > > macro calls, and have the different users #include it after defining > > > that macro appropriately. Having .c files include other .c files is > > > usually best avoided on least-surprise grounds. > > > > Seems doable. > > Attached. Minor fixes over the previous patch. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
Alvaro Herrera <alvherre@commandprompt.com> writes:
>> Seems doable.
> Attached.
The TWO_MEMBER_SCANKEYWORD business seems a bit dangerous --- if the
header file is read without having #defined that correctly, bad things
will happen. It might be better to leave that out, always define the
struct the same, and just have pg_dump define PG_KEYWORD to fill the
value field with zero. Given alignment considerations, you're not
saving any space by omitting the field anyhow.
regards, tom lane
Dne 3.03.09 22:55, Tom Lane napsal(a):
>
> One idea that comes to mind is to replace the entries like
>
> {"abort", ABORT_P, UNRESERVED_KEYWORD},
>
> with macro calls
>
> PG_KEYWORD("abort", ABORT_P, UNRESERVED_KEYWORD),
>
> and then the frontend build of the file could define the macro
> to ignore its second argument.
It sounds good.
> The way we do it now seems to have other disadvantages too: we are
> incorporating a backend .o file into pg_dump as-is, which would lead
> to large problems if there were differences in say the compiler flags
> needed. In fact, I thought Zdenek had been working on decoupling
> that sort of thing, so I'm a bit surprised it's still like this at all.
Yeah, it is still on my TODO list. There is still problem with
pg_resetxlog which needs lot of internals headers. :(
Zdenek
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > >> Seems doable. > > > Attached. > > The TWO_MEMBER_SCANKEYWORD business seems a bit dangerous --- if the > header file is read without having #defined that correctly, bad things > will happen. It might be better to leave that out, always define the > struct the same, and just have pg_dump define PG_KEYWORD to fill the > value field with zero. Given alignment considerations, you're not > saving any space by omitting the field anyhow. Fixed. I also added #include type.h to the ecpg keywords.c file, which means we don't need to redefine YYSTYPE at all on any of the three keywords.c file. Looks cleaner overall. Hopefully this is the last version of this patch. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
Zdenek Kotala wrote:
> Dne 3.03.09 22:55, Tom Lane napsal(a):
>
>>
>> One idea that comes to mind is to replace the entries like
>>
>> {"abort", ABORT_P, UNRESERVED_KEYWORD},
>>
>> with macro calls
>>
>> PG_KEYWORD("abort", ABORT_P, UNRESERVED_KEYWORD),
>>
>> and then the frontend build of the file could define the macro
>> to ignore its second argument.
>
> It sounds good.
Please have a look at the patch I just posted -- should be up in a few
minutes in
http://archives.postgresql.org/message-id/20090305131208.GA4087%40alvh.no-ip.org
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Mar 5, 2009 at 1:12 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre@commandprompt.com> writes:
>> >> Seems doable.
>>
>> > Attached.
>>
>> The TWO_MEMBER_SCANKEYWORD business seems a bit dangerous --- if the
>> header file is read without having #defined that correctly, bad things
>> will happen. It might be better to leave that out, always define the
>> struct the same, and just have pg_dump define PG_KEYWORD to fill the
>> value field with zero. Given alignment considerations, you're not
>> saving any space by omitting the field anyhow.
>
> Fixed.
>
> I also added #include type.h to the ecpg keywords.c file, which means we
> don't need to redefine YYSTYPE at all on any of the three keywords.c
> file. Looks cleaner overall.
>
> Hopefully this is the last version of this patch.
FWIW gcc does this kind of trick all over the place. They have lists
of various types of objects, not unlike our nodes and define them in
.h files which contain _just_ the equivalent of PG_KEYWORD. Then they
include those files in various places with the macro defined to do
different things. So for example they define an enum, an array for
external consumption, an array for internal consumption, and in many
places even switch statements with trivial bits of code from the macro
too.
If we're going to go this route I think it does make sense to move the
"const ScanKeyword ScanKeywords[] = {" preamble and to live with the
PG_KEYWORD definition. Even if we're not planning to have any other
kinds of macro definitions aside from ScanKeywords arrays today it
would be nice to have the flexibility and in any case I don't really
like the action-at-a-distance of having a macro definition in one
place which depends on the definition in another place.
--
greg
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Hopefully this is the last version of this patch.
A few more comments would help --- in particular the header comment
for kwlist.h should explain that the calling file is supposed to define
PG_KEYWORD appropriately for its needs. I also wonder whether Greg
isn't right that it would be better if the header contained *only*
the PG_KEYWORD macros, rather than presupposing that the caller wants
to build a constant table named ScanKeywords with them.
In general though it's certainly cleaner than the old way.
regards, tom lane