Re: [PATCH] Reworks for Access Control facilities (r2311)

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] Reworks for Access Control facilities (r2311)
Date: 2009-09-29 01:33:37
Message-ID: 20090929013337.GK17756@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> BTW, I raised a few issues. Do you have any opinions?

Certainly, though they're my opinions and I don't know if the committers
will agree, but I suspect they will.

> * deployment of the source code
>
> The current patch implements all the access control abstractions at the
> src/backend/security/access_control.c. Its size is about 4,500 lines
> which includs source comments.
> It is an approach to sort out a series of functionalities into a single
> big file, such as aclchk.c. One other approach is to put these codes in
> the short many files deployed in a directory, such as backend/catalog/*.
> Which is the preferable in PostgreSQL?

A single, larger file, as implemented, is preferable, I believe.

> * pg_class_ownercheck() in EnableDisableRule()
>
> As I mentioned in the another message, pg_class_ownercheck() in the
> EnableDisableRule() is redundant.
>
> The EnableDisableRule() is called from ATExecEnableDisableRule() only,
> and it is also called from the ATExecCmd() with AT_EnableRule,
> AT_EnableAlwaysRule, AT_EnableReplicaRule and AT_DisableRule.
> In this path, ATPrepCmd() already calls ATSimplePermissions() which
> also calls pg_class_ownercheck() for the target.
>
> I don't think it is necessary to check ownership of the relation twice.
> My opinion is to remove the checks from the EnableDisableRule() and
> the ac_rule_toggle() is also removed from the patch.
> It does not have any compatibility issue.
>
> Any comments?

I agree that we don't need to check the ownership twice. You might
check if there was some history to having both checks (perhaps there was
another code path before which didn't check before calling
EnableDisableRule()?). I'd feel alot more comfortable removing the
check if we can show why it was there originally as well as why it's not
needed now.

I'm working on a more comprehensive review, but wanted to answer these
questions first.

Thanks,

Stephen

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message KaiGai Kohei 2009-09-29 02:19:00 Re: [PATCH] Reworks for Access Control facilities (r2311)
Previous Message Itagaki Takahiro 2009-09-29 01:11:19 Re: Buffer usage in EXPLAIN and pg_stat_statements (review)