Re: Key management with tests

Hi,

On 2021-01-15 16:47:19 -0500, Bruce Momjian wrote:
> On Fri, Jan 15, 2021 at 04:23:22PM -0500, Robert Haas wrote:
> > On Fri, Jan 15, 2021 at 3:49 PM Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > I don't think that's appropriate. Several prominent community members
> > have told you that the patch, as committed the first time, needed a
> > lot more work. There hasn't been enough time between then and now for
> > you, or anyone, to do that amount of work. This patch needs detailed
> > and substantial review from senior community members, and multiple
> > rounds of feedback and improvement, before it should be considered for
> > commit.
> >
> > I am not even sure there is a consensus on the design, without which
> > any commit is always premature.
>
> If people want changes, I need to hear about it here. I have address
> everything people have mentioned in these threads so far.

I don't even know how anybody is supposed to realistically review the
design or the patch:

This thread started at
https://postgr.es/m/20210101045047.GB30966%40momjian.us - there's no
reference to any discussion of the design at all and the supposed links
to code are dead.

The last version of the code that I see posted ([1]), has the useless
commit message of "key squash commit" - nothing else. There's no design
documentation included in the patch either, as far as I can tell.

Manually searching for the topic brings me to
https://www.postgresql.org/message-id/20201202213814.GG20285%40momjian.us
, a thread of 52 messages, which provides a bit more context, but
largely just references another thread and a wiki article. The link to
the other thread is into the middle of a 112 message thread.

The wiki page doesn't really describe a design either. It has a very
long todo, a bunch of implementation details, but no design.

Nor did 978f869b99 include much in the way of design description.

You cannot expect anybody to review a patch if developing some basic
understanding of the intended design requires reading hundreds of
messages in which the design evolved. And I don't think it's acceptable
to push it due to lack of further feedback, given this situation - the
lack of design description is a blocker in itself.

There's a few things that stand out on a very very brief scan:
- the patch badly needs to be split up into independently reviewable
pieces
- tests:
- wait, a .sh test script? No, we shouldn't add any more of those,
they're nightmare across platforms
- Do the tests actually do anything useful? It's not clear to me what
they are trying to achieve. En/Decrypting test vectors doesn't seem to
buy that much?
- the new pg_alterckey is completely untested
- the pg_upgrade paths is untested
- ..
- Without further comment BootStrapKmgr() does "copy cluster file
encryption keys from an old cluster?", but there's no explanation as
to why / when that's the case. Presumably pg_upgrade, but, uh, explain
that.

- pg_alterckey.c
- appears to create it's own cluster lock file, using its
own routine for doing so. How does that lock file interact with the
running server?
- retrieve_cluster_keys() is missing (void).

I think this is at the very least a month away from being committable,
even if the design were completely correct (which I do not know, see
above).

Greetings,

Andres Freund

[1] https://www.postgresql.org/message-id/20210115204926.GD8740%40momjian.us