Re: Let file_fdw access COPY FROM PROGRAM
От | Corey Huinker |
---|---|
Тема | Re: Let file_fdw access COPY FROM PROGRAM |
Дата | |
Msg-id | CADkLM=eRMivEKLuhWiCb1__dK0aOSXi2Q8X4SSO2bVWPLVr0Gg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Let file_fdw access COPY FROM PROGRAM (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>) |
Ответы |
Re: Let file_fdw access COPY FROM PROGRAM
(Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>)
Re: Let file_fdw access COPY FROM PROGRAM (Tom Lane <tgl@sss.pgh.pa.us>) |
Список | pgsql-hackers |
On Mon, Sep 12, 2016 at 1:59 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.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
(reposting non-top-posted...sorry)
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.![](https://ssl.gstatic.com/ui/v1/icons/mail/images/cleardot.gif)
"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.
![](https://ssl.gstatic.com/ui/v1/icons/mail/images/cleardot.gif)
Вложения
В списке pgsql-hackers по дате отправления:
Следующее
От: Tom LaneДата:
Сообщение: Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)