From: Robbie Harwood <rharwood@redhat.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Michael Paquier <michael.paquier@gmail.com>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH v12] GSSAPI encryption support
Date: Tue, 05 Apr 2016 16:12:51 -0400
Message-ID: <jlgfuuzstks.fsf@thriss.redhat.com> (raw)
In-Reply-To: <20160405184346.GA317471@alvherre.pgsql>
References: <20160405184346.GA317471@alvherre.pgsql>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Robbie Harwood wrote:
>> Michael Paquier <michael.paquier@gmail.com> writes:
>>
>> > On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood <rharwood@redhat.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.
Attachments:
signature.asc (application/pgp-signature, 818B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: rharwood@redhat.com, alvherre@2ndquadrant.com, michael.paquier@gmail.com
Subject: Re: [PATCH v12] GSSAPI encryption support
In-Reply-To: <jlgfuuzstks.fsf@thriss.redhat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox