From b298b86ba164306cb142d70fbc3561a9503c0559 Mon Sep 17 00:00:00 2001
From: guo-shaoge <shaoge1994@163.com>
Date: Thu, 11 Mar 2021 15:30:55 +0800
Subject: [PATCH] executor: group_concat aggr panic when
 session.group_concat_max_len is small (#23131)

---
 executor/aggfuncs/func_group_concat.go | 31 +++++++++++++++++---------
 executor/aggregate_test.go             | 17 ++++++++++++++
 2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/executor/aggfuncs/func_group_concat.go b/executor/aggfuncs/func_group_concat.go
index 901ece6ea0625..ba80b91b3f3c1 100644
--- a/executor/aggfuncs/func_group_concat.go
+++ b/executor/aggfuncs/func_group_concat.go
@@ -281,6 +281,11 @@ type topNRows struct {
 	currSize  uint64
 	limitSize uint64
 	sepSize   uint64
+	// If sep is truncated, we need to append part of sep to result.
+	// In the following example, session.group_concat_max_len is 10 and sep is '---'.
+	// ('---', 'ccc') should be poped from heap, so '-' should be appended to result.
+	// eg: 'aaa---bbb---ccc' -> 'aaa---bbb-'
+	isSepTruncated bool
 }
 
 func (h topNRows) Len() int {
@@ -349,6 +354,7 @@ func (h *topNRows) tryToAdd(row sortRow) (truncated bool, memDelta int64) {
 				memDelta -= GetDatumMemSize(dt)
 			}
 			heap.Pop(h)
+			h.isSepTruncated = true
 		}
 	}
 	return true, memDelta
@@ -369,10 +375,11 @@ func (h *topNRows) concat(sep string, truncated bool) string {
 		}
 		buffer.Write(row.buffer.Bytes())
 	}
-	if truncated && uint64(buffer.Len()) < h.limitSize {
-		// append the last separator, because the last separator may be truncated in tryToAdd.
+	if h.isSepTruncated {
 		buffer.WriteString(sep)
-		buffer.Truncate(int(h.limitSize))
+		if uint64(buffer.Len()) > h.limitSize {
+			buffer.Truncate(int(h.limitSize))
+		}
 	}
 	return buffer.String()
 }
@@ -402,10 +409,11 @@ func (e *groupConcatOrder) AllocPartialResult() (pr PartialResult, memDelta int6
 	}
 	p := &partialResult4GroupConcatOrder{
 		topN: &topNRows{
-			desc:      desc,
-			currSize:  0,
-			limitSize: e.maxLen,
-			sepSize:   uint64(len(e.sep)),
+			desc:           desc,
+			currSize:       0,
+			limitSize:      e.maxLen,
+			sepSize:        uint64(len(e.sep)),
+			isSepTruncated: false,
 		},
 	}
 	return PartialResult(p), DefPartialResult4GroupConcatOrderSize + DefTopNRowsSize
@@ -504,10 +512,11 @@ func (e *groupConcatDistinctOrder) AllocPartialResult() (pr PartialResult, memDe
 	valSet, setSize := set.NewStringSetWithMemoryUsage()
 	p := &partialResult4GroupConcatOrderDistinct{
 		topN: &topNRows{
-			desc:      desc,
-			currSize:  0,
-			limitSize: e.maxLen,
-			sepSize:   uint64(len(e.sep)),
+			desc:           desc,
+			currSize:       0,
+			limitSize:      e.maxLen,
+			sepSize:        uint64(len(e.sep)),
+			isSepTruncated: false,
 		},
 		valSet: valSet,
 	}
diff --git a/executor/aggregate_test.go b/executor/aggregate_test.go
index 22b40aa9e1d8b..b9c4bf49bca91 100644
--- a/executor/aggregate_test.go
+++ b/executor/aggregate_test.go
@@ -641,6 +641,23 @@ func (s *testSuiteAgg) TestGroupConcatAggr(c *C) {
 
 	// issue #9920
 	tk.MustQuery("select group_concat(123, null)").Check(testkit.Rows("<nil>"))
+
+	// issue #23129
+	tk.MustExec("drop table if exists t1;")
+	tk.MustExec("create table t1(cid int, sname varchar(100));")
+	tk.MustExec("insert into t1 values(1, 'Bob'), (1, 'Alice');")
+	tk.MustExec("insert into t1 values(3, 'Ace');")
+	tk.MustExec("set @@group_concat_max_len=5;")
+	rows := tk.MustQuery("select group_concat(sname order by sname) from t1 group by cid;")
+	rows.Check(testkit.Rows("Alice", "Ace"))
+
+	tk.MustExec("drop table if exists t1;")
+	tk.MustExec("create table t1(c1 varchar(10));")
+	tk.MustExec("insert into t1 values('0123456789');")
+	tk.MustExec("insert into t1 values('12345');")
+	tk.MustExec("set @@group_concat_max_len=8;")
+	rows = tk.MustQuery("select group_concat(c1 order by c1) from t1 group by c1;")
+	rows.Check(testkit.Rows("01234567", "12345"))
 }
 
 func (s *testSuiteAgg) TestSelectDistinct(c *C) {