Re: Reducing pg_ctl's reaction time

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