Re: per-column generic option

Shigeru Hanada escribió:
> (2011/06/26 18:34), Kohei KaiGai wrote:
> > I checked your patch.
>
> Thanks for the review! Please find attached a revised patch.

Err, \dec seems to have a line in describe.h but nowhere else; are you
going to introduce that command?

The new ALTER TABLE grammar seems a bit strange -- ADD, SET, DROP. Is
this defined by the SQL/MED standard? 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?).

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

I think this should be removed:

+ foreach (clist, column->fdwoptions)
+ {
+ DefElem *option = (DefElem *) lfirst(clist);
+ elog(DEBUG3, "%s=%s", option->defname, strVal(option->arg));
+ }

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.

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.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support