Re: [HACKERS] Some cleanups/enhancements - Mailing list pgsql-hackers

> 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