Re: [COMMITTERS] pgsql-server/src backend/main/main.c backend/p ... - Mailing list pgsql-patches

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom Lane wrote:
> >> momjian@svr1.postgresql.org (Bruce Momjian) writes:
> >>> Remove elog() calls from find_my_exec;  do fprintf(stderr) instead.
> >>
> >> Isn't that a seriously bad idea?
>
> > Yes, it probably is bad, but I am not sure how frequently this will
> > happen, _and_ I need to set my_exec_path very far up in the tree to the
> > timezone stuff knows the location, hence the fact that elog() calls
> > wouldn't work at all anyway, I think.
>
> The more I think about this, the less I like it.
>
> find_my_exec and friends are *not* more critical to the backend than
> elog is, and should not be done sooner.  The argument that "we have to
> find postgresql.conf before elog works" is specious --- elog will work
> fine before we have read the config file, indeed had better do so since
> guc.c uses elog to report problems.  What will happen is that errors
> will get reported to the compiled-in-default location, which happens to
> be stderr at the moment.  All you are accomplishing with this change is
> to hard-wire that default and make it impossible to change in the
> future.

Sure, I didn't know elog() sent to stderr before it was initialized.

The following patch uses elog() when linked to the backend, and stderr
directly when used by client code.  The old code was really DEBUG2-level
stuff (it showed how it was searching the PATH mostly), but I have
remove those because for clients that is going to go to stderr, so now
it only does an elog(LOG) when something significant happens.

I did have to re-add the separate compile of exec.c to every place that
used it because clients don't have elog, but that's OK.

> Other concerns: I do not like changing random error reports from elog to
> printf, firstly because it will encourage people to code other error
> reports as printfs, which is something that took us a huge amount of
> work to stamp out, and secondly because the requirement will propagate.
> Are you going to avoid using palloc in find_my_exec, too?  What about in
> any subroutines that these things call?
>
> I recommend reverting both this change and the ones to do find_my_exec
> etc in main.c.  You're better off doing these things where they were
> done in PostmasterMain, PostgresMain, etc.  palloc and elog are simply
> fundamental parts of the backend programming environment; it does not
> make sense to push complex functionality into a part of the code where
> they aren't available.

In the old code, the exec path was only used for dynamic linking for
platforms that needed a full path for dynamic linking.  Now the timezone
code and even the $libdir path (used by GUC) is set from my_exec_path,
so it has to be earlier.  I can put it much earlier in both postgres and
postmaster, but by having it in main.c, I have it in only one place.  It
doesn't do any palloc or anything fancy, because of course it is also
used by client apps.

Patch attached and applied.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/bin/initdb/Makefile
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/initdb/Makefile,v
retrieving revision 1.39
diff -c -c -r1.39 Makefile
*** src/bin/initdb/Makefile    18 May 2004 20:18:58 -0000    1.39
--- src/bin/initdb/Makefile    19 May 2004 17:12:59 -0000
***************
*** 15,27 ****

  override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS)

! OBJS=    initdb.o

  all: submake-libpq submake-libpgport initdb

  initdb: $(OBJS) $(libpq_builddir)/libpq.a
      $(CC) $(CFLAGS) $(OBJS) $(libpq) $(LDFLAGS) $(LIBS) -o $@$(X)

  install: all installdirs
      $(INSTALL_PROGRAM) initdb$(X) $(DESTDIR)$(bindir)/initdb$(X)

--- 15,31 ----

  override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS)

! OBJS=    initdb.o \
!     $(filter exec.o, $(LIBOBJS))

  all: submake-libpq submake-libpgport initdb

  initdb: $(OBJS) $(libpq_builddir)/libpq.a
      $(CC) $(CFLAGS) $(OBJS) $(libpq) $(LDFLAGS) $(LIBS) -o $@$(X)

+ exec.c: % : $(top_srcdir)/src/port/%
+     rm -f $@ && $(LN_S) $< .
+
  install: all installdirs
      $(INSTALL_PROGRAM) initdb$(X) $(DESTDIR)$(bindir)/initdb$(X)

***************
*** 32,38 ****
      rm -f $(DESTDIR)$(bindir)/initdb$(X)

  clean distclean maintainer-clean:
!     rm -f initdb$(X) $(OBJS)


  # ensure that changes in datadir propagate into object file
--- 36,42 ----
      rm -f $(DESTDIR)$(bindir)/initdb$(X)

  clean distclean maintainer-clean:
!     rm -f initdb$(X) $(OBJS) exec.c


  # ensure that changes in datadir propagate into object file
Index: src/bin/pg_dump/Makefile
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/Makefile,v
retrieving revision 1.48
diff -c -c -r1.48 Makefile
*** src/bin/pg_dump/Makefile    18 May 2004 20:18:58 -0000    1.48
--- src/bin/pg_dump/Makefile    19 May 2004 17:12:59 -0000
***************
*** 18,23 ****
--- 18,24 ----
  OBJS=    pg_backup_archiver.o pg_backup_db.o pg_backup_custom.o \
      pg_backup_files.o pg_backup_null.o pg_backup_tar.o \
      dumputils.o
+ PG_DUMPALL_OBJS =    $(filter exec.o, $(LIBOBJS))

  EXTRA_OBJS = $(top_builddir)/src/backend/parser/keywords.o

***************
*** 32,39 ****
  pg_restore: pg_restore.o $(OBJS) $(libpq_builddir)/libpq.a
      $(CC) $(CFLAGS) pg_restore.o $(OBJS) $(EXTRA_OBJS) $(libpq) $(LDFLAGS) $(LIBS) -o $@$(X)

! pg_dumpall: pg_dumpall.o dumputils.o $(libpq_builddir)/libpq.a
!     $(CC) $(CFLAGS) pg_dumpall.o dumputils.o $(EXTRA_OBJS) $(libpq) $(LDFLAGS) $(LIBS) -o $@$(X)

  .PHONY: submake-backend
  submake-backend:
--- 33,43 ----
  pg_restore: pg_restore.o $(OBJS) $(libpq_builddir)/libpq.a
      $(CC) $(CFLAGS) pg_restore.o $(OBJS) $(EXTRA_OBJS) $(libpq) $(LDFLAGS) $(LIBS) -o $@$(X)

! pg_dumpall: pg_dumpall.o dumputils.o $(PG_DUMPALL_OBJS) $(libpq_builddir)/libpq.a
!     $(CC) $(CFLAGS) pg_dumpall.o dumputils.o $(PG_DUMPALL_OBJS) $(EXTRA_OBJS) $(libpq) $(LDFLAGS) $(LIBS) -o $@$(X)
!
! exec.c: % : $(top_srcdir)/src/port/%
!     rm -f $@ && $(LN_S) $< .

  .PHONY: submake-backend
  submake-backend:
***************
*** 52,55 ****
      rm -f $(addprefix $(DESTDIR)$(bindir)/, pg_dump$(X) pg_restore$(X) pg_dumpall$(X))

  clean distclean maintainer-clean:
!     rm -f pg_dump$(X) pg_restore$(X) pg_dumpall$(X) $(OBJS) pg_dump.o common.o pg_dump_sort.o pg_restore.o
pg_dumpall.o
--- 56,59 ----
      rm -f $(addprefix $(DESTDIR)$(bindir)/, pg_dump$(X) pg_restore$(X) pg_dumpall$(X))

  clean distclean maintainer-clean:
!     rm -f pg_dump$(X) pg_restore$(X) pg_dumpall$(X) $(OBJS) pg_dump.o common.o pg_dump_sort.o pg_restore.o
pg_dumpall.oexec.c 
Index: src/bin/psql/Makefile
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/psql/Makefile,v
retrieving revision 1.45
diff -c -c -r1.45 Makefile
*** src/bin/psql/Makefile    18 May 2004 20:18:58 -0000    1.45
--- src/bin/psql/Makefile    19 May 2004 17:12:59 -0000
***************
*** 19,25 ****

  OBJS=    command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
      startup.o prompt.o variables.o large_obj.o print.o describe.o \
!     psqlscan.o tab-complete.o mbprint.o

  FLEXFLAGS = -Cfe

--- 19,26 ----

  OBJS=    command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
      startup.o prompt.o variables.o large_obj.o print.o describe.o \
!     psqlscan.o tab-complete.o mbprint.o \
!     $(filter exec.o, $(LIBOBJS))

  FLEXFLAGS = -Cfe

***************
*** 29,34 ****
--- 30,38 ----
  psql: $(OBJS) $(libpq_builddir)/libpq.a
      $(CC) $(CFLAGS) $(OBJS) $(libpq) $(LDFLAGS) $(LIBS) -o $@$(X)

+ exec.c: % : $(top_srcdir)/src/port/%
+     rm -f $@ && $(LN_S) $< .
+
  help.o: $(srcdir)/sql_help.h

  ifdef PERL
***************
*** 60,66 ****

  # psqlscan.c is in the distribution tarball, so is not cleaned here
  clean distclean:
!     rm -f psql$(X) $(OBJS)

  maintainer-clean: distclean
      rm -f $(srcdir)/sql_help.h $(srcdir)/psqlscan.c
--- 64,70 ----

  # psqlscan.c is in the distribution tarball, so is not cleaned here
  clean distclean:
!     rm -f psql$(X) $(OBJS) exec.c

  maintainer-clean: distclean
      rm -f $(srcdir)/sql_help.h $(srcdir)/psqlscan.c
Index: src/interfaces/ecpg/preproc/Makefile
===================================================================
RCS file: /cvsroot/pgsql-server/src/interfaces/ecpg/preproc/Makefile,v
retrieving revision 1.105
diff -c -c -r1.105 Makefile
*** src/interfaces/ecpg/preproc/Makefile    18 May 2004 20:18:58 -0000    1.105
--- src/interfaces/ecpg/preproc/Makefile    19 May 2004 17:13:00 -0000
***************
*** 19,31 ****
  override CFLAGS += $(PTHREAD_CFLAGS)

  OBJS=    preproc.o type.o ecpg.o ecpg_keywords.o output.o\
!     keywords.o c_keywords.o ../ecpglib/typename.o descriptor.o variable.o

  all: submake-libpgport ecpg

  ecpg: $(OBJS)
      $(CC) $(CFLAGS) $(LDFLAGS) $^ $(LIBS) $(PTHREAD_LIBS) -o $@$(X)

  # pgc is compiled as part of preproc
  preproc.o: $(srcdir)/pgc.c

--- 19,35 ----
  override CFLAGS += $(PTHREAD_CFLAGS)

  OBJS=    preproc.o type.o ecpg.o ecpg_keywords.o output.o\
!     keywords.o c_keywords.o ../ecpglib/typename.o descriptor.o variable.o \
!     $(filter exec.o, $(LIBOBJS))

  all: submake-libpgport ecpg

  ecpg: $(OBJS)
      $(CC) $(CFLAGS) $(LDFLAGS) $^ $(LIBS) $(PTHREAD_LIBS) -o $@$(X)

+ exec.c: % : $(top_srcdir)/src/port/%
+     rm -f $@ && $(LN_S) $< .
+
  # pgc is compiled as part of preproc
  preproc.o: $(srcdir)/pgc.c

***************
*** 62,68 ****
      rm -f $(DESTDIR)$(bindir)/ecpg$(X)

  clean distclean:
!     rm -f *.o ecpg$(X)
  # garbage from partial builds
      @rm -f y.tab.c y.tab.h
  # garbage from development
--- 66,72 ----
      rm -f $(DESTDIR)$(bindir)/ecpg$(X)

  clean distclean:
!     rm -f *.o ecpg$(X) exec.c
  # garbage from partial builds
      @rm -f y.tab.c y.tab.h
  # garbage from development
Index: src/port/exec.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/port/exec.c,v
retrieving revision 1.9
diff -c -c -r1.9 exec.c
*** src/port/exec.c    19 May 2004 04:36:33 -0000    1.9
--- src/port/exec.c    19 May 2004 17:13:00 -0000
***************
*** 48,53 ****
--- 48,60 ----
  #define S_IXOTH         ((S_IXUSR)>>6)
  #endif

+ #ifndef FRONTEND
+ /* We use only 3-parameter elog calls in this file, for simplicity */
+ #define log_error(str, param)    elog(LOG, (str), (param))
+ #else
+ #define log_error(str, param)    fprintf(stderr, (str), (param))
+ #endif
+
  static void win32_make_absolute(char *path);

  /*
***************
*** 192,198 ****
      {
          if (*++p == '\0')
          {
!             fprintf(stderr, "argv[0] ends with a path separator \"%s\"", argv0);
              return -1;
          }
          if (is_absolute_path(argv0) || !getcwd(buf, MAXPGPATH))
--- 199,205 ----
      {
          if (*++p == '\0')
          {
!             log_error("argv[0] ends with a path separator \"%s\"", argv0);
              return -1;
          }
          if (is_absolute_path(argv0) || !getcwd(buf, MAXPGPATH))
***************
*** 208,214 ****
          }
          else
          {
!             fprintf(stderr, "invalid binary \"%s\"", buf);
              return -1;
          }
      }
--- 215,221 ----
          }
          else
          {
!             log_error("invalid binary \"%s\"", buf);
              return -1;
          }
      }
***************
*** 245,251 ****
                  case -1:        /* wasn't even a candidate, keep looking */
                      break;
                  case -2:        /* found but disqualified */
!                     fprintf(stderr, "could not read binary \"%s\"", buf);
                      free(path);
                      return -1;
              }
--- 252,258 ----
                  case -1:        /* wasn't even a candidate, keep looking */
                      break;
                  case -2:        /* found but disqualified */
!                     log_error("could not read binary \"%s\"", buf);
                      free(path);
                      return -1;
              }
***************
*** 255,261 ****
          free(path);
      }

!     fprintf(stderr, "could not find a \"%s\" to execute", argv0);
      return -1;

  #if 0
--- 262,268 ----
          free(path);
      }

!     log_error("could not find a \"%s\" to execute", argv0);
      return -1;

  #if 0
***************
*** 337,353 ****
      }
      else if (WIFEXITED(exitstatus))
      {
!         fprintf(stderr, _("child process exited with exit code %d\n"),
                  WEXITSTATUS(exitstatus));
      }
      else if (WIFSIGNALED(exitstatus))
      {
!         fprintf(stderr, _("child process was terminated by signal %d\n"),
                  WTERMSIG(exitstatus));
      }
      else
      {
!         fprintf(stderr, _("child process exited with unrecognized status %d\n"),
                  exitstatus);
      }

--- 344,360 ----
      }
      else if (WIFEXITED(exitstatus))
      {
!         log_error(_("child process exited with exit code %d\n"),
                  WEXITSTATUS(exitstatus));
      }
      else if (WIFSIGNALED(exitstatus))
      {
!         log_error(_("child process was terminated by signal %d\n"),
                  WTERMSIG(exitstatus));
      }
      else
      {
!         log_error(_("child process exited with unrecognized status %d\n"),
                  exitstatus);
      }

***************
*** 369,375 ****

      if (_fullpath(abspath, path, MAXPGPATH) == NULL)
      {
!         fprintf(stderr, "Win32 path expansion failed:  %s", strerror(errno));
          StrNCpy(abspath, path, MAXPGPATH);
      }
      canonicalize_path(abspath);
--- 376,382 ----

      if (_fullpath(abspath, path, MAXPGPATH) == NULL)
      {
!         log_error("Win32 path expansion failed:  %s", strerror(errno));
          StrNCpy(abspath, path, MAXPGPATH);
      }
      canonicalize_path(abspath);