From 7942df89eed700dfcf71294e9e0aed821b737281 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Fri, 13 Mar 2020 19:53:42 -0500 Subject: [PATCH v2 2/2] Clean up existing anti-pattern of freeing SRF resources See also: e4186762ffaa4188e16702e8f4f299ea70988b96 Discussion: https://www.postgresql.org/message-id/8034.1583699444%40sss.pgh.pa.us --- contrib/pageinspect/btreefuncs.c | 17 ++++++----------- contrib/pageinspect/ginfuncs.c | 5 +++-- contrib/pageinspect/hashfuncs.c | 8 +++----- doc/src/sgml/xfunc.sgml | 12 +++++++++--- src/backend/access/transam/multixact.c | 5 +---- src/backend/tsearch/wparser.c | 13 ++++--------- src/backend/utils/adt/jsonfuncs.c | 16 ++-------------- src/backend/utils/adt/tsvector_op.c | 2 +- src/include/funcapi.h | 7 ++++--- 9 files changed, 33 insertions(+), 52 deletions(-) diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c index e6a2fc1e15..0f244e4f30 100644 --- a/contrib/pageinspect/btreefuncs.c +++ b/contrib/pageinspect/btreefuncs.c @@ -502,12 +502,9 @@ bt_page_items(PG_FUNCTION_ARGS) uargs->offset++; SRF_RETURN_NEXT(fctx, result); } - else - { - pfree(uargs->page); - pfree(uargs); - SRF_RETURN_DONE(fctx); - } + + /* allocations in multi_call_memory_ctx are released automatically */ + SRF_RETURN_DONE(fctx); } /*------------------------------------------------------- @@ -590,11 +587,9 @@ bt_page_items_bytea(PG_FUNCTION_ARGS) uargs->offset++; SRF_RETURN_NEXT(fctx, result); } - else - { - pfree(uargs); - SRF_RETURN_DONE(fctx); - } + + /* allocations in multi_call_memory_ctx are released automatically */ + SRF_RETURN_DONE(fctx); } /* Number of output arguments (columns) for bt_metap() */ diff --git a/contrib/pageinspect/ginfuncs.c b/contrib/pageinspect/ginfuncs.c index 7e2cafab74..5109c4b133 100644 --- a/contrib/pageinspect/ginfuncs.c +++ b/contrib/pageinspect/ginfuncs.c @@ -260,6 +260,7 @@ gin_leafpage_items(PG_FUNCTION_ARGS) SRF_RETURN_NEXT(fctx, result); } - else - SRF_RETURN_DONE(fctx); + + /* allocations in multi_call_memory_ctx are released automatically */ + SRF_RETURN_DONE(fctx); } diff --git a/contrib/pageinspect/hashfuncs.c b/contrib/pageinspect/hashfuncs.c index 984ac33186..d024f1ac7b 100644 --- a/contrib/pageinspect/hashfuncs.c +++ b/contrib/pageinspect/hashfuncs.c @@ -374,11 +374,9 @@ hash_page_items(PG_FUNCTION_ARGS) SRF_RETURN_NEXT(fctx, result); } - else - { - pfree(uargs); - SRF_RETURN_DONE(fctx); - } + + /* allocations in multi_call_memory_ctx are released automatically */ + SRF_RETURN_DONE(fctx); } /* ------------------------------------------------ diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 432758e75e..f3e96087fa 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -2861,7 +2861,8 @@ typedef struct FuncCallContext * OPTIONAL pointer to miscellaneous user-provided context information * * user_fctx is for use as a pointer to your own data to retain - * arbitrary context information between calls of your function. + * context information between calls of your function. + * It should probably not include resources other than allocated memory. */ void *user_fctx; @@ -2881,7 +2882,8 @@ typedef struct FuncCallContext * multi_call_memory_ctx is set by SRF_FIRSTCALL_INIT() for you, and used * by SRF_RETURN_DONE() for cleanup. It is the most appropriate memory * context for any memory that is to be reused across multiple calls - * of the SRF. + * of the SRF. Memory palloc'd in this context will be pfree'd + * automatically. */ MemoryContext multi_call_memory_ctx; @@ -2935,6 +2937,7 @@ SRF_RETURN_NEXT(funcctx, result) SRF_RETURN_DONE(funcctx) to clean up and end the SRF. + Allocations within multi_call_memory_ctx are automatically pfree'd. @@ -3004,7 +3007,10 @@ my_set_returning_function(PG_FUNCTION_ARGS) } else { - /* Here we are done returning items and just need to clean up: */ + /* + * Here we are done returning items and just need to clean up, but no + * need to pfree allocations within multi_call_memory_ctx. + */ user code SRF_RETURN_DONE(funcctx); } diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index 50e98caaeb..cb92e049aa 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -3388,9 +3388,6 @@ pg_get_multixact_members(PG_FUNCTION_ARGS) SRF_RETURN_NEXT(funccxt, HeapTupleGetDatum(tuple)); } - if (multi->nmembers > 0) - pfree(multi->members); - pfree(multi); - + /* allocations in multi_call_memory_ctx are released automatically */ SRF_RETURN_DONE(funccxt); } diff --git a/src/backend/tsearch/wparser.c b/src/backend/tsearch/wparser.c index 88005c0519..85eaf041d6 100644 --- a/src/backend/tsearch/wparser.c +++ b/src/backend/tsearch/wparser.c @@ -104,9 +104,8 @@ tt_process_call(FuncCallContext *funcctx) st->cur++; return result; } - if (st->list) - pfree(st->list); - pfree(st); + + /* allocations in multi_call_memory_ctx are released automatically */ return (Datum) 0; } @@ -245,12 +244,8 @@ prs_process_call(FuncCallContext *funcctx) st->cur++; return result; } - else - { - if (st->list) - pfree(st->list); - pfree(st); - } + + /* allocations in multi_call_memory_ctx are released automatically */ return (Datum) 0; } diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index f92861d8d2..c5a286dd58 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -535,7 +535,6 @@ jsonb_object_keys(PG_FUNCTION_ARGS) { FuncCallContext *funcctx; OkeysState *state; - int i; if (SRF_IS_FIRSTCALL()) { @@ -598,12 +597,7 @@ jsonb_object_keys(PG_FUNCTION_ARGS) SRF_RETURN_NEXT(funcctx, CStringGetTextDatum(nxt)); } - /* cleanup to reduce or eliminate memory leaks */ - for (i = 0; i < state->result_count; i++) - pfree(state->result[i]); - pfree(state->result); - pfree(state); - + /* allocations in multi_call_memory_ctx are released automatically */ SRF_RETURN_DONE(funcctx); } @@ -706,7 +700,6 @@ json_object_keys(PG_FUNCTION_ARGS) { FuncCallContext *funcctx; OkeysState *state; - int i; if (SRF_IS_FIRSTCALL()) { @@ -755,12 +748,7 @@ json_object_keys(PG_FUNCTION_ARGS) SRF_RETURN_NEXT(funcctx, CStringGetTextDatum(nxt)); } - /* cleanup to reduce or eliminate memory leaks */ - for (i = 0; i < state->result_count; i++) - pfree(state->result[i]); - pfree(state->result); - pfree(state); - + /* allocations in multi_call_memory_ctx are released automatically */ SRF_RETURN_DONE(funcctx); } diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c index 108dd998c7..e7b8fbc3ca 100644 --- a/src/backend/utils/adt/tsvector_op.c +++ b/src/backend/utils/adt/tsvector_op.c @@ -706,7 +706,7 @@ tsvector_unnest(PG_FUNCTION_ARGS) } else { - pfree(tsin); + /* allocations in multi_call_memory_ctx are released automatically */ SRF_RETURN_DONE(funcctx); } } diff --git a/src/include/funcapi.h b/src/include/funcapi.h index 471100b7e5..8f6db97fcf 100644 --- a/src/include/funcapi.h +++ b/src/include/funcapi.h @@ -77,7 +77,7 @@ typedef struct FuncCallContext * OPTIONAL pointer to miscellaneous user-provided context information * * user_fctx is for use as a pointer to your own struct to retain - * arbitrary context information between calls of your function. + * context information between calls of your function. */ void *user_fctx; @@ -93,8 +93,9 @@ typedef struct FuncCallContext /* * memory context used for structures that must live for multiple calls * - * multi_call_memory_ctx is set by SRF_FIRSTCALL_INIT() for you, and used - * by SRF_RETURN_DONE() for cleanup. It is the most appropriate memory + * multi_call_memory_ctx is set by SRF_FIRSTCALL_INIT() for you, and + * automatically cleaned up by SRF_RETURN_DONE(). It is the most + * appropriate memory * context for any memory that is to be reused across multiple calls of * the SRF. */ -- 2.17.0