From 59ad4d20f16b6dd79a1328296fa79f9f997b47e9 Mon Sep 17 00:00:00 2001 From: Jerry Sievert Date: Sun, 19 Oct 2025 16:25:39 -0700 Subject: [PATCH 2/2] Updates SRF handling and timeouts for array spread tests The RISC-V architecture exposed two issues: 1. There was the possibility of undefined behaviour in Set Returing Functions (SRF), which had appeared to be transient, but was consistent in the RISC-V architecture. The SRF return functionality was seperated by use case and cleaned up. This code is a good target for refactoring and simplification in the future. In addition, some explicit memory deallocation was added, which should help memory pressure in large object or large array returns. 2. I had arbitrarily chosen a timeout of 5 seconds for the `array_spread` tests (these are plv8 regression tests that show sane handling for a known V8 issue, QuickJS should always pass these tests), some architectures can take longer than that, so the timout interrupt was triggered instead of the out of memory error. I've raised this to 60 seconds to help the execution speed differences. Hopefully this will be enough, but it can be adjusted as needed from this arbitary number. --- expected/array_spread.out | 2 +- sql/array_spread.sql | 4 +- src/functions.c | 12 +++- src/pljs.c | 23 +++++--- src/pljs.h | 6 +- src/types.c | 112 +++++++++++++++++++++++++++++--------- 6 files changed, 116 insertions(+), 43 deletions(-) diff --git a/expected/array_spread.out b/expected/array_spread.out index ff38959..b514358 100644 --- a/expected/array_spread.out +++ b/expected/array_spread.out @@ -1,5 +1,5 @@ -- ten seconds should be enough to show this doesn't destroy memory -set statement_timeout = '5s'; +set statement_timeout = '60s'; set pljs.memory_limit = '256'; do $$ Object.prototype [Symbol.iterator] = function() { return { next:() => this } }; [...({})]; diff --git a/sql/array_spread.sql b/sql/array_spread.sql index 930f268..690858f 100644 --- a/sql/array_spread.sql +++ b/sql/array_spread.sql @@ -1,5 +1,5 @@ -- ten seconds should be enough to show this doesn't destroy memory -set statement_timeout = '5s'; +set statement_timeout = '60s'; set pljs.memory_limit = '256'; do $$ Object.prototype [Symbol.iterator] = function() { return { next:() => this } }; @@ -8,4 +8,4 @@ $$ language pljs; do $$ Object.prototype [Symbol.iterator] = function() { return { next:() => this } }; [...({})]; -$$ language pljs; \ No newline at end of file +$$ language pljs; diff --git a/src/functions.c b/src/functions.c index 09ff9c5..03e5a9f 100644 --- a/src/functions.c +++ b/src/functions.c @@ -1069,9 +1069,15 @@ static JSValue pljs_return_next(JSContext *ctx, JSValueConst this_val, int argc, return js_throw("field name / property name mismatch", ctx); } - bool is_null = false; - pljs_jsvalue_to_record(argv[0], NULL, ctx, &is_null, retstate->tuple_desc, - retstate->tuple_store_state); + bool *nulls = (bool *)palloc0(sizeof(bool) * retstate->tuple_desc->natts); + Datum *values = pljs_jsvalue_to_datums(argv[0], NULL, ctx, &nulls, + retstate->tuple_desc); + + tuplestore_putvalues(retstate->tuple_store_state, retstate->tuple_desc, + values, nulls); + + pfree(nulls); + pfree(values); } else { bool is_null = false; Datum result = pljs_jsvalue_to_datum( diff --git a/src/pljs.c b/src/pljs.c index 6ab25eb..a84e3b9 100644 --- a/src/pljs.c +++ b/src/pljs.c @@ -986,8 +986,7 @@ static Datum call_trigger(FunctionCallInfo fcinfo, pljs_context *context) { pljs_type type; pljs_type_fill(&type, context->function->rettype); - Datum d = - pljs_jsvalue_to_record(ret, &type, context->ctx, NULL, tupdesc, NULL); + Datum d = pljs_jsvalue_to_record(ret, &type, context->ctx, NULL, tupdesc); HeapTupleHeader header = DatumGetHeapTupleHeader(d); @@ -1056,7 +1055,7 @@ static Datum call_function(FunctionCallInfo fcinfo, pljs_context *context, /* Shuts up the compiler, since ereports of ERROR stop execution. */ return (Datum)0; } else { - Datum datum; + Datum datum = 0; if (rettype == RECORDOID) { TupleDesc tupdesc; @@ -1065,8 +1064,7 @@ static Datum call_function(FunctionCallInfo fcinfo, pljs_context *context, pljs_type type; pljs_type_fill(&type, rettype); - datum = - pljs_jsvalue_to_record(ret, &type, context->ctx, NULL, tupdesc, NULL); + datum = pljs_jsvalue_to_record(ret, &type, context->ctx, NULL, tupdesc); } else { bool is_null; datum = @@ -1183,14 +1181,21 @@ static Datum call_srf_function(FunctionCallInfo fcinfo, pljs_context *context, return (Datum)0; } else { // Check to see if we have any values to append - if (!JS_IsUndefined(ret) || !JS_IsNull(ret)) { + if (!JS_IsUndefined(ret) && !JS_IsNull(ret)) { MemoryContextSwitchTo(rsinfo->econtext->ecxt_per_query_memory); - bool is_null; + bool is_null = false; if (state->is_composite) { - pljs_jsvalue_to_record(ret, NULL, context->ctx, &is_null, - state->tuple_desc, state->tuple_store_state); + bool *nulls = (bool *)palloc0(sizeof(bool) * state->tuple_desc->natts); + + Datum *values = pljs_jsvalue_to_datums(argv[0], NULL, context->ctx, + &nulls, state->tuple_desc); + tuplestore_putvalues(state->tuple_store_state, state->tuple_desc, + values, nulls); + + pfree(nulls); + pfree(values); } else { if (JS_IsArray(context->ctx, ret)) { for (uint32_t i = 0; i < pljs_js_array_length(ret, context->ctx); diff --git a/src/pljs.h b/src/pljs.h index 477a7f8..5dbea06 100644 --- a/src/pljs.h +++ b/src/pljs.h @@ -188,13 +188,15 @@ JSValue pljs_datum_to_object(Datum arg, pljs_type *type, JSContext *ctx); JSValue pljs_tuple_to_jsvalue(TupleDesc, HeapTuple, JSContext *ctx); JSValue pljs_spi_result_to_jsvalue(int, JSContext *); -// To Datum +// To Postgres Datum pljs_jsvalue_to_array(JSValue, pljs_type *, JSContext *, FunctionCallInfo); Datum pljs_jsvalue_to_datum(JSValue, Oid, JSContext *, FunctionCallInfo, bool *); Datum pljs_jsvalue_to_record(JSValue val, pljs_type *type, JSContext *ctx, - bool *is_null, TupleDesc, Tuplestorestate *); + bool *is_null, TupleDesc); +Datum *pljs_jsvalue_to_datums(JSValue val, pljs_type *type, JSContext *ctx, + bool **nulls, TupleDesc tupdesc); // Utility uint32_t pljs_js_array_length(JSValue, JSContext *); diff --git a/src/types.c b/src/types.c index 807ccbb..e874b77 100644 --- a/src/types.c +++ b/src/types.c @@ -644,6 +644,73 @@ bool pljs_jsvalue_object_contains_all_column_names(JSValue val, JSContext *ctx, return true; } +/** + * @brief Converts a composite Javascript object into an array of Datums. + * + * Takes a Javascript object and converts it into an array of Datums, setting + * the null flag for each Datum if it is null. Note that this function assumes + * that `nulls` is allocated and initialied to `0` (`false`) for each element. + * + * @param val #JSValue - the Javascript object to convert + * @oaram type #pljs_type - type information for the record + * @param ctx #JSContext - Javascript context to execute in + * @param is_null @c bool - pointer to fill of whether the record is null + * @param tupdesc #TupleDesc - can be `NULL` + * @returns Array of #Datum of the Javascript object + */ +Datum *pljs_jsvalue_to_datums(JSValue val, pljs_type *type, JSContext *ctx, + bool **nulls, TupleDesc tupdesc) { + Datum *values = (Datum *)palloc(sizeof(Datum) * tupdesc->natts); + + if (JS_IsNull(val) || JS_IsUndefined(val)) { + return NULL; + } + + // If we are not passed a tuple descriptor, then we need to look it up and + // release it after we are done. + bool cleanup_tupdesc = false; + + if (tupdesc == NULL) { + Oid rettype = type->typid; + + tupdesc = lookup_rowtype_tupdesc(rettype, -1); + cleanup_tupdesc = true; + } + + if (tupdesc != NULL) { + for (int16 c = 0; c < tupdesc->natts; c++) { + // If this is a dropped column, we can skip it, and set the null flag to + // true. + if (TupleDescAttr(tupdesc, c)->attisdropped) { + *nulls[c] = true; + continue; + } + + // Retrieve the column name of each attribute that we are expecting, we + // only care about named tuples. + char *colname = NameStr(TupleDescAttr(tupdesc, c)->attname); + + JSValue o = JS_GetPropertyStr(ctx, val, colname); + + if (JS_IsNull(o) || JS_IsUndefined(o)) { + *nulls[c] = true; + continue; + } + + // Set the value of each Datum, or set the `nulls` flag if it is + // considered `NULL`. + values[c] = pljs_jsvalue_to_datum(o, TupleDescAttr(tupdesc, c)->atttypid, + ctx, NULL, nulls[c]); + } + + if (cleanup_tupdesc) { + ReleaseTupleDesc(tupdesc); + } + } + + return values; +} + /** * @brief Converts a Javascript object into a Postgres record. * @@ -655,40 +722,33 @@ bool pljs_jsvalue_object_contains_all_column_names(JSValue val, JSContext *ctx, * @param ctx #JSContext - Javascript context to execute in * @param is_null @c bool - pointer to fill of whether the record is null * @param tupdesc #TupleDesc - can be `NULL` - * @param tupstore #Tuplestorestate * @returns #Datum of the Postgres record */ Datum pljs_jsvalue_to_record(JSValue val, pljs_type *type, JSContext *ctx, - bool *is_null, TupleDesc tupdesc, - Tuplestorestate *tupstore) { + bool *is_null, TupleDesc tupdesc) { Datum result = 0; + // If the value is null or undefined, we can simply set the record to null + // and return a `NULL` Datum. if (JS_IsNull(val) || JS_IsUndefined(val)) { *is_null = true; return (Datum)0; } + // If we are not passed a tuple descriptor, then we need to look it up and + // release it after we are done. bool cleanup_tupdesc = false; - PG_TRY(); - { - if (tupdesc == NULL) { - Oid rettype = type->typid; - tupdesc = lookup_rowtype_tupdesc(rettype, -1); - cleanup_tupdesc = true; - } - } - PG_CATCH(); - { - PG_RE_THROW(); + if (tupdesc == NULL) { + Oid rettype = type->typid; + + tupdesc = lookup_rowtype_tupdesc(rettype, -1); + cleanup_tupdesc = true; } - PG_END_TRY(); if (tupdesc != NULL) { - Datum *values = (Datum *)palloc(sizeof(Datum) * tupdesc->natts); - bool *nulls = (bool *)palloc(sizeof(bool) * tupdesc->natts); - - memset(nulls, 0, sizeof(bool) * tupdesc->natts); + Datum *values = (Datum *)palloc0(sizeof(Datum) * tupdesc->natts); + bool *nulls = (bool *)palloc0(sizeof(bool) * tupdesc->natts); for (int16 c = 0; c < tupdesc->natts; c++) { if (TupleDescAttr(tupdesc, c)->attisdropped) { @@ -709,12 +769,12 @@ Datum pljs_jsvalue_to_record(JSValue val, pljs_type *type, JSContext *ctx, ctx, NULL, &nulls[c]); } - if (tupstore != NULL) { - result = (Datum)0; - tuplestore_putvalues(tupstore, tupdesc, values, nulls); - } else { - result = HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)); - } + // Form a Tuple from the values and nulls using the tuple descriptor + // as the template for the tuple. + result = HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)); + + pfree(nulls); + pfree(values); if (cleanup_tupdesc) { ReleaseTupleDesc(tupdesc); @@ -822,7 +882,7 @@ Datum pljs_jsvalue_to_datum(JSValue val, Oid rettype, JSContext *ctx, } if (type.is_composite) { - return pljs_jsvalue_to_record(val, &type, ctx, isnull, NULL, NULL); + return pljs_jsvalue_to_record(val, &type, ctx, isnull, NULL); } if (JS_IsNull(val) || JS_IsUndefined(val)) { -- 2.51.0