Howard Chu wrote: > Johan wrote: >> Hello all, > >> We have an OpenLDAP instance proxying an active directory with back_meta >> and> mr_passthru. >> We also have pcache on top, and as it do not support >> LDAP_MATCHING_RULE_IN_CHAIN, I looked about implementing it. > >> I found that we could retrieve cached results with pcacheQueryID, but I can't >> get why this is not used in the current code? > > It is of course used in the current code, for refreshing and expiring queries.
>> Plesae see attached patch Please read https://openldap.org/devel/programming.html > --- servers/slapd/overlays/pcache.c.ori 2023-02-04 10:56:07.910235675 > +0100 > +++ servers/slapd/overlays/pcache.c 2023-02-04 10:57:14.319386931 +0100 > @@ -53,6 +53,16 @@ > */ > #define PCACHE_MONITOR > > +// Not cacheable reasons > +#define PCACHEMAXQUERIES_REACHED 0x1 > +#define PCACHEATTRSONLY 0x2 > +#define PCACHENOTEMPLATE 0x4 > +#define PCACHENOATTRSET 0x8 These values should be aligned. OpenLDAP source uses 4-column hard tabs for indents. > + > +// https://ldapwiki.com/wiki/LDAP_MATCHING_RULE_IN_CHAIN This URL is currently unreachable. Probably should link to MSDN instead. > +#define LDAP_MATCHING_RULE_IN_CHAIN "1.2.840.113556.1.4.1941" > + > + > /* query cache structs */ > /* query */ > > @@ -512,7 +522,7 @@ > int attr_cnt; > int t_cnt = 0; > struct berval bv; > - char *p1, *p2; > + char *p1, *p2, *p3; > AttributeDescription *ad; > AttributeName *attrs; > > @@ -532,9 +542,18 @@ > p2 = strchr( p1, '=' ); > if ( !p2 ) > break; > - if ( p2[-1] == '<' || p2[-1] == '>' ) p2--; > - bv.bv_val = p1; > - bv.bv_len = p2 - p1; > + // Incase of extended operation > + p3 = strchr( p1, ':' ); > + if ( p3 && p3 < p2 ) { > + // FIXME: Is this valid syntax : > "member:1.2.840.113556.1.4.1941:>=3" ? Read RFC4515. > + if ( p3[-1] == '<' || p3[-1] == '>' ) p3--; > + bv.bv_val = p1; > + bv.bv_len = p3 - p1; > + } else { > + if ( p2[-1] == '<' || p2[-1] == '>' ) p2--; > + bv.bv_val = p1; > + bv.bv_len = p2 - p1; > + } > ad = NULL; > i = slap_bv2ad( &bv, &ad, text ); > if ( i ) { > @@ -1136,7 +1155,6 @@ > return ret; > } > > - > static struct berval* > merge_init_final(Operation *op, struct berval* init, struct berval* any, > struct berval* final) > @@ -1342,8 +1360,13 @@ > case LDAP_FILTER_LE: > mrule = > fs->f_ava->aa_desc->ad_type->sat_ordering; > break; > + case LDAP_FILTER_EXT: > + if (0 == strncmp(fi->f_mr_rule_text.bv_val, > LDAP_MATCHING_RULE_IN_CHAIN, fi->f_mr_rule_text.bv_len)) { Probably should define the OID in a berval and use ber_bvcmp. We don't use "if (0 == xx)" we use "if (!xx)". > + mrule = > fs->f_ava->aa_desc->ad_type->sat_equality; > + } > + break; > default: > - mrule = NULL; > + mrule = NULL; > } > if (mrule) { > const char *text; > @@ -1394,6 +1417,7 @@ > fi=fi->f_next; > break; > case LDAP_FILTER_EQUALITY: > + case LDAP_FILTER_EXT: > if (ret == 0) > res = 1; > fs=fs->f_next; > @@ -1941,6 +1965,19 @@ > fstr->bv_len += len; > break; > > + case LDAP_FILTER_EXT: > + // Concatenate attribute name > + if ( f->f_mr_desc ) { > + ad = f->f_mr_desc; > + len = STRLENOF( "(::=)" ) + ad->ad_cname.bv_len + > f->f_mr_rule_text.bv_len; > + ret = snprintf( fstr->bv_val+fstr->bv_len, len + 1, > "(%s:%s:=)", ad->ad_cname.bv_val, f->f_mr_rule_text.bv_val ); > + assert( ret == len ); > + fstr->bv_len += len; > + } else { > + return -1; > + } Again, read RFC4515. > + break; > + > case LDAP_FILTER_AND: > case LDAP_FILTER_OR: > case LDAP_FILTER_NOT: { > @@ -2955,17 +2992,22 @@ > { > slap_overinst *on = (slap_overinst *)op->o_bd->bd_info; > cache_manager *cm = on->on_bi.bi_private; > - query_manager* qm = cm->qm; > + query_manager* qm = cm->qm; Don't make gratuitous whitespace changes. > > int i = -1; > > Query query; > QueryTemplate *qtemp = NULL; > - bindinfo *pbi = NULL; > + bindinfo *pbi = NULL; > > - int attr_set = -1; > - CachedQuery *answerable = NULL; > - int cacheable = 0; > + int attr_set = -1; > + CachedQuery *answerable = NULL; > + int cacheable = 0; > + > + char *fstr; > + Filter *f; > + > + int ncachereason = 0; > > struct berval tempstr; > > @@ -3051,6 +3093,10 @@ > if (answerable) > break; > } > + if (cacheable == 0) > + ncachereason |= PCACHENOTEMPLATE; > + } else { > + ncachereason |= PCACHENOATTRSET; > } > op->o_tmpfree( tempstr.bv_val, op->o_tmpmemctx ); > } > @@ -3098,7 +3144,16 @@ > } > } > } > + > + /* Replace original query with query to cache ID */ > + fstr = op->o_tmpalloc( > sizeof("(pcacheQueryID=12345678-abcd-1234-abcd-123456789abc)")+1, > op->o_tmpmemctx ); > + sprintf(fstr, "(pcacheQueryID=%s)", > answerable->q_uuid.bv_val); > + Debug( pcache_debug, "Replace search filter with %s\n", > fstr, 0, 0 ); No. The fact the query is answerable doesn't mean you can replace the user's filter. The user's filter may be more specific than the cached set, and will only return a subset of entries. This code will break such queries. > + > + f = str2filter(fstr); > + op->oq_search.rs_filter = f; > i = cm->db.bd_info->bi_op_search( op, rs ); > + op->o_tmpfree( fstr, op->o_tmpmemctx ); > } > ldap_pvt_thread_rdwr_wunlock(&answerable->rwlock); > /* locked by qtemp->qcfunc (query_containment) */ > @@ -3112,11 +3167,14 @@ > ldap_pvt_thread_mutex_lock(&cm->cache_mutex); > if (cm->num_cached_queries >= cm->max_queries) { > cacheable = 0; > + ncachereason |= PCACHEMAXQUERIES_REACHED; > } > ldap_pvt_thread_mutex_unlock(&cm->cache_mutex); > > - if (op->ors_attrsonly) > + if (op->ors_attrsonly) { > cacheable = 0; > + ncachereason |= PCACHEATTRSONLY; > + } > > if (cacheable) { > slap_callback *cb; > @@ -3170,7 +3228,27 @@ > } > > } else { > - Debug( pcache_debug, "QUERY NOT CACHEABLE\n" ); > + switch (ncachereason) { You defined the ncachereason as a set of bitflags. You can't use a switch() statement here, it will just hit default if more than 1 bit is set, which defeats the purpose of tracking any reasons. > + case PCACHEMAXQUERIES_REACHED: > + Debug( pcache_debug, "QUERY NOT CACHEABLE (max_entries > reached)\n", > + 0, 0, 0); > + break; > + case PCACHEATTRSONLY: > + Debug( pcache_debug, "QUERY NOT CACHEABLE (attrs > only)\n", > + 0, 0, 0); > + break; > + case PCACHENOTEMPLATE: > + Debug( pcache_debug, "QUERY NOT CACHEABLE (No > template)\n", > + 0, 0, 0); > + break; > + case PCACHENOATTRSET: > + Debug( pcache_debug, "QUERY NOT CACHEABLE (No > Attrset)\n", > + 0, 0, 0); > + break; > + default: > + Debug( pcache_debug, "QUERY NOT CACHEABLE (unkown > reason)\n", > + 0, 0, 0); > + } > } > > return SLAP_CB_CONTINUE; -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
