Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 77cd0dc

Browse files
committed
Fix handling of expanded objects in CoerceToDomain and CASE execution.
When the input value to a CoerceToDomain expression node is a read-write expanded datum, we should pass a read-only pointer to any domain CHECK expressions and then return the original read-write pointer as the expression result. Previously we were blindly passing the same pointer to all the consumers of the value, making it possible for a function in CHECK to modify or even delete the expanded value. (Since a plpgsql function will absorb a passed-in read-write expanded array as a local variable value, it will in fact delete the value on exit.) A similar hazard of passing the same read-write pointer to multiple consumers exists in domain_check() and in ExecEvalCase, so fix those too. The fix requires adding MakeExpandedObjectReadOnly calls at the appropriate places, which is simple enough except that we need to get the data type's typlen from somewhere. For the domain cases, solve this by redefining DomainConstraintRef.tcache as okay for callers to access; there wasn't any reason for the original convention against that, other than not wanting the API of typcache.c to be any wider than it had to be. For CASE, there's no good solution except to add a syscache lookup during executor start. Per bug #14472 from Marcos Castedo. Back-patch to 9.5 where expanded values were introduced. Discussion: https://postgr.es/m/15225.1482431619@sss.pgh.pa.us
1 parent 17742a0 commit 77cd0dc

File tree

8 files changed

+127
-8
lines changed

8 files changed

+127
-8
lines changed

src/backend/executor/execQual.c

+20-5
Original file line numberDiff line numberDiff line change
@@ -2987,12 +2987,18 @@ ExecEvalCase(CaseExprState *caseExpr, ExprContext *econtext,
29872987

29882988
if (caseExpr->arg)
29892989
{
2990+
Datum arg_value;
29902991
bool arg_isNull;
29912992

2992-
econtext->caseValue_datum = ExecEvalExpr(caseExpr->arg,
2993-
econtext,
2994-
&arg_isNull,
2995-
NULL);
2993+
arg_value = ExecEvalExpr(caseExpr->arg,
2994+
econtext,
2995+
&arg_isNull,
2996+
NULL);
2997+
/* Since caseValue_datum may be read multiple times, force to R/O */
2998+
econtext->caseValue_datum =
2999+
MakeExpandedObjectReadOnly(arg_value,
3000+
arg_isNull,
3001+
caseExpr->argtyplen);
29963002
econtext->caseValue_isNull = arg_isNull;
29973003
}
29983004

@@ -4053,11 +4059,18 @@ ExecEvalCoerceToDomain(CoerceToDomainState *cstate, ExprContext *econtext,
40534059
* nodes. We must save and restore prior setting of
40544060
* econtext's domainValue fields, in case this node is
40554061
* itself within a check expression for another domain.
4062+
*
4063+
* Also, if we are working with a read-write expanded
4064+
* datum, be sure that what we pass to CHECK expressions
4065+
* is a read-only pointer; else called functions might
4066+
* modify or even delete the expanded object.
40564067
*/
40574068
save_datum = econtext->domainValue_datum;
40584069
save_isNull = econtext->domainValue_isNull;
40594070

4060-
econtext->domainValue_datum = result;
4071+
econtext->domainValue_datum =
4072+
MakeExpandedObjectReadOnly(result, *isNull,
4073+
cstate->constraint_ref->tcache->typlen);
40614074
econtext->domainValue_isNull = *isNull;
40624075

40634076
conResult = ExecEvalExpr(con->check_expr,
@@ -4865,6 +4878,8 @@ ExecInitExpr(Expr *node, PlanState *parent)
48654878
}
48664879
cstate->args = outlist;
48674880
cstate->defresult = ExecInitExpr(caseexpr->defresult, parent);
4881+
if (caseexpr->arg)
4882+
cstate->argtyplen = get_typlen(exprType((Node *) caseexpr->arg));
48684883
state = (ExprState *) cstate;
48694884
}
48704885
break;

src/backend/utils/adt/domains.c

+8-2
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include "executor/executor.h"
3636
#include "lib/stringinfo.h"
3737
#include "utils/builtins.h"
38+
#include "utils/expandeddatum.h"
3839
#include "utils/lsyscache.h"
3940
#include "utils/syscache.h"
4041
#include "utils/typcache.h"
@@ -157,9 +158,14 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
157158
* Set up value to be returned by CoerceToDomainValue
158159
* nodes. Unlike ExecEvalCoerceToDomain, this econtext
159160
* couldn't be shared with anything else, so no need to
160-
* save and restore fields.
161+
* save and restore fields. But we do need to protect the
162+
* passed-in value against being changed by called
163+
* functions. (It couldn't be a R/W expanded object for
164+
* most uses, but that seems possible for domain_check().)
161165
*/
162-
econtext->domainValue_datum = value;
166+
econtext->domainValue_datum =
167+
MakeExpandedObjectReadOnly(value, isnull,
168+
my_extra->constraint_ref.tcache->typlen);
163169
econtext->domainValue_isNull = isnull;
164170

165171
conResult = ExecEvalExprSwitchContext(con->check_expr,

src/include/nodes/execnodes.h

+1
Original file line numberDiff line numberDiff line change
@@ -873,6 +873,7 @@ typedef struct CaseExprState
873873
ExprState *arg; /* implicit equality comparison argument */
874874
List *args; /* the arguments (list of WHEN clauses) */
875875
ExprState *defresult; /* the default result (ELSE clause) */
876+
int16 argtyplen; /* if arg is provided, its typlen */
876877
} CaseExprState;
877878

878879
/* ----------------

src/include/utils/typcache.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -131,9 +131,9 @@ typedef struct DomainConstraintRef
131131
{
132132
List *constraints; /* list of DomainConstraintState nodes */
133133
MemoryContext refctx; /* context holding DomainConstraintRef */
134+
TypeCacheEntry *tcache; /* typcache entry for domain type */
134135

135136
/* Management data --- treat these fields as private to typcache.c */
136-
TypeCacheEntry *tcache; /* owning typcache entry */
137137
DomainConstraintCache *dcc; /* current constraints, or NULL if none */
138138
MemoryContextCallback callback; /* used to release refcount when done */
139139
} DomainConstraintRef;

src/test/regress/expected/case.out

+26
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,32 @@ SELECT CASE volfoo('bar') WHEN 'foo'::foodomain THEN 'is foo' ELSE 'is not foo'
338338
is not foo
339339
(1 row)
340340

341+
ROLLBACK;
342+
-- Test multiple evaluation of a CASE arg that is a read/write object (#14472)
343+
-- Wrap this in a single transaction so the transient '=' operator doesn't
344+
-- cause problems in concurrent sessions
345+
BEGIN;
346+
CREATE DOMAIN arrdomain AS int[];
347+
CREATE FUNCTION make_ad(int,int) returns arrdomain as
348+
'declare x arrdomain;
349+
begin
350+
x := array[$1,$2];
351+
return x;
352+
end' language plpgsql volatile;
353+
CREATE FUNCTION ad_eq(arrdomain, arrdomain) returns boolean as
354+
'begin return array_eq($1, $2); end' language plpgsql;
355+
CREATE OPERATOR = (procedure = ad_eq,
356+
leftarg = arrdomain, rightarg = arrdomain);
357+
SELECT CASE make_ad(1,2)
358+
WHEN array[2,4]::arrdomain THEN 'wrong'
359+
WHEN array[2,5]::arrdomain THEN 'still wrong'
360+
WHEN array[1,2]::arrdomain THEN 'right'
361+
END;
362+
case
363+
-------
364+
right
365+
(1 row)
366+
341367
ROLLBACK;
342368
--
343369
-- Clean up

src/test/regress/expected/plpgsql.out

+20
Original file line numberDiff line numberDiff line change
@@ -5662,3 +5662,23 @@ end;
56625662
$$;
56635663
ERROR: value for domain plpgsql_domain violates check constraint "plpgsql_domain_check"
56645664
CONTEXT: PL/pgSQL function inline_code_block line 4 at assignment
5665+
-- Test handling of expanded array passed to a domain constraint (bug #14472)
5666+
create function plpgsql_arr_domain_check(val int[]) returns boolean as $$
5667+
begin return val[1] > 0; end
5668+
$$ language plpgsql immutable;
5669+
create domain plpgsql_arr_domain as int[] check(plpgsql_arr_domain_check(value));
5670+
do $$
5671+
declare v_test plpgsql_arr_domain;
5672+
begin
5673+
v_test := array[1];
5674+
v_test := v_test || 2;
5675+
end;
5676+
$$;
5677+
do $$
5678+
declare v_test plpgsql_arr_domain := array[1];
5679+
begin
5680+
v_test := 0 || v_test; -- fail
5681+
end;
5682+
$$;
5683+
ERROR: value for domain plpgsql_arr_domain violates check constraint "plpgsql_arr_domain_check"
5684+
CONTEXT: PL/pgSQL function inline_code_block line 4 at assignment

src/test/regress/sql/case.sql

+28
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,34 @@ SELECT CASE volfoo('bar') WHEN 'foo'::foodomain THEN 'is foo' ELSE 'is not foo'
200200

201201
ROLLBACK;
202202

203+
-- Test multiple evaluation of a CASE arg that is a read/write object (#14472)
204+
-- Wrap this in a single transaction so the transient '=' operator doesn't
205+
-- cause problems in concurrent sessions
206+
BEGIN;
207+
208+
CREATE DOMAIN arrdomain AS int[];
209+
210+
CREATE FUNCTION make_ad(int,int) returns arrdomain as
211+
'declare x arrdomain;
212+
begin
213+
x := array[$1,$2];
214+
return x;
215+
end' language plpgsql volatile;
216+
217+
CREATE FUNCTION ad_eq(arrdomain, arrdomain) returns boolean as
218+
'begin return array_eq($1, $2); end' language plpgsql;
219+
220+
CREATE OPERATOR = (procedure = ad_eq,
221+
leftarg = arrdomain, rightarg = arrdomain);
222+
223+
SELECT CASE make_ad(1,2)
224+
WHEN array[2,4]::arrdomain THEN 'wrong'
225+
WHEN array[2,5]::arrdomain THEN 'still wrong'
226+
WHEN array[1,2]::arrdomain THEN 'right'
227+
END;
228+
229+
ROLLBACK;
230+
203231
--
204232
-- Clean up
205233
--

src/test/regress/sql/plpgsql.sql

+23
Original file line numberDiff line numberDiff line change
@@ -4450,3 +4450,26 @@ begin
44504450
v_test := 0; -- fail
44514451
end;
44524452
$$;
4453+
4454+
-- Test handling of expanded array passed to a domain constraint (bug #14472)
4455+
4456+
create function plpgsql_arr_domain_check(val int[]) returns boolean as $$
4457+
begin return val[1] > 0; end
4458+
$$ language plpgsql immutable;
4459+
4460+
create domain plpgsql_arr_domain as int[] check(plpgsql_arr_domain_check(value));
4461+
4462+
do $$
4463+
declare v_test plpgsql_arr_domain;
4464+
begin
4465+
v_test := array[1];
4466+
v_test := v_test || 2;
4467+
end;
4468+
$$;
4469+
4470+
do $$
4471+
declare v_test plpgsql_arr_domain := array[1];
4472+
begin
4473+
v_test := 0 || v_test; -- fail
4474+
end;
4475+
$$;

0 commit comments

Comments
 (0)