From: Tom Lane <tgl@sss.pgh.pa.us>
To: KaiGai Kohei <kaigai@ak.jp.nec.com>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: robertmhaas@gmail.com
Cc: pgsql-hackers@postgresql.org
Cc: kaigai@kaigai.gr.jp
Subject: Re: Reworks for Access Control facilities (r2363)
Date: Thu, 15 Oct 2009 13:22:02 -0400
Message-ID: <10495.1255627322@sss.pgh.pa.us> (raw)
In-Reply-To: <4AD54082.9050001@ak.jp.nec.com>
References: <4AC5A14F.6050306@ak.jp.nec.com>
<20091012024604.GW17756@tamriel.snowman.net>
<4AD54082.9050001@ak.jp.nec.com>
KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
> [ patch r2363 ]
I promised I would review this today, but I just can't make myself do it
in any detail. This is too large, too ugly, and it is going in a
direction that I do not like or want to spend any of my time on.
The overwhelming impression after a brief read-through is that the
code has been hacked apart with a chainsaw and reassembled into a
Frankenstein's monster --- it's alive, but man is it ugly. Code
comments that refer to something "above" or "below" are still there,
but the referent is no longer close enough for that to be a reasonable
way of referring to it. It's impossible to follow what's going on or
why, either in the shim functions or in the callers --- in the original
coding there was context for the aclcheck calls, now there isn't.
I don't have any confidence that this is a sane way to proceed forward.
The shim layer knows everything about everything --- there may still
be a few backend .h files it doesn't include, but that's not for lack
of trying. The direction this is heading in is an unmaintainable
giant-bowl-of-spaghetti security module, rather than something that can
be divided into understandable parts. And I don't think it's really
removed any complexity from the callers, nor do I believe that it's
going to be a useful basis for imposing a different security policy
than the one we have now. Two specific examples of why not:
* The "skip permissions checks" arguments that have been added to
various random functions suggest strongly that the factoring still isn't
right --- I especially don't believe in that in the context of
performDeletion and friends.
* There are two special-purpose shims, ac_database_calculate_size and
ac_tablespace_calculate_size, that got added for the benefit of
utils/adt/dbsize.c. What if that code were still in contrib? How is it
different from a lot of the code that is in contrib now, eg dblink or
pgrowlocks, to say nothing of third-party modules? Presuming that the
shim layer can know explicitly about each individual permission-checking
requirement is a dead-end design.
Maybe if I weren't burned out after a month of CommitFesting, I could
muster a more positive reaction, but right now I just can't summon any
enthusiasm for this.
regards, tom lane
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: tgl@sss.pgh.pa.us, kaigai@ak.jp.nec.com, sfrost@snowman.net, robertmhaas@gmail.com, kaigai@kaigai.gr.jp
Subject: Re: Reworks for Access Control facilities (r2363)
In-Reply-To: <10495.1255627322@sss.pgh.pa.us>
* 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