From: Robbie Harwood <rharwood@redhat.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Michael Paquier <michael.paquier@gmail.com>,
        PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH v12] GSSAPI encryption support
In-Reply-To: <20160405184346.GA317471@alvherre.pgsql>
References: <20160405184346.GA317471@alvherre.pgsql>
User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1
 (x86_64-pc-linux-gnu)
Date: Tue, 05 Apr 2016 16:12:51 -0400
Message-ID: <jlgfuuzstks.fsf@thriss.redhat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> Robbie Harwood wrote:
>> Michael Paquier <michael.paquier@gmail.com> writes:
>>=20
>> > On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood <rharwood@redhat.com> w=
rote:
>> >> 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.
>>=20
>> Yeah, it's definitely worth thinking/talking about; this came up in IRC
>> discussion as well.
>>=20
>> 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.

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJXBBxDAAoJECUy+RdqlaRCvtgQAKq3JlPqdhuV0JQegSmKSfdr
kiko0QseIQ76xXQ8HsNPwrqmWj9W84I6TwMRQ7RLYwtvpWJ+ia7x7AFf7QUupesw
PPPSV63mSBAeMIPNapgBeF8Nzqr5opU5lmAPKqOkd0yRM/TN0erb0sfuuXwaWjw8
rhExauKcyU+m1QNyaQA05bQzxUf3ixdL1HrzwpJfAmDCsyh5Kfdmg6SXFbVwfC/S
qs2RAEePg5q/xSloPkiPS/kdEfARNnblCJBd7RfnOz5jB8ePaMOa79B0vIc98bvs
nrZHlcTPrx04RFQ9TenS8yvPcHxoDnMPrn8CsO791Q9TsaydKFr1Gkce2ZSHMPN5
LpNE6D7J3Y0135VZ0SQBCryBCankoEAL5eUd9kk1J0RnDCw/uXiIK449KbU6C2UQ
lVLR9aXFWyJE6u3Q7DuNPIF0TkDRn4G6zHrMIcaVvsdzcnQ3uNbiV3XCP5vUUYGT
aNZBdNatbCzwRJLz0ZGNS5lHekW0kiqO1bwK33IXrASSrAjjmi9fdzt9cZi33xnl
1KuiqN4P4nKL8srP2j0ASFkj6LuOF0tXkW5aVRC7usBgC9+6V3VZKt6IogchkFiQ
F5Y08e4UqmysBV8e6BY5yRF8mJn7V8ed8ConxjBsqo1Xn4IZEXjRsAIz/u4pgBrm
3iGMDEOYjx6w6jmzMSoT
=oKE7
-----END PGP SIGNATURE-----
--=-=-=--