On Thu, Apr 25, 2024 at 6:03 AM Noah Misch <n...@leadboat.com> wrote: > > On Mon, Apr 15, 2024 at 04:12:38PM +0700, John Naylor wrote: > > - Some paths for single-value leaves are not covered: > > > > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/include/lib/radixtree.h.gcov.html#L904 > > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/include/lib/radixtree.h.gcov.html#L954 > > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/include/lib/radixtree.h.gcov.html#L2606 > > > > However, these paths do get regression test coverage on 32-bit > > machines. 64-bit builds only have leaves in the TID store, which > > doesn't (currently) delete entries, and doesn't instantiate the tree > > with the debug option. > > > > - In RT_SET "if (found)" is not covered: > > > > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/include/lib/radixtree.h.gcov.html#L1768 > > > > That's because we don't yet have code that replaces an existing value > > with a value of a different length. > > I saw a SIGSEGV there when using tidstore to write a fix for something else. > Patch attached.
Great find, thank you for the patch! The fix looks good to me. I think we can improve regression tests for better coverage. In TidStore on a 64-bit machine, we can store 3 offsets in the header and these values are embedded to the leaf page. With more than 3 offsets, the value size becomes more than 16 bytes and a single value leaf. Therefore, if we can add the test with the array[1,2,3,4,100], we can cover the case of replacing a single-value leaf with a different size new single-value leaf. Now we add 9 pairs of do_gset_block_offset() and check_set_block_offsets(). If these are annoying, we can remove the cases of array[1] and array[1,2]. I've attached a new patch. In addition to the new test case I mentioned, I've added some new comments and removed an unnecessary added line in test_tidstore.sql. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
0001-radixtree-Fix-SIGSEGV-at-update-of-embeddable-value-.patch
Description: Binary data