Re: Let file_fdw access COPY FROM PROGRAM - Mailing list pgsql-hackers

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