Re: [PATCH v12] GSSAPI encryption support

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:

> Robbie Harwood wrote:
>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>
>> > On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>> >> Here's v12, both here and on my github:
>> >> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt12
>
>> > So you are saving everything in the top memory context. I am fine to
>> > give the last word to a committer here but I would just go with
>> > calloc/free to simplify those hunks.
>>
>> Yeah, it's definitely worth thinking/talking about; this came up in IRC
>> discussion as well.
>>
>> If we go the memory context route, it has to be TopMemoryContext since
>> nothing else lives long enough (i.e., entire connection).
> [...]
>> It turns out that's not actually required, but could easily be made
>> explicit here. According to the README for the memory context system,
>> pfree() and repalloc() do not require setting CurrentMemoryContext
>> (since 7.1).
>
> It seems to me that the right solution for this is to create a new
> memory context which is a direct child of TopMemoryContext, so that
> palloc can be used, and so that it can be reset separately, and that it
> doesn't suffer from resets of other contexts. (I think Michael's point
> is that if those chunks were it a context of their own, you wouldn't
> need the pfrees but a MemCxtReset would be enough.)

Hmm, that's also an option. I read Michael's point as arguing for
calloc()/free() rather than a new context, but I could be wrong.

A question, though: it it valuable for the context to be reset()able
separately? If there were more than just these two buffers going into
it, I could see it being convenient - especially if it were for
different encryption types, for instance - but it seems like it would be
overkill?

This is all new to me so I may be entirely mistaken though.

>> Side note: it's really irritating to work with having this file under
>> version control because of how different it ends up being when I
>> autoreconf (which I need to do because I'm changing the build system).
>> (I'm also idly curious what system/autotools version is generating this
>> file because it doesn't match any that I tried.)
>
> We use stock autoconf from the GNU package and it definitely produces
> matching output for me. Maybe your Linux distro includes a patched
> version? I know Debian does, but I suppose you're using some Redhat
> thing, so no idea.

Hmm, that explains the Debian behavior I was seeing (it does the
above). The Fedora one adds a couple blank lines in places but it's
much less... gratuitous... in its changes.