Postgres Pro
Downloads
Home   >   mailing lists

Thread: Create/alter policy and exclusive table lock

Create/alter policy and exclusive table lock

From
Konstantin Knizhnik
Date:
Hi hackers,

Right now changing policies (create/alter policy statements) requires 
exclusive lock of target table:

     /* Get id of table.  Also handles permissions checks. */
     table_id = RangeVarGetRelidExtended(stmt->table, AccessExclusiveLock,
                                         0,
                                         RangeVarCallbackForPolicy,
                                         (void *) stmt);

Unfortunately there are use cases where policies are changed quite 
frequently and this exclusive lock becomes a bottleneck.
I wonder why do we really need exclusive lock here?
Policies are stored in pg_policy table and we get  RowExclusiveLock on it.

May be I missed something, but why we can not rely on standard MVCC 
visibility rules for pg_policy table?
Until transaction executing CREATE/ALTER POLICY is committed, other 
transactions will not see its changes in pg_policy table and perform
RLS checks according to old policies. Once transaction is committed, 
everybody will switch to new policies.

I wonder if we it is possible to replace AccessExclusiveLock with 
AccessSharedLock in RangeVarGetRelidExtended in CreatePolicy and 
AlterPolicy?

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Create/alter policy and exclusive table lock

From
Tom Lane
Date:
Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
> Right now changing policies (create/alter policy statements) requires 
> exclusive lock of target table:

Yup.

> I wonder why do we really need exclusive lock here?

Because it affects the behavior of a SELECT.

> May be I missed something, but why we can not rely on standard MVCC 
> visibility rules for pg_policy table?

We cannot have a situation where the schema details of a table might
change midway through planning/execution of a statement.  The results
are unlikely to be as clean as "you get either the old behavior or the
new one", because that sequence might examine the details more than
once.  Also, even if you cleanly get the old behavior, that's hardly
satisfactory.  Consider

Session 1                        Session 2

begin;
alter policy ... on t1 ...;
insert new data into t1;

                                 begin planning SELECT on t1;

commit;

                                 begin executing SELECT on t1;

With your proposal, session 2 would see the new data in t1
(because the executor takes a fresh snapshot) but it would not
be affected by the new policy.  That's a security failure,
and it's one that does not happen today.

            regards, tom lane

Re: Create/alter policy and exclusive table lock

From
Konstantin Knizhnik
Date:

On 14.01.2020 17:40, Tom Lane wrote:
> Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
>> Right now changing policies (create/alter policy statements) requires
>> exclusive lock of target table:
> Yup.
>
>> I wonder why do we really need exclusive lock here?
> Because it affects the behavior of a SELECT.
>
>> May be I missed something, but why we can not rely on standard MVCC
>> visibility rules for pg_policy table?
> We cannot have a situation where the schema details of a table might
> change midway through planning/execution of a statement.  The results
> are unlikely to be as clean as "you get either the old behavior or the
> new one", because that sequence might examine the details more than
> once.  Also, even if you cleanly get the old behavior, that's hardly
> satisfactory.  Consider
>
> Session 1                        Session 2
>
> begin;
> alter policy ... on t1 ...;
> insert new data into t1;
>
>                                   begin planning SELECT on t1;
>
> commit;
>
>                                   begin executing SELECT on t1;
>
> With your proposal, session 2 would see the new data in t1
> (because the executor takes a fresh snapshot) but it would not
> be affected by the new policy.  That's a security failure,
> and it's one that does not happen today.

Thank you for explanation.
But let me ask you one more question: why do we obtaining snapshot twice 
in exec_simple_query:
first for analyze (pg_analyze_and_rewrite) and one for execution 
(PortalStart)?
GetSnapshotData is quite expensive operation and the fact that we are 
calling it twice for each query execution (with read committed isolation 
level)
seems to be strange. Also the problem in scenario you have described 
above is caused by using different snapshots by planner and executor. If 
them will share the same snapshot,
then there should be no security violation, right?

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Create/alter policy and exclusive table lock

From
Tom Lane
Date:
Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
> But let me ask you one more question: why do we obtaining snapshot twice 
> in exec_simple_query:
> first for analyze (pg_analyze_and_rewrite) and one for execution 
> (PortalStart)?

That would happen anyway if the plan is cached.  If we were to throw away
all plan caching and swear a mighty oath that we'll never put it back,
maybe we could build in a design assumption that planning and execution
use identical snapshots.  I doubt that would lead to a net win though.

Also note that our whole approach to cache invalidation is based on the
assumption that if session A needs to see the effects of session B,
they will be taking conflicting locks.  Otherwise sinval signaling
is not guaranteed to be detected at the necessary times.

            regards, tom lane