Обсуждение: per-column FDW options, v5
Here is a rebased version of per-column FDW options patch. I've proposed this patch in last CF, but it was marked as returned with feedback. So I would like to propose in next CF 2011-09. I already moved CF item into new topic "SQL/MED" of CF 2011-09. I didn't change the patch except rebasing against head, so this patch includes changes below: * New attribute attfdwoptions of pg_attribute hold per-column FDW options, so this patch needs bumping catversion. * CREATE/ALTER FOREIGN TABLE statement accept OPTIONS (key 'value') syntax for per-column FDW options. * New information_schema.column_options view shows per-column FDW options of accessible foreign tables. * \d+ shows per-column options, only for foreign tables. * pg_dump support dumping per-column FDW options as ALTER FOREIGN TABLE statement. IIUC, one of the reasons why this patch hasn't been accepted is that we don't have a consensus about storage for per-column FDW options. I think that separating attributes would be better for several reasons: * FDW options might conflict reloptions/attoptions. * FDW options and reloptions have different syntax, so pg_dump would need to switch the format depending on target table's relkind. I also attached a rebased version of force_not_null patch, which adds force_not_null option support to file_fdw. This is a use case of per-column FDW option. Regards, -- Shigeru Hanada * 英語 - 自動検出 * 英語 * 日本語 * 英語 * 日本語 <javascript:void(0);>
Вложения
2011/7/29 Shigeru Hanada <shigeru.hanada@gmail.com>: > Here is a rebased version of per-column FDW options patch. I've > proposed this patch in last CF, but it was marked as returned with > feedback. So I would like to propose in next CF 2011-09. I already > moved CF item into new topic "SQL/MED" of CF 2011-09. I did a quick review of this patch and it looks good to me, modulo some quibbles with the wording of the documentation and comments and a couple of minor stylistic nitpicks. Barring objections, I'll fix this up a bit and commit it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Jul 29, 2011 at 10:09 AM, Robert Haas <robertmhaas@gmail.com> wrote: > 2011/7/29 Shigeru Hanada <shigeru.hanada@gmail.com>: >> Here is a rebased version of per-column FDW options patch. I've >> proposed this patch in last CF, but it was marked as returned with >> feedback. So I would like to propose in next CF 2011-09. I already >> moved CF item into new topic "SQL/MED" of CF 2011-09. > > I did a quick review of this patch and it looks good to me, modulo > some quibbles with the wording of the documentation and comments and a > couple of minor stylistic nitpicks. Barring objections, I'll fix this > up a bit and commit it. Done. Incidentally, I notice that if you do: \d some_foreign_table ...the table-level options are not displayed. We probably ought to do something about that... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Sorry, I've missed sending copy to list, so I quoted off-list discussion.
> On Aug 5, 2011, at 7:59 PM, Shigeru Hanada<shigeru.hanada@gmail.com>
wrote:
>
>> 2011/8/6 Robert Haas<robertmhaas@gmail.com>:
>>> Done.
>>
>> Thanks!
>>
>>> Incidentally, I notice that if you do:
>>>
>>> \d some_foreign_table
>>>
>>> ...the table-level options are not displayed. We probably ought to do
>>> something about that...
>>
>> Currently table-level options are showin in result of \det+ command
>> (only verbose mode), in same style as fdw and foreign servers.
>>
>> But \d is more popular for table describing, so moving table-level
>> options from \det+ to \d might be better. Thoughts?
(2011/08/06 9:26), Robert Haas wrote:
> I'd show it both places.
After taking a look at describe.c, I think some styles are applicable to
FDW options in result of \d command.
(1) simple array literal style
Show table-level FDW options like other FDW options. It is simply a
result of array_out(); each options is shown as "key=value" with quoting
if necessary and delimited by ','. Whole line is surrounded by { and }.If an element includes any character which need
tobe escaped, such
element is quoted with double-quotation.
Ex) FDW Options: {"delimiter=,","quote=\""} #delimiter is a comma, and qutoe is a double-quote
(2) reloptions style
Show FDW options like reloptions of \d+ result. Each options is shown
as "key=value" without quoting. Some special characters might make it
little illegible.
Ex) FDW Options: delimiter=,, quote=" #delimiter is a comma, and qutoe is a double-quote
(3) OPTIONS clause style
Show FDW options as they were in OPTIONS clause. Each option is shown
as "key 'value'", and delimited with ','.
Ex) FDW Options: delimiter ',', quote '''' #delimiter is a comma, and qutoe is a single-quote
(1) can be implemented with minimum change, and it also keeps the
behavior consistent with other existing FDW objects. But IMHO (3) is
most readable, though it breaks backward compatibility about the format
of FDW options used in the result of \d* command. Thoughts?
BTW, I found wrong description about per-table FDW options; psql
document says that \d+ shows table-level FDW options, but actually it
doesn't show. I'll post this issue in another new thread.
Regards,
--
Shigeru Hanada
(2011/07/29 17:37), Shigeru Hanada wrote: > I also attached a rebased version of force_not_null patch, which adds > force_not_null option support to file_fdw. This is a use case of > per-column FDW option. [just for redirection] Robert has committed only per_column_option patch. So I posted force_not_null patch in a new thread, and added it to CF 2011-09 as an independent new item. https://commitfest.postgresql.org/action/patch_view?id=615 Regards, -- Shigeru Hanada
2011/8/8 Shigeru Hanada <shigeru.hanada@gmail.com>:
>>> Currently table-level options are showin in result of \det+ command
>>> (only verbose mode), in same style as fdw and foreign servers.
>>>
>>> But \d is more popular for table describing, so moving table-level
>>> options from \det+ to \d might be better. Thoughts?
>
> (2011/08/06 9:26), Robert Haas wrote:
>> I'd show it both places.
>
> After taking a look at describe.c, I think some styles are applicable to
> FDW options in result of \d command.
>
> (1) simple array literal style
> Show table-level FDW options like other FDW options. It is simply a
> result of array_out(); each options is shown as "key=value" with quoting
> if necessary and delimited by ','. Whole line is surrounded by { and }.
> If an element includes any character which need to be escaped, such
> element is quoted with double-quotation.
>
> Ex)
> FDW Options: {"delimiter=,","quote=\""}
> #delimiter is a comma, and qutoe is a double-quote
>
> (2) reloptions style
> Show FDW options like reloptions of \d+ result. Each options is shown
> as "key=value" without quoting. Some special characters might make it
> little illegible.
>
> Ex)
> FDW Options: delimiter=,, quote="
> #delimiter is a comma, and qutoe is a double-quote
>
> (3) OPTIONS clause style
> Show FDW options as they were in OPTIONS clause. Each option is shown
> as "key 'value'", and delimited with ','.
>
> Ex)
> FDW Options: delimiter ',', quote ''''
> #delimiter is a comma, and qutoe is a single-quote
>
> (1) can be implemented with minimum change, and it also keeps the
> behavior consistent with other existing FDW objects. But IMHO (3) is
> most readable, though it breaks backward compatibility about the format
> of FDW options used in the result of \d* command. Thoughts?
I'm against #2, but I could go either way on #1 vs. #3. If you pick
#3, would you also change the column options to be displayed that way,
or would we end up with table and column options displayed
differently?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
(2011/08/09 1:16), Robert Haas wrote:
> 2011/8/8 Shigeru Hanada<shigeru.hanada@gmail.com>:
>>>> Currently table-level options are showin in result of \det+ command
>>>> (only verbose mode), in same style as fdw and foreign servers.
>>>>
>>>> But \d is more popular for table describing, so moving table-level
>>>> options from \det+ to \d might be better. Thoughts?
>>
>> (2011/08/06 9:26), Robert Haas wrote:
>>> I'd show it both places.
>>
>> After taking a look at describe.c, I think some styles are applicable to
>> FDW options in result of \d command.
>>
>> (1) simple array literal style
>> Show table-level FDW options like other FDW options. It is simply a
>> result of array_out(); each options is shown as "key=value" with quoting
>> if necessary and delimited by ','. Whole line is surrounded by { and }.
>> If an element includes any character which need to be escaped, such
>> element is quoted with double-quotation.
>>
>> Ex)
>> FDW Options: {"delimiter=,","quote=\""}
>> #delimiter is a comma, and qutoe is a double-quote
>>
>> (2) reloptions style
>> Show FDW options like reloptions of \d+ result. Each options is shown
>> as "key=value" without quoting. Some special characters might make it
>> little illegible.
>>
>> Ex)
>> FDW Options: delimiter=,, quote="
>> #delimiter is a comma, and qutoe is a double-quote
>>
>> (3) OPTIONS clause style
>> Show FDW options as they were in OPTIONS clause. Each option is shown
>> as "key 'value'", and delimited with ','.
>>
>> Ex)
>> FDW Options: delimiter ',', quote ''''
>> #delimiter is a comma, and qutoe is a single-quote
>>
>> (1) can be implemented with minimum change, and it also keeps the
>> behavior consistent with other existing FDW objects. But IMHO (3) is
>> most readable, though it breaks backward compatibility about the format
>> of FDW options used in the result of \d* command. Thoughts?
>
> I'm against #2, but I could go either way on #1 vs. #3. If you pick
> #3, would you also change the column options to be displayed that way,
> or would we end up with table and column options displayed
> differently?
I'd like to pick #3, and also change per-column options format. In
addition, I'd like to change options format for other FDW objects such
as wrappers, servers and user mappings for consistency. Of course, only
if it's acceptable to break backward compatibility...
Regards,
--
Shigeru Hanada
2011/8/9 Shigeru Hanada <shigeru.hanada@gmail.com>:
> (2011/08/09 1:16), Robert Haas wrote:
>> 2011/8/8 Shigeru Hanada<shigeru.hanada@gmail.com>:
>>>>> Currently table-level options are showin in result of \det+ command
>>>>> (only verbose mode), in same style as fdw and foreign servers.
>>>>>
>>>>> But \d is more popular for table describing, so moving table-level
>>>>> options from \det+ to \d might be better. Thoughts?
>>>
>>> (2011/08/06 9:26), Robert Haas wrote:
>>>> I'd show it both places.
>>>
>>> After taking a look at describe.c, I think some styles are applicable to
>>> FDW options in result of \d command.
>>>
>>> (1) simple array literal style
>>> Show table-level FDW options like other FDW options. It is simply a
>>> result of array_out(); each options is shown as "key=value" with quoting
>>> if necessary and delimited by ','. Whole line is surrounded by { and }.
>>> If an element includes any character which need to be escaped, such
>>> element is quoted with double-quotation.
>>>
>>> Ex)
>>> FDW Options: {"delimiter=,","quote=\""}
>>> #delimiter is a comma, and qutoe is a double-quote
>>>
>>> (2) reloptions style
>>> Show FDW options like reloptions of \d+ result. Each options is shown
>>> as "key=value" without quoting. Some special characters might make it
>>> little illegible.
>>>
>>> Ex)
>>> FDW Options: delimiter=,, quote="
>>> #delimiter is a comma, and qutoe is a double-quote
>>>
>>> (3) OPTIONS clause style
>>> Show FDW options as they were in OPTIONS clause. Each option is shown
>>> as "key 'value'", and delimited with ','.
>>>
>>> Ex)
>>> FDW Options: delimiter ',', quote ''''
>>> #delimiter is a comma, and qutoe is a single-quote
>>>
>>> (1) can be implemented with minimum change, and it also keeps the
>>> behavior consistent with other existing FDW objects. But IMHO (3) is
>>> most readable, though it breaks backward compatibility about the format
>>> of FDW options used in the result of \d* command. Thoughts?
>>
>> I'm against #2, but I could go either way on #1 vs. #3. If you pick
>> #3, would you also change the column options to be displayed that way,
>> or would we end up with table and column options displayed
>> differently?
>
> I'd like to pick #3, and also change per-column options format. In
> addition, I'd like to change options format for other FDW objects such
> as wrappers, servers and user mappings for consistency. Of course, only
> if it's acceptable to break backward compatibility...
I think it's fine to change the display format. We haven't had these
features for very long, so users hopefully shouldn't be expecting that
everything is set in stone. We have made far bigger changes to
backslash commands that have been around for far longer (\df, I'm
looking at you).
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of jue ago 11 11:50:40 -0400 2011: > 2011/8/9 Shigeru Hanada <shigeru.hanada@gmail.com>: > >>> (3) OPTIONS clause style > >>> Show FDW options as they were in OPTIONS clause. Each option is shown > >>> as "key 'value'", and delimited with ','. > >>> > >>> Ex) > >>> FDW Options: delimiter ',', quote '''' > >>> #delimiter is a comma, and qutoe is a single-quote > >> I'm against #2, but I could go either way on #1 vs. #3. If you pick > >> #3, would you also change the column options to be displayed that way, > >> or would we end up with table and column options displayed > >> differently? > > > > I'd like to pick #3, and also change per-column options format. In > > addition, I'd like to change options format for other FDW objects such > > as wrappers, servers and user mappings for consistency. Of course, only > > if it's acceptable to break backward compatibility... > > I think it's fine to change the display format. We haven't had these > features for very long, so users hopefully shouldn't be expecting that > everything is set in stone. We have made far bigger changes to > backslash commands that have been around for far longer (\df, I'm > looking at you). We've never promised that backslash commands behave identically across releases. I think they are more for human consumption than machine, so why would we care about changing one of them a bit? -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Aug 11, 2011 at 12:04 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Excerpts from Robert Haas's message of jue ago 11 11:50:40 -0400 2011: >> 2011/8/9 Shigeru Hanada <shigeru.hanada@gmail.com>: > >> >>> (3) OPTIONS clause style >> >>> Show FDW options as they were in OPTIONS clause. Each option is shown >> >>> as "key 'value'", and delimited with ','. >> >>> >> >>> Ex) >> >>> FDW Options: delimiter ',', quote '''' >> >>> #delimiter is a comma, and qutoe is a single-quote > >> >> I'm against #2, but I could go either way on #1 vs. #3. If you pick >> >> #3, would you also change the column options to be displayed that way, >> >> or would we end up with table and column options displayed >> >> differently? >> > >> > I'd like to pick #3, and also change per-column options format. In >> > addition, I'd like to change options format for other FDW objects such >> > as wrappers, servers and user mappings for consistency. Of course, only >> > if it's acceptable to break backward compatibility... >> >> I think it's fine to change the display format. We haven't had these >> features for very long, so users hopefully shouldn't be expecting that >> everything is set in stone. We have made far bigger changes to >> backslash commands that have been around for far longer (\df, I'm >> looking at you). > > We've never promised that backslash commands behave identically across > releases. I think they are more for human consumption than machine, so > why would we care about changing one of them a bit? Yeah, I agree. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Change format of FDW options used in \d* commands (was: Re: per-column FDW options, v5)
От
Shigeru Hanada
Дата:
(2011/08/12 1:05), Robert Haas wrote:
> On Thu, Aug 11, 2011 at 12:04 PM, Alvaro Herrera
> <alvherre@commandprompt.com> wrote:
>> Excerpts from Robert Haas's message of jue ago 11 11:50:40 -0400 2011:
>>> 2011/8/9 Shigeru Hanada<shigeru.hanada@gmail.com>:
>>>> I'd like to pick #3, and also change per-column options format. In
>>>> addition, I'd like to change options format for other FDW objects such
>>>> as wrappers, servers and user mappings for consistency. Of course, only
>>>> if it's acceptable to break backward compatibility...
>>>
>>> I think it's fine to change the display format. We haven't had these
>>> features for very long, so users hopefully shouldn't be expecting that
>>> everything is set in stone. We have made far bigger changes to
>>> backslash commands that have been around for far longer (\df, I'm
>>> looking at you).
>>
>> We've never promised that backslash commands behave identically across
>> releases. I think they are more for human consumption than machine, so
>> why would we care about changing one of them a bit?
>
> Yeah, I agree.
Thanks for the comments.
Attached patch changes various \d*+ commands to show FDW options in same
format as OPTIONS clause. IMHO the new format is easier to read.
Example)
old: {"delimiter=,","quote=\""}
new: delimiter ',', quote '"'
All regression tests including contrib's installcheck passed.
I'll add this patch to CF app as new item.
Regards,
--
Shigeru Hanada
Вложения
2011/8/12 Shigeru Hanada <shigeru.hanada@gmail.com>:
> (2011/08/12 1:05), Robert Haas wrote:
>> On Thu, Aug 11, 2011 at 12:04 PM, Alvaro Herrera
>> <alvherre@commandprompt.com> wrote:
>>> Excerpts from Robert Haas's message of jue ago 11 11:50:40 -0400 2011:
>>>> 2011/8/9 Shigeru Hanada<shigeru.hanada@gmail.com>:
>>>>> I'd like to pick #3, and also change per-column options format. In
>>>>> addition, I'd like to change options format for other FDW objects such
>>>>> as wrappers, servers and user mappings for consistency. Of course, only
>>>>> if it's acceptable to break backward compatibility...
>>>>
>>>> I think it's fine to change the display format. We haven't had these
>>>> features for very long, so users hopefully shouldn't be expecting that
>>>> everything is set in stone. We have made far bigger changes to
>>>> backslash commands that have been around for far longer (\df, I'm
>>>> looking at you).
>>>
>>> We've never promised that backslash commands behave identically across
>>> releases. I think they are more for human consumption than machine, so
>>> why would we care about changing one of them a bit?
>>
>> Yeah, I agree.
>
> Thanks for the comments.
>
> Attached patch changes various \d*+ commands to show FDW options in same
> format as OPTIONS clause. IMHO the new format is easier to read.
>
> Example)
> old: {"delimiter=,","quote=\""}
> new: delimiter ',', quote '"'
>
> All regression tests including contrib's installcheck passed.
>
> I'll add this patch to CF app as new item.
IMHO, the new format should put parentheses around the options list.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Thanks for the review. (2011/08/13 3:59), Robert Haas wrote: > IMHO, the new format should put parentheses around the options list. Agreed. Revised version of patch has been attached. This version puts parentheses around FDW option only when it was not NULL. Regards, -- Shigeru Hanada
Вложения
2011/8/18 Shigeru Hanada <shigeru.hanada@gmail.com>:
> Thanks for the review.
>
> (2011/08/13 3:59), Robert Haas wrote:
>> IMHO, the new format should put parentheses around the options list.
>
> Agreed. Revised version of patch has been attached. This version puts
> parentheses around FDW option only when it was not NULL.
ISTM you should do quote_ident() on the option names. For example, after:
create foreign data wrapper dummy;
create server s1 foreign data wrapper dummy;
create foreign table ft1 (a int) server s1 options ("(" ')');
With your patch, I get:
rhaas=# \d+ ft1 Foreign table "public.ft1"Column | Type | Modifiers | FDW Options | Storage |
Description
--------+---------+-----------+-------------+---------+-------------a | integer | | | plain
|
Server: s1
FDW Options: (( ')')
Has OIDs: no
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
(2011/08/18 23:21), Robert Haas wrote:
> ISTM you should do quote_ident() on the option names. For example, after:
>
> create foreign data wrapper dummy;
> create server s1 foreign data wrapper dummy;
> create foreign table ft1 (a int) server s1 options ("(" ')');
>
> With your patch, I get:
>
> rhaas=# \d+ ft1
> Foreign table "public.ft1"
> Column | Type | Modifiers | FDW Options | Storage | Description
> --------+---------+-----------+-------------+---------+-------------
> a | integer | | | plain |
> Server: s1
> FDW Options: (( ')')
> Has OIDs: no
>
Oops, good catch. I've fixed psql to use quote_ident() for option_name,
and modified regression tests to use special characters in option names.
Please try revised version of patch.
BTW, I noticed that pg_dump has same issue since 8.4, initial release of
SQL/MED infrastructure. If a FDW option was defined on any FDW object
with a name which contains one of special characters such as space and
parentheses, pg_dump generates invalid OPTIONS clause such as "OPTIONS
(separated name 'value')".
~~~~~~~~~~~~~~
Perhaps this issue has been overlooked because dblink is practically the
only use case of FDW option before 9.1. Since 9.1, users might get
various FDW and some of those might use special characters in option
name. ISTM that this fix should be back-patched, at least to 9.1.
Please find attached patches for each STABLE branch.
Regards,
--
Shigeru Hanada
Вложения
2011/8/19 Shigeru Hanada <shigeru.hanada@gmail.com>: > BTW, I noticed that pg_dump has same issue since 8.4, initial release of > SQL/MED infrastructure. If a FDW option was defined on any FDW object > with a name which contains one of special characters such as space and > parentheses, pg_dump generates invalid OPTIONS clause such as "OPTIONS > (separated name 'value')". > ~~~~~~~~~~~~~~ > Perhaps this issue has been overlooked because dblink is practically the > only use case of FDW option before 9.1. Since 9.1, users might get > various FDW and some of those might use special characters in option > name. ISTM that this fix should be back-patched, at least to 9.1. > Please find attached patches for each STABLE branch. Good catch, committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/8/19 Shigeru Hanada <shigeru.hanada@gmail.com>: > Oops, good catch. I've fixed psql to use quote_ident() for option_name, > and modified regression tests to use special characters in option names. > Please try revised version of patch. This part looks good to me, too. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company