Josh Berkus <josh@agliodbs.com> writes:
> For hackers who don't understand security frameworks, I'm going to make a
> strong case for KaiGai's patch.
> ...
> So it would be much better to have this functionality be "mainstream"
> rather than a fork. If it does get bounced, please do it becuase of code
> quality and not because "nobody is asking for this".
Well, let's be perfectly blunt here: this is an extremely sizable patch
that will be of interest to less than 1% of our users (and I think I'm
being generous in that estimate). If it's going to get in, it's going
to have to not cost us anything much on reliability, maintainability,
or performance --- rank those however you please, there is someone out
there who will be most interested in any one of them.
My look at the code today convinced me that it's not going to get
applied without a whole lot of further work. I think what we must
figure out in this commit-fest is whether to encourage KaiGai to start
doing that work, or to decide that it probably is a dead issue and he
shouldn't bother.
As to reliability: the issue that worries me the most was already raised
by Greg Smith --- this patch puts security checks and consequent catalog
lookups into some mighty low-level places that IMHO have no business
triggering catalog accesses. If I'd been able to make the patch work
today, I'd have tested whether it could get through the regression tests
with CLOBBER_CACHE_ALWAYS defined. I'd be happier if it were refactored
to put the security checks only into executor code paths, and not try to
enforce security restrictions against the system itself. (Thought
experiment: what happens if SELinux denies access to a row of
pg_security to the code that is supposed to be checking security?
Or try this one: it looks to me like you can set up the system to
pretend to some user that the pg_class rows for certain indexes don't
exist, even though he has update rights on their parent table. Instant
index corruption.)
As to maintainability: I already posted a lot of unhappy remarks on code
style and readability. I don't think there's anything unfixable there,
but there's a lot of work to do to convert what was evidently written as
an arms-length add-on into a sensible part of core Postgres.
As to performance: frankly, I'm afraid the performance is abysmal.
This was what I was mainly hoping to check out if I could have gotten
it to build today. We need to at least get some rough pgbench readings
before deciding whether it's worth pushing forward.
regards, tom lane
