Thread: AcquireRewriteLocks/acquireLocksOnSubLinks vs. rowsecurity
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
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
* 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
Attachment
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
* 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
Attachment
* 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