GH Issue 1512: user defined queries causes performance issues with lookup field query listing

[XingY]

Rationale

Related Pull Requests

• GH Issue 1512: user defined queries causes performance issues with lookup field query listing labkey-api-js#226
• GH Issue 1512: user defined queries causes performance issues with lookup field query listing labkey-ui-components#2085
• GH Issue 1512: user defined queries causes performance issues with lookup field query listing #8119
• https://github.com/LabKey/limsModules/pull/2513

Changes

• Add an includeUserQueriesForLookups parameter to the getQueries API: when includeUserQueries is false, restrict the returned user-defined queries to those that expose a primary key.
• Treat a query as a lookup target only when it has a real primary key — ignore synthetic/auto-added key columns, and exclude queries that fail to resolve.
• Cache each user query's has-primary-key result, keyed on the query's last-modified stamp, so repeat enumerations skip re-resolving queries already known to lack a key.

[labkey-nicka]

Caching query-based objects has always seemed a little fraught so I'm somewhat skeptical here. That said, I think this is workable. We should consider how to get automated test coverage for this.

[labkey-nicka]

I would expect this comment describe the cache and purpose for the cache more directly. This would describe what it is keyed by and what the value represents.

[labkey-nicka]

the DAY TTL bounds the one stale case, a source table's PK changing with no query edit.

When would this happen? I'm skeptical of leaning on this as the only uncaching behavior.

[labkey-nicka]

Claude Finding 1 — High — The cache key uses the folder that defines the query

QueryDefinitionImpl.java:832

Issue: The key uses _queryDef.getContainerId(), the folder where the query is defined. But the query is evaluated in a different folder:

• QueryServiceImpl.getAllQueryDefs builds each CustomQueryDefinitionImpl with the requesting folder (QueryServiceImpl.java:809, and the inherited and /Shared loops after it).
• getTable(schema, null, true) (QueryController.java:7042) resolves the SQL against the requesting folder's schema.

As a result, inheritable queries and /Shared queries share one cache entry across every folder that uses them, even though the tables they read, and those tables' keys, can differ by folder.

Failure scenario:

1. A project-level inheritable query P is SELECT * FROM <per-folder source>.
2. The source has no key in folder B but does have one in folder A.
3. Listing lookups in B caches FALSE under P's single key.
4. Folder A then skips P at QueryController.java:6975.

This produces a wrong value, not just a stale one. The result flips depending on which folder wrote last, so the one-day lifetime doesn't fix it.

Fix: Key on getContainer().getId(), the folder that resolves the query.

[labkey-matthewb]

This my first thought when I heard this described. Because query text can be shared across containers, the result of compiling the same query can be different. Using the container of the schema that resolved the query is correct.

[XingY]

Container fixed.

[labkey-nicka]

Claude Finding 2 — High — Nothing ever removes entries, and a cached FALSE corrects itself only when the query's own row changes

QueryDefinitionImpl.java:115 (the cache), QueryController.java:6975 (the skip)

Issue: No code ever removes an entry. Several things change whether a query has a key without changing that query's own Modified:

• Chained queries. A referenced user query is inlined into the outer query (sql/Query.java). The outer query's key comes from its single source's key (QuerySelect.getKeyColumns, QuerySelect.java:1950-1975).
  1. Create A = SELECT Name FROM lists.Foo, which has no key.
  2. Create B = SELECT * FROM A.
  3. Open the field designer and pick that schema for a lookup. B is cached FALSE.
  4. Edit A to SELECT Key, Name FROM lists.Foo.
  5. A now appears in the designer, but B stays hidden for up to 24 hours.
• Metadata on the source table. Metadata for a source table is stored in separate QueryDef rows (customQuery=false) or in the external schema's metadata, not on the user query's row. The standard way to make a keyless external view usable as a lookup is to add <isKeyField>true</isKeyField> to that view's metadata. The dependent user query stays hidden, so the fix looks broken.
• External and linked schema reloads (QueryManager.java:428-447), and edits to linked-schema templates.
• Module .query.xml metadata. Which modules are enabled in a folder can change key fields through overlayMetadata. This is a niche case.

The comment's claims that the result is "structural" and that a changed source key is "the one stale case" are both wrong.

Fix:

• Add a static method that calls HAS_PK_COLUMN_CACHE.clear().
• Call it from QueryDefCache.uncache(Container) (QueryDefCache.java:168). Every QueryManager insert, update, renameQuery, renameSchema and delete already goes through that method, and so does containerDeleted (QueryManager.java:135-175, 535-547). That covers every QueryDef row change, including saves that change only metadata.
• Also call it from QueryManager.updateExternalSchemas and reloadExternalSchema.
• Clearing the whole cache is fine for a cache that exists only for speed, and it avoids tracking which queries depend on which.

Trap: a QueryChangeListener alone is not enough. QueryDefinitionImpl.setMetadataXml records no property change (see its "CONSIDER: Add metadata QueryPropertyChange" note), so a metadata-only save fires no change event.

[XingY]

Wired up clearHasPkColumnCache at various places

[labkey-nicka]

Claude Finding 3 — Medium — The cache only helps queries with no key

QueryController.java:6972-6977, QueryDefinitionImpl.java:827-832

Issue: Several kinds of query get nothing from the cache:

• Queries that have a key. A cached TRUE is never read, so these are fully resolved on every request. They still pay for the detail-column JSON (JsonWriter.getNativeColProps), because the client sends queryDetailColumns: true and leaves includeColumns at its default of true (ui-components domainproperties/actions.ts).
• File-based module queries and linked-schema queries. ModuleQueryDef.toQueryDef and ModuleQueryMetadataDef.toQueryDef never set Modified, and neither does LinkedSchemaQueryDefinition. The cache key is therefore null and these are never cached.
• Queries that fail to parse. These get a null table or an exception, so they are never cached and are re-parsed every time.
• The first request after a restart, an edit, or the one-day expiry costs the same as before the PR.

A query instance's own _cache doesn't help either: getQueryDefs builds new instances on every request.

Impact: A schema made mostly of module .sql queries (common in LIMS/EHR), or mostly of queries that do have keys, gets no speed-up. Keyless queries (joins, GROUP BY, DISTINCT, aggregates) are probably the expensive ones, so the gain is real. Still, measure it against the GH Issue 1512 data before taking on the cache's complexity.

Suggestion: Consider whether lookup mode needs full detail columns at all. If the client can pass includeColumns=false, that probably saves more than the cache does.

[XingY]

Unfortunately, includeColumns=false (or queryDetailColumns=false) doesn't work. It returns queries with no usable key info, so the dropdown comes back empty.

Change made to handle file-based module queries caching.

You're right that the cache only short-circuits the no-PK case.