[Rt-devel] [PATCH] More ACL checks from cache

[Jesse Vincent]

On Fri, Apr 21, 2006 at 12:44:33AM +0400, Ruslan Zakirov wrote:
> Hello.
> Here is patch that cut number of the queries we run on the DB by using
> cache more aggressivly.

Any basic stats on how effective this is?  I think I'm most comfortable
starting with this in 3.7, since 3.6 is so close to going out the door.

I know it's pedantic, but can this get turned into something that reads
a little easier to someone poking at the code?

(also $big_hashkey doesn't tell me what it's for, just that it's large
;)


> +    $_ACL_CACHE->set( $big_hashkey => $hitcount? 1:-1 );

> It's against 3.4.5
> 
> --
> Best regards, Ruslan.

> === lib/RT/Principal_Overlay.pm
> ==================================================================
> --- lib/RT/Principal_Overlay.pm	(revision 2396)
> +++ lib/RT/Principal_Overlay.pm	(local)
> @@ -328,47 +328,40 @@
>          # this is a little bit hacky, but basically, now that we've done
>          # the ticket roles magic, we load the queue object
>          # and ask all the rest of our questions about the queue.
> -        push( @{ $args{'EquivObjects'} }, $args{'Object'}->QueueObj );
> +        unshift @{ $args{'EquivObjects'} }, $args{'Object'}->QueueObj;
>  
>      }
>  
> -    # {{{ If we've cached a win or loss for this lookup say so
> +    unshift @{ $args{'EquivObjects'} }, $RT::System
> +        unless $self->can('_IsOverrideGlobalACL')
> +               && $self->_IsOverrideGlobalACL( $args{'Object'} );
>  
> -    # {{{ Construct a hashkey to cache decisions in
> -    my $hashkey = do {
> -        no warnings 'uninitialized';
>  
> -        # We don't worry about the hash ordering, as this is only
> -        # temporarily used; also if the key changes it would be
> -        # invalidated anyway.
> -        join(
> -            ";:;",
> -            $self->Id,
> -            map {
> -                $_,    # the key of each arguments
> -                  ( $_ eq 'EquivObjects' )    # for object arrayref...
> -                  ? map( _ReferenceId($_), @{ $args{$_} } )    # calculate each
> -                  : _ReferenceId( $args{$_} )    # otherwise just the value
> -              } keys %args
> -        );
> -    };
> +    # {{{ If we've cached a win or loss for this lookup say so
>  
> -    # }}}
> +    # Construct a hashkeys to cache decisions in
> +    my $self_id = $self->id;
> +    my $big_hashkey = join ";:;", $self_id, $args{'Right'};
> +    foreach ( @{ $args{'EquivObjects'} } ) {
> +        my $ref_id = _ReferenceId($_);
> +        $big_hashkey .= ";:;$ref_id";
>  
> -    # Returns undef on cache miss
> -    my $cached_answer = $_ACL_CACHE->fetch($hashkey);
> -    if ( defined $cached_answer ) {
> -        if ( $cached_answer == 1 ) {
> -            return (1);
> -        }
> -        elsif ( $cached_answer == -1 ) {
> -            return (undef);
> -        }
> +        my $hashkey = join ";:;", $self_id, $args{'Right'}, $ref_id;
> +        my $cached_answer = $_ACL_CACHE->fetch($hashkey);
> +        return $cached_answer > 0 if defined $cached_answer;
>      }
>  
> -    my $hitcount = $self->_HasRight( %args );
> +    {
> +        my $cached_answer = $_ACL_CACHE->fetch($big_hashkey);
> +        return $cached_answer > 0 if defined $cached_answer;
> +    }
>  
> -    $_ACL_CACHE->set( $hashkey => $hitcount? 1:-1 );
> +
> +    my ($hitcount, $via_obj) = $self->_HasRight( %args );
> +
> +    $_ACL_CACHE->set( $big_hashkey => $hitcount? 1:-1 );
> +    $_ACL_CACHE->set( "$self_id;:;$args{'Right'};:;$via_obj" => $hitcount? 1:-1 ) if $via_obj;
> +
>      return ($hitcount);
>  }
>  
> @@ -393,10 +386,6 @@
>  
>      # If an object is defined, we want to look at rights for that object
>  
> -    push( @objects, 'RT::System' )
> -      unless $self->can('_IsOverrideGlobalACL')
> -             && $self->_IsOverrideGlobalACL( $args{Object} );
> -
>      my ($check_roles, $check_objects) = ('','');
>      if( @objects ) {
>          my @role_clauses;
> @@ -423,10 +412,11 @@
>      }
>  
>      my $query_base =
> -      "SELECT ACL.id from ACL, Groups, Principals, CachedGroupMembers WHERE  " .
> +      "SELECT ACL.id, ACL.ObjectType, ACL.ObjectId " .
> +      "FROM ACL, Groups, Principals, CachedGroupMembers WHERE " .
>  
>        # Only find superuser or rights with the name $right
> -      "(ACL.RightName = 'SuperUser' OR  ACL.RightName = '$right') "
> +      "(ACL.RightName = 'SuperUser' OR ACL.RightName = '$right') "
>  
>        # Never find disabled groups.
>        . "AND Principals.Disabled = 0 "
> @@ -454,18 +444,25 @@
>        . "AND ACL.PrincipalId = Principals.id "
>        . "AND ACL.PrincipalType = 'Group' ";
>  
> -    $self->_Handle->ApplyLimits( \$groups_query, 1 ); #only return one result
> -    my $hitcount = $self->_Handle->FetchResult($groups_query);
> -    return 1 if $hitcount; # get out of here if success
> +    {
> +        # fetch only first result
> +        $self->_Handle->ApplyLimits( \$groups_query, 1 );
> +        my ($hitcount, $obj, $id) = $self->_Handle->FetchResult($groups_query);
> +        $obj = "$obj-$id" if $id;
> +        return (1, $obj) if $hitcount; # get out of here if success
> +    }
>  
>      # The roles query does the query based on roles
>      my $roles_query = $query_base
>        . "AND ACL.PrincipalType = Groups.Type "
>        . "AND ($check_roles) ";
> -    $self->_Handle->ApplyLimits( \$roles_query, 1 ); #only return one result
>  
> -    $hitcount = $self->_Handle->FetchResult($roles_query);
> -    return 1 if $hitcount; # get out of here if success
> +    {
> +        # fetch only first result
> +        $self->_Handle->ApplyLimits( \$roles_query, 1 );
> +        my ($hitcount, $obj, $id) = $self->_Handle->FetchResult($roles_query);
> +        return (1) if $hitcount; # get out of here if success
> +    }
>  
>      return 0;
>  }

> _______________________________________________
> List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel
> 
> Best Practical is hiring! Come hack Perl for us: http://bestpractical.com/about/jobs.html

--