From: Justin Pryzby <pryzby@telsasoft.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org, Fabien COELHO <coelho@cri.ensmp.fr>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: David Steele <david@pgmasters.net>
Cc: Bossart, Nathan <bossartn@amazon.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Subject: Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction
Date: Wed, 11 Mar 2020 06:19:21 -0500
Message-ID: <20200311111921.GQ29065@telsasoft.com> (raw)
In-Reply-To: <5679.1583696409@sss.pgh.pa.us>
References: <20200308173103.GC1357@telsasoft.com>
<27334.1583692669@sss.pgh.pa.us>
<20200308191456.GD1357@telsasoft.com>
<5679.1583696409@sss.pgh.pa.us>
On Sun, Mar 08, 2020 at 03:40:09PM -0400, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > On Sun, Mar 08, 2020 at 02:37:49PM -0400, Tom Lane wrote:
> >> I guess we ought to change that function to use returns-a-tuplestore
> >> protocol instead of thinking it can hold a directory open across calls.
> > Thanks for the analysis.
> > Do you mean it should enumerate all files during the initial SRF call, or use
> > something other than the SRF_* macros ?
> It has to enumerate all the files during the first call. I suppose it
> I've just finished scanning the source code and concluding that all
> of these functions are similarly broken:
> pg_ls_dir_files
I patched this one to see what it looks like and to allow /hopefully/ moving
forward one way or another with the pg_ls_tmpfile() patch set (or at least
avoid trying to do anything there which is too inconsistent with this fix).
> I don't see anything in the documentation (either funcapi.h or
> xfunc.sgml) warning that the function might not get run to completion,
> either ...
Also, at first glance, these seem to be passing constant "randomAccess=true"
rather than (bool) (rsinfo->allowedModes&SFRM_Materialize_Random)
$ git grep -wl SFRM_Materialize |xargs grep -l 'tuplestore_begin_heap(true'
contrib/dblink/dblink.c
contrib/pageinspect/brinfuncs.c
contrib/pg_stat_statements/pg_stat_statements.c
src/backend/access/transam/xlogfuncs.c
src/backend/commands/event_trigger.c
src/backend/commands/extension.c
src/backend/foreign/foreign.c
src/backend/replication/logical/launcher.c
src/backend/replication/logical/logicalfuncs.c
src/backend/replication/logical/origin.c
src/backend/replication/slotfuncs.c
src/backend/replication/walsender.c
src/backend/storage/ipc/shmem.c
src/backend/utils/adt/pgstatfuncs.c
src/backend/utils/misc/guc.c
src/backend/utils/misc/pg_config.c
--
Justin
Attachments:
[text/x-diff] v1-0001-pg_ls_dir_files-avoid-leaking-DIR-if-not-run-to-c.patch (4.7K, ../20200311111921.GQ29065@telsasoft.com/2-v1-0001-pg_ls_dir_files-avoid-leaking-DIR-if-not-run-to-c.patch)
download | inline diff:
From 1fd2918d65ed8cc64158e407e56ed61b44e951db Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Tue, 10 Mar 2020 21:01:47 -0500
Subject: [PATCH v1] pg_ls_dir_files: avoid leaking DIR if not run to
completion
Change to return a tuplestore rather than leaving directory open across calls.
Discussion: https://www.postgresql.org/message-id/20200308173103.GC1357%40telsasoft.com
See also: 9cb7db3f0
---
src/backend/utils/adt/genfile.c | 90 +++++++++++++++------------------
1 file changed, 42 insertions(+), 48 deletions(-)
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.cindex 5bda2af87c..046d218ffa 100644--- a/src/backend/utils/adt/genfile.c+++ b/src/backend/utils/adt/genfile.c@@ -527,67 +527,63 @@ pg_ls_dir_1arg(PG_FUNCTION_ARGS)
static Datum
pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok)
{
- FuncCallContext *funcctx;
struct dirent *de;
- directory_fctx *fctx;-- if (SRF_IS_FIRSTCALL())- {- MemoryContext oldcontext;- TupleDesc tupdesc;+ MemoryContext oldcontext;+ TupleDesc tupdesc;+ ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;+ bool randomAccess = (rsinfo->allowedModes&SFRM_Materialize_Random) != 0;+ DIR *dirdesc;+ Tuplestorestate *tupstore;- funcctx = SRF_FIRSTCALL_INIT();- oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);+ 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")));+ /* check to see if caller supports us returning a tuplestore */+ if (!(rsinfo->allowedModes & SFRM_Materialize))+ ereport(ERROR,+ (errcode(ERRCODE_SYNTAX_ERROR),+ errmsg("materialize mode required, but it is not "+ "allowed in this context")));- fctx = palloc(sizeof(directory_fctx));+ /* The Tuplestore and TupleDesc should be created in ecxt_per_query_memory */+ oldcontext = MemoryContextSwitchTo(rsinfo->econtext->ecxt_per_query_memory);- tupdesc = CreateTemplateTupleDesc(3);- TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",- TEXTOID, -1, 0);- TupleDescInitEntry(tupdesc, (AttrNumber) 2, "size",- INT8OID, -1, 0);- TupleDescInitEntry(tupdesc, (AttrNumber) 3, "modification",- TIMESTAMPTZOID, -1, 0);- funcctx->tuple_desc = BlessTupleDesc(tupdesc);+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)+ elog(ERROR, "return type must be a row type");- fctx->location = pstrdup(dir);- fctx->dirdesc = AllocateDir(fctx->location);+ tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);+ rsinfo->returnMode = SFRM_Materialize;+ rsinfo->setResult = tupstore;+ rsinfo->setDesc = tupdesc;- 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)));- }+ MemoryContextSwitchTo(oldcontext);- funcctx->user_fctx = fctx;- MemoryContextSwitchTo(oldcontext);+ dirdesc = AllocateDir(dir);+ if (!dirdesc)+ {+ if (missing_ok && errno == ENOENT)+ return NULL; // XXX+ else+ ereport(ERROR,+ (errcode_for_file_access(),+ errmsg("could not open directory \"%s\": %m",+ dir)));
}
- funcctx = SRF_PERCALL_SETUP();- fctx = (directory_fctx *) funcctx->user_fctx;-- while ((de = ReadDir(fctx->dirdesc, fctx->location)) != NULL)+ while ((de = ReadDir(dirdesc, dir)) != NULL)
{
Datum values[3];
- bool nulls[3];+ bool nulls[3] = {0};
char path[MAXPGPATH * 2];
struct stat attrib;
- HeapTuple tuple;
/* Skip hidden files */
if (de->d_name[0] == '.')
continue;
/* Get the file info */
- snprintf(path, sizeof(path), "%s/%s", fctx->location, de->d_name);+ snprintf(path, sizeof(path), "%s/%s", dir, de->d_name);
if (stat(path, &attrib) < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -600,14 +596,12 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok)
values[0] = CStringGetTextDatum(de->d_name);
values[1] = Int64GetDatum((int64) attrib.st_size);
values[2] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_mtime));
- memset(nulls, 0, sizeof(nulls));-- tuple = heap_form_tuple(funcctx->tuple_desc, values, nulls);- SRF_RETURN_NEXT(funcctx, HeapTupleGetDatum(tuple));+ tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
- FreeDir(fctx->dirdesc);- SRF_RETURN_DONE(funcctx);+ tuplestore_donestoring(tupstore);+ FreeDir(dirdesc);+ return NULL;
}
/* Function to return the list of files in the log directory */
--
2.17.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, tgl@sss.pgh.pa.us, coelho@cri.ensmp.fr, alvherre@2ndquadrant.com, david@pgmasters.net, bossartn@amazon.com, thomas.munro@gmail.com
Subject: Re: pg11+: pg_ls_*dir LIMIT 1: temporary files .. not closed at end-of-transaction
In-Reply-To: <20200311111921.GQ29065@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox