Re: Key management with tests - Mailing list pgsql-hackers

Hi,

On 2021-01-15 19:21:32 -0500, Bruce Momjian wrote:
> On Fri, Jan 15, 2021 at 02:37:56PM -0800, Andres Freund wrote:
> > On 2021-01-15 16:47:19 -0500, Bruce Momjian wrote:
> > > > 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.
> 
> You have to understand cryptography and Postgres internals to understand
> the design, and I don't think it is realistic to explain that all to the
> community.  We did much of this in voice calls over months because it
> was too much of a burden to explain all the cryptographic details so
> everyone could follow along.

I think that's not at all acceptable. I don't mind hashing out details
on calls / off-list, but the design needs to be public, documented, and
reviewable.  And if it's something the community can't understand, then
it can't get in. We're going to have to maintain this going forward.

I don't mean to say that we need to re-hash all design details from
scratch - but that there needs to be an explanation somewhere that
describes what's being done on a medium-high level, and what drove those
design decisions.


> > 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.
> 
> I am not sure what design document you are requesting.  I thought the
> TODO was that.

The TODO in https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Other_requirements
is a design document?



> > 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.
> 
> OK, I will just move on to something else then.  It is not worth the
> feature to go into that kind of discussion again.  I am willing to have
> voice calls with individuals to explain the logic, but repeatedly
> explaining it to the entire group I find unproductive.  I don't think
> another 400-email thread would help anyone.

Explaining something over voice doesn't help with people in a year or
five trying to understand the code and the design, so they can adapt it
when making half-related changes. Nor do I see why another 400 email
thread would be a necessary consequence of you explaining the design
that you came up with.

This isn't specific to this topic? I don't really understand why this
specific feature gets to avoid normal community development processes?



> > - tests:
> >   - wait, a .sh test script? No, we shouldn't add any more of those,
> >     they're nightmare across platforms
> 
> The script originatad from pg_upgrade.  I don't know how to do things
> like initdb and stuff another way, at least in our code.

We have had perl tap tests for quite a while now? And all new tests that
aren't regression / isolation tests are expected to be written in it.

Greetings,

Andres Freund