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

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