Re: [HACKERS] Reducing pg_ctl's reaction time - Mailing list pgsql-hackers

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