Re: per-column generic option - Mailing list pgsql-hackers
From | Shigeru Hanada |
---|---|
Subject | Re: per-column generic option |
Date | |
Msg-id | 4E1B0511.4000209@gmail.com Whole thread Raw |
In response to | Re: per-column generic option (Alvaro Herrera <alvherre@commandprompt.com>) |
List | pgsql-hackers |
Thanks for the review. (2011/07/10 12:49), Alvaro Herrera wrote: > Err, \dec seems to have a line in describe.h but nowhere else; are you > going to introduce that command? \dec command is obsolete, so I removed that line. > The new ALTER TABLE grammar seems a bit strange -- ADD, SET, DROP. Is > this defined by the SQL/MED standard? Yes, syntax for altering foreign table is defined by the SQL/MED standard as below, and <alter generic option> is common to all SQL/MED objects: <alter foreign table statement> ::= ALTER FOREIGN TABLE <table name> <alter foreign table action> <alter foreign table action> ::= <add basic column definition> | <alter basic column definition> | <drop basic column definition> | <alter generic options> <alter generic options> ::= OPTIONS <left paren> <alter generic option list> <right paren> <alter generic option list> ::= <alter generic option> [ { <comma> <alter generic option> }... ] <alter generic option> ::= [ <alter operation> ] <option name> [ <option value> ] <alter operation> ::= ADD | SET | DROP <generic option> ::= <option name> [ <option value> ] <option value> ::= <character string literal> FYI, default for <alter operation> is ADD. > It seems at odds with our > handling of attoptions (and the pg_dump query seems rather bizarre in > comparison to the handling of attoptions there; why do we need > pg_options_to_table when attoptions do not?). That's because of the syntax difference between FDW options and AM options. AM options should be dumped as "key=value, key=value, ...", but FDW options should be dumped as "key 'value', key 'value', ...". The pg_options_to_table() is used to construct list in the latter form. The way used to handle per-column options in my patch is same as the way used for other existing FDW objects, such as FDW, server, and user mapping. > What's the reason for this: > > @@ -3681,7 +3691,7 @@ AlterFdwStmt: ALTER FOREIGN DATA_P WRAPPER name opt_fdw_options alter_generic_op > /* Options definition for CREATE FDW, SERVER and USER MAPPING */ > create_generic_options: > OPTIONS '(' generic_option_list ')' { $$ = $3; } > - | /*EMPTY*/ { $$ = NIL; } > + | /*EMPTY*/ { $$ = NIL } > ; Reverted this unintended change. > I think this should be removed: > > + foreach (clist, column->fdwoptions) > + { > + DefElem *option = (DefElem *) lfirst(clist); > + elog(DEBUG3, "%s=%s", option->defname, strVal(option->arg)); > + } Removed, the codes were used only for debug. > As for whether attfdwoptions needs to be separate from attoptions, I am > not sure that they really need to be; but if they are, I think we should > use a different name than attfdwoptions, because we might want to use > them for something else. Maybe attgenoptions (and note that the alter > table code already calls them "generic options" so I'm not sure why the > rest of the code is calling them FDW options) ... but then I really > start to question whether they need to be separate from attoptions. For now I got +1 for attfdwoptions and +1 for attgenoptions for the naming. I prefer attgenoptions because it follows SQL/MED standard, but I don't have strong feeling for naming, so I've inspected usage in the current HEAD... Hm, "gen.*option" appears twice much as "fdw.*option" in the source code with case insensitive grep, and most of "fdw.*option" were hit "fdwoptions", per-wrapper options. ISTM that "generic option" would be better to mean options used by FDW for consistency, so I unified the wording to "generic option" from "fdw option". I hope that the name "generic option" doesn't confuse users who aren't familiar to SQL/MED standard. > Currently, attoptions are used to store n_distinct and > n_distinct_inherited. Do those options make sense for foreign tables? > If they do make sense for some types of foreign servers, maybe we should > decree that they need to be specifically declared as such by the FDW -- > that is, the FDW needs to provide its own attribute_reloptions routine > (or equivalent therein) for validation that includes those core options. > There is no saying that, even if all existing core options such as > n_distinct apply to a FDW now, more core options that we might invent in > the future are going to apply as well. > > In short: in my opinion, attoptions and attfdwoptions need to be one > thing and the same. The n_distinct might make sense for every foreign tables in a sense, though syntax to set it is not supported. It would allow users to specify not-FDW-specific statistics information to control costs for the scan. However each FDW would be able to support such option too. I think that reloptions and attoptions should be used by only PG core, and FDW should use generic options. So I prefer separated design. The attached patch fixes issues other than generic options separation. Regards, -- Shigeru Hanada
Attachment
pgsql-hackers by date: