Some cleanups/enhancements

Started by Jeroen van Vianenabout 28 years ago14 messageshackers
Jump to latest
#1Jeroen van Vianen
jeroenv@design.nl

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

#2Bruce Momjian
bruce@momjian.us
In reply to: Jeroen van Vianen (#1)
Re: [HACKERS] Some cleanups/enhancements

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

#3The Hermit Hacker
scrappy@hub.org
In reply to: Jeroen van Vianen (#1)
Re: [HACKERS] Some cleanups/enhancements

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...:(

#4Bruce Momjian
bruce@momjian.us
In reply to: The Hermit Hacker (#3)
Re: [HACKERS] Some cleanups/enhancements

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

#5Thomas Lockhart
lockhart@alumni.caltech.edu
In reply to: Jeroen van Vianen (#1)
Re: [HACKERS] Some cleanups/enhancements

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

#6Bruce Momjian
bruce@momjian.us
In reply to: Thomas Lockhart (#5)
Re: [HACKERS] Some cleanups/enhancements

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

#7Brian E Gallew
geek+@cmu.edu
In reply to: Thomas Lockhart (#5)
Re: [HACKERS] Some cleanups/enhancements

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

#8Jeroen van Vianen
jeroenv@design.nl
In reply to: The Hermit Hacker (#3)
Re: [HACKERS] Some cleanups/enhancements

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

#9Thomas Lockhart
lockhart@alumni.caltech.edu
In reply to: Brian E Gallew (#7)
Re: [HACKERS] Some cleanups/enhancements

- 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

#10Bruce Momjian
bruce@momjian.us
In reply to: Jeroen van Vianen (#8)
Re: [HACKERS] Some cleanups/enhancements

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

#11Bruce Momjian
bruce@momjian.us
In reply to: Thomas Lockhart (#5)
Re: [HACKERS] Some cleanups/enhancements

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)
#12Jeroen van Vianen
jeroenv@design.nl
In reply to: Bruce Momjian (#11)
Re: [HACKERS] Some cleanups/enhancements

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

Attachments:

compiler.warnings.patchtext/plain; charset=us-ascii; name=compiler.warnings.patchDownload+137-107
qsort.patchtext/plain; charset=us-ascii; name=qsort.patchDownload+9-14
#13Bruce Momjian
bruce@momjian.us
In reply to: Jeroen van Vianen (#12)
Re: [HACKERS] Some cleanups/enhancements

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)
#14Bruce Momjian
bruce@momjian.us
In reply to: Jeroen van Vianen (#12)
Re: [HACKERS] Some cleanups/enhancements

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)