Lists: | pgsql-hackers |
---|
From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | pgsql-hackers(at)postgresql(dot)org, Stephen Frost <sfrost(at)snowman(dot)net> |
Subject: | AcquireRewriteLocks/acquireLocksOnSubLinks vs. rowsecurity |
Date: | 2015-08-27 12:49:31 |
Message-ID: | 20150827124931.GD15922@awork2.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Hi,
The locking around rowsecurity policy expressions seems to be
insufficient:
SELECT * FROM document WHERE f_leak(dtitle) ORDER BY did;
WARNING: RelationIdGetRelation(247984) without holding lock on the relation
WARNING: relation_open(247984, NoLock) of relation "uaccount" without previously held lock
I don't know the relevant code well. But as far as I can see that's
because normally the expectation is that relevant locks have either been
taken by the parser or by AcquireRewriteLocks(). But before
static Query *
fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
{
...
/*
* Fetch any new security quals that must be applied to this RTE.
*/
get_row_security_policies(parsetree, parsetree->commandType, rte,
rt_index, &securityQuals, &withCheckOptions,
&hasRowSecurity, &hasSubLinks);
if (securityQuals != NIL || withCheckOptions != NIL)
{
...
if (hasSubLinks)
{
...
expression_tree_walker((Node *) securityQuals,
fireRIRonSubLink, (void *) activeRIRs);
...
}
rte->securityQuals = list_concat(securityQuals,
rte->securityQuals);
neither will have acquired relevant locks. The parser because it doesn't
know about rowsecurity, AcquireRewriteLocks/acquireLocksOnSubLinks
because rte->securityQuals wan't even set and range_table_walker() uses
that.
Istmt that something like
context.for_execute = true;
acquireLocksOnSubLinks((Node *) securityQuals, &context);
acquireLocksOnSubLinks((Node *) withCheckOptions, &context);
needs to be added to that code.
Greetings,
Andres Freund
From: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Stephen Frost <sfrost(at)snowman(dot)net> |
Subject: | Re: AcquireRewriteLocks/acquireLocksOnSubLinks vs. rowsecurity |
Date: | 2015-08-27 18:00:18 |
Message-ID: | CAEZATCUc9sB4vJoqNJwexWa6PaAxDZX7oCaBL_FgLn6a4BK1HA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On 27 August 2015 at 13:49, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Hi,
>
> The locking around rowsecurity policy expressions seems to be
> insufficient:
> SELECT * FROM document WHERE f_leak(dtitle) ORDER BY did;
> WARNING: RelationIdGetRelation(247984) without holding lock on the relation
> WARNING: relation_open(247984, NoLock) of relation "uaccount" without previously held lock
>
> I don't know the relevant code well. But as far as I can see that's
> because normally the expectation is that relevant locks have either been
> taken by the parser or by AcquireRewriteLocks(). But before
>
> static Query *
> fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
> {
> ...
> /*
> * Fetch any new security quals that must be applied to this RTE.
> */
> get_row_security_policies(parsetree, parsetree->commandType, rte,
> rt_index, &securityQuals, &withCheckOptions,
> &hasRowSecurity, &hasSubLinks);
>
> if (securityQuals != NIL || withCheckOptions != NIL)
> {
> ...
> if (hasSubLinks)
> {
> ...
> expression_tree_walker((Node *) securityQuals,
> fireRIRonSubLink, (void *) activeRIRs);
> ...
> }
>
> rte->securityQuals = list_concat(securityQuals,
> rte->securityQuals);
>
> neither will have acquired relevant locks. The parser because it doesn't
> know about rowsecurity, AcquireRewriteLocks/acquireLocksOnSubLinks
> because rte->securityQuals wan't even set and range_table_walker() uses
> that.
>
> Istmt that something like
> context.for_execute = true;
> acquireLocksOnSubLinks((Node *) securityQuals, &context);
> acquireLocksOnSubLinks((Node *) withCheckOptions, &context);
> needs to be added to that code.
>
Yes, I think you're right. It needs to happen before fireRIRonSubLink,
and only if hasSubLinks is true.
Regards,
Dean
From: | Stephen Frost <sfrost(at)snowman(dot)net> |
---|---|
To: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: AcquireRewriteLocks/acquireLocksOnSubLinks vs. rowsecurity |
Date: | 2015-08-28 12:49:24 |
Message-ID: | 20150828124924.GT3685@tamriel.snowman.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 27 August 2015 at 13:49, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > The locking around rowsecurity policy expressions seems to be
> > insufficient:
> > SELECT * FROM document WHERE f_leak(dtitle) ORDER BY did;
> > WARNING: RelationIdGetRelation(247984) without holding lock on the relation
> > WARNING: relation_open(247984, NoLock) of relation "uaccount" without previously held lock
[...]
> > Istmt that something like
> > context.for_execute = true;
> > acquireLocksOnSubLinks((Node *) securityQuals, &context);
> > acquireLocksOnSubLinks((Node *) withCheckOptions, &context);
> > needs to be added to that code.
>
> Yes, I think you're right. It needs to happen before fireRIRonSubLink,
> and only if hasSubLinks is true.
Attached appears to fix this for the RLS case from my testing.
Any comments?
Barring concerns, I'll push this later today and back-patch to 9.5.
Thanks!
Stephen
Attachment | Content-Type | Size |
---|---|---|
fix-rls-locking.patch | text/x-diff | 1.8 KB |
From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Stephen Frost <sfrost(at)snowman(dot)net> |
Cc: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: AcquireRewriteLocks/acquireLocksOnSubLinks vs. rowsecurity |
Date: | 2015-08-28 12:53:34 |
Message-ID: | 20150828125334.GB4857@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On 2015-08-28 08:49:24 -0400, Stephen Frost wrote:
>
> + /*
> + * get_row_security_policies just added to securityQuals and/or
> + * withCheckOptions, and there were SubLinks, so make sure
> + * we lock any relations which were added as a result.
> + */
Very minor comment: Strictly speaking the quals/wces haven't yet been
added to the Query, that happens only few lines down. I think it makes
sense to mention that we normally rely on the parser to acquire locks,
but that can't work here since sec quals/wces aren't visible to the
parser.
Greetings,
Andres Freund
From: | Stephen Frost <sfrost(at)snowman(dot)net> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: AcquireRewriteLocks/acquireLocksOnSubLinks vs. rowsecurity |
Date: | 2015-08-28 13:03:22 |
Message-ID: | 20150828130321.GU3685@tamriel.snowman.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
* Andres Freund (andres(at)anarazel(dot)de) wrote:
> On 2015-08-28 08:49:24 -0400, Stephen Frost wrote:
> >
> > + /*
> > + * get_row_security_policies just added to securityQuals and/or
> > + * withCheckOptions, and there were SubLinks, so make sure
> > + * we lock any relations which were added as a result.
> > + */
>
> Very minor comment: Strictly speaking the quals/wces haven't yet been
> added to the Query, that happens only few lines down. I think it makes
> sense to mention that we normally rely on the parser to acquire locks,
> but that can't work here since sec quals/wces aren't visible to the
> parser.
Ok, I'll add a comment to that effect.
Thanks!
Stephen
From: | Stephen Frost <sfrost(at)snowman(dot)net> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: AcquireRewriteLocks/acquireLocksOnSubLinks vs. rowsecurity |
Date: | 2015-08-28 15:13:00 |
Message-ID: | 20150828151300.GJ3685@tamriel.snowman.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
* Andres Freund (andres(at)anarazel(dot)de) wrote:
> On 2015-08-28 08:49:24 -0400, Stephen Frost wrote:
> >
> > + /*
> > + * get_row_security_policies just added to securityQuals and/or
> > + * withCheckOptions, and there were SubLinks, so make sure
> > + * we lock any relations which were added as a result.
> > + */
>
> Very minor comment: Strictly speaking the quals/wces haven't yet been
> added to the Query, that happens only few lines down. I think it makes
> sense to mention that we normally rely on the parser to acquire locks,
> but that can't work here since sec quals/wces aren't visible to the
> parser.
Better?
Thanks!
Stephen
Attachment | Content-Type | Size |
---|---|---|
fix-rls-locking-v2.patch | text/x-diff | 1.9 KB |
From: | Stephen Frost <sfrost(at)snowman(dot)net> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: AcquireRewriteLocks/acquireLocksOnSubLinks vs. rowsecurity |
Date: | 2015-08-28 15:41:04 |
Message-ID: | 20150828154104.GK3685@tamriel.snowman.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
* Andres Freund (andres(at)anarazel(dot)de) wrote:
> On 2015-08-28 08:49:24 -0400, Stephen Frost wrote:
> >
> > + /*
> > + * get_row_security_policies just added to securityQuals and/or
> > + * withCheckOptions, and there were SubLinks, so make sure
> > + * we lock any relations which were added as a result.
> > + */
>
> Very minor comment: Strictly speaking the quals/wces haven't yet been
> added to the Query, that happens only few lines down. I think it makes
> sense to mention that we normally rely on the parser to acquire locks,
> but that can't work here since sec quals/wces aren't visible to the
> parser.
Pushed.
Will work on the rewriteTargetView fix.
Thanks!
Stephen