Copilot commented on code in PR #2448:
URL: https://github.com/apache/age/pull/2448#discussion_r3484716493
##########
regress/age_load/data/junk.csv:
##########
@@ -0,0 +1,54 @@
+id,name,iso3,iso2,numeric_code,phone_code,capital,currency,currency_symbol,tld,native,region,subregion,latitude,longitude,emoji,emojiU
+2,Aland Islands,ALA,AX,248,+358-18,Mariehamn,EUR,€,.ax,Åland,Europe,Northern
Europe,60.116667,19.9,🇦🇽,U+1F1E6 U+1F1FD
+3,Albania,ALB,AL,8,355,Tirana,ALL,Lek,.al,Shqipëria,Europe,Southern
Europe,41.0,20.0,🇦🇱,U+1F1E6 U+1F1F1
+6,Andorra,AND,AD,20,376,Andorra la Vella,EUR,€,.ad,Andorra,Europe,Southern
Europe,42.5,1.5,🇦🇩,U+1F1E6 U+1F1E9
+15,Austria,AUT,AT,40,43,Vienna,EUR,€,.at,Österreich,Europe,Western
Europe,47.33333333,13.33333333,🇦🇹,U+1F1E6 U+1F1F9
Review Comment:
This new CSV fixture does not appear to be referenced by any regression SQL
(e.g. regress/sql/age_load.sql copies the directory but only loads specific
files like countries.csv/cities.csv/edges.csv). If it is not needed for tests,
consider removing it to avoid adding unused data to the repository.
##########
src/backend/utils/adt/agtype.c:
##########
@@ -11718,8 +11746,43 @@ Datum age_reduce_transfn(PG_FUNCTION_ARGS)
rc->params[1].isnull = false;
rc->params[1].execPlan = NULL;
- /* evaluate the fold body for this element */
- ResetExprContext(rc->econtext);
+ /*
+ * Bind the captured loop-invariant outer values to params 2 .. The values
+ * are pulled from the extras array every row because correlated captures
+ * differ between groups; the per-row deconstruction is done in the
+ * econtext's per-tuple memory (reset above) so it does not leak. A NULL
+ * array element is normalized to agtype 'null' like the accumulator and
+ * element.
+ */
+ if (rc->nparams > 2 && !PG_ARGISNULL(4))
+ {
+ ArrayType *extras_arr = PG_GETARG_ARRAYTYPE_P(4);
+ Oid elemtype = ARR_ELEMTYPE(extras_arr);
+ int16 typlen;
+ bool typbyval;
+ char typalign;
+ Datum *ex_vals;
+ bool *ex_nulls;
+ int ex_n;
+ int i;
+ MemoryContext per_tuple = rc->econtext->ecxt_per_tuple_memory;
+ MemoryContext save = MemoryContextSwitchTo(per_tuple);
+
+ get_typlenbyvalalign(elemtype, &typlen, &typbyval, &typalign);
+ deconstruct_array(extras_arr, elemtype, typlen, typbyval, typalign,
+ &ex_vals, &ex_nulls, &ex_n);
+
+ for (i = 0; i < ex_n && (2 + i) < rc->nparams; i++)
+ {
+ rc->params[2 + i].value = ex_nulls[i] ? reduce_agtype_null()
+ : ex_vals[i];
+ rc->params[2 + i].isnull = false;
+ rc->params[2 + i].execPlan = NULL;
+ }
Review Comment:
If the extras array deconstructs to fewer elements than rc->nparams - 2
(e.g. SQL-callable age_reduce() invoked with a varying-length extras array),
the remaining PARAM_EXEC slots 2.. stay as their previous
values/zero-initialized (ParamExecData.isnull defaults to false). That can
yield stale outer values or backend crashes when the fold body reads those
params. Bind all captured slots 2.. on every call, filling missing entries with
an agtype 'null' Datum.
##########
src/backend/utils/adt/agtype.c:
##########
@@ -11660,6 +11669,20 @@ Datum age_reduce_transfn(PG_FUNCTION_ARGS)
errmsg("age_reduce: missing fold expression")));
}
+ /*
+ * The number of captured outer values is fixed for this aggregate
+ * call (the body's structure does not change between rows), so it is
+ * read once here to size the param array. Their values are bound per
+ * row below because a correlated capture changes between groups.
+ */
+ if (!PG_ARGISNULL(4))
+ {
+ ArrayType *extras_arr = PG_GETARG_ARRAYTYPE_P(4);
+
+ n_extras = ArrayGetNItems(ARR_NDIM(extras_arr),
+ ARR_DIMS(extras_arr));
+ }
Review Comment:
age_reduce_transfn() unconditionally reads argument 4 (extras) during
one-time initialization. If an older extension definition (or any SQL wrapper)
still calls this C symbol with the pre-change 4-argument signature,
PG_ARGISNULL(4)/PG_GETARG_ARRAYTYPE_P(4) will read past fcinfo->nargs and can
crash the backend. Guard accesses to arg 4 with PG_NARGS() > 4 and treat
missing extras as zero captures for backward/upgrade safety.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
