[hawq] branch master updated: HAWQ-1850. Fix life cycle of PlannedStmt and its resource

[zt...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

ztao1987 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hawq.git


The following commit(s) were added to refs/heads/master by this push:
     new 1f0bd6031 HAWQ-1850. Fix life cycle of PlannedStmt and its resource
1f0bd6031 is described below

commit 1f0bd60319a5376d8b81421ecf17247812f94c64
Author: ztao1987 <zh...@gmail.com>
AuthorDate: Thu Nov 10 00:21:21 2022 +0800

    HAWQ-1850. Fix life cycle of PlannedStmt and its resource
---
 src/backend/executor/spi.c           | 37 +++++++++++++++---------------------
 src/backend/optimizer/plan/planner.c | 27 ++++++++++++++++++++++++--
 2 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 5190670e7..1bc2e6801 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1963,17 +1963,21 @@ _SPI_execute_plan(_SPI_plan * plan, Datum *Values, const char *Nulls,
           originalStmt =
               refineCachedPlan(lfirst(plan_list_item),
                                queryTree, 0, NULL);
-          stmt = copyObject(originalStmt);
-          if (Debug_udf_plan &&
-              originalStmt->datalocalityInfo) {
-            stmt->datalocalityTime =
-                originalStmt->datalocalityTime;
-            stmt->datalocalityInfo = makeStringInfo();
-            stmt->datalocalityInfo->cursor =
-                originalStmt->datalocalityInfo->cursor;
-            appendStringInfoString(
-                stmt->datalocalityInfo,
-                originalStmt->datalocalityInfo->data);
+          if (lfirst(plan_list_item) == originalStmt) {
+            stmt = copyObject(originalStmt);
+            if (Debug_udf_plan &&
+                originalStmt->datalocalityInfo) {
+              stmt->datalocalityTime =
+                  originalStmt->datalocalityTime;
+              stmt->datalocalityInfo = makeStringInfo();
+              stmt->datalocalityInfo->cursor =
+                  originalStmt->datalocalityInfo->cursor;
+              appendStringInfoString(
+                  stmt->datalocalityInfo,
+                  originalStmt->datalocalityInfo->data);
+            }
+          } else {
+            stmt = originalStmt;
           }
         }
 
@@ -2648,17 +2652,6 @@ _SPI_copy_plan(SPIPlanPtr plan, int location)
 					                       ((PlannedStmt*)lfirst(lc))->datalocalityInfo->data);
 				}
 				ps->qdContext = plancxt;
-				if (ps->resource) {
-					List *prevSegList = ps->resource->segments;
-					ps->resource->segments = NULL;
-					ListCell *lc = NULL;
-					foreach (lc, prevSegList) {
-						Segment *seg = (Segment *)lfirst(lc);
-						ps->resource->segments =
-								lappend(ps->resource->segments,
-												CopySegment(seg, plancxt));
-					}
-				}
 			}
 			newplan->ptlist = lappend(newplan->ptlist, node);
 		}
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index f82b5c029..a3be57878 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -311,8 +311,31 @@ PlannedStmt *refineCachedPlan(PlannedStmt * plannedstmt,
   SplitAllocResult *allocResult = NULL;
   Query *my_parse = copyObject(parse);
 
-  /* If this is a parallel plan. request resource and allocate split again*/
-  if (my_parse->commandType != CMD_UTILITY && plannedstmt->planTree->dispatch == DISPATCH_PARALLEL)
+  // The conditions for re-allocating resource are complicated:
+  // 1. It is not a CMD_UTILITY
+  // 2. It is a parallel plan or a previously inherited resource
+  //
+  // We want to replace pre-allocated inherited resource forcefully because the
+  // life cycle of PlannedStmt is inconsistent with its resource.
+  //
+  // There is a cache mechanism for plan in plpgsql, which means that executing
+  // the same statement will access the same 'plan' object, and the resource for
+  // executing is stored in this object. The problem occurs after the first
+  // execution, the resource will be actively freed, but the cached plan still
+  // retains the freed resource, which means that when the same
+  // statement is executed the second time, an illegal address will be accessed
+  // (of course the occurrence of the problem depends on whether the memory
+  // address is polluted. In addition, we can forcibly set the address of pfree
+  // to 0 to increase the possibility of the problem).
+  //
+  // Although it is difficult for us to fundamentally control the life cycle of
+  // PlannedStmt and its resource to be consistent, we can avoid the problem
+  // through a tricky way: inheritating resource from ActiveQueryResource when
+  // fetching PlannedStmt.
+
+  if (my_parse->commandType != CMD_UTILITY && // not a CMD_UTILITY
+      (plannedstmt->planTree->dispatch == DISPATCH_PARALLEL || // parallel paln
+       (plannedstmt->resource && plannedstmt->resource->life == QRL_INHERIT))) // replace pre-allocated inherited resource forcefully
   {
     /*
      * Now, we want to allocate resource.