Re: Decoding speculative insert with toast leaks memory

On Tue, Jun 8, 2021 at 5:16 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> Based on the off list discussion, I have modified the test based on
> the idea showed in
> "isolation/specs/insert-conflict-specconflict.spec", other open point
> we had about the race condition that how to ensure that when we unlock
> any session it make progress, IMHO the isolation tested is designed in
> a way that either all the waiting session returns with the output or
> again block on a heavy weight lock before performing the next step.
>

Few comments:
1. The test has a lot of similarities and test duplication with what
we are doing in insert-conflict-specconflict.spec. Can we move it to
insert-conflict-specconflict.spec? I understand that having it in
test_decoding has the advantage that we can have all decoding tests in
one place but OTOH, we can avoid a lot of test-code duplication if we
add it in insert-conflict-specconflict.spec.

2.
+#permutation "s1_session" "s1_lock_s2" "s1_lock_s3" "s1_begin"
"s1_insert_tbl1" "s2_session" "s2_begin" "s2_insert_tbl1" "s3_session"
"s3_begin" "s3_insert_tbl1" "s1_unlock_s2" "s1_unlock_s3" "s1_lock_s2"
"s1_abort" "s3_commit" "s1_unlock_s2" "s2_insert_tbl2" "s2_commit"
"s1_get_changes"

This permutation is not matching with what we are actually doing.

3.
+# Test that speculative locks are correctly acquired and released, s2
+# inserts, s1 updates.

This test description doesn't seem to be correct. Can we change it to
something like: "Test logical decoding of speculative aborts for toast
insertion followed by insertion into a different table which doesn't
have a toast"?

Also, let's prepare and test the patches for back-branches. It would
be better if you can prepare separate patches for code and test-case
for each branch then I can merge them before commit. This helps with
testing on back-branches.

-- 
With Regards,
Amit Kapila.