Hi,

On 2017-06-27 14:59:18 -0400, Tom Lane wrote:
> Here's a draft patch for that.  I quite like the results --- this seems
> way simpler and more reliable than what pg_ctl has done up to now.

Yea, I like that too.


> However, it's certainly arguable that this is too much change for an
> optional post-beta patch.

Yea, I think there's a valid case to be made for that. I'm still
inclined to go along with this, it seems we're otherwise just adding
complexity we're going to remove in a bit again.


> If we decide that it has to wait for v11,
> I'd address Jeff's complaint by hacking the loop behavior in
> test_postmaster_connection, which'd be ugly but not many lines of code.

Basically increasing the wait time over time?


> Note that I followed the USE_SYSTEMD patch's lead as to where to report
> postmaster state changes.  Arguably, in the standby-server case, we
> should not report the postmaster is ready until we've reached consistency.
> But that would require additional signaling from the startup process
> to the postmaster, so it seems like a separate change if we want it.

I suspect we're going to want to add more states to this over time, but
as you say, there's no need to hurry.


>       /*
> +      * Report postmaster status in the postmaster.pid file, to allow pg_ctl 
> to
> +      * see what's happening.  Note that all strings written to the status 
> line
> +      * must be the same length, per comments for AddToDataDirLockFile().  We
> +      * currently make them all 8 bytes, padding with spaces.
> +      */
> +     AddToDataDirLockFile(LOCK_FILE_LINE_PM_STATUS, "starting");

The 8-space thing in multiple places is a bit ugly.  How about having a
a bunch of constants declared in one place? Alternatively make it
something like: status: $c, where $c is a one character code for the
various states?


> @@ -1149,8 +1149,9 @@ TouchSocketLockFiles(void)
>   *
>   * Note: because we don't truncate the file, if we were to rewrite a line
>   * with less data than it had before, there would be garbage after the last
> - * line.  We don't ever actually do that, so not worth adding another kernel
> - * call to cover the possibility.
> + * line.  While we could fix that by adding a truncate call, that would make
> + * the file update non-atomic, which we'd rather avoid.  Therefore, callers
> + * should endeavor never to shorten a line once it's been written.
>   */
>  void
>  AddToDataDirLockFile(int target_line, const char *str)
> @@ -1193,19 +1194,26 @@ AddToDataDirLockFile(int target_line, co
>       srcptr = srcbuffer;
>       for (lineno = 1; lineno < target_line; lineno++)
>       {
> -             if ((srcptr = strchr(srcptr, '\n')) == NULL)
> -             {
> -                     elog(LOG, "incomplete data in \"%s\": found only %d 
> newlines while trying to add line %d",
> -                              DIRECTORY_LOCK_FILE, lineno - 1, target_line);
> -                     close(fd);
> -                     return;
> -             }
> -             srcptr++;
> +             char       *eol = strchr(srcptr, '\n');
> +
> +             if (eol == NULL)
> +                     break;                          /* not enough lines in 
> file yet */
> +             srcptr = eol + 1;
>       }
>       memcpy(destbuffer, srcbuffer, srcptr - srcbuffer);
>       destptr = destbuffer + (srcptr - srcbuffer);
>  
>       /*
> +      * Fill in any missing lines before the target line, in case lines are
> +      * added to the file out of order.
> +      */
> +     for (; lineno < target_line; lineno++)
> +     {
> +             if (destptr < destbuffer + sizeof(destbuffer))
> +                     *destptr++ = '\n';
> +     }
> +
> +     /*
>        * Write or rewrite the target line.
>        */
>       snprintf(destptr, destbuffer + sizeof(destbuffer) - destptr,
> "%s\n", str);

Not this patches fault, and shouldn't be changed now, but this is a
fairly weird way to manage and update this file.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to