Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit14a158f

Browse files
committed
Fix interaction of CASE and ArrayCoerceExpr.
An array-type coercion appearing within a CASE that has a constant(after const-folding) test expression was mangled by the planner, causingall the elements of the resulting array to be equal to the coerced valueof the CASE's test expression. This is my oversight in commitc12d570:that changed ArrayCoerceExpr to use a subexpression involving aCaseTestExpr, and I didn't notice that eval_const_expressions needed anadjustment to keep from folding such a CaseTestExpr to a constant whenit's inside a suitable CASE.This is another in what's getting to be a depressingly long line of bugsassociated with misidentification of the referent of a CaseTestExpr.We're overdue to redesign that mechanism; but any such fix is unlikelyto be back-patchable into v11. As a stopgap, fix eval_const_expressionsto do what it must here. Also add a bunch of comments pointing out therestrictions and assumptions that are needed to make this work at all.Also fix a related oversight: contain_context_dependent_node() was notaware of the relationship of ArrayCoerceExpr to CaseTestExpr. That wassomewhat fail-soft, in that the outcome of a wrong answer would be toprevent optimizations that could have been made, but let's fix it whilewe're at it.Per bug #15471 from Matt Williams. Back-patch to v11 where the faultylogic came in.Discussion:https://postgr.es/m/15471-1117f49271989bad@postgresql.org
1 parentc2c7c26 commit14a158f

File tree

7 files changed

+108
-16
lines changed

7 files changed

+108
-16
lines changed

‎src/backend/executor/execExpr.c

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1516,10 +1516,11 @@ ExecInitExprRec(Expr *node, ExprState *state,
15161516
/*
15171517
* Read from location identified by innermost_caseval. Note
15181518
* that innermost_caseval could be NULL, if this node isn't
1519-
* actually within a CASE structure; some parts of the system
1520-
* abuse CaseTestExpr to cause a read of a value externally
1521-
* supplied in econtext->caseValue_datum. We'll take care of
1522-
* that scenario at runtime.
1519+
* actually within a CaseExpr, ArrayCoerceExpr, etc structure.
1520+
* That can happen because some parts of the system abuse
1521+
* CaseTestExpr to cause a read of a value externally supplied
1522+
* in econtext->caseValue_datum. We'll take care of that
1523+
* scenario at runtime.
15231524
*/
15241525
scratch.opcode=EEOP_CASE_TESTVAL;
15251526
scratch.d.casetest.value=state->innermost_caseval;

‎src/backend/optimizer/util/clauses.c

Lines changed: 49 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1452,7 +1452,8 @@ contain_nonstrict_functions_walker(Node *node, void *context)
14521452
* CaseTestExpr nodes must appear directly within the corresponding CaseExpr,
14531453
* not nested within another one, or they'll see the wrong test value. If one
14541454
* appears "bare" in the arguments of a SQL function, then we can't inline the
1455-
* SQL function for fear of creating such a situation.
1455+
* SQL function for fear of creating such a situation. The same applies for
1456+
* CaseTestExpr used within the elemexpr of an ArrayCoerceExpr.
14561457
*
14571458
* CoerceToDomainValue would have the same issue if domain CHECK expressions
14581459
* could get inlined into larger expressions, but presently that's impossible.
@@ -1468,16 +1469,16 @@ contain_context_dependent_node(Node *clause)
14681469
returncontain_context_dependent_node_walker(clause,&flags);
14691470
}
14701471

1471-
#defineCCDN_IN_CASEEXPR0x0001/* CaseTestExpr okay here? */
1472+
#defineCCDN_CASETESTEXPR_OK0x0001/* CaseTestExpr okay here? */
14721473

14731474
staticbool
14741475
contain_context_dependent_node_walker(Node*node,int*flags)
14751476
{
14761477
if (node==NULL)
14771478
return false;
14781479
if (IsA(node,CaseTestExpr))
1479-
return !(*flags&CCDN_IN_CASEEXPR);
1480-
if (IsA(node,CaseExpr))
1480+
return !(*flags&CCDN_CASETESTEXPR_OK);
1481+
elseif (IsA(node,CaseExpr))
14811482
{
14821483
CaseExpr*caseexpr= (CaseExpr*)node;
14831484

@@ -1499,14 +1500,32 @@ contain_context_dependent_node_walker(Node *node, int *flags)
14991500
* seem worth any extra code. If there are any bare CaseTestExprs
15001501
* elsewhere in the CASE, something's wrong already.
15011502
*/
1502-
*flags |=CCDN_IN_CASEEXPR;
1503+
*flags |=CCDN_CASETESTEXPR_OK;
15031504
res=expression_tree_walker(node,
15041505
contain_context_dependent_node_walker,
15051506
(void*)flags);
15061507
*flags=save_flags;
15071508
returnres;
15081509
}
15091510
}
1511+
elseif (IsA(node,ArrayCoerceExpr))
1512+
{
1513+
ArrayCoerceExpr*ac= (ArrayCoerceExpr*)node;
1514+
intsave_flags;
1515+
boolres;
1516+
1517+
/* Check the array expression */
1518+
if (contain_context_dependent_node_walker((Node*)ac->arg,flags))
1519+
return true;
1520+
1521+
/* Check the elemexpr, which is allowed to contain CaseTestExpr */
1522+
save_flags=*flags;
1523+
*flags |=CCDN_CASETESTEXPR_OK;
1524+
res=contain_context_dependent_node_walker((Node*)ac->elemexpr,
1525+
flags);
1526+
*flags=save_flags;
1527+
returnres;
1528+
}
15101529
returnexpression_tree_walker(node,contain_context_dependent_node_walker,
15111530
(void*)flags);
15121531
}
@@ -3125,10 +3144,31 @@ eval_const_expressions_mutator(Node *node,
31253144
}
31263145
caseT_ArrayCoerceExpr:
31273146
{
3128-
ArrayCoerceExpr*ac;
3147+
ArrayCoerceExpr*ac=makeNode(ArrayCoerceExpr);
3148+
Node*save_case_val;
31293149

3130-
/* Copy the node and const-simplify its arguments */
3131-
ac= (ArrayCoerceExpr*)ece_generic_processing(node);
3150+
/*
3151+
* Copy the node and const-simplify its arguments. We can't
3152+
* use ece_generic_processing() here because we need to mess
3153+
* with case_val only while processing the elemexpr.
3154+
*/
3155+
memcpy(ac,node,sizeof(ArrayCoerceExpr));
3156+
ac->arg= (Expr*)
3157+
eval_const_expressions_mutator((Node*)ac->arg,
3158+
context);
3159+
3160+
/*
3161+
* Set up for the CaseTestExpr node contained in the elemexpr.
3162+
* We must prevent it from absorbing any outer CASE value.
3163+
*/
3164+
save_case_val=context->case_val;
3165+
context->case_val=NULL;
3166+
3167+
ac->elemexpr= (Expr*)
3168+
eval_const_expressions_mutator((Node*)ac->elemexpr,
3169+
context);
3170+
3171+
context->case_val=save_case_val;
31323172

31333173
/*
31343174
* If constant argument and the per-element expression is
@@ -3142,6 +3182,7 @@ eval_const_expressions_mutator(Node *node,
31423182
ac->elemexpr&& !IsA(ac->elemexpr,CoerceToDomain)&&
31433183
!contain_mutable_functions((Node*)ac->elemexpr))
31443184
returnece_evaluate_expr(ac);
3185+
31453186
return (Node*)ac;
31463187
}
31473188
caseT_CollateExpr:

‎src/backend/parser/parse_coerce.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -907,7 +907,12 @@ build_coercion_expression(Node *node,
907907
sourceBaseTypeId=getBaseTypeAndTypmod(exprType(node),
908908
&sourceBaseTypeMod);
909909

910-
/* Set up CaseTestExpr representing one element of source array */
910+
/*
911+
* Set up a CaseTestExpr representing one element of the source array.
912+
* This is an abuse of CaseTestExpr, but it's OK as long as there
913+
* can't be any CaseExpr or ArrayCoerceExpr within the completed
914+
* elemexpr.
915+
*/
911916
ctest->typeId=get_element_type(sourceBaseTypeId);
912917
Assert(OidIsValid(ctest->typeId));
913918
ctest->typeMod=sourceBaseTypeMod;

‎src/backend/parser/parse_target.c

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -691,7 +691,13 @@ transformAssignmentIndirection(ParseState *pstate,
691691

692692
if (indirection&& !basenode)
693693
{
694-
/* Set up a substitution. We reuse CaseTestExpr for this. */
694+
/*
695+
* Set up a substitution. We abuse CaseTestExpr for this. It's safe
696+
* to do so because the only nodes that will be above the CaseTestExpr
697+
* in the finished expression will be FieldStore and ArrayRef nodes.
698+
* (There could be other stuff in the tree, but it will be within
699+
* other child fields of those node types.)
700+
*/
695701
CaseTestExpr*ctest=makeNode(CaseTestExpr);
696702

697703
ctest->typeId=targetTypeId;

‎src/include/nodes/primnodes.h

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -934,8 +934,20 @@ typedef struct CaseWhen
934934
* This is effectively like a Param, but can be implemented more simply
935935
* since we need only one replacement value at a time.
936936
*
937-
* We also use this in nested UPDATE expressions.
938-
* See transformAssignmentIndirection().
937+
* We also abuse this node type for some other purposes, including:
938+
** Placeholder for the current array element value in ArrayCoerceExpr;
939+
* see build_coercion_expression().
940+
** Nested FieldStore/ArrayRef assignment expressions in INSERT/UPDATE;
941+
* see transformAssignmentIndirection().
942+
*
943+
* The uses in CaseExpr and ArrayCoerceExpr are safe only to the extent that
944+
* there is not any other CaseExpr or ArrayCoerceExpr between the value source
945+
* node and its child CaseTestExpr(s). This is true in the parse analysis
946+
* output, but the planner's function-inlining logic has to be careful not to
947+
* break it.
948+
*
949+
* The nested-assignment-expression case is safe because the only node types
950+
* that can be above such CaseTestExprs are FieldStore and ArrayRef.
939951
*/
940952
typedefstructCaseTestExpr
941953
{

‎src/test/regress/expected/case.out

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,20 @@ SELECT CASE make_ad(1,2)
372372
right
373373
(1 row)
374374

375+
ROLLBACK;
376+
-- Test interaction of CASE with ArrayCoerceExpr (bug #15471)
377+
BEGIN;
378+
CREATE TYPE casetestenum AS ENUM ('e', 'f', 'g');
379+
SELECT
380+
CASE 'foo'::text
381+
WHEN 'foo' THEN ARRAY['a', 'b', 'c', 'd'] || enum_range(NULL::casetestenum)::text[]
382+
ELSE ARRAY['x', 'y']
383+
END;
384+
array
385+
-----------------
386+
{a,b,c,d,e,f,g}
387+
(1 row)
388+
375389
ROLLBACK;
376390
--
377391
-- Clean up

‎src/test/regress/sql/case.sql

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,19 @@ SELECT CASE make_ad(1,2)
233233

234234
ROLLBACK;
235235

236+
-- Test interaction of CASE with ArrayCoerceExpr (bug #15471)
237+
BEGIN;
238+
239+
CREATETYPEcasetestenumAS ENUM ('e','f','g');
240+
241+
SELECT
242+
CASE'foo'::text
243+
WHEN'foo' THEN ARRAY['a','b','c','d']|| enum_range(NULL::casetestenum)::text[]
244+
ELSE ARRAY['x','y']
245+
END;
246+
247+
ROLLBACK;
248+
236249
--
237250
-- Clean up
238251
--

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp