On 2018/06/13 16:35, Amit Langote wrote:
> Fwiw, I see that the crash can also occur even when using a
> non-partitioned table in the query, as shown in the following example
> which reuses Rajkumar's test data and query:
> 
> create table foo (a int, b int, c text);
> postgres=# insert into foo select i%20, i%30, to_char(i%12, 'FM0000') from
> generate_series(0, 36) i;
> 
> select dense_rank(b) within group (order by a) from foo group by b order by 1;
> server closed the connection unexpectedly
>       This probably means the server terminated abnormally
>       before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> 
> Following query in the regression test suite can also be made to crash by
> adding a group by clause:
> 
> select dense_rank(3) within group (order by x) from (values
> (1),(1),(2),(2),(3),(3),(4)) v(x) group by (x);
> server closed the connection unexpectedly
>       This probably means the server terminated abnormally
>       before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> 
> Looking at the core dump of this, it seems the following commit may be
> relevant:
> 
> commit bf6c614a2f2c58312b3be34a47e7fb7362e07bcb
> Author: Andres Freund <and...@anarazel.de>
> Date:   Thu Feb 15 21:55:31 2018 -0800
> 
>     Do execGrouping.c via expression eval machinery, take two.

I studied this a bit and found a bug that's causing the crash.

The above mentioned commit has this hunk:

@@ -1309,6 +1311,9 @@ hypothetical_dense_rank_final(PG_FUNCTION_ARGS)
         PG_RETURN_INT64(rank);

     osastate = (OSAPerGroupState *) PG_GETARG_POINTER(0);
+    econtext = osastate->qstate->econtext;
+    if (!econtext)
+        osastate->qstate->econtext = econtext =
CreateStandaloneExprContext();

In CreateStandloneExprContext(), we have this:

    econtext->ecxt_per_query_memory = CurrentMemoryContext;

    /*
     * Create working memory for expression evaluation in this context.
     */
    econtext->ecxt_per_tuple_memory =
        AllocSetContextCreate(CurrentMemoryContext,
                              "ExprContext",
                              ALLOCSET_DEFAULT_SIZES);

I noticed when debugging the crashing query that CurrentMemoryContext is
actually per-tuple memory context of some expression context of the
calling code, which would get reset before getting here again.  So, it's
wrong of hypothetical_dense_rank_final to call CreateStandloneExprContext
without first switching to an actual per-query context.

Attached patch seems to fix the crash.

Thanks,
Amit
>From d1365b84ccf7c1028fca5f7056781b2fc95093a1 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 2 Jul 2018 16:29:19 +0900
Subject: [PATCH v1] Set correct memory context in
 hypothetical_dense_rank_final

One of the memory context that it caches in OSAPerGroupState, which
is passed to it on every call, is parented by a memory context of
wrong life time.  Fix that by switching to a per-query context before
creating the aforementioned context.
---
 src/backend/utils/adt/orderedsetaggs.c   | 8 ++++++++
 src/test/regress/expected/aggregates.out | 9 +++++++++
 src/test/regress/sql/aggregates.sql      | 3 +++
 3 files changed, 20 insertions(+)

diff --git a/src/backend/utils/adt/orderedsetaggs.c 
b/src/backend/utils/adt/orderedsetaggs.c
index ed36851fdd..a758748cc7 100644
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -1310,7 +1310,15 @@ hypothetical_dense_rank_final(PG_FUNCTION_ARGS)
        osastate = (OSAPerGroupState *) PG_GETARG_POINTER(0);
        econtext = osastate->qstate->econtext;
        if (!econtext)
+       {
+               MemoryContext qcontext = osastate->qstate->qcontext;
+               MemoryContext oldcontext;
+
+               /* Make sure to we create econtext under correct parent 
context. */
+               oldcontext = MemoryContextSwitchTo(qcontext);
                osastate->qstate->econtext = econtext = 
CreateStandaloneExprContext();
+               MemoryContextSwitchTo(oldcontext);
+       }
 
        /* Adjust nargs to be the number of direct (or aggregated) args */
        if (nargs % 2 != 0)
diff --git a/src/test/regress/expected/aggregates.out 
b/src/test/regress/expected/aggregates.out
index 10d6bb6824..a120dd83f7 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -2092,3 +2092,12 @@ SELECT variance(unique1::int4), sum(unique1::int8) FROM 
tenk1;
 (1 row)
 
 ROLLBACK;
+-- test coverage for dense_rank
+SELECT dense_rank(x) WITHIN GROUP (ORDER BY x) FROM (VALUES 
(1),(1),(2),(2),(3),(3)) v(x) GROUP BY (x) ORDER BY 1;
+ dense_rank 
+------------
+          1
+          1
+          1
+(3 rows)
+
diff --git a/src/test/regress/sql/aggregates.sql 
b/src/test/regress/sql/aggregates.sql
index 1b6db50956..7e77467ecd 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -925,3 +925,6 @@ EXPLAIN (COSTS OFF)
 SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
 
 ROLLBACK;
+
+-- test coverage for dense_rank
+SELECT dense_rank(x) WITHIN GROUP (ORDER BY x) FROM (VALUES 
(1),(1),(2),(2),(3),(3)) v(x) GROUP BY (x) ORDER BY 1;
-- 
2.11.0

Reply via email to