Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 8e91e12

Browse files
committed
Allow contrib/file_fdw to read from a program, like COPY FROM PROGRAM.
This patch just exposes COPY's FROM PROGRAM option in contrib/file_fdw. There don't seem to be any security issues with that that are any worse than what already exist with file_fdw and COPY; as in the existing cases, only superusers are allowed to control what gets executed. A regression test case might be nice here, but choosing a 100% portable command to run is hard. (We haven't got a test for COPY FROM PROGRAM itself, either.) Corey Huinker and Adam Gomaa, reviewed by Amit Langote Discussion: <CADkLM=dGDGmaEiZ=UDepzumWg-CVn7r8MHPjr2NArj8S3TsROQ@mail.gmail.com>
1 parent 728a3e7 commit 8e91e12

File tree

3 files changed

+137
-66
lines changed

3 files changed

+137
-66
lines changed

contrib/file_fdw/file_fdw.c

+88-43
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/*-------------------------------------------------------------------------
22
*
33
* file_fdw.c
4-
* foreign-data wrapper for server-side flat files.
4+
* foreign-data wrapper for server-side flat files (or programs).
55
*
66
* Copyright (c) 2010-2016, PostgreSQL Global Development Group
77
*
@@ -57,8 +57,9 @@ struct FileFdwOption
5757
* fileGetOptions(), which currently doesn't bother to look at user mappings.
5858
*/
5959
static const struct FileFdwOption valid_options[] = {
60-
/* File options */
60+
/* Data source options */
6161
{"filename", ForeignTableRelationId},
62+
{"program", ForeignTableRelationId},
6263

6364
/* Format options */
6465
/* oids option is not supported */
@@ -85,20 +86,24 @@ static const struct FileFdwOption valid_options[] = {
8586
*/
8687
typedef struct FileFdwPlanState
8788
{
88-
char *filename; /* file to read */
89-
List *options; /* merged COPY options, excluding filename */
89+
char *filename; /* file or program to read from */
90+
bool is_program; /* true if filename represents an OS command */
91+
List *options; /* merged COPY options, excluding filename and
92+
* is_program */
9093
BlockNumber pages; /* estimate of file's physical size */
91-
double ntuples; /* estimate of number of rows in file */
94+
double ntuples; /* estimate of number of data rows */
9295
} FileFdwPlanState;
9396

9497
/*
9598
* FDW-specific information for ForeignScanState.fdw_state.
9699
*/
97100
typedef struct FileFdwExecutionState
98101
{
99-
char *filename; /* file to read */
100-
List *options; /* merged COPY options, excluding filename */
101-
CopyState cstate; /* state of reading file */
102+
char *filename; /* file or program to read from */
103+
bool is_program; /* true if filename represents an OS command */
104+
List *options; /* merged COPY options, excluding filename and
105+
* is_program */
106+
CopyState cstate; /* COPY execution state */
102107
} FileFdwExecutionState;
103108

104109
/*
@@ -139,7 +144,9 @@ static bool fileIsForeignScanParallelSafe(PlannerInfo *root, RelOptInfo *rel,
139144
*/
140145
static bool is_valid_option(const char *option, Oid context);
141146
static void fileGetOptions(Oid foreigntableid,
142-
char **filename, List **other_options);
147+
char **filename,
148+
bool *is_program,
149+
List **other_options);
143150
static List *get_file_fdw_attribute_options(Oid relid);
144151
static bool check_selective_binary_conversion(RelOptInfo *baserel,
145152
Oid foreigntableid,
@@ -196,16 +203,16 @@ file_fdw_validator(PG_FUNCTION_ARGS)
196203

197204
/*
198205
* Only superusers are allowed to set options of a file_fdw foreign table.
199-
* This is because the filename is one of those options, and we don't want
200-
* non-superusers to be able to determine which file gets read.
206+
* This is because we don't want non-superusers to be able to control
207+
* which file gets read or which program gets executed.
201208
*
202209
* Putting this sort of permissions check in a validator is a bit of a
203210
* crock, but there doesn't seem to be any other place that can enforce
204211
* the check more cleanly.
205212
*
206-
* Note that the valid_options[] array disallows setting filename at any
207-
* options level other than foreign table --- otherwise there'd still be a
208-
* security hole.
213+
* Note that the valid_options[] array disallows setting filename and
214+
* program at any options level other than foreign table --- otherwise
215+
* there'd still be a security hole.
209216
*/
210217
if (catalog == ForeignTableRelationId && !superuser())
211218
ereport(ERROR,
@@ -247,11 +254,11 @@ file_fdw_validator(PG_FUNCTION_ARGS)
247254
}
248255

249256
/*
250-
* Separate out filename and column-specific options, since
257+
* Separate out filename, program, and column-specific options, since
251258
* ProcessCopyOptions won't accept them.
252259
*/
253-
254-
if (strcmp(def->defname, "filename") == 0)
260+
if (strcmp(def->defname, "filename") == 0 ||
261+
strcmp(def->defname, "program") == 0)
255262
{
256263
if (filename)
257264
ereport(ERROR,
@@ -296,12 +303,13 @@ file_fdw_validator(PG_FUNCTION_ARGS)
296303
ProcessCopyOptions(NULL, NULL, true, other_options);
297304

298305
/*
299-
* Filename option is required for file_fdw foreign tables.
306+
* Either filename or program option is required for file_fdw foreign
307+
* tables.
300308
*/
301309
if (catalog == ForeignTableRelationId && filename == NULL)
302310
ereport(ERROR,
303311
(errcode(ERRCODE_FDW_DYNAMIC_PARAMETER_VALUE_NEEDED),
304-
errmsg("filename is required for file_fdw foreign tables")));
312+
errmsg("either filename or program is required for file_fdw foreign tables")));
305313

306314
PG_RETURN_VOID();
307315
}
@@ -326,12 +334,12 @@ is_valid_option(const char *option, Oid context)
326334
/*
327335
* Fetch the options for a file_fdw foreign table.
328336
*
329-
* We have to separate out "filename" from the other options because
330-
* it must not appear in the options list passed to the core COPY code.
337+
* We have to separate out filename/program from the other options because
338+
* those must not appear in the options list passed to the core COPY code.
331339
*/
332340
static void
333341
fileGetOptions(Oid foreigntableid,
334-
char **filename, List **other_options)
342+
char **filename, bool *is_program, List **other_options)
335343
{
336344
ForeignTable *table;
337345
ForeignServer *server;
@@ -359,9 +367,11 @@ fileGetOptions(Oid foreigntableid,
359367
options = list_concat(options, get_file_fdw_attribute_options(foreigntableid));
360368

361369
/*
362-
* Separate out the filename.
370+
* Separate out the filename or program option (we assume there is only
371+
* one).
363372
*/
364373
*filename = NULL;
374+
*is_program = false;
365375
prev = NULL;
366376
foreach(lc, options)
367377
{
@@ -373,15 +383,22 @@ fileGetOptions(Oid foreigntableid,
373383
options = list_delete_cell(options, lc, prev);
374384
break;
375385
}
386+
else if (strcmp(def->defname, "program") == 0)
387+
{
388+
*filename = defGetString(def);
389+
*is_program = true;
390+
options = list_delete_cell(options, lc, prev);
391+
break;
392+
}
376393
prev = lc;
377394
}
378395

379396
/*
380-
* The validator should have checked that a filename was included in the
381-
* options, but check again, just in case.
397+
* The validator should have checked that filename or program was included
398+
* in the options, but check again, just in case.
382399
*/
383400
if (*filename == NULL)
384-
elog(ERROR, "filename is required for file_fdw foreign tables");
401+
elog(ERROR, "either filename or program is required for file_fdw foreign tables");
385402

386403
*other_options = options;
387404
}
@@ -475,12 +492,15 @@ fileGetForeignRelSize(PlannerInfo *root,
475492
FileFdwPlanState *fdw_private;
476493

477494
/*
478-
* Fetch options. We only need filename at this point, but we might as
479-
* well get everything and not need to re-fetch it later in planning.
495+
* Fetch options. We only need filename (or program) at this point, but
496+
* we might as well get everything and not need to re-fetch it later in
497+
* planning.
480498
*/
481499
fdw_private = (FileFdwPlanState *) palloc(sizeof(FileFdwPlanState));
482500
fileGetOptions(foreigntableid,
483-
&fdw_private->filename, &fdw_private->options);
501+
&fdw_private->filename,
502+
&fdw_private->is_program,
503+
&fdw_private->options);
484504
baserel->fdw_private = (void *) fdw_private;
485505

486506
/* Estimate relation size */
@@ -583,20 +603,25 @@ static void
583603
fileExplainForeignScan(ForeignScanState *node, ExplainState *es)
584604
{
585605
char *filename;
606+
bool is_program;
586607
List *options;
587608

588-
/* Fetch options --- we only need filename at this point */
609+
/* Fetch options --- we only need filename and is_program at this point */
589610
fileGetOptions(RelationGetRelid(node->ss.ss_currentRelation),
590-
&filename, &options);
611+
&filename, &is_program, &options);
591612

592-
ExplainPropertyText("Foreign File", filename, es);
613+
if (is_program)
614+
ExplainPropertyText("Foreign Program", filename, es);
615+
else
616+
ExplainPropertyText("Foreign File", filename, es);
593617

594618
/* Suppress file size if we're not showing cost details */
595619
if (es->costs)
596620
{
597621
struct stat stat_buf;
598622

599-
if (stat(filename, &stat_buf) == 0)
623+
if (!is_program &&
624+
stat(filename, &stat_buf) == 0)
600625
ExplainPropertyLong("Foreign File Size", (long) stat_buf.st_size,
601626
es);
602627
}
@@ -611,6 +636,7 @@ fileBeginForeignScan(ForeignScanState *node, int eflags)
611636
{
612637
ForeignScan *plan = (ForeignScan *) node->ss.ps.plan;
613638
char *filename;
639+
bool is_program;
614640
List *options;
615641
CopyState cstate;
616642
FileFdwExecutionState *festate;
@@ -623,7 +649,7 @@ fileBeginForeignScan(ForeignScanState *node, int eflags)
623649

624650
/* Fetch options of foreign table */
625651
fileGetOptions(RelationGetRelid(node->ss.ss_currentRelation),
626-
&filename, &options);
652+
&filename, &is_program, &options);
627653

628654
/* Add any options from the plan (currently only convert_selectively) */
629655
options = list_concat(options, plan->fdw_private);
@@ -635,7 +661,7 @@ fileBeginForeignScan(ForeignScanState *node, int eflags)
635661
cstate = BeginCopyFrom(NULL,
636662
node->ss.ss_currentRelation,
637663
filename,
638-
false,
664+
is_program,
639665
NIL,
640666
options);
641667

@@ -645,6 +671,7 @@ fileBeginForeignScan(ForeignScanState *node, int eflags)
645671
*/
646672
festate = (FileFdwExecutionState *) palloc(sizeof(FileFdwExecutionState));
647673
festate->filename = filename;
674+
festate->is_program = is_program;
648675
festate->options = options;
649676
festate->cstate = cstate;
650677

@@ -709,7 +736,7 @@ fileReScanForeignScan(ForeignScanState *node)
709736
festate->cstate = BeginCopyFrom(NULL,
710737
node->ss.ss_currentRelation,
711738
festate->filename,
712-
false,
739+
festate->is_program,
713740
NIL,
714741
festate->options);
715742
}
@@ -738,11 +765,22 @@ fileAnalyzeForeignTable(Relation relation,
738765
BlockNumber *totalpages)
739766
{
740767
char *filename;
768+
bool is_program;
741769
List *options;
742770
struct stat stat_buf;
743771

744772
/* Fetch options of foreign table */
745-
fileGetOptions(RelationGetRelid(relation), &filename, &options);
773+
fileGetOptions(RelationGetRelid(relation), &filename, &is_program, &options);
774+
775+
/*
776+
* If this is a program instead of a file, just return false to skip
777+
* analyzing the table. We could run the program and collect stats on
778+
* whatever it currently returns, but it seems likely that in such cases
779+
* the output would be too volatile for the stats to be useful. Maybe
780+
* there should be an option to enable doing this?
781+
*/
782+
if (is_program)
783+
return false;
746784

747785
/*
748786
* Get size of the file. (XXX if we fail here, would it be better to just
@@ -769,8 +807,8 @@ fileAnalyzeForeignTable(Relation relation,
769807

770808
/*
771809
* fileIsForeignScanParallelSafe
772-
* Reading a file in a parallel worker should work just the same as
773-
* reading it in the leader, so mark scans safe.
810+
* Reading a file, or external program, in a parallel worker should work
811+
* just the same as reading it in the leader, so mark scans safe.
774812
*/
775813
static bool
776814
fileIsForeignScanParallelSafe(PlannerInfo *root, RelOptInfo *rel,
@@ -916,9 +954,10 @@ estimate_size(PlannerInfo *root, RelOptInfo *baserel,
916954

917955
/*
918956
* Get size of the file. It might not be there at plan time, though, in
919-
* which case we have to use a default estimate.
957+
* which case we have to use a default estimate. We also have to fall
958+
* back to the default if using a program as the input.
920959
*/
921-
if (stat(fdw_private->filename, &stat_buf) < 0)
960+
if (fdw_private->is_program || stat(fdw_private->filename, &stat_buf) < 0)
922961
stat_buf.st_size = 10 * BLCKSZ;
923962

924963
/*
@@ -1000,6 +1039,11 @@ estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
10001039
* that I/O costs are equivalent to a regular table file of the same size.
10011040
* However, we take per-tuple CPU costs as 10x of a seqscan, to account
10021041
* for the cost of parsing records.
1042+
*
1043+
* In the case of a program source, this calculation is even more divorced
1044+
* from reality, but we have no good alternative; and it's not clear that
1045+
* the numbers we produce here matter much anyway, since there's only one
1046+
* access path for the rel.
10031047
*/
10041048
run_cost += seq_page_cost * pages;
10051049

@@ -1036,6 +1080,7 @@ file_acquire_sample_rows(Relation onerel, int elevel,
10361080
bool *nulls;
10371081
bool found;
10381082
char *filename;
1083+
bool is_program;
10391084
List *options;
10401085
CopyState cstate;
10411086
ErrorContextCallback errcallback;
@@ -1050,12 +1095,12 @@ file_acquire_sample_rows(Relation onerel, int elevel,
10501095
nulls = (bool *) palloc(tupDesc->natts * sizeof(bool));
10511096

10521097
/* Fetch options of foreign table */
1053-
fileGetOptions(RelationGetRelid(onerel), &filename, &options);
1098+
fileGetOptions(RelationGetRelid(onerel), &filename, &is_program, &options);
10541099

10551100
/*
10561101
* Create CopyState from FDW options.
10571102
*/
1058-
cstate = BeginCopyFrom(NULL, onerel, filename, false, NIL, options);
1103+
cstate = BeginCopyFrom(NULL, onerel, filename, is_program, NIL, options);
10591104

10601105
/*
10611106
* Use per-tuple memory context to prevent leak of memory used to read

contrib/file_fdw/output/file_fdw.source

+3-3
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', null '
7676
'); -- ERROR
7777
ERROR: COPY null representation cannot use newline or carriage return
7878
CREATE FOREIGN TABLE tbl () SERVER file_server; -- ERROR
79-
ERROR: filename is required for file_fdw foreign tables
79+
ERROR: either filename or program is required for file_fdw foreign tables
8080
CREATE FOREIGN TABLE agg_text (
8181
a int2 CHECK (a >= 0),
8282
b float4
@@ -132,7 +132,7 @@ ERROR: invalid option "force_not_null"
132132
HINT: There are no valid options in this context.
133133
CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (force_not_null '*'); -- ERROR
134134
ERROR: invalid option "force_not_null"
135-
HINT: Valid options in this context are: filename, format, header, delimiter, quote, escape, null, encoding
135+
HINT: Valid options in this context are: filename, program, format, header, delimiter, quote, escape, null, encoding
136136
-- force_null is not allowed to be specified at any foreign object level:
137137
ALTER FOREIGN DATA WRAPPER file_fdw OPTIONS (ADD force_null '*'); -- ERROR
138138
ERROR: invalid option "force_null"
@@ -145,7 +145,7 @@ ERROR: invalid option "force_null"
145145
HINT: There are no valid options in this context.
146146
CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (force_null '*'); -- ERROR
147147
ERROR: invalid option "force_null"
148-
HINT: Valid options in this context are: filename, format, header, delimiter, quote, escape, null, encoding
148+
HINT: Valid options in this context are: filename, program, format, header, delimiter, quote, escape, null, encoding
149149
-- basic query tests
150150
SELECT * FROM agg_text WHERE b > 10.0 ORDER BY a;
151151
a | b

0 commit comments

Comments
 (0)