propagate gate status in Node4::DeleteChild #20044
Conversation
|
Changes make sense to me! |
|
We tried to extract it from the huge reproducer attached in the issue - but it's unfortunately not as straightforward. I ran into a similar issue here: #19204... I opened an internal issue around the topic here and I think we should put some effort into that in the upcoming months. There are also plans to add a scalar helper function for ART scanning, and maybe as part of that we can make another push to add minimal reproducers for these two PRs? |
|
@pdet I tried to make a minimal test, the issue is that since it is the nested ART storing rowids, reproducing some nested structures could require inserting a lot of entries to generate rowids with variation in more significant bytes, then deleting in the proper order to get to the same exact structures. |
|
Alright, that's fine, thanks for the explanations! |
propagate gate status in Node4::DeleteChild (duckdb/duckdb#20044)
propagate gate status in Node4::DeleteChild (duckdb/duckdb#20044) Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
Bugfix for #19975
Propagate gate status in Node4::DeleteChild, since Prefix::Concat logic depends on both the prev_node4 gate status (was the node4 being freed a gate node), as well as the overall gate status (did we enter through a gate at some point earlier). The latter was not being propagated correctly, and node compression logic for nested ARTs storing rowids depends on this, since we do not need to maintain prefix chains, as they are implicit in the rowid's.
Unfortunately it is not possible to create a minimal test for this scenario, however once https://github.com/duckdblabs/duckdb-internal/issues/6083 is solved, there is a large reproducer for this scenario that I downloaded from the person who opened this issue (and the data/sql file is not confidential, so can be used).
Thank you @taniabogatsch for dealing the final blow to this bug!