Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace btree with btree_gin indexes for PostgreSQL to allow larger metadata and faster nested search #588

Merged
merged 14 commits into from
Nov 14, 2023

Conversation

Kezzsim
Copy link
Contributor

@Kezzsim Kezzsim commented Oct 24, 2023

The title explains it all, this PR is intended to have the following effects:

  • Enable the pg ingestion of real bluesky runs from existing beamline datasets which can occasionally have very large metadata, previously when attempting to do this using the default btree index type the operation would fail with an error.
  • Enable the eq binary operation for queries like .search(Key("start.purpose") == "test") to utilize the new index for rapid traversals, greatly reducing the time needed to query when searching for nested strings in metadata objects.

Caveats:

  • Currently the btree_gin index is not being invoked for all other binary operations including ne,lt,le,gt,ge leading to slow seq scans in those conditions. Hypothetically this can be achieved but there is no known way currently.
  • The eq operator is technically being converted to the postgresql @> operator which is technically an IN, this may have unintended side effects.

@danielballan
Copy link
Member

Point of clarification: in the end, was the issue about the size of a given key or was it the overall size of the document? I thought it seemed to be the former but turned out to be the latter, but I could be misremembering or behind. Worth nailing that down here for the record in case we revisit later.

elif dialect_name == "postgresql" and operation == operator.eq:
condition = orm.Node.metadata_.op("@>")(
type_coerce(
{keys[0]: reduce(lambda x, y: {y: x}, keys[1:][::-1], query.value)},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever. Can you refactor this into a utility function so that it can be precisely targeted with a unit test? It might also end up being reused in the future, elsewhere.

tiled/catalog/adapter.py Outdated Show resolved Hide resolved
@danielballan
Copy link
Member

CI shows that this test is failing:

async def test_metadata_index_is_used(a):

I think this test (mine) was not the right approach. The query planner's internal details are slippery, and may depend on the number of records, perhaps the available system resources, the version of Postgres or SQLite...

I think it would be best to develop a separate test harness, which may or may not use pytest, to evaluate index usage. Like performance benchmarks, this is just a different category of test and does not sit well inside the unit test suite. I think I'd be in favor of just ripping this out for now, but I'm open to alternatives.

@danielballan
Copy link
Member

@jmaruland Your (C2QA) use case is one motivation for this fix. At your convenience, would you check out this PR branch locally, from @Kezzsim's fork, start a temp catalog server, and test that you can insert your data?

@Kezzsim
Copy link
Contributor Author

Kezzsim commented Oct 25, 2023

Point of clarification: in the end, was the issue about the size of a given key or was it the overall size of the document? I thought it seemed to be the former but turned out to be the latter, but I could be misremembering or behind. Worth nailing that down here for the record in case we revisit later.

Technically it isn't key related, it's overall metadata size related. Here's an example error from importing a bluesky run:

sqlalchemy.exc.DBAPIError: (sqlalchemy.dialects.postgresql.asyncpg.Error) <class 'asyncpg.exceptions.ProgramLimitExceededError'>: index row size 2792 exceeds btree version 4 maximum 2704 for index "top_level_metadata"
DETAIL:  Index row references tuple (40,10) in relation "nodes".
HINT:  Values larger than 1/3 of a buffer page cannot be indexed.
Consider a function index of an MD5 hash of the value, or use full text indexing.
[SQL: INSERT INTO nodes (key, ancestors, structure_family, metadata, specs) VALUES ($1::VARCHAR, $2::JSONB, $3::structurefamily, $4::JSONB, $5::JSONB) RETURNING nodes.id, nodes.time_created, nodes.time_updated]
[parameters: ('b603b6ae-5cd8-43d7-b340-b7d8678ad7c4', '[]', 'container', '{"start":{"time":1527797320.2107823,"uid":"b603b6ae-5cd8-43d7-b340-b7d8678ad7c4","motors":["dwell_time_dwell_time","dcm_energy"],"XDI,Mono,name":"Si( ... (5836 characters truncated) ... mp":1527797320.2107823,"datetime":"2018-05-31T20:08:40.210782+00:00","plan_name":"scan_nd","stream_names":["primary"],"duration":178.91345500946045}}', '[]')]
(Background on this error at: https://sqlalche.me/e/20/dbapi)

I will remove the term keys from the PR title and description for clarity

@Kezzsim Kezzsim changed the title Replace btree with btree_gin indexes for PostgreSQL to allow larger metadata keys and faster nested search Replace btree with btree_gin indexes for PostgreSQL to allow larger metadata and faster nested search Oct 25, 2023
@danielballan
Copy link
Member

Once the CI passing, we must remember to add a database migration that will drop the old index and replace it with the new one, on existing deployments. (I can pair-code this on Zoom, @Kezzsim; the process is not too complicated, but a little obscure.)

@danielballan danielballan merged commit d9fb7f8 into bluesky:main Nov 14, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants