Repository navigation
Conversation
Since graphprotocol#6434 (v0.44.0) a copied deployment is created from the layout's default index set instead of the source's indexes. Only the indexes that `to_postpone()` covers are taken from the source afterwards; the GiST reference indexes, the BTree on `id` and any manually created index come from the layout, blind to what the source carries. An operator who drops indexes from a large deployment to save space gets all of them back on every copy - 1.35 TB on a 1.6 TB deployment in the report - which also slows the copy's catch-up and can push the destination shard over its disk threshold. graphprotocol#5140 asked for the opposite and graphprotocol#5425 implemented it: create the indexes based on the index creation DDL of the source. This restores that by passing the source's index list down to the DDL again. The time-travel and aggregate indexes stay layout-driven. They describe how history is stored rather than something an operator tuned, and they exist on every deployment, so taking them from the source would let a filter drop something the schema needs. Grafts keep working: a column that only exists on the destination still gets the layout's indexes for it, because the source list has no entry to match. Signed-off-by: Liang Xu <24490353+lx3133584@users.noreply.github.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6722
Problem
Since #6434 (v0.44.0)
graphman copycreates the destination's schema from thelayout's default index set. Only the indexes
to_postpone()covers are takenfrom the source afterwards; the GiST
(reference, block_range)indexes, theBTree on
idand any manually created index are written from the layout,whatever the source carries.
index_defwas dropped from the copy path in thatchange, and nothing in it refers to the behaviour #5140 asked for.
For an operator who drops indexes from a large deployment before copying it out
to rebalance shards, every copy comes back with all of them: +1.35 TB on a 1.6 TB
deployment in the report, +35 indexes on a 228 MB one. Maintaining them also
slows the copy's catch-up and pushed a destination shard over its disk threshold
mid-copy.
Fix
Pass the source's index list down to the DDL again, as #5425 had it:
SubgraphStore::copy_deploymentloads the source's indexes and hands them tocreate_deployment.Layout::create_relational_schema_with_source_indexes(new; the old entrypoint passes
None) forwards them toTable::as_ddl, which writes theattribute indexes from that list instead of from the layout.
Two details that are not simply "restore the old code":
brin_*,*_block_range_closed) and the aggregateindexes stay layout-driven. They describe how history is stored rather than
something an operator tunes, and every deployment has them. A source list that
filtered them would otherwise drop something the schema needs.
indexes_for_tableskips theattr_N_0_<table>_idstyle indexes because thelayout recreates them anyway, which is not true once the attribute indexes
come from the source.
indexes_for_copy_tablekeeps them, and still drops theones the layout always creates.
Grafts are unaffected: a column that exists only on the destination has no entry
in the source list, so it keeps the layout's indexes for that column.
prune.rscreates the destination of a truncation, not a copy, and passesNone, which keeps its behaviour.Testing
Added
source_indexes_replace_the_layout_indexes_on_creationinstore/postgres/src/relational/ddl_tests.rs, which works on the generated DDLwithout a database:
create index attr_0_1_thing_big_thing ... using gist— which the layout describes — is not writtenbrin_thing,thing_block_range_closed) are stillwritten, since they are not the source's to drop
Reverting the
Table::as_ddlchange makes the first of those fail with thegenerated DDL printed, so the test is pinned to the behaviour and not to the
ordering of the output.
cargo fmt --all -- --checkandcargo clippy -p graph-store-postgres --all-targetsare clean.The
graphman copy createpath itself is covered bypostgres::graft::copyinstore/test-store, which needs the two-shardconfig.sharded.toml. Those testsfail on unmodified
masteron this machine (could not connect to server "shard_sgd"after a fresh database), so I could not use them to confirm theend-to-end behaviour here; the unit test above and the DDL path it exercises are
what this change was verified against.
Deployment note
A copy made under v0.44.0+ of a slimmed-down source has the full default index
set. That is not corrected by this change — it needs a re-copy, or dropping the
indexes again on the destination.