Обсуждение: Re: [GENERAL] pg crashing

[moving over to hackers]

Tom Lane wrote:
> BTW, just looking at win32_shmem.c ...
>
>     retptr = malloc(bufsize + 1 + 18);    /* 1 NULL and 18 for
>                                            * Global\PostgreSQL: */
>     if (retptr == NULL)
>         elog(FATAL, "could not allocate memory for shared memory name");
>
>     strcpy(retptr, "Global\\PostgreSQL:");
>     r = GetFullPathName(DataDir, bufsize, retptr + 11, NULL);
>
> Surely that "11" ought to be "18".  Also, since the loop immediately

Yes. Very true. It still *works*, since it guts off on the proper side
of the \, but it still makes sense.

> below this is going to convert \ to /, wouldn't it be clearer to
> describe the path prefix as Global/PostgreSQL: in the first place?

Eh, that shows another bug I think. It should *not* convert the \ in
"Global\", because that one is is interpreted by the Win32 API call!

I think it should be per this patch. Seems right?


> (BTW, as far as I can tell the +1 added to the malloc request is
> useless: bufsize includes the trailing null, and the code would
> not work if it did not.)

Yeah, also true.

//Magnus
Index: win32_shmem.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/port/win32_shmem.c,v
retrieving revision 1.4
diff -c -r1.4 win32_shmem.c
*** win32_shmem.c    1 Jan 2008 19:45:51 -0000    1.4
--- win32_shmem.c    2 Jul 2008 16:44:40 -0000
***************
*** 47,64 ****
          elog(FATAL, "could not get size for full pathname of datadir %s: %lu",
               DataDir, GetLastError());

!     retptr = malloc(bufsize + 1 + 18);    /* 1 NULL and 18 for
                                           * Global\PostgreSQL: */
      if (retptr == NULL)
          elog(FATAL, "could not allocate memory for shared memory name");

      strcpy(retptr, "Global\\PostgreSQL:");
!     r = GetFullPathName(DataDir, bufsize, retptr + 11, NULL);
      if (r == 0 || r > bufsize)
          elog(FATAL, "could not generate full pathname for datadir %s: %lu",
               DataDir, GetLastError());

!     for (cp = retptr; *cp; cp++)
          if (*cp == '\\')
              *cp = '/';

--- 47,64 ----
          elog(FATAL, "could not get size for full pathname of datadir %s: %lu",
               DataDir, GetLastError());

!     retptr = malloc(bufsize  + 18);        /* 1 NULL and 18 for
                                           * Global\PostgreSQL: */
      if (retptr == NULL)
          elog(FATAL, "could not allocate memory for shared memory name");

      strcpy(retptr, "Global\\PostgreSQL:");
!     r = GetFullPathName(DataDir, bufsize, retptr + 18, NULL);
      if (r == 0 || r > bufsize)
          elog(FATAL, "could not generate full pathname for datadir %s: %lu",
               DataDir, GetLastError());

!     for (cp = retptr + 18; *cp; cp++)
          if (*cp == '\\')
              *cp = '/';

Magnus Hagander <magnus@hagander.net> writes:
>> below this is going to convert \ to /, wouldn't it be clearer to
>> describe the path prefix as Global/PostgreSQL: in the first place?

> Eh, that shows another bug I think. It should *not* convert the \ in
> "Global\", because that one is is interpreted by the Win32 API call!

I was wondering about that.  What are the implications of that?

> I think it should be per this patch. Seems right?

Pls fix the comment on the malloc, too.
        regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>>> below this is going to convert \ to /, wouldn't it be clearer to
>>> describe the path prefix as Global/PostgreSQL: in the first place?
> 
>> Eh, that shows another bug I think. It should *not* convert the \ in
>> "Global\", because that one is is interpreted by the Win32 API call!
> 
> I was wondering about that.  What are the implications of that?

First, that the name isn't nicely readable when browsing with Process
Explorer. Second, that they will all go in the local namespace, which
means you can in theory start two postmasters in the same directory from
different terminal server sessions (this was the way it was on 8.2
already, it's a new feature for 8.3 that simply didn't work)



>> I think it should be per this patch. Seems right?
> 
> Pls fix the comment on the malloc, too.

Right, will do and commit.

//Magnus