Обсуждение: Win32 testing needed

I have just committed a somewhat trimmed-down version of Andreas Pflug's
syslogger patch.  It seems to work okay on Unix (with or without
EXEC_BACKEND) but I'm not in a position to test it on Windows.  Would
someone give it a try and report back?  Please check in particular that
the logger shuts down cleanly after the postmaster is gone.

Also, Andreas' patch to enable symlinks (ie tablespaces) on Windows
looks very interesting, but with the hour so late we need confirmation
from someone else that it works.  Anyone?

            regards, tom lane

I can build and check both these on win 2003 server at approx 1800-1900 my time
(it's 1615 here right now). In that advent that I cannot get access to said
machine, I can do the checks on win 2000 pro tomorrow morning.

Let me know if that sort of time frame is ok.

regards

Mark

Quoting Tom Lane <tgl@sss.pgh.pa.us>:

> I have just committed a somewhat trimmed-down version of Andreas Pflug's
> syslogger patch.  It seems to work okay on Unix (with or without
> EXEC_BACKEND) but I'm not in a position to test it on Windows.  Would
> someone give it a try and report back?  Please check in particular that
> the logger shuts down cleanly after the postmaster is gone.
>
> Also, Andreas' patch to enable symlinks (ie tablespaces) on Windows
> looks very interesting, but with the hour so late we need confirmation
> from someone else that it works.  Anyone?
>
>             regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
>

> Let me know if that sort of time frame is ok.

Sure... much appreciated ...

            regards, tom lane

just tried to build on win2003 and get an error:

./configure --prefix="c:/progra~1/pgsql" --with-libs=/usr/local/lib
--with-includes=/usr/local/include
make

......

dlltool --dllname postgres.exe --output-exp postgres.exp --def postgres.def

gcc -O2 -fno-strict-aliasing -Wall -Wmissing-prototypes
-Wmissing-declarations -L../../src/port -L/usr/local/lib  -o
postgres.exe -Wl,--base-file,postgres.base postgres.exp access/SUBSYS.o
bootstrap/SUBSYS.o catalog/SUBSYS.o parser/SUBSYS.o commands/SUBSYS.o
executor/SUBSYS.o lib/SUBSYS.o libpq/SUBSYS.o main/SUBSYS.o
nodes/SUBSYS.o optimizer/SUBSYS.o port/SUBSYS.o postmaster/SUBSYS.o
regex/SUBSYS.o rewrite/SUBSYS.o storage/SUBSYS.o tcop/SUBSYS.o
utils/SUBSYS.o ../../src/timezone/SUBSYS.o -lpgport -lz -lwsock32 -lm
-lws2_32

../../src/port/libpgport.a(dirmod.o)(.text+0x33a):dirmod.c: undefined
reference to `_imp__CurrentMemoryContext'

../../src/port/libpgport.a(dirmod.o)(.text+0x39b):dirmod.c: undefined
reference to `_imp__CurrentMemoryContext'

make[2]: *** [postgres] Error 1

make[2]: Leaving directory
`/home/markir/develop/c/postgresql-8.0devel/src/backend'

Mark Kirkwood <markir@coretech.co.nz> writes:
> ../../src/port/libpgport.a(dirmod.o)(.text+0x33a):dirmod.c: undefined
> reference to `_imp__CurrentMemoryContext'

> ../../src/port/libpgport.a(dirmod.o)(.text+0x39b):dirmod.c: undefined
> reference to `_imp__CurrentMemoryContext'

Hmmm ... the only recent change I can see that looks likely to be
related is this one:

2004-08-01 14:07  tgl

    * src/backend/Makefile: Add libpgport to postgres.def for Windows
    build.    Per Magnus Hagander.

If you back that out, does it help?

            regards, tom lane

I'm seeing it too. Just built from clean, but it has been a few days, so...

../../src/port/libpgport.a(dirmod.o)(.text+0x33a): In function `rmtree':
c:/cygwin/opt/wip/pgsql/src/port/dirmod.c:216: undefined reference to
`_imp__CurrentMemoryContext'
../../src/port/libpgport.a(dirmod.o)(.text+0x39b):c:/cygwin/opt/wip/pgsql/sr
c/port/dirmod.c:222: undefined reference to `_imp__CurrentMemoryContext'

The xstrdup (aka pstrdup) call in rmtree is the problem, as it relies on the
backend only CurrentMemoryContext (IIRC).

Cheers,
Claudio



> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> Sent: Friday, 6 August 2004 3:58 PM
> To: Mark Kirkwood
> Cc: pgsql-hackers-win32@postgresql.org
> Subject: Re: [pgsql-hackers-win32] Win32 testing needed
>
>
> Mark Kirkwood <markir@coretech.co.nz> writes:
> > ../../src/port/libpgport.a(dirmod.o)(.text+0x33a):dirmod.c:
> undefined
> > reference to `_imp__CurrentMemoryContext'
>
> > ../../src/port/libpgport.a(dirmod.o)(.text+0x39b):dirmod.c:
> undefined
> > reference to `_imp__CurrentMemoryContext'
>
> Hmmm ... the only recent change I can see that looks likely to be
> related is this one:
>
> 2004-08-01 14:07  tgl
>
>     * src/backend/Makefile: Add libpgport to postgres.def
> for Windows
>     build.    Per Magnus Hagander.
>
> If you back that out, does it help?
>
>             regards, tom lane
>
> ---------------------------(end of
> broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings
>

---
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see
<a
href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
ailpolicy.html</a>

> The xstrdup (aka pstrdup) call in rmtree is the problem, as
> it relies on the backend only CurrentMemoryContext (IIRC).

Which (pstrdup) shouldn't be used as xstrdup on FRONTEND; should be the
provided function.

Has a FRONTEND define gone awol?

Cheers,
Claudio

---
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see
<a
href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
ailpolicy.html</a>

Claudio Natoli <claudio.natoli@memetrics.com> writes:
>> it relies on the backend only CurrentMemoryContext (IIRC).

> Which (pstrdup) shouldn't be used as xstrdup on FRONTEND; should be the
> provided function.

But Mark's failure was in building the backend.

            regards, tom lane

I will try a *completely* fresh checkout .. just in case I have  some
CVS strangeness....

Tom Lane wrote:

>Claudio Natoli <claudio.natoli@memetrics.com> writes:
>
>
>>>it relies on the backend only CurrentMemoryContext (IIRC).
>>>
>>>
>
>
>
>>Which (pstrdup) shouldn't be used as xstrdup on FRONTEND; should be the
>>provided function.
>>
>>
>
>But Mark's failure was in building the backend.
>
>            regards, tom lane
>
>---------------------------(end of broadcast)---------------------------
>TIP 4: Don't 'kill -9' the postmaster
>
>

> But Mark's failure was in building the backend.

Ah, um, right (sorry, divided attention atm).

FWIW, backing out the most recent changes to src/backend/Makefile, as
suggested, doesn't fix the problem.

---
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see
<a
href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
ailpolicy.html</a>

I can confirm that too (fresh checkout made no difference either)...


Claudio Natoli wrote:

>>But Mark's failure was in building the backend.
>>
>>
>
>Ah, um, right (sorry, divided attention atm).
>
>FWIW, backing out the most recent changes to src/backend/Makefile, as
>suggested, doesn't fix the problem.
>
>---
>Certain disclaimers and policies apply to all email sent from Memetrics.
>For the full text of these disclaimers and policies see
><a
>href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
>ailpolicy.html</a>
>
>---------------------------(end of broadcast)---------------------------
>TIP 9: the planner will ignore your desire to choose an index scan if your
>      joining column's datatypes do not match
>
>

> I know this message. It does not appear for me if I do a clean checkout,
> and make from the pgsql top directory. If I make from src/backend (to
> skip the lengthy locale makes on my slow machine), that said undefined
> reference comes up, and won't go even if I make from the top dir unless
> dirmod.c is touched.

I can confirm that I've also experienced this previously, in the mode
described. However, with the introduction of rmtree, it looks like it is now
broken even from the top directory.


> So it's better if src/port sources don't use backend's functions.

Agree. Or rather, better if src/port doesn't use backend DATA.

Cheers,
Claudio

---
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see
<a
href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
ailpolicy.html</a>

Mark Kirkwood wrote:
>
> ../../src/port/libpgport.a(dirmod.o)(.text+0x33a):dirmod.c: undefined
> reference to `_imp__CurrentMemoryContext'
>

I know this message. It does not appear for me if I do a clean checkout,
and make from the pgsql top directory. If I make from src/backend (to
skip the lengthy locale makes on my slow machine), that said undefined
reference comes up, and won't go even if I make from the top dir unless
dirmod.c is touched.

Apparently the reason is crossover referencing between libs, i.e.
libpgport.a using libpostgres.a and vice versa.

So it's better if src/port sources don't use backend's functions.

Regards,
Andreas

Claudio Natoli wrote:
>
>
>>So it's better if src/port sources don't use backend's functions.
>
>
> Agree. Or rather, better if src/port doesn't use backend DATA.

Not easy to avoid, many functions are really macros using backend data.

I wonder why rmtree uses xmalloc at all; instead of creating an array of
dir names and then iterating the array this could be done in one step.
There may be more than one DIR* open, right?

Regards,
Andreas

Tom Lane wrote:
> I have just committed a somewhat trimmed-down version of Andreas Pflug's
> syslogger patch.  It seems to work okay on Unix (with or without
> EXEC_BACKEND) but I'm not in a position to test it on Windows.  Would
> someone give it a try and report back?  Please check in particular that
> the logger shuts down cleanly after the postmaster is gone.

Attached a patch with several issues resolved; only win32 checked.

After your changes, the error from ReadFile is not ERROR_HANDLE_EOF any 
more, but ERROR_PIPE_BROKEN (which should be expected either), check is 
done for both now.

The logger should *not* use proc_exit but exit(0), because proc_exit 
might try to elog something, after we just closed the log file. IMHO 
there's nothing left to cleanup anyway.

Changing setvbuf to use line buffered mode broke win32; apparently a 
line is not a line there.... changed back to _IONBUF which should be 
identical in result because we're always writing a complete line.

An observation I didn't track down so far:
Some LOG messages (e.g. the final logger shutdown, or "received fast 
shutdown request") don't have proper CRLF line ending in win32, but only 
LF.

If the logger subprocess is killed, it will come up again ok, but 
redirecting to NULL_DEV doesn't work (open returns -1; that's what I had 
realStdErr for). Opening c:\temp\dummy instead does help, but that's not 
a solution. So what now? I'd propose inheriting the old stderr handle 
instead of redirection_done so it can be reused in this case, as in my 
original posting.

Finally, you don't seem to be a friend of a logfile rotation user 
trigger... Please consider including it anyway.



Regards,
Andreas
Index: backend/postmaster/postmaster.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/postmaster/postmaster.c,v
retrieving revision 1.420
diff -u -r1.420 postmaster.c
--- backend/postmaster/postmaster.c    5 Aug 2004 23:32:10 -0000    1.420
+++ backend/postmaster/postmaster.c    6 Aug 2004 10:01:57 -0000
@@ -3082,7 +3082,11 @@
              */
             kill(PgArchPID, SIGUSR1);
         }
-    }
+        }
+        if (CheckPostmasterSignal(PMSIGNAL_ROTATE_LOGFILE) && SysLoggerPID != 0)
+        {
+                kill(SysLoggerPID, SIGUSR1);
+        }
 
     PG_SETMASK(&UnBlockSig);
 
Index: backend/postmaster/syslogger.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/postmaster/syslogger.c,v
retrieving revision 1.1
diff -u -r1.1 syslogger.c
--- backend/postmaster/syslogger.c    5 Aug 2004 23:32:10 -0000    1.1
+++ backend/postmaster/syslogger.c    6 Aug 2004 10:01:57 -0000
@@ -37,6 +37,7 @@
 #include "pgtime.h"
 #include "storage/ipc.h"
 #include "storage/pg_shmem.h"
+#include "storage/pmsignal.h"
 #include "utils/guc.h"
 #include "utils/ps_status.h"
 
@@ -83,6 +84,7 @@
  * Flags set by interrupt handlers for later service in the main loop.
  */
 static volatile sig_atomic_t got_SIGHUP = false;
+static volatile sig_atomic_t rotation_requested = false;
 
 
 /* Local subroutines */
@@ -96,6 +98,7 @@
 static void logfile_rotate(void);
 static char* logfile_getname(pg_time_t timestamp);
 static void sigHupHandler(SIGNAL_ARGS);
+static void rotationHandler(SIGNAL_ARGS);
 
 
 /*
@@ -168,7 +171,7 @@
     pqsignal(SIGQUIT, SIG_IGN);
     pqsignal(SIGALRM, SIG_IGN);
     pqsignal(SIGPIPE, SIG_IGN);
-    pqsignal(SIGUSR1, SIG_IGN);
+    pqsignal(SIGUSR1, rotationHandler);
     pqsignal(SIGUSR2, SIG_IGN);
 
     /*
@@ -201,7 +204,6 @@
     /* main worker loop */
     for (;;)
     {
-        bool rotation_requested = false;
 #ifndef WIN32
         char        logbuffer[1024];
         int          bytesRead;
@@ -255,7 +257,10 @@
         }
 
         if (rotation_requested)
-            logfile_rotate();
+                {
+            logfile_rotate();            
+                        rotation_requested = false;
+                }
 
 #ifndef WIN32
         /*
@@ -320,7 +325,7 @@
             if (syslogFile)
                 fclose(syslogFile);
             /* normal exit from the syslogger is here */
-            proc_exit(0);
+            exit(0);
         }
     }
 }
@@ -401,7 +406,7 @@
                  (errmsg("could not create logfile \"%s\": %m",
                          filename))));
 
-    setvbuf(syslogFile, NULL, _IOLBF, 0);
+    setvbuf(syslogFile, NULL, _IONBF, 0);
 
     pfree(filename);
 
@@ -557,7 +562,7 @@
     if (fd != -1)
     {
         syslogFile = fdopen(fd, "a");
-        setvbuf(syslogFile, NULL, _IOLBF, 0);
+        setvbuf(syslogFile, NULL, _IONBF, 0);
     }
     redirection_done = (bool) atoi(*argv++);
 #else  /* WIN32 */
@@ -568,7 +573,7 @@
         if (fd != 0)
         {
             syslogFile = fdopen(fd, "a");
-            setvbuf(syslogFile, NULL, _IOLBF, 0);
+            setvbuf(syslogFile, NULL, _IONBF, 0);
         }
     }
     redirection_done = (bool) atoi(*argv++);
@@ -631,11 +636,11 @@
         {
             DWORD error = GetLastError();
 
-            if (error == ERROR_HANDLE_EOF)
+            if (error == ERROR_HANDLE_EOF || error == ERROR_BROKEN_PIPE)
                 break;
             ereport(LOG,
                     (errcode_for_file_access(),
-                     errmsg("could not read from logger pipe: %m")));
+                     errmsg("could not read from logger pipe: %lu", error)));
         }
         else if (bytesRead > 0)
             write_syslogger_file(logbuffer, bytesRead);
@@ -689,7 +694,7 @@
         return;
     }
 
-    setvbuf(fh, NULL, _IOLBF, 0);
+    setvbuf(fh, NULL, _IONBF, 0);
 
     /* On Windows, need to interlock against data-transfer thread */
 #ifdef WIN32
@@ -735,6 +740,26 @@
     return filename;
 }
 
+
+/*
+ * Rotate log file
+ */
+bool
+LogFileRotate(void)
+{
+        if (!(Redirect_stderr))
+        {
+                ereport(NOTICE,
+                (errcode(ERRCODE_WARNING),
+                 errmsg("no logfile configured; rotation not supported")));
+                return false;
+        }
+
+        SendPostmasterSignal(PMSIGNAL_ROTATE_LOGFILE);
+
+        return true;
+}
+
 /* --------------------------------
  *        signal handler routines
  * --------------------------------
@@ -746,3 +771,10 @@
 {
     got_SIGHUP = true;
 }
+
+/* SIGUSR1: set flag to rotate logfile */
+static void
+rotationHandler(SIGNAL_ARGS)
+{
+    rotation_requested = true;
+}
Index: include/postmaster/syslogger.h
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/include/postmaster/syslogger.h,v
retrieving revision 1.1
diff -u -r1.1 syslogger.h
--- include/postmaster/syslogger.h    5 Aug 2004 23:32:12 -0000    1.1
+++ include/postmaster/syslogger.h    6 Aug 2004 10:01:59 -0000
@@ -32,6 +32,8 @@
 
 extern void write_syslogger_file(const char *buffer, int count);
 
+extern bool LogFileRotate(void);
+
 #ifdef EXEC_BACKEND
 extern void SysLoggerMain(int argc, char *argv[]);
 #endif
Index: include/storage/pmsignal.h
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/include/storage/pmsignal.h,v
retrieving revision 1.9
diff -u -r1.9 pmsignal.h
--- include/storage/pmsignal.h    19 Jul 2004 02:47:15 -0000    1.9
+++ include/storage/pmsignal.h    6 Aug 2004 10:01:59 -0000
@@ -25,7 +25,7 @@
     PMSIGNAL_PASSWORD_CHANGE,    /* pg_pwd file has changed */
     PMSIGNAL_WAKEN_CHILDREN,    /* send a SIGUSR1 signal to all backends */
     PMSIGNAL_WAKEN_ARCHIVER,    /* send a NOTIFY signal to xlog archiver */
-
+    PMSIGNAL_ROTATE_LOGFILE,    /* send SIGUSR1 to syslogger to rotate logfile */
     NUM_PMSIGNALS                /* Must be last value of enum! */
 } PMSignalReason;

Andreas Pflug <pgadmin@pse-consulting.de> writes:
> Attached a patch with several issues resolved; only win32 checked.

> After your changes, the error from ReadFile is not ERROR_HANDLE_EOF any
> more, but ERROR_PIPE_BROKEN (which should be expected either), check is
> done for both now.

Got it.

> The logger should *not* use proc_exit but exit(0), because proc_exit
> might try to elog something, after we just closed the log file. IMHO
> there's nothing left to cleanup anyway.

No good.  If we can't survive exiting via proc_exit() then we're in deep
trouble anyway, because that is the path that any elog(ERROR) will take.
It might be best to just leave syslogFile open --- it should be properly
flushed and closed by exit() anyway, no?

> Changing setvbuf to use line buffered mode broke win32; apparently a
> line is not a line there.... changed back to _IONBUF which should be
> identical in result because we're always writing a complete line.

[ looks around ... ]  Oh, we dealt with this before: Windows treats
_IOLBF the same as _IOFBF, which we don't want.  Okay, ifdef time.
We do want _IOLBF on every other platform.

> An observation I didn't track down so far:
> Some LOG messages (e.g. the final logger shutdown, or "received fast
> shutdown request") don't have proper CRLF line ending in win32, but only
> LF.

Weird.  No ideas about that.  Can you determine whether the data coming
through the pipe has the problem?

> If the logger subprocess is killed, it will come up again ok, but
> redirecting to NULL_DEV doesn't work (open returns -1; that's what I had
> realStdErr for).

Why doesn't it work?  Do we just need a different spelling of
"/dev/null" for Windows?  Worst case, we could forcibly close
fileno(stderr) and just not have it pointing at anything if the
open of NULL_DEV doesn't work ... we never check ferror(stderr)
so it wouldn't really matter if output fails ...

> So what now? I'd propose inheriting the old stderr handle
> instead of redirection_done so it can be reused in this case, as in my
> original posting.

No, that was too much of a mess to solve a non-problem.  We do not
actually care whether stderr works in the logger, because everything it
has to say should go through elog anyway.  All we really care about is
that stderr is *not* still connected to the pipe.

I left stderr output enabled in the first instantation of the logger,
because it was easy to do and might provide a way to help debug
otherwise difficult logger problems.  But I don't think we need to go
out of our way to keep it enabled in re-instantiations, seeing that we
don't really expect the logger to crash anyway (see below).

> Finally, you don't seem to be a friend of a logfile rotation user
> trigger...

Nope, I'm not.  I think it's a bad solution to a nonexistent problem.
The logger's control parameters are more than sufficient.  Furthermore,
we'd really prefer that the logger doesn't ever crash (the restart
business is a bit ticklish) and so the fewer features it has, the better.

            regards, tom lane

Andreas Pflug <pgadmin@pse-consulting.de> writes:
> Claudio Natoli wrote:
>> Agree. Or rather, better if src/port doesn't use backend DATA.

> Not easy to avoid, many functions are really macros using backend data.

Perhaps we should insist that nothing in src/port can include postgres.h.
If it needs something that's not in c.h, it doesn't belong in src/port.

These are the affected files:

$ grep postgres.h *.c
copydir.c:#include "postgres.h"
dirmod.c:#include "postgres.h"
exec.c:#include "postgres.h"
pipe.c:#include "postgres.h"

            regards, tom lane

Andreas Pflug <pgadmin@pse-consulting.de> writes:
>              ereport(LOG,
>                      (errcode_for_file_access(),
> -                     errmsg("could not read from logger pipe: %m")));
> +                     errmsg("could not read from logger pipe: %lu", error)));

Does %m actually give a wrong result here?  Because if it does,
errcode_for_file_access() is wrong too.

            regards, tom lane

Andreas Pflug <pgadmin@pse-consulting.de> writes:
> Apparently, this is a mixture of binary and text file mode. Initially,
> stderr is in text mode. When redirecting with dup2, it will be binary;
> this must be corrected with

> dup2(_open_osfhandle(...., _O_APPEND | _O_TEXT), ...

Okay, I added _O_TEXT to that call; the #ifdef WIN32 part now looks like

                fflush(stderr);
                if (dup2(_open_osfhandle((long)syslogPipe[1],
                                         _O_APPEND | _O_TEXT),
                         _fileno(stderr)) < 0)
                    ereport(FATAL,
                            (errcode_for_file_access(),
                             errmsg("could not redirect stderr: %m")));
                /* Now we are done with the write end of the pipe. */
                CloseHandle(syslogPipe[1]);
                syslogPipe[1] = 0;

One question about this: isn't this coding leaking a file descriptor?
That is, shouldn't we catch the result of _open_osfhandle and do a
CloseHandle on it after the dup2 step?

BTW, is it correct to use 0 as "invalid handle"?  Or should we be using
-1 or some such?

> Now, the pipe ReadFile will receive completely formatted data, which
> must be written binary (otherwise we will get CRCRLF), OTOH, the
> logger's calls to write_syslogger_file should write in text mode or
> replace \n by \r\n. Seems we need another function for elog to call.

Yeah.  What do you think is the most convenient way to do that?  I'd
be inclined to build a function that just expands \n to \r\n and then
calls write_syslogger_file, but maybe there's an easier way.

            regards, tom lane

Andreas Pflug <pgadmin@pse-consulting.de> writes:
> We could have *two* handles open to the syslogger file, but that's
> probably more fragile. Making the current write_syslogger_file static
> and providing another function that converts in a local buffer and
> calles write_syslogger_file seems best.

Agreed (although actually I was thinking of preserving
write_syslogger_file as the externally visible name and renaming the
existing function to a static "write_syslogger_file_binary" or some such
--- fewer files to touch that way).  Do you have time to write and check
this today?  I could write it but am not in a good position to test it.

            regards, tom lane

Andreas Pflug <pgadmin@pse-consulting.de> writes:
> Maybe we should consider examining GetLastError() and FormatMessage()
> (the equivalent of strerror, a sample is in the symlink/junction code)
> for %m under win32; these will work for standard posix operations also
> and might give much more detailed messages in many situations.

Hmm.  It's a bit attractive but it would break a lot of existing code
that assumes saving/restoring errno is a sufficient way to not clobber
the error result info between getting an error and reporting it.  For
instance, xlog.c has several repetitions of this pattern:

        errno = 0;
        if ((int) write(fd, zbuffer, sizeof(zbuffer)) != (int) sizeof(zbuffer))
        {
            int            save_errno = errno;

            /*
             * If we fail to make the file, delete it to release disk
             * space
             */
            unlink(tmppath);
            /* if write didn't set errno, assume problem is no disk space */
            errno = save_errno ? save_errno : ENOSPC;

            ereport(PANIC,
                    (errcode_for_file_access(),
                     errmsg("could not write to file \"%s\": %m", tmppath)));
        }

and I don't think I want to add Windows #ifdefs to every such place.

What would be more supportable is something that assigns a suitable
value to errno after a failure of a Windows-specific call.  Thus
something like

        if (!ReadFile(syslogPipe[0], logbuffer, sizeof(logbuffer),
                      &bytesRead, 0))
        {
            DWORD error = GetLastError();

            if (error == ERROR_HANDLE_EOF ||
                error == ERROR_BROKEN_PIPE)
                break;
            errno = Win32ErrorToErrno(error);
            ereport(LOG,
                    (errcode_for_file_access(),
                     errmsg("could not read from logger pipe: %m")));
        }

While this is admittedly ugly, it confines the ugliness to the fairly
small number of places where we call Windows-specific routines (ie,
we're already inside an #ifdef WIN32).  If we do the other, the
messiness is going to spread into what had been perfectly good
Posix-standard code.

            regards, tom lane

Andreas Pflug <pgadmin@pse-consulting.de> writes:
> The attached code snippet works. I hesitated to use palloc(count*2) to
> avoid problems in low mem conditions; might be paranoid.

No, I quite agree.  What we can do is adjust this to call
write_syslogger_file_binary more than once if the convbuf gets full;
then we can use a pretty small convbuf without fear.

Will fix and commit --- thanks!

            regards, tom lane

Tom Lane wrote:
> It might be best to just leave syslogFile open --- it should be properly
> flushed and closed by exit() anyway, no?

Agreed.

> Windows treats _IOLBF the same as _IOFBF, which we don't want.
> Okay, ifdef time.

:->

>>An observation I didn't track down so far:
>>Some LOG messages (e.g. the final logger shutdown, or "received fast
>>shutdown request") don't have proper CRLF line ending in win32, but only
>>LF.
>
>
> Weird.  No ideas about that.  Can you determine whether the data coming
> through the pipe has the problem?

It has, that's where I noticed. It is restricted to the postmaster and
the syslogger; all other processes ereport correctly.
Apparently, this is a mixture of binary and text file mode. Initially,
stderr is in text mode. When redirecting with dup2, it will be binary;
this must be corrected with

dup2(_open_osfhandle(...., _O_APPEND | _O_TEXT), ...


which solves the issue for postmaster. Child processes will have stderr
in text mode automatically, even if inherited and redirected into a pipe
(which is always binary).

Now, the pipe ReadFile will receive completely formatted data, which
must be written binary (otherwise we will get CRCRLF), OTOH, the
logger's calls to write_syslogger_file should write in text mode or
replace \n by \r\n. Seems we need another function for elog to call.

>
> Why doesn't it work?  Do we just need a different spelling of
> "/dev/null" for Windows?

Er, /dev/null? no such beast under win32.
Just checked, closing stderr works.


>
>>Finally, you don't seem to be a friend of a logfile rotation user
>>trigger...
>
>
> Nope, I'm not.  I think it's a bad solution to a nonexistent problem.
> The logger's control parameters are more than sufficient.  Furthermore,
> we'd really prefer that the logger doesn't ever crash (the restart
> business is a bit ticklish) and so the fewer features it has, the better.

It's no additional feature, it's just using an existing communication
path (signal), which has to be handled anyway, to setting an existing
flag. The code path for sighup is certainly much larger.

Regards,
Andreas

Tom Lane wrote:

> Perhaps we should insist that nothing in src/port can include postgres.h.
> If it needs something that's not in c.h, it doesn't belong in src/port.
>

This is just consequent; files using postgres.h should be under
src/backend/port, not src/port which might be used by frontend tools too.

Regards,
Andreas

Tom Lane wrote:
> Andreas Pflug <pgadmin@pse-consulting.de> writes:
>
>>             ereport(LOG,
>>                     (errcode_for_file_access(),
>>-                     errmsg("could not read from logger pipe: %m")));
>>+                     errmsg("could not read from logger pipe: %lu", error)));
>
>
> Does %m actually give a wrong result here?  Because if it does,
> errcode_for_file_access() is wrong too.

Yes, errno is not set. Same issue for CreatePipe; I missed that.

Maybe we should consider examining GetLastError() and FormatMessage()
(the equivalent of strerror, a sample is in the symlink/junction code)
for %m under win32; these will work for standard posix operations also
and might give much more detailed messages in many situations.

Regards,
Andreas

Tom Lane wrote:
>                 if (dup2(_open_osfhandle((long)syslogPipe[1],
>                                          _O_APPEND | _O_TEXT),
>                          _fileno(stderr)) < 0)
>                     ereport(FATAL,
>                             (errcode_for_file_access(),
>                              errmsg("could not redirect stderr: %m")));
>                 /* Now we are done with the write end of the pipe. */
>                 CloseHandle(syslogPipe[1]);
>                 syslogPipe[1] = 0;
>
> One question about this: isn't this coding leaking a file descriptor?
> That is, shouldn't we catch the result of _open_osfhandle and do a
> CloseHandle on it after the dup2 step?

Yes, it does.
int fd=_open_osfhandle(....);    // additional fdes from winhandle
_dup2(fd, ...)
close(fd);


>
> BTW, is it correct to use 0 as "invalid handle"?  Or should we be using
> -1 or some such?

0 is usually ok, valid handles are never 0. The official value is
INVALID_HANDLE_VALUE which expands to (HANDLE)-1.


>
>
>>Now, the pipe ReadFile will receive completely formatted data, which
>>must be written binary (otherwise we will get CRCRLF), OTOH, the
>>logger's calls to write_syslogger_file should write in text mode or
>>replace \n by \r\n. Seems we need another function for elog to call.
>
>
> Yeah.  What do you think is the most convenient way to do that?  I'd
> be inclined to build a function that just expands \n to \r\n and then
> calls write_syslogger_file, but maybe there's an easier way.

We could have *two* handles open to the syslogger file, but that's
probably more fragile. Making the current write_syslogger_file static
and providing another function that converts in a local buffer and
calles write_syslogger_file seems best.

Regards,
Andreas

Tom Lane wrote:
> Andreas Pflug <pgadmin@pse-consulting.de> writes:

> 
> Agreed (although actually I was thinking of preserving
> write_syslogger_file as the externally visible name and renaming the
> existing function to a static "write_syslogger_file_binary" or some such
> --- fewer files to touch that way).  Do you have time to write and check
> this today?  I could write it but am not in a good position to test it.

The attached code snippet works. I hesitated to use palloc(count*2) to 
avoid problems in low mem conditions; might be paranoid.

Regards,
Andreas

void
write_syslogger_file(const char *buffer, int count)
{
    char convbuf[1024+1], *p=convbuf;
    int i;
    
    for (i=0 ; *buffer != 0 && i < 1024 && count > 0 ; i++, count--)
    {
        if (*buffer == '\n')
        {
            i++;
            *p++ = '\r';
        }
        *p++ = *buffer++;
    }
    write_syslogger_file_binary(convbuf, i);
}

Managed to compile by adding #define FRONTEND to dirmod.c - not sure why I
didn't think to try that last night....

So, started up and tried to create a tablespace :

template1=# create tablespace big1 location 'c:/databases/pgtablespaces/big1';
ERROR:  tablespaces are not supported on this platform

...not quite what I expected. I did a cvs update -d -P this morning. I am on win
2000 with latest updates.

any ideas? (I will boot back into Freebsd and check I have the latest
updates...)

regards

Mark

Quoting Andreas Pflug <pgadmin@pse-consulting.de>:
> Mark Kirkwood wrote:
> >
> > ../../src/port/libpgport.a(dirmod.o)(.text+0x33a):dirmod.c: undefined
> > reference to `_imp__CurrentMemoryContext'
> >
>
>
> Apparently the reason is crossover referencing between libs, i.e.
> libpgport.a using libpostgres.a and vice versaa.

We have a symlink patch to make it work but it isn't in CVS yet.

---------------------------------------------------------------------------

markir@coretech.co.nz wrote:
> Managed to compile by adding #define FRONTEND to dirmod.c - not sure why I
> didn't think to try that last night....
>
> So, started up and tried to create a tablespace :
>
> template1=# create tablespace big1 location 'c:/databases/pgtablespaces/big1';
> ERROR:  tablespaces are not supported on this platform
>
> ...not quite what I expected. I did a cvs update -d -P this morning. I am on win
> 2000 with latest updates.
>
> any ideas? (I will boot back into Freebsd and check I have the latest
> updates...)
>
> regards
>
> Mark
>
> Quoting Andreas Pflug <pgadmin@pse-consulting.de>:
> > Mark Kirkwood wrote:
> > >
> > > ../../src/port/libpgport.a(dirmod.o)(.text+0x33a):dirmod.c: undefined
> > > reference to `_imp__CurrentMemoryContext'
> > >
> >
> >
> > Apparently the reason is crossover referencing between libs, i.e.
> > libpgport.a using libpostgres.a and vice versaa.
>
>
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
>                http://archives.postgresql.org
>

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

Sorry guys.... looks like I misunderstood.....I thought Tom had
committed the SYMLINK patch (and enabled tablespaces) as well as the
logging one!

markir@coretech.co.nz wrote:

>Managed to compile by adding #define FRONTEND to dirmod.c - not sure why I
>didn't think to try that last night....
>
>So, started up and tried to create a tablespace ....
>
>
>

Well, the only suitable punishment seemed to be to apply the patch and check it
out :-)

Looks good :

benchw=# create tablespace big1 location 'c:/databases/pgtablespaces/big1';
CREATE TABLESPACE
benche=# CREATE TABLE dim0 (
        d0key                          INTEGER NOT NULL,
        ddate                          DATE NOT NULL,
        dyr                            INTEGER NOT NULL,
        dmth                           INTEGER NOT NULL,
        dday                           INTEGER NOT NULL
) TABLESPACE big1;
CREATE
benchw=# COPY dim0 FROM 'c:/databases/dim0.dat'
USING DELIMITERS ',';
COPY

benchw=# analyze verbose dim0;
INFO:  analyzing "public.dim0"
INFO:  "dim0": scanned 74 of 74 pages, containing 10000 live rows and 0 dead row
s; 3000 rows in sample, 10000 estimated total rows
ANALYZE


regards

Mark

Quoting Bruce Momjian <pgman@candle.pha.pa.us>:
>
> We have a symlink patch to make it work but it isn't in CVS yet.

This seems to work well, for both destination types 'stderr' (with
redirect_stderr set), and 'eventlog'. The only minor gripe is that the
'eventlog' case is a bit laiden with extraneous DLL warnings.

The logger seems to shutdown cleanly, for both the case where pg_ctl is used,
and where the service manager is.

Here are some sample outputs:

i) Eventlog

Logging information gets into the event log ok: (including errors and shutdown
notification)

An error (select from non-existent table):

The description for Event ID ( 0 ) in Source ( PostgreSQL ) cannot be found.
The local computer may not have the necessary registry information or message
DLL files to display messages from a remote computer. The following information
is part of the event: ERROR:  relation "pg_classes" does not exist

Final shutdown:

The description for Event ID ( 0 ) in Source ( PostgreSQL ) cannot be found.
The local computer may not have the necessary registry information or message
DLL files to display messages from a remote computer. The following information
is part of the event: LOG:  database system is shut down


ii) Stderr

new files are created for seach startup, and file contents includes both error
and notification messages. Note that this case was shutdown with the service
manager (while I had an idle connection still open)

LOG:  database system was shut down at 2004-08-07 15:04:08 New Zealand Standard
Time
LOG:  checkpoint record is at 0/6A3FB998
LOG:  redo record is at 0/6A3FB998; undo record is at 0/0; shutdown TRUE
LOG:  next transaction ID: 545; next OID: 10047239
LOG:  database system is ready
ERROR:  relation "pg_classes" does not exist
LOG:  received fast shutdown request
LOG:  aborting any active transactions
FATAL:  terminating connection due to administrator command
LOG:  shutting down
LOG:  database system is shut down
LOG:  logger shutting down


regards

Mark

Quoting Tom Lane <tgl@sss.pgh.pa.us>:
> I have just committed a somewhat trimmed-down version of Andreas Pflug's
> syslogger patch.  It seems to work okay on Unix (with or without
> EXEC_BACKEND) but I'm not in a position to test it on Windows.  Would
> someone give it a try and report back?  Please check in particular that
> the logger shuts down cleanly after the postmaster is gone.

markir@coretech.co.nz writes:
> This seems to work well, for both destination types 'stderr' (with
> redirect_stderr set), and 'eventlog'. The only minor gripe is that the
> 'eventlog' case is a bit laiden with extraneous DLL warnings.

Just for clarity --- when did you pull the snapshot you're testing?
I made several commits this morning (US east coast time, ie, some eight
or ten hours ago now) in response to Andreas' early feedback.

The most certain way to say what you tested is to report the file
version number in the $PostgreSQL$ tag in
src/backend/postmaster/syslogger.c.

            regards, tom lane

I should have mentioned that, sorry. About 9:30 NZST (about 6.5 hours ago)
The tag for syslogger.c is:

$PostgreSQL: pgsql-server/src/backend/postmaster/syslogger.c,v 1.4 200
4/08/06 19:17:31 tgl Exp $


Quoting Tom Lane <tgl@sss.pgh.pa.us>:

> markir@coretech.co.nz writes:
> > This seems to work well, for both destination types 'stderr' (with
> > redirect_stderr set), and 'eventlog'. The only minor gripe is that the
> > 'eventlog' case is a bit laiden with extraneous DLL warnings.
>
> Just for clarity --- when did you pull the snapshot you're testing?
> I made several commits this morning (US east coast time, ie, some eight
> or ten hours ago now) in response to Andreas' early feedback.
>
> The most certain way to say what you tested is to report the file
> version number in the $PostgreSQL$ tag in
> src/backend/postmaster/syslogger.c.
>
>             regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 8: explain analyze is your friend
>

markir@coretech.co.nz writes:
> I should have mentioned that, sorry. About 9:30 NZST (about 6.5 hours ago)
> The tag for syslogger.c is:

> $PostgreSQL: pgsql-server/src/backend/postmaster/syslogger.c,v 1.4 200
> 4/08/06 19:17:31 tgl Exp $

Okay, that's CVS tip from my perspective too.  Anyone have comments
about why the eventlog log is so noisy?  It's outside my competence...

            regards, tom lane

Presumably this is because I compiled from source rather than using the Pg
Installer?

Quoting Andreas Pflug <pgadmin@pse-consulting.de>:
>
> This looks like an installation problem; I don't see that on my machine.
>
> The eventlog will receive an eventID (postgresql uses only 0), which is
> an index into a message table provided by the service (it's a binary
> resource). If that message is not present, or if the message provider
> isn't registered for the server, the text mentioned is displayed.
> For pgsql, this message is just "%s", to use it generically.
>
> To check if the dll is registered correctly:
>
> You should have
>
>
[HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Eventlog\Application\PostgreSQL]
> "EventMessageFile"="C:\\Program Files\\ PostgreSQL\\7.5\\lib\\pgevent.dll"
>
> or another valid pgevent.dll path. Adding this might need a machine
> restart (yes, it's win...)
>
> I noticed a trailing '.' on a single line in every eventlog entry, the
> attached patch will remove this.
>
> Regards,
> Andreas
>

Tom Lane wrote:

> 
> 
> Okay, that's CVS tip from my perspective too.  Anyone have comments
> about why the eventlog log is so noisy?  It's outside my competence...


This looks like an installation problem; I don't see that on my machine.

The eventlog will receive an eventID (postgresql uses only 0), which is 
an index into a message table provided by the service (it's a binary 
resource). If that message is not present, or if the message provider 
isn't registered for the server, the text mentioned is displayed.
For pgsql, this message is just "%s", to use it generically.

To check if the dll is registered correctly:

You should have

[HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Eventlog\Application\PostgreSQL] 
"EventMessageFile"="C:\\Program Files\\ PostgreSQL\\7.5\\lib\\pgevent.dll"

or another valid pgevent.dll path. Adding this might need a machine 
restart (yes, it's win...)

I noticed a trailing '.' on a single line in every eventlog entry, the 
attached patch will remove this.

Regards,
Andreas
Index: pgmsgevent.mc
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/bin/pgevent/pgmsgevent.mc,v
retrieving revision 1.1
diff -u -r1.1 pgmsgevent.mc
--- pgmsgevent.mc    20 Jun 2004 01:32:49 -0000    1.1
+++ pgmsgevent.mc    7 Aug 2004 08:44:06 -0000
@@ -1,5 +1,5 @@
 MessageId=0
 SymbolicName=PGWIN32_EVENTLOG_MSG
 Language=English
-%1.
+%1
 .

Andreas Pflug <pgadmin@pse-consulting.de> writes:
> I noticed a trailing '.' on a single line in every eventlog entry, the
> attached patch will remove this.

This kinda bears out Peter's point about not wanting binary files in
CVS: I can install the patch as given, but I don't think it will fix
anything.  May I trouble you for updated copies of the derived files
in src/bin/pgevent?

            regards, tom lane

markir@coretech.co.nz wrote:
> Presumably this is because I compiled from source rather than using the Pg
> Installer?

Yes, probably. I'd recommend using the installer and continuing from
source then.

Regards,
Andreas

Andreas Pflug <pgadmin@pse-consulting.de> writes:
> I noticed a trailing '.' on a single line in every eventlog entry, the
> attached patch will remove this.

Applied.

            regards, tom lane

Tom Lane wrote:
> Andreas Pflug <pgadmin@pse-consulting.de> writes:
>
>>I noticed a trailing '.' on a single line in every eventlog entry, the
>>attached patch will remove this.
>
>
> This kinda bears out Peter's point about not wanting binary files in
> CVS: I can install the patch as given, but I don't think it will fix
> anything.  May I trouble you for updated copies of the derived files
> in src/bin/pgevent?

No problem.
Unfortunately, AFAIK there's no way around this binary resource to
address the eventlog service.
There are more issues about eventlog, but we can leave that for
after-beta-1 time.

Regards,
Andreas