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