Re: AcquireRewriteLocks/acquireLocksOnSubLinks vs. rowsecurity

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