Re: Key management with tests

From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: Tom Kincaid <tomjohnkincaid(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Stephen Frost <sfrost(at)snowman(dot)net>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>
Subject: Re: Key management with tests
Date: 2021-01-26 00:09:44
Message-ID: 20210126000944.GA18075@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, Jan 25, 2021 at 08:12:01PM -0300, Álvaro Herrera wrote:
> In patch 1,
>
> * The docs are not clear on what happens if --auth-prompt is not given
> but an auth prompt is required for the program to work. Should it exit
> with a status other than 0?

Uh, I think the docs talk about this:

It can prompt from the terminal if
option>--authprompt</option> is used. In the parameter
value, <literal>%R</literal> is replaced by a file descriptor
number opened to the terminal that started the server. A file
descriptor is only available if enabled at server start via
<option>-R</option>. If <literal>%R</literal> is specified and
no file descriptor is available, the server will not start.

The code is:

case 'R':
{
char fd_str[20];

if (terminal_fd == -1)
{
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
errmsg("cluster key command referenced %%R, but --authprompt not specified")));
}

Does that help?

> * BootStrapKmgr claims it is called by initdb, but that doesn't seem to
> be the case.

Well, initdb starts the postmaster in --boot mode, and that calls
BootStrapKmgr(). Does that help?

> * Also, BootStrapKmgr is the only one that checks USE_OPENSSL; what if a
> with-openssl build inits the datadir, and then a non-openssl runs it?
> What if it's the other way around? I think you'd get a failure in
> stat() ...

Wow, I never considered that. I have added a check to InitializeKmgr().
Thanks.

> * ... oh, KMGR_DIR_PID is used but not defined anywhere. Is it defined
> in some later commit? If so, then I think you've chosen to split the
> patch series wrong.

OK, fixed. It is in include/common/kmgr_utils.c, which was in #3.

> May I suggest to use "git format-patch" to produce the patch files? When
> working with a series like this, trying to do patch handling manually
> like you seem to be doing, is much more time-consuming and error prone.
> For example, with a branch containing individual commits, you could use
> git rebase -i origin/master -x "make install check-world"
> or similar, so that each commit is built and tested individually.

I used "git format-patch". Are you asking for seven commits that then
generate seven files via one format-patch run? Or is the primary issue
that you want compile testing for each patch?

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2021-01-26 01:22:42 Re: Tid scan improvements
Previous Message David G. Johnston 2021-01-25 23:52:44 Re: About to add WAL write/fsync statistics to pg_stat_wal view