hi. also based on v16.
-----------------tests.
drop table if exists for_portion_of_test1;
CREATE unlogged TABLE for_portion_of_test1 (id int4range, valid_at
tsrange,name text );
INSERT INTO for_portion_of_test1 VALUES ('[1,1]', NULL,
'[1,1]_NULL'),('[1,1]', '(,)', '()_[1,]')
,('[1,1]', 'empty', '[1,1]_empty'),(NULL,NULL, NULL), (nuLL,
'(2018-01-01,2019-01-01)','misc');
--1
UPDATE for_portion_of_test1 FOR PORTION OF valid_at FROM NULL TO NULL
SET name = 'for_portition_NULLtoNULL';
select * from for_portion_of_test1;
--2
UPDATE for_portion_of_test1 FOR PORTION OF valid_at FROM null TO
UNBOUNDED SET name = 'NULL_TO_UNBOUNDED';
select * from for_portion_of_test1;
--3
UPDATE for_portion_of_test1 FOR PORTION OF valid_at FROM UNBOUNDED TO
null SET name = 'UNBOUNDED__TO_NULL';
select * from for_portion_of_test1;
--4
UPDATE for_portion_of_test1 FOR PORTION OF valid_at FROM UNBOUNDED TO
UNBOUNDED SET name = 'UNBOUNDED__TO_UNBOUNDED';
select * from for_portion_of_test1;
------------------------
File: /src/backend/executor/nodeModifyTable.c
1277: oldRange = slot_getattr(oldtupleSlot,
forPortionOf->rangeVar->varattno, &isNull);
1278:
1279: if (isNull)
1280: elog(ERROR, "found a NULL range in a temporal table");
1281: oldRangeType = DatumGetRangeTypeP(oldRange);
I wonder when this isNull will be invoked. the above tests won't
invoke the error.
also the above test, NULL seems equivalent to unbounded. FOR PORTION
OF "from" and "to" both bound should not be null?
which means the following code does not work as intended? I also
cannot find a way to invoke the following elog error branch.
File:src/backend/executor/nodeModifyTable.c
4458: exprState = ExecPrepareExpr((Expr *) forPortionOf->targetRange, estate);
4459: targetRange = ExecEvalExpr(exprState, econtext, &isNull);
4460: if (isNull)
4461: elog(ERROR, "Got a NULL FOR PORTION OF target range");
---------------------------
i also made some changes in the function range_leftover_internal,
ExecForPortionOfLeftovers.
please see the attached patch.
From 5cdd99f9a63d381985541b0df539c2bb92ef7276 Mon Sep 17 00:00:00 2001
From: pgaddict <jian.universal...@gmail.com>
Date: Sat, 21 Oct 2023 17:18:50 +0800
Subject: [PATCH v1 1/1] refactor function range_leftover_internal
renaming variable.
better comment.
---
src/backend/utils/adt/rangetypes.c | 55 +++++++++++++++++-------------
src/include/utils/rangetypes.h | 5 ++-
2 files changed, 33 insertions(+), 27 deletions(-)
diff --git a/src/backend/utils/adt/rangetypes.c b/src/backend/utils/adt/rangetypes.c
index d4e17323..a5a34450 100644
--- a/src/backend/utils/adt/rangetypes.c
+++ b/src/backend/utils/adt/rangetypes.c
@@ -1207,47 +1207,54 @@ range_split_internal(TypeCacheEntry *typcache, const RangeType *r1, const RangeT
}
/*
- * range_leftover_internal - Sets output1 and output2 to the remaining parts of r1
- * after subtracting r2, or if nothing is left then to the empty range.
- * output1 will always be "before" r2 and output2 "after".
+ * Sets leftoverRange and rightoverRange to the remaining parts of oldRange
+ * after subtracting targetRange, or if nothing is left then to the empty range.
+ * leftoverRange will always be "before" targetRange and rightoverRange "after" targetRange.
*/
void
-range_leftover_internal(TypeCacheEntry *typcache, const RangeType *r1,
- const RangeType *r2, RangeType **output1, RangeType **output2)
+range_leftover_internal(TypeCacheEntry *typcache, const RangeType *oldRange,
+ const RangeType *targetRange, RangeType **leftoverRange, RangeType **rightoverRange)
{
- RangeBound lower1,
- lower2;
- RangeBound upper1,
- upper2;
- bool empty1,
- empty2;
+ RangeBound lower_oldRange,
+ lower_targetRange;
+ RangeBound upper_oldRange,
+ upper_targetRange;
+ bool oldRange_is_empty,
+ targetRange_is_empty;
- range_deserialize(typcache, r1, &lower1, &upper1, &empty1);
- range_deserialize(typcache, r2, &lower2, &upper2, &empty2);
+ range_deserialize(typcache, oldRange, &lower_oldRange, &upper_oldRange, &oldRange_is_empty);
+ range_deserialize(typcache, targetRange, &lower_targetRange, &upper_targetRange, &targetRange_is_empty);
- if (range_cmp_bounds(typcache, &lower1, &lower2) < 0)
+ /*
+ * FOR PORTION OF. upper_targetRange, if oldRange do not overlaps with targetRangeType.
+ * So these two range must overlaps, that means both range should not be empty.
+ *
+ */
+ Assert(!oldRange_is_empty);
+ Assert(!targetRange_is_empty);
+
+ if (range_cmp_bounds(typcache, &lower_oldRange, &lower_targetRange) < 0)
{
- lower2.inclusive = !lower2.inclusive;
- lower2.lower = false;
- *output1 = make_range(typcache, &lower1, &lower2, false, NULL);
+ lower_targetRange.inclusive = !lower_targetRange.inclusive;
+ lower_targetRange.lower = false;
+ *leftoverRange = make_range(typcache, &lower_oldRange, &lower_targetRange, false, NULL);
}
else
{
- *output1 = make_empty_range(typcache);
+ *leftoverRange = make_empty_range(typcache);
}
- if (range_cmp_bounds(typcache, &upper1, &upper2) > 0)
+ if (range_cmp_bounds(typcache, &upper_oldRange, &upper_targetRange) > 0)
{
- upper2.inclusive = !upper2.inclusive;
- upper2.lower = true;
- *output2 = make_range(typcache, &upper2, &upper1, false, NULL);
+ upper_targetRange.inclusive = !upper_targetRange.inclusive;
+ upper_targetRange.lower = true;
+ *rightoverRange = make_range(typcache, &upper_targetRange, &upper_oldRange, false, NULL);
}
else
{
- *output2 = make_empty_range(typcache);
+ *rightoverRange = make_empty_range(typcache);
}
}
-
/* range -> range aggregate functions */
Datum
diff --git a/src/include/utils/rangetypes.h b/src/include/utils/rangetypes.h
index 48ee2deb..f67ae402 100644
--- a/src/include/utils/rangetypes.h
+++ b/src/include/utils/rangetypes.h
@@ -164,8 +164,7 @@ extern RangeType *make_empty_range(TypeCacheEntry *typcache);
extern bool range_split_internal(TypeCacheEntry *typcache, const RangeType *r1,
const RangeType *r2, RangeType **output1,
RangeType **output2);
-extern void range_leftover_internal(TypeCacheEntry *typcache, const RangeType *r1,
- const RangeType *r2, RangeType **output1,
- RangeType **output2);
+extern void range_leftover_internal(TypeCacheEntry *typcache, const RangeType *oldRange,
+ const RangeType *targetRange, RangeType **leftoverRange, RangeType **rightoverRange);
#endif /* RANGETYPES_H */
--
2.34.1
From 1dff6266fabca760681512fc69ec6b7ccce60896 Mon Sep 17 00:00:00 2001
From: pgaddict <jian.universal...@gmail.com>
Date: Sun, 22 Oct 2023 12:48:05 +0800
Subject: [PATCH v1 1/1] refactor ExecForPortionOfLeftovers.
FOR PORTION OF UPDATE/DELETE, upper and lower leftover range tuple
will have same the partition key as pre-update/delete tuple.
So refactor some comments.
ExecForPortionOfLeftovers is excuted inside ExecUpdateEpilogue or
ExecDeleteEpilogue, it means at least one part of leftover range
is not empty. in leftoverRangeType1, leftoverRangeType2 at least one is
not empty. So instead of call ExecFetchSlotHeapTuple twice, call it
once.
based on other code, add bool shouldFree.
---
src/backend/executor/nodeModifyTable.c | 33 +++++++++++++++-----------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 92d6611f..063efa79 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1232,7 +1232,7 @@ static void set_leftover_tuple_bounds(TupleTableSlot *leftoverTuple,
/* ----------------------------------------------------------------
* ExecForPortionOfLeftovers
*
- * Insert tuples for the untouched timestamp of a row in a FOR
+ * Insert tuples for the untouched range of a row in a FOR
* PORTION OF UPDATE/DELETE
* ----------------------------------------------------------------
*/
@@ -1257,6 +1257,8 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
TupleTableSlot *oldtupleSlot = fpoState->fp_Existing;
TupleTableSlot *leftoverTuple1 = fpoState->fp_Leftover1;
TupleTableSlot *leftoverTuple2 = fpoState->fp_Leftover2;
+ HeapTuple oldtuple;
+ bool shouldFree;
/*
* Get the range of the old pre-UPDATE/DELETE tuple,
@@ -1303,18 +1305,19 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
range_leftover_internal(typcache, oldRangeType, targetRangeType, &leftoverRangeType1,
&leftoverRangeType2);
+ /* fetch the existing, old pre-UPDATE/DELETE tuple */
+ oldtuple = ExecFetchSlotHeapTuple(oldtupleSlot, false, &shouldFree);
+
/*
* Insert a copy of the tuple with the lower leftover range.
- * Even if the table is partitioned,
- * our insert won't extend past the current row, so we don't need to re-route.
- * TODO: Really? What if you update the partition key?
+ * Even if the table is partitioned and we did UPDATE/DELETE the partition key.
+ * the partition key of the lower leftover range tuple
+ * will be the same as the partition key of the original tuple.
+ *
*/
-
if (!RangeIsEmpty(leftoverRangeType1))
{
- // TODO: anything we need to clear here?
- // Are we in the row context?
- HeapTuple oldtuple = ExecFetchSlotHeapTuple(oldtupleSlot, false, NULL);
+ // TODO: Are we in the row context?
ExecForceStoreHeapTuple(oldtuple, leftoverTuple1, false);
set_leftover_tuple_bounds(leftoverTuple1, forPortionOf, typcache, leftoverRangeType1);
@@ -1325,15 +1328,14 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
}
/*
- * Insert a copy of the tuple with the upper leftover range
- * Even if the table is partitioned,
- * our insert won't extend past the current row, so we don't need to re-route.
- * TODO: Really? What if you update the partition key?
+ * Insert a copy of the tuple with the upper leftover range.
+ * Even if the table is partitioned and we did UPDATE/DELETE the partion key.
+ * the partition key of the (upper leftover range) tuple
+ * will be the same as the partition key of the original tuple.
+ *
*/
-
if (!RangeIsEmpty(leftoverRangeType2))
{
- HeapTuple oldtuple = ExecFetchSlotHeapTuple(oldtupleSlot, false, NULL);
ExecForceStoreHeapTuple(oldtuple, leftoverTuple2, false);
set_leftover_tuple_bounds(leftoverTuple2, forPortionOf, typcache, leftoverRangeType2);
@@ -1342,6 +1344,9 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
// TODO: Need to save context->mtstate->mt_transition_capture? (See comment on ExecInsert)
ExecInsert(context, resultRelInfo, leftoverTuple2, node->canSetTag, NULL, NULL);
}
+
+ if (shouldFree)
+ heap_freetuple(oldtuple);
}
/* ----------------------------------------------------------------
--
2.34.1
