Обсуждение: Some cleanups/enhancements
Hi,
I'm running PostgreSQL 6.3 on Linux 2.1.85 with gcc 2.8.0 and libc5. So
far no problems, however I noted some cleanups / enhancements which I
would like to do. Before I send you a bunch of patches I thought I'll
tell you what I'm planning to do. Please comment on my list and indicate
whether I should go ahead.
- Fix all Makefiles so 'make dep' and 'make depend' work
- Fix all Makefiles so 'make clean' throws away the depend file
- Some other Makefile cleanups
- gcc 2.8.0 issues some additional warnings which are very easy to fix:
- register i --> register int i
- Ambiguous else --> add braces:
if (cond1)
if (cond2)
...
else
...
- etc.
- Add a template for linux-elf-586 with (optimized) code for a Pentium
(gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro).
Why not use template names similar to the output of config.guess (maybe
with some symbolic links)?
- Fix for tools/find_static: add two (in my opinion funny) indices to
improve speed tremendously:
Nested Loop (cost=6.05 size=1 width=42)
-> Index Scan on debug (cost=2.05 size=2 width=12)
-> Index Scan on debug2 (cost=2.00 size=2 width=30)
rather than
Hash Join (cost=993.76 size=1 width=42)
-> Seq Scan on debug (cost=495.81 size=2 width=12)
-> Hash (cost=0.00 size=0 width=0)
-> Seq Scan on debug2 (cost=495.81 size=2 width=30)
- Cleanup of some code that uses heap_formtuple to allow a NULL value
for the nulls parameter, indicating there are no null columns (comes in
handy for catalog/pg_*.c), among others.
- Why is there some code to change the case of the procedural language
to lower case except for 'C' (in fact it's there twice)? Why not use
strcasecmp and remove these pices of code?
- Add pcalloc(n,s) to allocate n*s bytes and set them to zero and use
them where appropriate (I won't touch all code :-))
- Shouldn't same_tuple in executor/nodeGroup.c use the equality operator
for the type concerned to test for equality of the attributes rather
than print them to a buffer and use strcmp? Shouldn't the pointers for
these functions be looked up once in ExecInitGroup and stored somewhere?
Shouldn't this function go to heaptuple.c and be renamed heap_sametuple?
- Lump heaptuple.c and heapvalid.c together
- I also saw quite some #ifdef NOT_USED and other similar stuff. I don't
want to touch these now, but shouldn't some of these be removed soon?
- Add a pg_version function that returns a string like 'PostgreSQL 6.3'
to indicate the version of PostgreSQL a user is using (with 'select
pg_version()'). Might be handy to include in the bug reports.
These are all the things that I found after browsing through the code
one night (primarily in backend/access, backend/catalog and
backend/executor).
Let me know what you think of the above list and I will proceed. If you
have any hints on how I might proceed (especially with same_tuple)
please don't hesitate. Expect the changes to be available somewhere
after the weekend.
Cheers,
Jeroen van Vianen
These all sound good to me. The NOT_USED stuff was left because we thought some day we may need them. If you see stuff that clearly doesn't do anything valuable, get rid of it. > > Hi, > > I'm running PostgreSQL 6.3 on Linux 2.1.85 with gcc 2.8.0 and libc5. So > far no problems, however I noted some cleanups / enhancements which I > would like to do. Before I send you a bunch of patches I thought I'll > tell you what I'm planning to do. Please comment on my list and indicate > whether I should go ahead. > > - Fix all Makefiles so 'make dep' and 'make depend' work > - Fix all Makefiles so 'make clean' throws away the depend file > - Some other Makefile cleanups > - gcc 2.8.0 issues some additional warnings which are very easy to fix: > - register i --> register int i > - Ambiguous else --> add braces: > if (cond1) > if (cond2) > ... > else > ... > - etc. > - Add a template for linux-elf-586 with (optimized) code for a Pentium > (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro). > Why not use template names similar to the output of config.guess (maybe > with some symbolic links)? > - Fix for tools/find_static: add two (in my opinion funny) indices to > improve speed tremendously: > > Nested Loop (cost=6.05 size=1 width=42) > -> Index Scan on debug (cost=2.05 size=2 width=12) > -> Index Scan on debug2 (cost=2.00 size=2 width=30) > > rather than > > Hash Join (cost=993.76 size=1 width=42) > -> Seq Scan on debug (cost=495.81 size=2 width=12) > -> Hash (cost=0.00 size=0 width=0) > -> Seq Scan on debug2 (cost=495.81 size=2 width=30) > > - Cleanup of some code that uses heap_formtuple to allow a NULL value > for the nulls parameter, indicating there are no null columns (comes in > handy for catalog/pg_*.c), among others. > - Why is there some code to change the case of the procedural language > to lower case except for 'C' (in fact it's there twice)? Why not use > strcasecmp and remove these pices of code? > - Add pcalloc(n,s) to allocate n*s bytes and set them to zero and use > them where appropriate (I won't touch all code :-)) > - Shouldn't same_tuple in executor/nodeGroup.c use the equality operator > for the type concerned to test for equality of the attributes rather > than print them to a buffer and use strcmp? Shouldn't the pointers for > these functions be looked up once in ExecInitGroup and stored somewhere? > Shouldn't this function go to heaptuple.c and be renamed heap_sametuple? > - Lump heaptuple.c and heapvalid.c together > - I also saw quite some #ifdef NOT_USED and other similar stuff. I don't > want to touch these now, but shouldn't some of these be removed soon? > - Add a pg_version function that returns a string like 'PostgreSQL 6.3' > to indicate the version of PostgreSQL a user is using (with 'select > pg_version()'). Might be handy to include in the bug reports. > > These are all the things that I found after browsing through the code > one night (primarily in backend/access, backend/catalog and > backend/executor). > > Let me know what you think of the above list and I will proceed. If you > have any hints on how I might proceed (especially with same_tuple) > please don't hesitate. Expect the changes to be available somewhere > after the weekend. > > Cheers, > > Jeroen van Vianen > > -- Bruce Momjian maillist@candle.pha.pa.us
On Wed, 11 Feb 1998, Jeroen van Vianen wrote:
> Hi,
>
> I'm running PostgreSQL 6.3 on Linux 2.1.85 with gcc 2.8.0 and libc5. So
> far no problems, however I noted some cleanups / enhancements which I
> would like to do. Before I send you a bunch of patches I thought I'll
> tell you what I'm planning to do. Please comment on my list and indicate
> whether I should go ahead.
>
> - Fix all Makefiles so 'make dep' and 'make depend' work
Can someone explain what, exactly, 'make depend' accomplishes? We
don't use it right now, so I'm wondering why (if?) we need it now?
> - Some other Makefile cleanups
> - gcc 2.8.0 issues some additional warnings which are very easy to fix:
> - register i --> register int i
> - Ambiguous else --> add braces:
> if (cond1)
> if (cond2)
> ...
> else
> ...
> - etc.
Sounds great...
> - Add a template for linux-elf-586 with (optimized) code for a Pentium
> (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro).
> Why not use template names similar to the output of config.guess (maybe
> with some symbolic links)?
Erk...I think 'templates' are getting a little out of hand here,
no?
> - Why is there some code to change the case of the procedural language
> to lower case except for 'C' (in fact it's there twice)? Why not use
> strcasecmp and remove these pices of code?
I question this in *alot* of places...like why pg_dlopen is
defined as just 'dlopen()' in some ports *shrug* Why not just call it
directly? *raised eyebrow*
> These are all the things that I found after browsing through the code
> one night (primarily in backend/access, backend/catalog and
> backend/executor).
>
> Let me know what you think of the above list and I will proceed. If you
> have any hints on how I might proceed (especially with same_tuple)
> please don't hesitate. Expect the changes to be available somewhere
> after the weekend.
The only thing I ask is that you submit these in such a way that
they can be easily reviewed before committing them...we are in a beta mode
right now, and altho some of this makes for nice cleanups, some of this
should most likely be gingerly added...
If at all possible, a seperate patch for each point above would be
really good, with an explanation of each. If it weren't for beta-status,
I wouldn't care, since we could debug after, but with only 2/2.5 weeks
till release, we are getting tight for debugging...:(
I recommend running the regression test before/after the changes, to make sure something didn't get broken. > > On Wed, 11 Feb 1998, Jeroen van Vianen wrote: > > > Hi, > > > > I'm running PostgreSQL 6.3 on Linux 2.1.85 with gcc 2.8.0 and libc5. So > > far no problems, however I noted some cleanups / enhancements which I > > would like to do. Before I send you a bunch of patches I thought I'll > > tell you what I'm planning to do. Please comment on my list and indicate > > whether I should go ahead. > > > > - Fix all Makefiles so 'make dep' and 'make depend' work > > Can someone explain what, exactly, 'make depend' accomplishes? We > don't use it right now, so I'm wondering why (if?) we need it now? > > > - Some other Makefile cleanups > > - gcc 2.8.0 issues some additional warnings which are very easy to fix: > > - register i --> register int i > > - Ambiguous else --> add braces: > > if (cond1) > > if (cond2) > > ... > > else > > ... > > - etc. > > Sounds great... > > > - Add a template for linux-elf-586 with (optimized) code for a Pentium > > (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro). > > Why not use template names similar to the output of config.guess (maybe > > with some symbolic links)? > > Erk...I think 'templates' are getting a little out of hand here, > no? > > > - Why is there some code to change the case of the procedural language > > to lower case except for 'C' (in fact it's there twice)? Why not use > > strcasecmp and remove these pices of code? > > I question this in *alot* of places...like why pg_dlopen is > defined as just 'dlopen()' in some ports *shrug* Why not just call it > directly? *raised eyebrow* > > > These are all the things that I found after browsing through the code > > one night (primarily in backend/access, backend/catalog and > > backend/executor). > > > > Let me know what you think of the above list and I will proceed. If you > > have any hints on how I might proceed (especially with same_tuple) > > please don't hesitate. Expect the changes to be available somewhere > > after the weekend. > > The only thing I ask is that you submit these in such a way that > they can be easily reviewed before committing them...we are in a beta mode > right now, and altho some of this makes for nice cleanups, some of this > should most likely be gingerly added... > > If at all possible, a seperate patch for each point above would be > really good, with an explanation of each. If it weren't for beta-status, > I wouldn't care, since we could debug after, but with only 2/2.5 weeks > till release, we are getting tight for debugging...:( > > > > > -- Bruce Momjian maillist@candle.pha.pa.us
> I'm running PostgreSQL 6.3 on Linux 2.1.85 with gcc 2.8.0 and libc5. So
> far no problems, however I noted some cleanups / enhancements which I
> would like to do. Before I send you a bunch of patches I thought I'll
> tell you what I'm planning to do. Please comment on my list and indicate
> whether I should go ahead.
>
> - Fix all Makefiles so 'make dep' and 'make depend' work
> - Fix all Makefiles so 'make clean' throws away the depend file
> - Some other Makefile cleanups
These all sound good. If there is a possibility of large breakage, wait
until after v6.3.
> - gcc 2.8.0 issues some additional warnings which are very easy to fix:
> - register i --> register int i
I'm sure there will be differing opinions :-/, but imho all register
declarations should be removed. Modern compilers do a good job of
optimization, and register declarations can get in the way by forcing the
compiler to burn a register to accomodate the declared item.
> - Ambiguous else --> add braces:
> if (cond1)
> if (cond2)
> ...
> else
> ...
Sure.
> - Add a template for linux-elf-586 with (optimized) code for a Pentium
> (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro).
> Why not use template names similar to the output of config.guess (maybe
> with some symbolic links)?
Does gcc 2.7.x support the -mpentium and -mpentiumpro switches? If not,
then the template should be more explicit in name (e.g.
"linux-elf-586-gcc2.8") or we should update the FAQ or include comments in
linux-elf with some suggestions. It was only in the last release or two
that the -m486 was added, and I worried about causing trouble for _all_ of
those 386 users :)
> - Shouldn't same_tuple in executor/nodeGroup.c use the equality operator
> for the type concerned to test for equality of the attributes rather
> than print them to a buffer and use strcmp? Shouldn't the pointers for
> these functions be looked up once in ExecInitGroup and stored somewhere?
> Shouldn't this function go to heaptuple.c and be renamed heap_sametuple?
Yes, I would think so. The only downside to this is that, since two items
which fail the equality test may look identical when formatted (e.g.
floating point numbers with the lsb differing) the results may look a bit
funny and be difficult to track down. Still, I think this is the right way
to go...
> - Lump heaptuple.c and heapvalid.c together
> - I also saw quite some #ifdef NOT_USED and other similar stuff. I don't
> want to touch these now, but shouldn't some of these be removed soon?
Only when the module is completely understood. So, don't remove blindly,
but if it is clear that it is obsolete code which is not providing hints on
what should be done in the future, then it is OK to remove.
> - Add a pg_version function that returns a string like 'PostgreSQL 6.3'
> to indicate the version of PostgreSQL a user is using (with 'select
> pg_version()'). Might be handy to include in the bug reports.
Good idea.
Some or all of these changes might not be appropriate for v6.3, since we
are in beta testing and since they do not affect the current functionality.
For those cases, how about submitting patches based on the final v6.3
release?
- Tom
> > > I'm running PostgreSQL 6.3 on Linux 2.1.85 with gcc 2.8.0 and libc5. So > > far no problems, however I noted some cleanups / enhancements which I > > would like to do. Before I send you a bunch of patches I thought I'll > > tell you what I'm planning to do. Please comment on my list and indicate > > whether I should go ahead. > > > > - Fix all Makefiles so 'make dep' and 'make depend' work > > - Fix all Makefiles so 'make clean' throws away the depend file > > - Some other Makefile cleanups > > These all sound good. If there is a possibility of large breakage, wait > until after v6.3. > > > - gcc 2.8.0 issues some additional warnings which are very easy to fix: > > - register i --> register int i > > I'm sure there will be differing opinions :-/, but imho all register > declarations should be removed. Modern compilers do a good job of > optimization, and register declarations can get in the way by forcing the > compiler to burn a register to accomodate the declared item. Totally agree. Get rid of all register's competely. I don't think this will affect Vadim, as most of them are in utility directories. I agree with your other points too. > > > - Ambiguous else --> add braces: > > if (cond1) > > if (cond2) > > ... > > else > > ... > > Sure. > > > - Add a template for linux-elf-586 with (optimized) code for a Pentium > > (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro). > > Why not use template names similar to the output of config.guess (maybe > > with some symbolic links)? > > Does gcc 2.7.x support the -mpentium and -mpentiumpro switches? If not, > then the template should be more explicit in name (e.g. > "linux-elf-586-gcc2.8") or we should update the FAQ or include comments in > linux-elf with some suggestions. It was only in the last release or two > that the -m486 was added, and I worried about causing trouble for _all_ of > those 386 users :) > > > - Shouldn't same_tuple in executor/nodeGroup.c use the equality operator > > for the type concerned to test for equality of the attributes rather > > than print them to a buffer and use strcmp? Shouldn't the pointers for > > these functions be looked up once in ExecInitGroup and stored somewhere? > > Shouldn't this function go to heaptuple.c and be renamed heap_sametuple? > > Yes, I would think so. The only downside to this is that, since two items > which fail the equality test may look identical when formatted (e.g. > floating point numbers with the lsb differing) the results may look a bit > funny and be difficult to track down. Still, I think this is the right way > to go... > > > - Lump heaptuple.c and heapvalid.c together > > - I also saw quite some #ifdef NOT_USED and other similar stuff. I don't > > want to touch these now, but shouldn't some of these be removed soon? > > Only when the module is completely understood. So, don't remove blindly, > but if it is clear that it is obsolete code which is not providing hints on > what should be done in the future, then it is OK to remove. > > > - Add a pg_version function that returns a string like 'PostgreSQL 6.3' > > to indicate the version of PostgreSQL a user is using (with 'select > > pg_version()'). Might be handy to include in the bug reports. > > Good idea. > > Some or all of these changes might not be appropriate for v6.3, since we > are in beta testing and since they do not affect the current functionality. > For those cases, how about submitting patches based on the final v6.3 > release? > > - Tom > > > -- Bruce Momjian maillist@candle.pha.pa.us
-----BEGIN PGP SIGNED MESSAGE----- Then <lockhart@alumni.caltech.edu> spoke up and said: > > - Add a template for linux-elf-586 with (optimized) code for a Pentium > > (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro). > > Why not use template names similar to the output of config.guess (maybe > > with some symbolic links)? IMHO, none of the gcc-specific optimization switches belong in any templates. If you're using gcc on a pentium, and your version supportes -mpentium, then it should be in the SPECS file in your gcc installation. Admittedly, this presumes a certain clue level on the part of the gcc installer/maintainer, but it reduces the clutter level with templates somewhat. - -- ===================================================================== | "If you're all through trying to burn the field down, will you | | kindly get up and tell me why you're sitting in a fruit field, | | stark naked, frying peaches?" | ===================================================================== | Finger geek@andrew.cmu.edu for my public key. | ===================================================================== -----BEGIN PGP SIGNATURE----- Version: 2.6.2 iQBVAwUBNOHZkIdzVnzma+gdAQEg7QH+MOviZ+pcq5wdpE1d7tK3yB2Ai/03qGti 7cBxzMQLq82g1/5wT+lXm9Rh3plbyvTBCUpU48kpXTYAYjeAvVIzzQ== =C0DN -----END PGP SIGNATURE-----
Thomas G. Lockhart wrote: > > > I'm running PostgreSQL 6.3 on Linux 2.1.85 with gcc 2.8.0 and libc5. So > > far no problems, however I noted some cleanups / enhancements which I > > would like to do. Before I send you a bunch of patches I thought I'll > > tell you what I'm planning to do. Please comment on my list and indicate > > whether I should go ahead. > > > > - Fix all Makefiles so 'make dep' and 'make depend' work > > - Fix all Makefiles so 'make clean' throws away the depend file > > - Some other Makefile cleanups > > These all sound good. If there is a possibility of large breakage, wait > until after v6.3. > Some or all of these changes might not be appropriate for v6.3, since we > are in beta testing and since they do not affect the current functionality. > For those cases, how about submitting patches based on the final v6.3 > release? After the messages I've read so far, I'll wait until after the final release of 6.3 and try to do the patches one at a time, so there'll be plenty of time :-) to review them. > [snip] > > - Add a template for linux-elf-586 with (optimized) code for a Pentium > > (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro). > > Why not use template names similar to the output of config.guess (maybe > > with some symbolic links)? > > Does gcc 2.7.x support the -mpentium and -mpentiumpro switches? If not, > then the template should be more explicit in name (e.g. > "linux-elf-586-gcc2.8") or we should update the FAQ or include comments in > linux-elf with some suggestions. It was only in the last release or two > that the -m486 was added, and I worried about causing trouble for _all_ of > those 386 users :) No, it doesn't. linux-elf-586-gcc2.8 sounds OK to me. > [snip] The Hermit Hacker wrote: > > - Fix all Makefiles so 'make dep' and 'make depend' work > > Can someone explain what, exactly, 'make depend' accomplishes? We > don't use it right now, so I'm wondering why (if?) we need it now? It makes sure that your C files get compiled if you change a header file. Your C compiler should be able to find out which files are included and create lines which can be included in the Makefile (man gcc :-) ) > > - Some other Makefile cleanups > > - gcc 2.8.0 issues some additional warnings which are very easy to fix: > > - register i --> register int i > > - Ambiguous else --> add braces: > > if (cond1) > > if (cond2) > > ... > > else > > ... > > - etc. > > Sounds great... If I find something like this, I'll remove the register as well. > > - Add a template for linux-elf-586 with (optimized) code for a Pentium > > (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro). > > Why not use template names similar to the output of config.guess (maybe > > with some symbolic links)? > > Erk...I think 'templates' are getting a little out of hand here, > no? > > > - Why is there some code to change the case of the procedural language > > to lower case except for 'C' (in fact it's there twice)? Why not use > > strcasecmp and remove these pices of code? > > I question this in *alot* of places...like why pg_dlopen is > defined as just 'dlopen()' in some ports *shrug* Why not just call it > directly? *raised eyebrow* > [snip] Bruce Momjian wrote: > > I recommend running the regression test before/after the changes, to > make sure something didn't get broken. Sure. > [snip] See you in a 2-2.5 weeks :-) Jeroen van Vianen
> > Thomas G. Lockhart wrote: > > > > > I'm running PostgreSQL 6.3 on Linux 2.1.85 with gcc 2.8.0 and libc5. So > > > far no problems, however I noted some cleanups / enhancements which I > > > would like to do. Before I send you a bunch of patches I thought I'll > > > tell you what I'm planning to do. Please comment on my list and indicate > > > whether I should go ahead. > > > > > > - Fix all Makefiles so 'make dep' and 'make depend' work > > > - Fix all Makefiles so 'make clean' throws away the depend file > > > - Some other Makefile cleanups > > > > These all sound good. If there is a possibility of large breakage, wait > > until after v6.3. > > > Some or all of these changes might not be appropriate for v6.3, since we > > are in beta testing and since they do not affect the current functionality. > > For those cases, how about submitting patches based on the final v6.3 > > release? > > After the messages I've read so far, I'll wait until after the final > release of 6.3 and try to do the patches one at a time, so there'll be > plenty of time :-) to review them. > > > [snip] > > > > - Add a template for linux-elf-586 with (optimized) code for a Pentium > > > (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro). > > > Why not use template names similar to the output of config.guess (maybe > > > with some symbolic links)? > > > > Does gcc 2.7.x support the -mpentium and -mpentiumpro switches? If not, > > then the template should be more explicit in name (e.g. > > "linux-elf-586-gcc2.8") or we should update the FAQ or include comments in > > linux-elf with some suggestions. It was only in the last release or two > > that the -m486 was added, and I worried about causing trouble for _all_ of > > those 386 users :) > > No, it doesn't. linux-elf-586-gcc2.8 sounds OK to me. > > > - Some other Makefile cleanups > > > - gcc 2.8.0 issues some additional warnings which are very easy to fix: > > > - register i --> register int i > > > - Ambiguous else --> add braces: > > > if (cond1) > > > if (cond2) > > > ... > > > else > > > ... > > > - etc. > > > > Sounds great... > > If I find something like this, I'll remove the register as well. Register is gone. Just removed them. -- Bruce Momjian maillist@candle.pha.pa.us
> > > - Add a template for linux-elf-586 with (optimized) code for a Pentium
> > > (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro).
> > > Why not use template names similar to the output of config.guess (maybe
> > > with some symbolic links)?
>
> IMHO, none of the gcc-specific optimization switches belong in any
> templates. If you're using gcc on a pentium, and your version
> supportes -mpentium, then it should be in the SPECS file in your gcc
> installation. Admittedly, this presumes a certain clue level on the
> part of the gcc installer/maintainer, but it reduces the clutter level
> with templates somewhat.
Great! Want to write it up for the docs? Need a cookbook and an explanation;
I'll add it in...
If it requires a new install of gcc, then the other option might be to include
it in Makefile.custom as
CFLAGS+= -mpentium
- Tom
jeroenv@design.nl, can you send in patches for these? > > > I'm running PostgreSQL 6.3 on Linux 2.1.85 with gcc 2.8.0 and libc5. So > > far no problems, however I noted some cleanups / enhancements which I > > would like to do. Before I send you a bunch of patches I thought I'll > > tell you what I'm planning to do. Please comment on my list and indicate > > whether I should go ahead. > > > > - Fix all Makefiles so 'make dep' and 'make depend' work > > - Fix all Makefiles so 'make clean' throws away the depend file > > - Some other Makefile cleanups > > These all sound good. If there is a possibility of large breakage, wait > until after v6.3. > > > - gcc 2.8.0 issues some additional warnings which are very easy to fix: > > - register i --> register int i > > I'm sure there will be differing opinions :-/, but imho all register > declarations should be removed. Modern compilers do a good job of > optimization, and register declarations can get in the way by forcing the > compiler to burn a register to accomodate the declared item. > > > - Ambiguous else --> add braces: > > if (cond1) > > if (cond2) > > ... > > else > > ... > > Sure. > > > - Add a template for linux-elf-586 with (optimized) code for a Pentium > > (gcc 2.8.0 not only supports -m486 but also -mpentium and -mpentiumpro). > > Why not use template names similar to the output of config.guess (maybe > > with some symbolic links)? > > Does gcc 2.7.x support the -mpentium and -mpentiumpro switches? If not, > then the template should be more explicit in name (e.g. > "linux-elf-586-gcc2.8") or we should update the FAQ or include comments in > linux-elf with some suggestions. It was only in the last release or two > that the -m486 was added, and I worried about causing trouble for _all_ of > those 386 users :) > > > - Shouldn't same_tuple in executor/nodeGroup.c use the equality operator > > for the type concerned to test for equality of the attributes rather > > than print them to a buffer and use strcmp? Shouldn't the pointers for > > these functions be looked up once in ExecInitGroup and stored somewhere? > > Shouldn't this function go to heaptuple.c and be renamed heap_sametuple? > > Yes, I would think so. The only downside to this is that, since two items > which fail the equality test may look identical when formatted (e.g. > floating point numbers with the lsb differing) the results may look a bit > funny and be difficult to track down. Still, I think this is the right way > to go... > > > - Lump heaptuple.c and heapvalid.c together > > - I also saw quite some #ifdef NOT_USED and other similar stuff. I don't > > want to touch these now, but shouldn't some of these be removed soon? > > Only when the module is completely understood. So, don't remove blindly, > but if it is clear that it is obsolete code which is not providing hints on > what should be done in the future, then it is OK to remove. > > > - Add a pg_version function that returns a string like 'PostgreSQL 6.3' > > to indicate the version of PostgreSQL a user is using (with 'select > > pg_version()'). Might be handy to include in the bug reports. > > Good idea. > > Some or all of these changes might not be appropriate for v6.3, since we > are in beta testing and since they do not affect the current functionality. > For those cases, how about submitting patches based on the final v6.3 > release? > > - Tom > > > -- Bruce Momjian | 830 Blythe Avenue maillist@candle.pha.pa.us | Drexel Hill, Pennsylvania 19026 + If your life is a hard drive, | (610) 353-9879(w) + Christ can be your backup. | (610) 853-3000(h)
Bruce Momjian wrote:
>
> jeroenv@design.nl, can you send in patches for these?
>
> >
> > > I'm running PostgreSQL 6.3 on Linux 2.1.85 with gcc 2.8.0 and libc5. So
> > > far no problems, however I noted some cleanups / enhancements which I
> > > would like to do. Before I send you a bunch of patches I thought I'll
> > > tell you what I'm planning to do. Please comment on my list and indicate
> > > whether I should go ahead.
> > >
> > > - Fix all Makefiles so 'make dep' and 'make depend' work
> > > - Fix all Makefiles so 'make clean' throws away the depend file
> > > - Some other Makefile cleanups
> >
> > These all sound good. If there is a possibility of large breakage, wait
> > until after v6.3.
> >
> > > - gcc 2.8.0 issues some additional warnings which are very easy to fix:
> > > - register i --> register int i
There's a patch attached to fix gcc 2.8.x warnings, except for the
yyerror ones from bison. It also includes a few 'enhancements' to the C
programming style (which are, of course, personal).
The other patch removes the compilation of backend/lib/qsort.c, as
qsort() is a standard function in stdlib.h and can be used any where
else (and it is). It was only used in
backend/optimizer/geqo/geqo_pool.c, backend/optimizer/path/predmig.c,
and backend/storage/page/bufpage.c
> > Some or all of these changes might not be appropriate for v6.3, since we
> > are in beta testing and since they do not affect the current functionality.
> > For those cases, how about submitting patches based on the final v6.3
> > release?
There's more to come. Please review these patches. I ran the regression
tests and they only failed where this was expected (random, geo, etc).
Cheers,
Jeroen
*** backend/commands/copy.c.orig Mon Mar 16 21:03:12 1998
--- backend/commands/copy.c Mon Mar 16 21:04:04 1998
***************
*** 1160,1165 ****
--- 1160,1166 ----
(c == '\\' && !is_array))
fputc('\\', fp);
else if (c == '\\' && is_array)
+ {
if (*(string + 1) == '\\')
{
/* translate \\ to \\\\ */
***************
*** 1174,1179 ****
--- 1175,1181 ----
fputc('\\', fp);
fputc('\\', fp);
}
+ }
fputc(*string, fp);
}
}
*** backend/commands/sequence.c.orig Mon Mar 16 21:04:40 1998
--- backend/commands/sequence.c Mon Mar 16 21:05:28 1998
***************
*** 499,516 ****
--- 499,520 ----
elog(ERROR, "DefineSequence: can't INCREMENT by 0");
if (max_value == (DefElem *) NULL) /* MAXVALUE */
+ {
if (new->increment_by > 0)
new->max_value = SEQ_MAXVALUE; /* ascending seq */
else
new->max_value = -1;/* descending seq */
+ }
else
new->max_value = get_param(max_value);
if (min_value == (DefElem *) NULL) /* MINVALUE */
+ {
if (new->increment_by > 0)
new->min_value = 1; /* ascending seq */
else
new->min_value = SEQ_MINVALUE; /* descending seq */
+ }
else
new->min_value = get_param(min_value);
***************
*** 519,528 ****
--- 523,534 ----
new->min_value, new->max_value);
if (last_value == (DefElem *) NULL) /* START WITH */
+ {
if (new->increment_by > 0)
new->last_value = new->min_value; /* ascending seq */
else
new->last_value = new->max_value; /* descending seq */
+ }
else
new->last_value = get_param(last_value);
*** backend/commands/variable.c.orig Mon Mar 16 21:05:34 1998
--- backend/commands/variable.c Mon Mar 16 21:06:23 1998
***************
*** 444,456 ****
{
/* Not yet tried to save original value from environment? */
if (defaultTZ == NULL)
/* found something? then save it for later */
if ((defaultTZ = getenv("TZ")) != NULL)
strcpy(TZvalue, defaultTZ);
! /* found nothing so mark with an invalid pointer */
else
defaultTZ = (char *) -1;
strcpy(tzbuf, "TZ=");
strcat(tzbuf, tok);
--- 444,458 ----
{
/* Not yet tried to save original value from environment? */
if (defaultTZ == NULL)
+ {
/* found something? then save it for later */
if ((defaultTZ = getenv("TZ")) != NULL)
strcpy(TZvalue, defaultTZ);
! /* found nothing so mark with an invalid pointer */
else
defaultTZ = (char *) -1;
+ }
strcpy(tzbuf, "TZ=");
strcat(tzbuf, tok);
*** backend/executor/nodeIndexscan.c.orig Tue Mar 3 21:46:44 1998
--- backend/executor/nodeIndexscan.c Mon Mar 16 21:06:59 1998
***************
*** 91,97 ****
IndexScanDesc scandesc;
Relation heapRelation;
RetrieveIndexResult result;
- ItemPointer iptr;
HeapTuple tuple;
TupleTableSlot *slot;
Buffer buffer = InvalidBuffer;
--- 91,96 ----
***************
*** 116,173 ****
* ----------------
*/
! for (;;)
{
! result = index_getnext(scandesc, direction);
! /* ----------------
! * if scanning this index succeeded then return the
! * appropriate heap tuple.. else return NULL.
! * ----------------
! */
! if (result)
! {
! iptr = &result->heap_iptr;
! tuple = heap_fetch(heapRelation,
! false,
! iptr,
! &buffer);
! /* be tidy */
! pfree(result);
!
! if (tuple == NULL)
! {
! /* ----------------
! * we found a deleted tuple, so keep on scanning..
! * ----------------
! */
! if (BufferIsValid(buffer))
! ReleaseBuffer(buffer);
! continue;
! }
/* ----------------
! * store the scanned tuple in the scan tuple slot of
! * the scan state. Eventually we will only do this and not
! * return a tuple. Note: we pass 'false' because tuples
! * returned by amgetnext are pointers onto disk pages and
! * were not created with palloc() and so should not be pfree()'d.
! * ----------------
! */
ExecStoreTuple(tuple, /* tuple to store */
! slot,/* slot to store in */
! buffer, /* buffer associated with tuple */
! false); /* don't pfree */
!
return slot;
}
!
! /* ----------------
! * if we get here it means the index scan failed so we
! * are at the end of the scan..
! * ----------------
! */
! return ExecClearTuple(slot);
}
}
/* ----------------------------------------------------------------
--- 115,161 ----
* ----------------
*/
! /* ----------------
! * if scanning this index succeeded then return the
! * appropriate heap tuple.. else return NULL.
! * ----------------
! */
! while ((result = index_getnext(scandesc, direction)) != NULL)
{
! tuple = heap_fetch(heapRelation, false, &result->heap_iptr, &buffer);
! /* be tidy */
! pfree(result);
+ if (tuple != NULL)
+ {
/* ----------------
! * store the scanned tuple in the scan tuple slot of
! * the scan state. Eventually we will only do this and not
! * return a tuple. Note: we pass 'false' because tuples
! * returned by amgetnext are pointers onto disk pages and
! * were not created with palloc() and so should not be pfree()'d.
! * ----------------
! */
ExecStoreTuple(tuple, /* tuple to store */
! slot, /* slot to store in */
! buffer, /* buffer associated with tuple */
! false); /* don't pfree */
!
return slot;
}
! else
! {
! if (BufferIsValid(buffer))
! ReleaseBuffer(buffer);
! }
}
+
+ /* ----------------
+ * if we get here it means the index scan failed so we
+ * are at the end of the scan..
+ * ----------------
+ */
+ return ExecClearTuple(slot);
}
/* ----------------------------------------------------------------
***************
*** 194,207 ****
TupleTableSlot *
ExecIndexScan(IndexScan *node)
{
- TupleTableSlot *returnTuple;
-
/* ----------------
* use IndexNext as access method
* ----------------
*/
! returnTuple = ExecScan(&node->scan, IndexNext);
! return returnTuple;
}
/* ----------------------------------------------------------------
--- 182,192 ----
TupleTableSlot *
ExecIndexScan(IndexScan *node)
{
/* ----------------
* use IndexNext as access method
* ----------------
*/
! return ExecScan(&node->scan, IndexNext);
}
/* ----------------------------------------------------------------
***************
*** 377,383 ****
{
if (scanKeys[i] != NULL)
pfree(scanKeys[i]);
-
}
/* ----------------
--- 362,367 ----
*** backend/executor/nodeSeqscan.c.orig Tue Mar 3 21:45:08 1998
--- backend/executor/nodeSeqscan.c Mon Mar 16 21:07:57 1998
***************
*** 128,135 ****
* else, scan the relation
* ----------------
*/
! outerPlan = outerPlan((Plan *) node);
! if (outerPlan)
{
slot = ExecProcNode(outerPlan, (Plan *) node);
}
--- 128,134 ----
* else, scan the relation
* ----------------
*/
! if ((outerPlan = outerPlan((Plan *) node)) != NULL)
{
slot = ExecProcNode(outerPlan, (Plan *) node);
}
***************
*** 375,382 ****
scanstate = node->scanstate;
estate = node->plan.state;
! outerPlan = outerPlan((Plan *) node);
! if (outerPlan)
{
/* we are scanning a subplan */
outerPlan = outerPlan((Plan *) node);
--- 374,380 ----
scanstate = node->scanstate;
estate = node->plan.state;
! if ((outerPlan = outerPlan((Plan *) node)) != NULL)
{
/* we are scanning a subplan */
outerPlan = outerPlan((Plan *) node);
*** backend/lib/qsort.c.orig Mon Mar 16 21:08:25 1998
--- backend/lib/qsort.c Mon Mar 16 21:09:47 1998
***************
*** 117,129 ****
* comparisons than the insertion sort.
*/
#define SORT(bot, n) { \
! if (n > 1) \
! if (n == 2) { \
! t1 = bot + size; \
! if (compar(t1, bot) < 0) \
! SWAP(t1, bot); \
! } else \
! insertion_sort(bot, n, size, compar); \
}
static void
--- 117,130 ----
* comparisons than the insertion sort.
*/
#define SORT(bot, n) { \
! if (n > 1) { \
! if (n == 2) { \
! t1 = bot + size; \
! if (compar(t1, bot) < 0) \
! SWAP(t1, bot); \
! } else \
! insertion_sort(bot, n, size, compar); \
! } \
}
static void
*** backend/libpq/be-dumpdata.c.orig Mon Mar 16 21:10:26 1998
--- backend/libpq/be-dumpdata.c Mon Mar 16 21:10:45 1998
***************
*** 305,314 ****
--- 305,316 ----
lengths[i] = typeinfo->attrs[i]->attlen;
if (lengths[i] == -1) /* variable length attribute */
+ {
if (!isnull)
lengths[i] = VARSIZE(attr) - VARHDRSZ;
else
lengths[i] = 0;
+ }
if (!isnull && OidIsValid(typoutput))
{
*** backend/optimizer/path/joinrels.c.orig Mon Mar 16 21:11:08 1998
--- backend/optimizer/path/joinrels.c Mon Mar 16 21:11:22 1998
***************
*** 70,79 ****
--- 70,81 ----
Rel *outer_rel = (Rel *) lfirst(r);
if (!(joins = find_clause_joins(root, outer_rel, outer_rel->joininfo)))
+ {
if (BushyPlanFlag)
joins = find_clauseless_joins(outer_rel, outer_rels);
else
joins = find_clauseless_joins(outer_rel, root->base_relation_list_);
+ }
join_list = nconc(join_list, joins);
}
*** backend/parser/analyze.c.orig Mon Mar 16 21:11:53 1998
--- backend/parser/analyze.c Mon Mar 16 21:12:14 1998
***************
*** 583,593 ****
--- 583,595 ----
elog(ERROR, "parser: internal error; unrecognized deferred node", NULL);
if (constraint->contype == CONSTR_PRIMARY)
+ {
if (have_pkey)
elog(ERROR, "CREATE TABLE/PRIMARY KEY multiple primary keys"
" for table %s are not legal", stmt->relname);
else
have_pkey = TRUE;
+ }
else if (constraint->contype != CONSTR_UNIQUE)
elog(ERROR, "parser: internal error; unrecognized deferred constraint", NULL);
*** backend/postmaster/postmaster.c.orig Mon Mar 16 21:14:22 1998
--- backend/postmaster/postmaster.c Mon Mar 16 21:22:55 1998
***************
*** 60,66 ****
#include <sys/socket.h>
#ifdef HAVE_LIMITS_H
#include <limits.h>
- #define MAXINT INT_MAX
#else
#include <values.h>
#endif
--- 60,65 ----
***************
*** 100,105 ****
--- 99,108 ----
#else
#define FORK() vfork()
#endif
+ #endif
+
+ #if !defined(MAXINT)
+ #define MAXINT INT_MAX
#endif
#define INVALID_SOCK (-1)
*** backend/utils/adt/arrayfuncs.c.orig Mon Mar 16 21:15:50 1998
--- backend/utils/adt/arrayfuncs.c Mon Mar 16 21:16:22 1998
***************
*** 78,85 ****
static void
_ReadArray(int st[], int endp[], int bsize, int srcfd, int destfd,
ArrayType *array, int isDestLO, bool *isNull);
! static ArrayCastAndSet(char *src, bool typbyval, int typlen, char *dest);
! static SanityCheckInput(int ndim, int n, int dim[], int lb[], int indx[]);
static int array_read(char *destptr, int eltsize, int nitems, char *srcptr);
static char *array_seek(char *ptr, int eltsize, int nitems);
--- 78,85 ----
static void
_ReadArray(int st[], int endp[], int bsize, int srcfd, int destfd,
ArrayType *array, int isDestLO, bool *isNull);
! static int ArrayCastAndSet(char *src, bool typbyval, int typlen, char *dest);
! static int SanityCheckInput(int ndim, int n, int dim[], int lb[], int indx[]);
static int array_read(char *destptr, int eltsize, int nitems, char *srcptr);
static char *array_seek(char *ptr, int eltsize, int nitems);
***************
*** 1023,1028 ****
--- 1023,1029 ----
pfree(buff);
}
if (isDestLO)
+ {
if (ARR_IS_CHUNKED(array))
{
_ReadChunkArray(lowerIndx, upperIndx, len, fd, (char *) newfd, array,
***************
*** 1032,1037 ****
--- 1033,1039 ----
{
_ReadArray(lowerIndx, upperIndx, len, fd, newfd, array, 1, isNull);
}
+ }
#ifdef LOARRAY
LOclose(fd);
LOclose(newfd);
*** bin/pg_dump/pg_dump.c.orig Mon Mar 16 21:17:10 1998
--- bin/pg_dump/pg_dump.c Mon Mar 16 21:17:38 1998
***************
*** 1591,1600 ****
--- 1591,1602 ----
findx++;
}
if (TRIGGER_FOR_UPDATE(tgtype))
+ {
if (findx > 0)
strcat(query, " OR UPDATE");
else
strcat(query, " UPDATE");
+ }
sprintf(query, "%s ON %s FOR EACH ROW EXECUTE PROCEDURE %s (",
query, tblinfo[i].relname, tgfunc);
for (findx = 0; findx < tgnargs; findx++)
***************
*** 2508,2513 ****
--- 2510,2516 ----
{
ACLlist = ParseACL(tblinfo[i].relacl, &l);
if (ACLlist == (ACL *) NULL)
+ {
if (l == 0)
continue;
else
***************
*** 2516,2521 ****
--- 2519,2525 ----
tblinfo[i].relname);
exit_nicely(g_conn);
}
+ }
/* Revoke Default permissions for PUBLIC */
fprintf(fout,
*** bin/pg_version/pg_version.c.orig Mon Mar 16 21:36:07 1998
--- bin/pg_version/pg_version.c Mon Mar 16 21:36:18 1998
***************
*** 14,20 ****
#include <stdlib.h>
#include <stdio.h>
! #include <version.h> /* interface to SetPgVersion */
--- 14,20 ----
#include <stdlib.h>
#include <stdio.h>
! #include "version.h" /* interface to SetPgVersion */
*** backend/optimizer/geqo/geqo_pool.c.orig Mon Mar 16 21:40:24 1998
--- backend/optimizer/geqo/geqo_pool.c Mon Mar 16 21:41:50 1998
***************
*** 35,42 ****
#include "optimizer/clauses.h"
#include "optimizer/cost.h"
- #include "lib/qsort.h"
-
#include "optimizer/geqo_gene.h"
#include "optimizer/geqo.h"
#include "optimizer/geqo_pool.h"
--- 35,40 ----
***************
*** 127,134 ****
void
sort_pool(Pool *pool)
{
! pg_qsort(pool->data, pool->size, sizeof(Chromosome), compare);
!
}
/*
--- 125,131 ----
void
sort_pool(Pool *pool)
{
! qsort(pool->data, pool->size, sizeof(Chromosome), compare);
}
/*
*** backend/optimizer/path/predmig.c.orig Mon Mar 16 21:41:30 1998
--- backend/optimizer/path/predmig.c Mon Mar 16 21:41:34 1998
***************
*** 47,53 ****
#include "optimizer/cost.h"
#include "optimizer/keys.h"
#include "optimizer/tlist.h"
- #include "lib/qsort.h"
#define is_clause(node) (get_cinfo(node)) /* a stream node
* represents a clause
--- 47,52 ----
***************
*** 698,704 ****
nodearray[i] = tmp;
/* sort the array */
! pg_qsort(nodearray, num, sizeof(LispValue), xfunc_stream_compare);
/* paste together the array elements */
output = nodearray[num - 1];
--- 697,703 ----
nodearray[i] = tmp;
/* sort the array */
! qsort(nodearray, num, sizeof(LispValue), xfunc_stream_compare);
/* paste together the array elements */
output = nodearray[num - 1];
*** backend/storage/page/bufpage.c.orig Mon Mar 16 21:42:09 1998
--- backend/storage/page/bufpage.c Mon Mar 16 21:42:24 1998
***************
*** 24,31 ****
#include "utils/memutils.h"
#include "storage/bufpage.h"
- #include "lib/qsort.h"
-
static void
PageIndexTupleDeleteAdjustLinePointers(PageHeader phdr,
char *location, Size size);
--- 24,29 ----
***************
*** 330,336 ****
}
/* sort itemIdSortData array... */
! pg_qsort((char *) itemidbase, nused, sizeof(struct itemIdSortData),
itemidcompare);
/* compactify page */
--- 328,334 ----
}
/* sort itemIdSortData array... */
! qsort((char *) itemidbase, nused, sizeof(struct itemIdSortData),
itemidcompare);
/* compactify page */
*** backend/lib/Makefile.orig Mon Mar 16 21:42:40 1998
--- backend/lib/Makefile Mon Mar 16 21:42:50 1998
***************
*** 15,21 ****
CFLAGS+=$(INCLUDE_OPT)
! OBJS = bit.o fstack.o hasht.o lispsort.o qsort.o stringinfo.o dllist.o
all: SUBSYS.o
--- 15,21 ----
CFLAGS+=$(INCLUDE_OPT)
! OBJS = bit.o fstack.o hasht.o lispsort.o stringinfo.o dllist.o
all: SUBSYS.o
> There's a patch attached to fix gcc 2.8.x warnings, except for the > yyerror ones from bison. It also includes a few 'enhancements' to the C > programming style (which are, of course, personal). > > The other patch removes the compilation of backend/lib/qsort.c, as > qsort() is a standard function in stdlib.h and can be used any where > else (and it is). It was only used in > backend/optimizer/geqo/geqo_pool.c, backend/optimizer/path/predmig.c, > and backend/storage/page/bufpage.c > Woh, this is some pretty serious code. Nice. -- Bruce Momjian | 830 Blythe Avenue maillist@candle.pha.pa.us | Drexel Hill, Pennsylvania 19026 + If your life is a hard drive, | (610) 353-9879(w) + Christ can be your backup. | (610) 853-3000(h)
Applied. Looked good, and qsort.c removed as you suggested. > There's a patch attached to fix gcc 2.8.x warnings, except for the > yyerror ones from bison. It also includes a few 'enhancements' to the C > programming style (which are, of course, personal). > > The other patch removes the compilation of backend/lib/qsort.c, as > qsort() is a standard function in stdlib.h and can be used any where > else (and it is). It was only used in > backend/optimizer/geqo/geqo_pool.c, backend/optimizer/path/predmig.c, > and backend/storage/page/bufpage.c > > > > Some or all of these changes might not be appropriate for v6.3, since we > > > are in beta testing and since they do not affect the current functionality. > > > For those cases, how about submitting patches based on the final v6.3 > > > release? > > There's more to come. Please review these patches. I ran the regression > tests and they only failed where this was expected (random, geo, etc). > > Cheers, > > Jeroen -- Bruce Momjian | 830 Blythe Avenue maillist@candle.pha.pa.us | Drexel Hill, Pennsylvania 19026 + If your life is a hard drive, | (610) 353-9879(w) + Christ can be your backup. | (610) 853-3000(h)