Re: Let file_fdw access COPY FROM PROGRAM

Thanks for the review!

I agree with all the code cleanups suggested and have made then in the
attached patch, to save the committer some time.

Also in this patch, I changed sgml para to
<para>
Changing table-level options requires superuser privileges, for security
reasons: only a superuser should be able to determine which file is read
or which program is run. In principle non-superusers could be allowed to
change the other options, but that's not supported at present.
</para>

"Determine" is an odd word in this context. I understand it to mean
"set/change", but I can see where a less familiar reader would take the
meaning to be "has permission to see the value already set". Either way, it
now mentions program as an option in addition to filename.

On Mon, Sep 12, 2016 at 1:59 AM, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp
> wrote:

> On 2016/09/11 8:04, Corey Huinker wrote:
> > V2 of this patch:
> >
> > Changes:
> > * rebased to most recent master
> > * removed non-tap test that assumed the existence of Unix sed program
> > * added non-tap test that assumes the existence of perl
> > * switched from filename/program to filename/is_program to more closely
> > follow patterns in copy.c
> > * slight wording change in C comments
>
> This version looks mostly good to me. Except some whitespace noise in
> some hunks:
>
> @@ -139,7 +142,9 @@ static bool fileIsForeignScanParallelSafe(PlannerInfo
> *root, RelOptInfo *rel,
> */
> static bool is_valid_option(const char *option, Oid context);
> static void fileGetOptions(Oid foreigntableid,
> - char **filename, List **other_options);
> + char **filename,
> + bool *is_program,
>
> Space after "is_program,"
>
> @@ -196,16 +201,17 @@ file_fdw_validator(PG_FUNCTION_ARGS)
>
> /*
> * Only superusers are allowed to set options of a file_fdw foreign
> table.
> - * This is because the filename is one of those options, and we don't
> want
> - * non-superusers to be able to determine which file gets read.
> + * The reason for this is to prevent non-superusers from changing the
>
> Space after "the"
>
> - if (stat(filename, &stat_buf) == 0)
> + if ((! is_program) && (stat(filename, &stat_buf) == 0)))
>
> Space between ! and is_program.
>
>
> - if (strcmp(def->defname, "filename") == 0)
> + if ((strcmp(def->defname, "filename") == 0) ||
> (strcmp(def->defname, "program") == 0))
>
> I think the usual style would be to split the if statement into two lines
> as follows to keep within 80 characters per line [1]:
>
> + if ((strcmp(def->defname, "filename") == 0) ||
> + (strcmp(def->defname, "program") == 0))
>
> And likewise for:
>
> - &fdw_private->filename, &fdw_private->options);
> + &fdw_private->filename, &fdw_private->is_program,
> &fdw_private->options);
>
> By the way, doesn't the following paragraph in file-fdw.sgml need an
> update?
>
> <para>
> Changing table-level options requires superuser privileges, for security
> reasons: only a superuser should be able to determine which file is read.
> In principle non-superusers could be allowed to change the other options,
> but that's not supported at present.
> </para>
>
>
> I would like to mark this now as "Ready for Committer".
>
> Thanks,
> Amit
>
> [1] https://www.postgresql.org/docs/devel/static/source-format.html
>
>
>