Re: per-column generic option - Mailing 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