From 76a5c975c166f03a4874bc9fdd44181ce7cc52ea Mon Sep 17 00:00:00 2001
From: Timothy Elgersma <timothy.elgersma@gmail.com>
Date: Mon, 23 Sep 2024 17:05:31 +0000
Subject: [PATCH] [#23943] YSQL: Fix Bitmap Scan GCC11 crash (follow-up)

Summary:
89b69cfc010fc5c0ef31ce968026d1f42b9a0eae / D38080 attempted to fix the crash in GCC11, but after an initial successful run on Jenkins I made a small change and tested locally, omitting the `--gcc11` flag that was required to reproduce the crash. Since I had already seen a successful Jenkins run and believed the local results were still good, I didn't look closely at the Jenkins results and missed that it had started failing again on gcc11 fastdebug.

The previous hypothesis was that `delete[]`ing the memory caused the `Slice`'s data to point to a garbage value, which could potentially cause errors while erasing the slice from the unordered_set. This proved not to be true, as calling `Slice::Clear` after deleting the memory did not resolve the problem.

The problem is actually mentioned by the [[ https://en.cppreference.com/w/cpp/container/unordered_set | C++ standard ]]
> Container elements may not be modified (even by non const iterators) since modification could change an element's hash and corrupt the container.

Rearranging the instructions so that the modification of the Slice occurs after it is removed from the set solves the problem. This is also the same as the first draft of the previous attempt of a fix, but now the reason is well understood.
Jira: DB-12839

Test Plan:
Jenkins,

Manually:
```
Build:
./yb_build.sh fastdebug --gcc11
Restart cluster
Run
/*+ Set(yb_enable_bitmapscan true) Set(yb_enable_base_scans_cost_model true) BitmapScan(t) */
EXPLAIN (ANALYZE, SUMMARY OFF, COSTS OFF) SELECT * FROM test_and t WHERE a < 5 AND b < 5;

\watch .1
```

Reviewers: amartsinchyk

Reviewed By: amartsinchyk

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D38318
---
 src/yb/yql/pggate/ybc_pggate.cc | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/yb/yql/pggate/ybc_pggate.cc b/src/yb/yql/pggate/ybc_pggate.cc
index 65c309d6574..6c0856c4f48 100644
--- a/src/yb/yql/pggate/ybc_pggate.cc
+++ b/src/yb/yql/pggate/ybc_pggate.cc
@@ -646,7 +646,6 @@ typedef std::unordered_set<Slice, Slice::Hash> UnorderedSliceSet;
 
 static void FreeSlice(Slice slice) {
   delete[] slice.data(), slice.size();
-  slice.Clear();
 }
 
 SliceSet YBCBitmapCreateSet() {
@@ -686,8 +685,13 @@ SliceSet YBCBitmapIntersectSet(SliceSet sa, SliceSet sb) {
   auto iterb = b->begin();
   for (auto itera = a->begin(); itera != a->end();) {
     if ((iterb = b->find(*itera)) == b->end()) {
-      FreeSlice(*itera);
+      // We cannot modify the slice while it is in the set because
+      // std::unordered_set stores const Slices. Since the slice contains
+      // malloc'd memory, we grab a reference to it to delete the memory after
+      // removing it from the set.
+      auto data = itera->data();
       itera = a->erase(itera);
+      delete[] data;
     } else {
       ++itera;
     }