AcquireRewriteLocks/acquireLocksOnSubLinks vs. rowsecurity

On 27 August 2015 at 13:49, Andres Freund <andres@anarazel.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

On 27 August 2015 at 13:49, Andres Freund <andres@anarazel.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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Andres Freund (andres@anarazel.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

* Andres Freund (andres@anarazel.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

* Andres Freund (andres@anarazel.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