From b4570d33aa045df330bb325ba8a2cbf02266a555 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 16 Mar 2020 21:05:28 -0400 Subject: [PATCH] Avoid holding a directory FD open across assorted SRF calls. This extends the fixes made in commit 085b6b667 to other SRFs with the same bug, namely pg_logdir_ls(), pgrowlocks(), pg_timezone_names(), pg_ls_dir(), and pg_tablespace_databases(). Also adjust various comments and documentation to warn against expecting to clean up resources during a ValuePerCall SRF's final call. Back-patch to all supported branches, since these functions were all born broken. Justin Pryzby, with cosmetic tweaks by me Discussion: https://postgr.es/m/20200308173103.GC1357@telsasoft.com --- contrib/adminpack/adminpack.c | 78 +++++----- contrib/pgrowlocks/pgrowlocks.c | 153 ++++++++----------- doc/src/sgml/xfunc.sgml | 90 +++++++---- src/backend/utils/adt/datetime.c | 107 ++++++------- src/backend/utils/adt/genfile.c | 120 +++++++-------- src/backend/utils/adt/misc.c | 125 ++++++++------- src/backend/utils/fmgr/README | 16 +- src/include/funcapi.h | 13 +- src/test/regress/expected/misc_functions.out | 21 +++ src/test/regress/sql/misc_functions.sql | 9 ++ 10 files changed, 386 insertions(+), 346 deletions(-) diff --git a/contrib/adminpack/adminpack.c b/contrib/adminpack/adminpack.c index bc45e79895..7d0a19b294 100644 --- a/contrib/adminpack/adminpack.c +++ b/contrib/adminpack/adminpack.c @@ -56,11 +56,6 @@ static int64 pg_file_write_internal(text *file, text *data, bool replace); static bool pg_file_rename_internal(text *file1, text *file2, text *file3); static Datum pg_logdir_ls_internal(FunctionCallInfo fcinfo); -typedef struct -{ - char *location; - DIR *dirdesc; -} directory_fctx; /*----------------------- * some helper functions @@ -504,50 +499,51 @@ pg_logdir_ls_v1_1(PG_FUNCTION_ARGS) static Datum pg_logdir_ls_internal(FunctionCallInfo fcinfo) { - FuncCallContext *funcctx; + ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; + bool randomAccess; + TupleDesc tupdesc; + Tuplestorestate *tupstore; + AttInMetadata *attinmeta; + DIR *dirdesc; struct dirent *de; - directory_fctx *fctx; + MemoryContext oldcontext; if (strcmp(Log_filename, "postgresql-%Y-%m-%d_%H%M%S.log") != 0) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("the log_filename parameter must equal 'postgresql-%%Y-%%m-%%d_%%H%%M%%S.log'"))); - if (SRF_IS_FIRSTCALL()) - { - MemoryContext oldcontext; - TupleDesc tupdesc; + /* check to see if caller supports us returning a tuplestore */ + if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("set-valued function called in context that cannot accept a set"))); + if (!(rsinfo->allowedModes & SFRM_Materialize)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("materialize mode required, but it is not allowed in this context"))); - funcctx = SRF_FIRSTCALL_INIT(); - oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx); + /* The tupdesc and tuplestore must be created in ecxt_per_query_memory */ + oldcontext = MemoryContextSwitchTo(rsinfo->econtext->ecxt_per_query_memory); - fctx = palloc(sizeof(directory_fctx)); + tupdesc = CreateTemplateTupleDesc(2); + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "starttime", + TIMESTAMPOID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 2, "filename", + TEXTOID, -1, 0); - tupdesc = CreateTemplateTupleDesc(2); - TupleDescInitEntry(tupdesc, (AttrNumber) 1, "starttime", - TIMESTAMPOID, -1, 0); - TupleDescInitEntry(tupdesc, (AttrNumber) 2, "filename", - TEXTOID, -1, 0); + randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0; + tupstore = tuplestore_begin_heap(randomAccess, false, work_mem); + rsinfo->returnMode = SFRM_Materialize; + rsinfo->setResult = tupstore; + rsinfo->setDesc = tupdesc; - funcctx->attinmeta = TupleDescGetAttInMetadata(tupdesc); + MemoryContextSwitchTo(oldcontext); - fctx->location = pstrdup(Log_directory); - fctx->dirdesc = AllocateDir(fctx->location); + attinmeta = TupleDescGetAttInMetadata(tupdesc); - if (!fctx->dirdesc) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not open directory \"%s\": %m", - fctx->location))); - - funcctx->user_fctx = fctx; - MemoryContextSwitchTo(oldcontext); - } - - funcctx = SRF_PERCALL_SETUP(); - fctx = (directory_fctx *) funcctx->user_fctx; - - while ((de = ReadDir(fctx->dirdesc, fctx->location)) != NULL) + dirdesc = AllocateDir(Log_directory); + while ((de = ReadDir(dirdesc, Log_directory)) != NULL) { char *values[2]; HeapTuple tuple; @@ -584,13 +580,13 @@ pg_logdir_ls_internal(FunctionCallInfo fcinfo) /* Seems the timestamp is OK; prepare and return tuple */ values[0] = timestampbuf; - values[1] = psprintf("%s/%s", fctx->location, de->d_name); + values[1] = psprintf("%s/%s", Log_directory, de->d_name); - tuple = BuildTupleFromCStrings(funcctx->attinmeta, values); + tuple = BuildTupleFromCStrings(attinmeta, values); - SRF_RETURN_NEXT(funcctx, HeapTupleGetDatum(tuple)); + tuplestore_puttuple(tupstore, tuple); } - FreeDir(fctx->dirdesc); - SRF_RETURN_DONE(funcctx); + FreeDir(dirdesc); + return (Datum) 0; } diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c index a2c44a916c..714398831b 100644 --- a/contrib/pgrowlocks/pgrowlocks.c +++ b/contrib/pgrowlocks/pgrowlocks.c @@ -54,13 +54,6 @@ PG_FUNCTION_INFO_V1(pgrowlocks); #define NCHARS 32 -typedef struct -{ - Relation rel; - TableScanDesc scan; - int ncolumns; -} MyData; - #define Atnum_tid 0 #define Atnum_xmax 1 #define Atnum_ismulti 2 @@ -71,84 +64,86 @@ typedef struct Datum pgrowlocks(PG_FUNCTION_ARGS) { - FuncCallContext *funcctx; + text *relname = PG_GETARG_TEXT_PP(0); + ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; + bool randomAccess; + TupleDesc tupdesc; + Tuplestorestate *tupstore; + AttInMetadata *attinmeta; + Relation rel; + RangeVar *relrv; TableScanDesc scan; HeapScanDesc hscan; HeapTuple tuple; - TupleDesc tupdesc; - AttInMetadata *attinmeta; - Datum result; - MyData *mydata; - Relation rel; + MemoryContext oldcontext; + AclResult aclresult; + char **values; - if (SRF_IS_FIRSTCALL()) - { - text *relname; - RangeVar *relrv; - MemoryContext oldcontext; - AclResult aclresult; + /* check to see if caller supports us returning a tuplestore */ + if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("set-valued function called in context that cannot accept a set"))); + if (!(rsinfo->allowedModes & SFRM_Materialize)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("materialize mode required, but it is not allowed in this context"))); - funcctx = SRF_FIRSTCALL_INIT(); - oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx); + /* The tupdesc and tuplestore must be created in ecxt_per_query_memory */ + oldcontext = MemoryContextSwitchTo(rsinfo->econtext->ecxt_per_query_memory); - /* Build a tuple descriptor for our result type */ - if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) - elog(ERROR, "return type must be a row type"); + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) + elog(ERROR, "return type must be a row type"); - attinmeta = TupleDescGetAttInMetadata(tupdesc); - funcctx->attinmeta = attinmeta; + randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0; + tupstore = tuplestore_begin_heap(randomAccess, false, work_mem); + rsinfo->returnMode = SFRM_Materialize; + rsinfo->setResult = tupstore; + rsinfo->setDesc = tupdesc; - relname = PG_GETARG_TEXT_PP(0); - relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname)); - rel = relation_openrv(relrv, AccessShareLock); + MemoryContextSwitchTo(oldcontext); - if (rel->rd_rel->relam != HEAP_TABLE_AM_OID) - ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("only heap AM is supported"))); + /* Access the table */ + relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname)); + rel = relation_openrv(relrv, AccessShareLock); - if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is a partitioned table", - RelationGetRelationName(rel)), - errdetail("Partitioned tables do not contain rows."))); - else if (rel->rd_rel->relkind != RELKIND_RELATION) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a table", - RelationGetRelationName(rel)))); + if (rel->rd_rel->relam != HEAP_TABLE_AM_OID) + ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("only heap AM is supported"))); - /* - * check permissions: must have SELECT on table or be in - * pg_stat_scan_tables - */ - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(), - ACL_SELECT); - if (aclresult != ACLCHECK_OK) - aclresult = is_member_of_role(GetUserId(), DEFAULT_ROLE_STAT_SCAN_TABLES) ? ACLCHECK_OK : ACLCHECK_NO_PRIV; + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is a partitioned table", + RelationGetRelationName(rel)), + errdetail("Partitioned tables do not contain rows."))); + else if (rel->rd_rel->relkind != RELKIND_RELATION) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is not a table", + RelationGetRelationName(rel)))); - if (aclresult != ACLCHECK_OK) - aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), - RelationGetRelationName(rel)); + /* + * check permissions: must have SELECT on table or be in + * pg_stat_scan_tables + */ + aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(), + ACL_SELECT); + if (aclresult != ACLCHECK_OK) + aclresult = is_member_of_role(GetUserId(), DEFAULT_ROLE_STAT_SCAN_TABLES) ? ACLCHECK_OK : ACLCHECK_NO_PRIV; - scan = table_beginscan(rel, GetActiveSnapshot(), 0, NULL); - hscan = (HeapScanDesc) scan; - mydata = palloc(sizeof(*mydata)); - mydata->rel = rel; - mydata->scan = scan; - mydata->ncolumns = tupdesc->natts; - funcctx->user_fctx = mydata; + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), + RelationGetRelationName(rel)); - MemoryContextSwitchTo(oldcontext); - } - - funcctx = SRF_PERCALL_SETUP(); - attinmeta = funcctx->attinmeta; - mydata = (MyData *) funcctx->user_fctx; - scan = mydata->scan; + /* Scan the relation */ + scan = table_beginscan(rel, GetActiveSnapshot(), 0, NULL); hscan = (HeapScanDesc) scan; - /* scan the relation */ + attinmeta = TupleDescGetAttInMetadata(tupdesc); + + values = (char **) palloc(tupdesc->natts * sizeof(char *)); + while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { TM_Result htsu; @@ -169,10 +164,6 @@ pgrowlocks(PG_FUNCTION_ARGS) */ if (htsu == TM_BeingModified) { - char **values; - - values = (char **) palloc(mydata->ncolumns * sizeof(char *)); - values[Atnum_tid] = (char *) DirectFunctionCall1(tidout, PointerGetDatum(&tuple->t_self)); @@ -297,16 +288,7 @@ pgrowlocks(PG_FUNCTION_ARGS) /* build a tuple */ tuple = BuildTupleFromCStrings(attinmeta, values); - - /* make the tuple into a datum */ - result = HeapTupleGetDatum(tuple); - - /* - * no need to pfree what we allocated; it's on a short-lived - * memory context anyway - */ - - SRF_RETURN_NEXT(funcctx, result); + tuplestore_puttuple(tupstore, tuple); } else { @@ -315,7 +297,6 @@ pgrowlocks(PG_FUNCTION_ARGS) } table_endscan(scan); - table_close(mydata->rel, AccessShareLock); - - SRF_RETURN_DONE(funcctx); + table_close(rel, AccessShareLock); + return (Datum) 0; } diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index a91f887119..6350f92f86 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -2812,22 +2812,50 @@ HeapTupleGetDatum(HeapTuple tuple) Returning Sets - There is also a special API that provides support for returning - sets (multiple rows) from a C-language function. A set-returning - function must follow the version-1 calling conventions. Also, - source files must include funcapi.h, as - above. + C-language functions have two options for returning sets (multiple + rows). In one method, called ValuePerCall + mode, a set-returning function is called repeatedly (passing the same + arguments each time) and it returns one new row on each call, until + it has no more rows to return and signals that by returning NULL. + The set-returning function (SRF) must therefore + save enough state across calls to remember what it was doing and + return the correct next item on each call. + In the other method, called Materialize mode, + a SRF fills and returns a tuplestore object containing its + entire result; then only one call occurs for the whole result, and + no inter-call state is needed. - A set-returning function (SRF) is called - once for each item it returns. The SRF must - therefore save enough state to remember what it was doing and - return the next item on each call. - The structure FuncCallContext is provided to help - control this process. Within a function, fcinfo->flinfo->fn_extra - is used to hold a pointer to FuncCallContext - across calls. + When using ValuePerCall mode, it is important to remember that the + query is not guaranteed to be run to completion; that is, due to + options such as LIMIT, the executor might stop + making calls to the set-returning function before all rows have been + fetched. This means it is not safe to perform cleanup activities in + the last call, because that might not ever happen. It's recommended + to use Materialize mode for functions that need access to external + resources, such as file descriptors. + + + + The remainder of this section documents a set of helper macros that + are commonly used (though not required to be used) for SRFs using + ValuePerCall mode. Additional details about Materialize mode can be + found in src/backend/utils/fmgr/README. Also, + the contrib modules in + the PostgreSQL source distribution contain + many examples of SRFs using both ValuePerCall and Materialize mode. + + + + To use the ValuePerCall support macros described here, + include funcapi.h. These macros work with a + structure FuncCallContext that contains the + state that needs to be saved across calls. Within the calling + SRF, fcinfo->flinfo->fn_extra is used to + hold a pointer to FuncCallContext across + calls. The macros automatically fill that field on first use, + and expect to find the same pointer there on subsequent uses. typedef struct FuncCallContext { @@ -2892,29 +2920,26 @@ typedef struct FuncCallContext - An SRF uses several functions and macros that - automatically manipulate the FuncCallContext - structure (and expect to find it via fn_extra). Use: + The macros to be used by an SRF using this + infrastructure are: SRF_IS_FIRSTCALL() - to determine if your function is being called for the first or a - subsequent time. On the first call (only) use: + Use this to determine if your function is being called for the first or a + subsequent time. On the first call (only), call: SRF_FIRSTCALL_INIT() to initialize the FuncCallContext. On every function call, - including the first, use: + including the first, call: SRF_PERCALL_SETUP() - to properly set up for using the FuncCallContext - and clearing any previously returned data left over from the - previous pass. + to set up for using the FuncCallContext. - If your function has data to return, use: + If your function has data to return in the current call, use: SRF_RETURN_NEXT(funcctx, result) @@ -2938,7 +2963,14 @@ SRF_RETURN_DONE(funcctx) multi_call_memory_ctx is a suitable location for any data that needs to survive until the SRF is finished running. In most cases, this means that you should switch into - multi_call_memory_ctx while doing the first-call setup. + multi_call_memory_ctx while doing the + first-call setup. + Use funcctx->user_fctx to hold a pointer to + any such cross-call data structures. + (Data you allocate + in multi_call_memory_ctx will go away + automatically when the query ends, so it is not necessary to free + that data manually, either.) @@ -2995,8 +3027,8 @@ my_set_returning_function(PG_FUNCTION_ARGS) } else { - /* Here we are done returning items and just need to clean up: */ - user code + /* Here we are done returning items, so just report that fact. */ + /* (Resist the temptation to put cleanup code here.) */ SRF_RETURN_DONE(funcctx); } } @@ -3118,12 +3150,6 @@ CREATE OR REPLACE FUNCTION retcomposite(IN integer, IN integer, Notice that in this method the output type of the function is formally an anonymous record type. - - - The directory contrib/tablefunc - module in the source distribution contains more examples of - set-returning functions. - diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c index 4f109111d1..9c80894281 100644 --- a/src/backend/utils/adt/datetime.c +++ b/src/backend/utils/adt/datetime.c @@ -4755,12 +4755,12 @@ pg_timezone_abbrevs(PG_FUNCTION_ARGS) Datum pg_timezone_names(PG_FUNCTION_ARGS) { - MemoryContext oldcontext; - FuncCallContext *funcctx; + ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; + bool randomAccess; + TupleDesc tupdesc; + Tuplestorestate *tupstore; pg_tzenum *tzenum; pg_tz *tz; - Datum result; - HeapTuple tuple; Datum values[4]; bool nulls[4]; int tzoff; @@ -4769,59 +4769,41 @@ pg_timezone_names(PG_FUNCTION_ARGS) const char *tzn; Interval *resInterval; struct pg_tm itm; + MemoryContext oldcontext; - /* stuff done only on the first call of the function */ - if (SRF_IS_FIRSTCALL()) - { - TupleDesc tupdesc; + /* check to see if caller supports us returning a tuplestore */ + if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("set-valued function called in context that cannot accept a set"))); + if (!(rsinfo->allowedModes & SFRM_Materialize)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("materialize mode required, but it is not allowed in this context"))); - /* create a function context for cross-call persistence */ - funcctx = SRF_FIRSTCALL_INIT(); + /* The tupdesc and tuplestore must be created in ecxt_per_query_memory */ + oldcontext = MemoryContextSwitchTo(rsinfo->econtext->ecxt_per_query_memory); - /* - * switch to memory context appropriate for multiple function calls - */ - oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx); + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) + elog(ERROR, "return type must be a row type"); - /* initialize timezone scanning code */ - tzenum = pg_tzenumerate_start(); - funcctx->user_fctx = (void *) tzenum; + randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0; + tupstore = tuplestore_begin_heap(randomAccess, false, work_mem); + rsinfo->returnMode = SFRM_Materialize; + rsinfo->setResult = tupstore; + rsinfo->setDesc = tupdesc; - /* - * build tupdesc for result tuples. This must match this function's - * pg_proc entry! - */ - tupdesc = CreateTemplateTupleDesc(4); - TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name", - TEXTOID, -1, 0); - TupleDescInitEntry(tupdesc, (AttrNumber) 2, "abbrev", - TEXTOID, -1, 0); - TupleDescInitEntry(tupdesc, (AttrNumber) 3, "utc_offset", - INTERVALOID, -1, 0); - TupleDescInitEntry(tupdesc, (AttrNumber) 4, "is_dst", - BOOLOID, -1, 0); + MemoryContextSwitchTo(oldcontext); - funcctx->tuple_desc = BlessTupleDesc(tupdesc); - MemoryContextSwitchTo(oldcontext); - } - - /* stuff done on every call of the function */ - funcctx = SRF_PERCALL_SETUP(); - tzenum = (pg_tzenum *) funcctx->user_fctx; + /* initialize timezone scanning code */ + tzenum = pg_tzenumerate_start(); /* search for another zone to display */ for (;;) { - oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx); tz = pg_tzenumerate_next(tzenum); - MemoryContextSwitchTo(oldcontext); - if (!tz) - { - pg_tzenumerate_end(tzenum); - funcctx->user_fctx = NULL; - SRF_RETURN_DONE(funcctx); - } + break; /* Convert now() to local time in this zone */ if (timestamp2tm(GetCurrentTransactionStartTimestamp(), @@ -4840,25 +4822,22 @@ pg_timezone_names(PG_FUNCTION_ARGS) if (tzn && strlen(tzn) > 31) continue; - /* Found a displayable zone */ - break; + MemSet(nulls, 0, sizeof(nulls)); + + values[0] = CStringGetTextDatum(pg_get_timezone_name(tz)); + values[1] = CStringGetTextDatum(tzn ? tzn : ""); + + MemSet(&itm, 0, sizeof(struct pg_tm)); + itm.tm_sec = -tzoff; + resInterval = (Interval *) palloc(sizeof(Interval)); + tm2interval(&itm, 0, resInterval); + values[2] = IntervalPGetDatum(resInterval); + + values[3] = BoolGetDatum(tm.tm_isdst > 0); + + tuplestore_putvalues(tupstore, tupdesc, values, nulls); } - MemSet(nulls, 0, sizeof(nulls)); - - values[0] = CStringGetTextDatum(pg_get_timezone_name(tz)); - values[1] = CStringGetTextDatum(tzn ? tzn : ""); - - MemSet(&itm, 0, sizeof(struct pg_tm)); - itm.tm_sec = -tzoff; - resInterval = (Interval *) palloc(sizeof(Interval)); - tm2interval(&itm, 0, resInterval); - values[2] = IntervalPGetDatum(resInterval); - - values[3] = BoolGetDatum(tm.tm_isdst > 0); - - tuple = heap_form_tuple(funcctx->tuple_desc, values, nulls); - result = HeapTupleGetDatum(tuple); - - SRF_RETURN_NEXT(funcctx, result); + pg_tzenumerate_end(tzenum); + return (Datum) 0; } diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index bcf9bd1b97..01185f218b 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -36,13 +36,6 @@ #include "utils/syscache.h" #include "utils/timestamp.h" -typedef struct -{ - char *location; - DIR *dirdesc; - bool include_dot_dirs; -} directory_fctx; - /* * Convert a "text" filename argument to C string, and check it's allowable. @@ -447,67 +440,79 @@ pg_stat_file_1arg(PG_FUNCTION_ARGS) Datum pg_ls_dir(PG_FUNCTION_ARGS) { - FuncCallContext *funcctx; + ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; + char *location; + bool missing_ok = false; + bool include_dot_dirs = false; + bool randomAccess; + TupleDesc tupdesc; + Tuplestorestate *tupstore; + DIR *dirdesc; struct dirent *de; - directory_fctx *fctx; MemoryContext oldcontext; - if (SRF_IS_FIRSTCALL()) + location = convert_and_check_filename(PG_GETARG_TEXT_PP(0)); + + /* check the optional arguments */ + if (PG_NARGS() == 3) { - bool missing_ok = false; - bool include_dot_dirs = false; - - /* check the optional arguments */ - if (PG_NARGS() == 3) - { - if (!PG_ARGISNULL(1)) - missing_ok = PG_GETARG_BOOL(1); - if (!PG_ARGISNULL(2)) - include_dot_dirs = PG_GETARG_BOOL(2); - } - - funcctx = SRF_FIRSTCALL_INIT(); - oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx); - - fctx = palloc(sizeof(directory_fctx)); - fctx->location = convert_and_check_filename(PG_GETARG_TEXT_PP(0)); - - fctx->include_dot_dirs = include_dot_dirs; - fctx->dirdesc = AllocateDir(fctx->location); - - if (!fctx->dirdesc) - { - if (missing_ok && errno == ENOENT) - { - MemoryContextSwitchTo(oldcontext); - SRF_RETURN_DONE(funcctx); - } - else - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not open directory \"%s\": %m", - fctx->location))); - } - funcctx->user_fctx = fctx; - MemoryContextSwitchTo(oldcontext); + if (!PG_ARGISNULL(1)) + missing_ok = PG_GETARG_BOOL(1); + if (!PG_ARGISNULL(2)) + include_dot_dirs = PG_GETARG_BOOL(2); } - funcctx = SRF_PERCALL_SETUP(); - fctx = (directory_fctx *) funcctx->user_fctx; + /* check to see if caller supports us returning a tuplestore */ + if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("set-valued function called in context that cannot accept a set"))); + if (!(rsinfo->allowedModes & SFRM_Materialize)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("materialize mode required, but it is not allowed in this context"))); - while ((de = ReadDir(fctx->dirdesc, fctx->location)) != NULL) + /* The tupdesc and tuplestore must be created in ecxt_per_query_memory */ + oldcontext = MemoryContextSwitchTo(rsinfo->econtext->ecxt_per_query_memory); + + tupdesc = CreateTemplateTupleDesc(1); + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "pg_ls_dir", TEXTOID, -1, 0); + + randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0; + tupstore = tuplestore_begin_heap(randomAccess, false, work_mem); + rsinfo->returnMode = SFRM_Materialize; + rsinfo->setResult = tupstore; + rsinfo->setDesc = tupdesc; + + MemoryContextSwitchTo(oldcontext); + + dirdesc = AllocateDir(location); + if (!dirdesc) { - if (!fctx->include_dot_dirs && + /* Return empty tuplestore if appropriate */ + if (missing_ok && errno == ENOENT) + return (Datum) 0; + /* Otherwise, we can let ReadDir() throw the error */ + } + + while ((de = ReadDir(dirdesc, location)) != NULL) + { + Datum values[1]; + bool nulls[1]; + + if (!include_dot_dirs && (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0)) continue; - SRF_RETURN_NEXT(funcctx, CStringGetTextDatum(de->d_name)); + values[0] = CStringGetTextDatum(de->d_name); + nulls[0] = false; + + tuplestore_putvalues(tupstore, tupdesc, values, nulls); } - FreeDir(fctx->dirdesc); - - SRF_RETURN_DONE(funcctx); + FreeDir(dirdesc); + return (Datum) 0; } /* @@ -548,8 +553,7 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok) if (!(rsinfo->allowedModes & SFRM_Materialize)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("materialize mode required, but it is not " - "allowed in this context"))); + errmsg("materialize mode required, but it is not allowed in this context"))); /* The tupdesc and tuplestore must be created in ecxt_per_query_memory */ oldcontext = MemoryContextSwitchTo(rsinfo->econtext->ecxt_per_query_memory); @@ -575,10 +579,7 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok) { /* Return empty tuplestore if appropriate */ if (missing_ok && errno == ENOENT) - { - tuplestore_donestoring(tupstore); return (Datum) 0; - } /* Otherwise, we can let ReadDir() throw the error */ } @@ -613,7 +614,6 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok) } FreeDir(dirdesc); - tuplestore_donestoring(tupstore); return (Datum) 0; } diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c index 323e36b81c..ee340fb0f0 100644 --- a/src/backend/utils/adt/misc.c +++ b/src/backend/utils/adt/misc.c @@ -194,72 +194,82 @@ current_query(PG_FUNCTION_ARGS) /* Function to find out which databases make use of a tablespace */ -typedef struct -{ - char *location; - DIR *dirdesc; -} ts_db_fctx; - Datum pg_tablespace_databases(PG_FUNCTION_ARGS) { - FuncCallContext *funcctx; + Oid tablespaceOid = PG_GETARG_OID(0); + ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; + bool randomAccess; + TupleDesc tupdesc; + Tuplestorestate *tupstore; + char *location; + DIR *dirdesc; struct dirent *de; - ts_db_fctx *fctx; + MemoryContext oldcontext; - if (SRF_IS_FIRSTCALL()) + /* check to see if caller supports us returning a tuplestore */ + if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("set-valued function called in context that cannot accept a set"))); + if (!(rsinfo->allowedModes & SFRM_Materialize)) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("materialize mode required, but it is not allowed in this context"))); + + /* The tupdesc and tuplestore must be created in ecxt_per_query_memory */ + oldcontext = MemoryContextSwitchTo(rsinfo->econtext->ecxt_per_query_memory); + + tupdesc = CreateTemplateTupleDesc(1); + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "pg_tablespace_databases", + OIDOID, -1, 0); + + randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0; + tupstore = tuplestore_begin_heap(randomAccess, false, work_mem); + + rsinfo->returnMode = SFRM_Materialize; + rsinfo->setResult = tupstore; + rsinfo->setDesc = tupdesc; + + MemoryContextSwitchTo(oldcontext); + + if (tablespaceOid == GLOBALTABLESPACE_OID) { - MemoryContext oldcontext; - Oid tablespaceOid = PG_GETARG_OID(0); - - funcctx = SRF_FIRSTCALL_INIT(); - oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx); - - fctx = palloc(sizeof(ts_db_fctx)); - - if (tablespaceOid == GLOBALTABLESPACE_OID) - { - fctx->dirdesc = NULL; - ereport(WARNING, - (errmsg("global tablespace never has databases"))); - } - else - { - if (tablespaceOid == DEFAULTTABLESPACE_OID) - fctx->location = psprintf("base"); - else - fctx->location = psprintf("pg_tblspc/%u/%s", tablespaceOid, - TABLESPACE_VERSION_DIRECTORY); - - fctx->dirdesc = AllocateDir(fctx->location); - - if (!fctx->dirdesc) - { - /* the only expected error is ENOENT */ - if (errno != ENOENT) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not open directory \"%s\": %m", - fctx->location))); - ereport(WARNING, - (errmsg("%u is not a tablespace OID", tablespaceOid))); - } - } - funcctx->user_fctx = fctx; - MemoryContextSwitchTo(oldcontext); + ereport(WARNING, + (errmsg("global tablespace never has databases"))); + /* return empty tuplestore */ + return (Datum) 0; } - funcctx = SRF_PERCALL_SETUP(); - fctx = (ts_db_fctx *) funcctx->user_fctx; + if (tablespaceOid == DEFAULTTABLESPACE_OID) + location = psprintf("base"); + else + location = psprintf("pg_tblspc/%u/%s", tablespaceOid, + TABLESPACE_VERSION_DIRECTORY); - if (!fctx->dirdesc) /* not a tablespace */ - SRF_RETURN_DONE(funcctx); + dirdesc = AllocateDir(location); - while ((de = ReadDir(fctx->dirdesc, fctx->location)) != NULL) + if (!dirdesc) + { + /* the only expected error is ENOENT */ + if (errno != ENOENT) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not open directory \"%s\": %m", + location))); + ereport(WARNING, + (errmsg("%u is not a tablespace OID", tablespaceOid))); + /* return empty tuplestore */ + return (Datum) 0; + } + + while ((de = ReadDir(dirdesc, location)) != NULL) { Oid datOid = atooid(de->d_name); char *subdir; bool isempty; + Datum values[1]; + bool nulls[1]; /* this test skips . and .., but is awfully weak */ if (!datOid) @@ -267,18 +277,21 @@ pg_tablespace_databases(PG_FUNCTION_ARGS) /* if database subdir is empty, don't report tablespace as used */ - subdir = psprintf("%s/%s", fctx->location, de->d_name); + subdir = psprintf("%s/%s", location, de->d_name); isempty = directory_is_empty(subdir); pfree(subdir); if (isempty) continue; /* indeed, nothing in it */ - SRF_RETURN_NEXT(funcctx, ObjectIdGetDatum(datOid)); + values[0] = ObjectIdGetDatum(datOid); + nulls[0] = false; + + tuplestore_putvalues(tupstore, tupdesc, values, nulls); } - FreeDir(fctx->dirdesc); - SRF_RETURN_DONE(funcctx); + FreeDir(dirdesc); + return (Datum) 0; } diff --git a/src/backend/utils/fmgr/README b/src/backend/utils/fmgr/README index a4d6a07bdd..1e4c4b94a9 100644 --- a/src/backend/utils/fmgr/README +++ b/src/backend/utils/fmgr/README @@ -239,8 +239,6 @@ tuple toaster will decide whether toasting is needed. Functions Accepting or Returning Sets ------------------------------------- -[ this section revised 29-Aug-2002 for 7.3 ] - If a function is marked in pg_proc as returning a set, then it is called with fcinfo->resultinfo pointing to a node of type ReturnSetInfo. A function that desires to return a set should raise an error "called in @@ -277,10 +275,16 @@ been returned, the next call should set isDone to ExprEndResult and return a null result. (Note it is possible to return an empty set by doing this on the first call.) -The ReturnSetInfo node also contains a link to the ExprContext within which -the function is being evaluated. This is useful for value-per-call functions -that need to close down internal state when they are not run to completion: -they can register a shutdown callback function in the ExprContext. +Value-per-call functions MUST NOT assume that they will be run to completion; +the executor might simply stop calling them, for example because of a LIMIT. +Therefore, it's unsafe to attempt to perform any resource cleanup in the +final call. It's usually not necessary to clean up memory, anyway. If it's +necessary to clean up other types of resources, such as file descriptors, +one can register a shutdown callback function in the ExprContext pointed to +by the ReturnSetInfo node. (But note that file descriptors are a limited +resource, so it's generally unwise to hold those open across calls; SRFs +that need file access are better written to do it in a single call using +Materialize mode.) Materialize mode works like this: the function creates a Tuplestore holding the (possibly empty) result set, and returns it. There are no multiple calls. diff --git a/src/include/funcapi.h b/src/include/funcapi.h index f9b75ae390..b047acdc1a 100644 --- a/src/include/funcapi.h +++ b/src/include/funcapi.h @@ -234,7 +234,7 @@ extern Datum HeapTupleHeaderGetDatum(HeapTupleHeader tuple); /*---------- * Support for Set Returning Functions (SRFs) * - * The basic API for SRFs looks something like: + * The basic API for SRFs using ValuePerCall mode looks something like this: * * Datum * my_Set_Returning_Function(PG_FUNCTION_ARGS) @@ -271,6 +271,17 @@ extern Datum HeapTupleHeaderGetDatum(HeapTupleHeader tuple); * SRF_RETURN_DONE(funcctx); * } * + * NOTE: there is no guarantee that a SRF using ValuePerCall mode will be + * run to completion; for example, a query with LIMIT might stop short of + * fetching all the rows. Therefore, do not expect that you can do resource + * cleanup just before SRF_RETURN_DONE(). You need not worry about releasing + * memory allocated in multi_call_memory_ctx, but holding file descriptors or + * other non-memory resources open across calls is a bug. SRFs that need + * such resources should not use these macros, but instead populate a + * tuplestore during a single call, and return that using SFRM_Materialize + * mode (see fmgr/README). Alternatively, set up a callback to release + * resources at query shutdown, using RegisterExprContextCallback(). + * *---------- */ diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index e217b678d7..d3acb98d04 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -180,6 +180,27 @@ select count(*) >= 0 as ok from pg_ls_archive_statusdir(); t (1 row) +select * from (select pg_ls_dir('.') a) a where a = 'base' limit 1; + a +------ + base +(1 row) + +select * from (select (pg_timezone_names()).name) ptn where name='UTC' limit 1; + name +------ + UTC +(1 row) + +select count(*) > 0 from + (select pg_tablespace_databases(oid) as pts from pg_tablespace + where spcname = 'pg_default') pts + join pg_database db on pts.pts = db.oid; + ?column? +---------- + t +(1 row) + -- -- Test adding a support function to a subject function -- diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 1e11eb3554..094e8f8296 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -51,6 +51,15 @@ from (select pg_ls_waldir() w) ss where length((w).name) = 24 limit 1; select count(*) >= 0 as ok from pg_ls_archive_statusdir(); +select * from (select pg_ls_dir('.') a) a where a = 'base' limit 1; + +select * from (select (pg_timezone_names()).name) ptn where name='UTC' limit 1; + +select count(*) > 0 from + (select pg_tablespace_databases(oid) as pts from pg_tablespace + where spcname = 'pg_default') pts + join pg_database db on pts.pts = db.oid; + -- -- Test adding a support function to a subject function --