Обсуждение: TRAP: failed Assert("offsets[i] > offsets[i - 1]"), File: "tidstore.c"
Hi, While experimenting with query plans, I periodically see test_tidstore fail on the assertion in TidStoreSetBlockOffsets(). The cause is that the harness function do_set_block_offsets() forwards the SQL array straight to TidStoreSetBlockOffsets(), which has an explicit contract: "The offset numbers 'offsets' must be sorted in ascending order." array_agg() without ORDER BY gives no such guarantee, and plan shapes that reshuffle the input can deliver the offsets out of order and trip the Assert. The issue is minor and doesn't expose any underlying bug, but it is still worth fixing: it removes a source of flaky test runs and makes life easier for extension developers who reuse the same pattern. Patch attached. -- regards, Andrei Lepikhov, pgEdge
Вложения
On Wed, Apr 15, 2026 at 5:48 AM Andrei Lepikhov <lepihov@gmail.com> wrote: > > Hi, > > While experimenting with query plans, I periodically see test_tidstore > fail on the assertion in TidStoreSetBlockOffsets(). > > The cause is that the harness function do_set_block_offsets() forwards > the SQL array straight to TidStoreSetBlockOffsets(), which has an > explicit contract: > > "The offset numbers 'offsets' must be sorted in ascending order." > > array_agg() without ORDER BY gives no such guarantee, and plan shapes > that reshuffle the input can deliver the offsets out of order and trip > the Assert. > > The issue is minor and doesn't expose any underlying bug, but it is > still worth fixing: it removes a source of flaky test runs and makes > life easier for extension developers who reuse the same pattern. > > Patch attached. Thank you for the report. Could you provide the reproducer of the assertion failure? IIRC there have not been such reports on the community so far and the test should be included in the patch anyway. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
On 15/04/2026 22:50, Masahiko Sawada wrote: > On Wed, Apr 15, 2026 at 5:48 AM Andrei Lepikhov <lepihov@gmail.com> wrote: > Could you provide the reproducer of the assertion failure? IIRC there > have not been such reports on the community so far and the test should > be included in the patch anyway. Sure! See in attachment. -- regards, Andrei Lepikhov, pgEdge
Вложения
On Thu, Apr 16, 2026 at 12:13 AM Andrei Lepikhov <lepihov@gmail.com> wrote: > > On 15/04/2026 22:50, Masahiko Sawada wrote: > > On Wed, Apr 15, 2026 at 5:48 AM Andrei Lepikhov <lepihov@gmail.com> wrote: > > Could you provide the reproducer of the assertion failure? IIRC there > > have not been such reports on the community so far and the test should > > be included in the patch anyway. > Sure! See in attachment. Thank you for updating the patch. IIUC the assertion failure could happen only where we do random TIDs test like below because it's not guaranteed that the offset numbers in the array after applying DISTICT are sorted. -- Random TIDs test. The offset numbers are randomized and must be -- unique and ordered. INSERT INTO hideblocks (blockno) SELECT do_set_block_offsets(blkno, array_agg(DISTINCT greatest((random() * :maxoffset)::int, 1))::int2[]) FROM generate_series(1, 100) num_offsets, generate_series(1000, 1100, 1) blkno GROUP BY blkno; While I agree that we need to sort the offset numbers, I think it would be better to make sure the offset numbers in the array to be sorted in a test_tidstore.sql file where required, instead of doing so for all cases. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
On 16/04/2026 10:11, Masahiko Sawada wrote: > On Thu, Apr 16, 2026 at 12:13 AM Andrei Lepikhov <lepihov@gmail.com> wrote: > -- Random TIDs test. The offset numbers are randomized and must be -- > unique and ordered. INSERT INTO hideblocks (blockno) SELECT > do_set_block_offsets(blkno, array_agg(DISTINCT greatest((random() * > :maxoffset)::int, 1))::int2[]) FROM generate_series(1, 100) > num_offsets, generate_series(1000, 1100, 1) blkno GROUP BY blkno; Alright, I used an explicit sort in reverse order to make sure the test is stable. I usually create modules that may change different paths, costs, and orders, and using random can make things unpredictable. But for this specific test, I don't see any risk. > > While I agree that we need to sort the offset numbers, I think it > would be better to make sure the offset numbers in the array to be > sorted in a test_tidstore.sql file where required, instead of doing so > for all cases. I'm not sure I follow. Are you saying that do_set_block_offsets shouldn't sort the incoming offsets? I made this change mainly to meet the TidStoreSetBlockOffsets contract. Since this is just a simple test, performance isn't really a concern. -- regards, Andrei Lepikhov, pgEdge
On Thu, Apr 16, 2026 at 1:26 AM Andrei Lepikhov <lepihov@gmail.com> wrote: > > On 16/04/2026 10:11, Masahiko Sawada wrote: > > On Thu, Apr 16, 2026 at 12:13 AM Andrei Lepikhov <lepihov@gmail.com> wrote: > > -- Random TIDs test. The offset numbers are randomized and must be -- > > unique and ordered. INSERT INTO hideblocks (blockno) SELECT > > do_set_block_offsets(blkno, array_agg(DISTINCT greatest((random() * > > :maxoffset)::int, 1))::int2[]) FROM generate_series(1, 100) > > num_offsets, generate_series(1000, 1100, 1) blkno GROUP BY blkno; > > Alright, I used an explicit sort in reverse order to make sure the test is > stable. I usually create modules that may change different paths, costs, and > orders, and using random can make things unpredictable. But for this specific > test, I don't see any risk. > > > > > While I agree that we need to sort the offset numbers, I think it > > would be better to make sure the offset numbers in the array to be > > sorted in a test_tidstore.sql file where required, instead of doing so > > for all cases. > > I'm not sure I follow. Are you saying that do_set_block_offsets shouldn't sort > the incoming offsets? No, I wanted to mean that if we sort the given array in do_set_block_offsets() as the proposed patch does, we end up always sorting arrays even if the sorting is no actually required (e.g., when executing "SELECT do_set_block_offsets(1, array[1,2,3,4,100]::int2[]);"). So an alternative idea to stabilize the regression test would be to create a SQL function to return a list of sorted offsets and use it where it's required. While the patch gets a little bigger, It would also help simplify the tests somewhat by removing the redundant codes. I've attached the patch for this idea. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Вложения
On 16/04/2026 19:58, Masahiko Sawada wrote: > On Thu, Apr 16, 2026 at 1:26 AM Andrei Lepikhov <lepihov@gmail.com> wrote: >> >> On 16/04/2026 10:11, Masahiko Sawada wrote: >>> On Thu, Apr 16, 2026 at 12:13 AM Andrei Lepikhov <lepihov@gmail.com> wrote: >>> -- Random TIDs test. The offset numbers are randomized and must be -- >>> unique and ordered. INSERT INTO hideblocks (blockno) SELECT >>> do_set_block_offsets(blkno, array_agg(DISTINCT greatest((random() * >>> :maxoffset)::int, 1))::int2[]) FROM generate_series(1, 100) >>> num_offsets, generate_series(1000, 1100, 1) blkno GROUP BY blkno; >> >> Alright, I used an explicit sort in reverse order to make sure the test is >> stable. I usually create modules that may change different paths, costs, and >> orders, and using random can make things unpredictable. But for this specific >> test, I don't see any risk. >> >>> >>> While I agree that we need to sort the offset numbers, I think it >>> would be better to make sure the offset numbers in the array to be >>> sorted in a test_tidstore.sql file where required, instead of doing so >>> for all cases. >> >> I'm not sure I follow. Are you saying that do_set_block_offsets shouldn't sort >> the incoming offsets? > > No, I wanted to mean that if we sort the given array in > do_set_block_offsets() as the proposed patch does, we end up always > sorting arrays even if the sorting is no actually required (e.g., when > executing "SELECT do_set_block_offsets(1, > array[1,2,3,4,100]::int2[]);"). So an alternative idea to stabilize > the regression test would be to create a SQL function to return a list > of sorted offsets and use it where it's required. While the patch gets > a little bigger, It would also help simplify the tests somewhat by > removing the redundant codes. I've attached the patch for this idea. Ok. No objections. Both changes are just test routines registered by the test_tidstore module. I decided to add C code, mostly following the idea that we reuse examples from the Postgres codebase when writing our patches/extensions. An explicit demonstration of the sort contract on the TidStoreSetBlockOffsets() call might help developers who don't read function comments each time. -- regards, Andrei Lepikhov, pgEdge