Re: Let file_fdw access COPY FROM PROGRAM

From: Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Craig Ringer <craig(dot)ringer(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Let file_fdw access COPY FROM PROGRAM
Date: 2016-09-12 16:21:08
Message-ID: CADkLM=eBd-dwqeP4LpQp0dhbK=ONYkEgmFpqdN2EYu0uZdzM-g@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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
>
>
>

Attachment Content-Type Size
file_fdw_program_v3.diff text/plain 15.8 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2016-09-12 16:25:34 Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)
Previous Message Andres Freund 2016-09-12 16:16:59 Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)