Re: per-column generic option

I checked your patch.

The backend portion seems to me OK, but I have several questions/comments.

* This patch should be rebased.
It conflicts with the latest bin/psql/describe.c and
include/catalog/catversion.h.
IIRC, we should not touch catversion.h in submission stage. (It might
be a task of
committer when a patch get upstreamed.)

* It might be an option to extend attreloptions, instead of the new
attfdwoptions.
Although I didn't track the discussion when pg_foreign_table catalog
that provides
relation level fdw-options, was it impossible or unreasonable to extend existing
design of reloptions/attoptions?
Right now, it accepts only hard-wired options listed at reloptions.c.
But, it seems
to me worthwhile, if it could accept options validated by loadable modules.

* pg_dump shall die when we run it for older postgresql version.

This patch does not modify queries to older postgresql version at
getTableAttrs().
In the result, this index shall be set by -1.
+ i_attfdwoptions = PQfnumber(res, "attfdwoptions");

Then, PGgetvalue() returns NULL for unranged column number, and strdup()
shall cause segmentation fault.
+ tbinfo->attfdwoptions[j] = strdup(PQgetvalue(res, j,
i_attfdwoptions));

In fact, I tried to run the patched pg_dump towards v9.0.2
[kaigai(at)vmlinux pg_dump]$ ./pg_dump postgres
pg_dump: column number -1 is out of range 0..14
Segmentation fault

My recommendation is to append "NULL as attfdwoptions" on the queries to
older versions. It eventually makes PGgetvalue() to return an empty string,
then strdup() does not cause a problem.

Thanks,

2011年6月15日10:57 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> (2011/06/14 21:20), Robert Haas wrote:
>> I haven't looked at the patch yet, but here are a few comments on the
>> design, which overall looks good.
>
> Thanks for the review. Please find attached a revised patch.
>
> In addition to responding to your comments, I also added pg_dump
> support. Now pg_dump dumps per-column generic options with ALTER
> FOREIGN TABLE ALTER COLUMN statement just after CREATE FOREIGN TABLE.
> Once I've though to dump them in each column definition of a CREATE
> FOREIGN TABLE statement, but that seems to makes the statement too complex.
>
>> 2011/6/14 Shigeru Hanada<shigeru(dot)hanada(at)gmail(dot)com>:
>>> 1) psql should support describing per-column generic options, so \dec
>>> command was added. If the form \dec+ is used, generic options are also
>>> displayed. Output sample is:
>>
>> I would not add a new backslash command for this - it's unlikely to be
>> useful to see this information across all tables. It would be more
>> helpful to somehow (not sure of the details) incorporate this into the
>> output of running \d on a foreign table.
>
> Hm, belatedly I noticed that relation-kind-specific column are added
> preceding to verbose-only columns such as expression for indexes and
> column values for sequences. It seems suitable place to show per-column
> generic options. Please see attached "desc_results.txt" as sample.
>
> I also noticed that relation-kind-specific information are not mentioned
> in any document (at least in the section of psql[1]), even about
> existing ones such as sequence values and index definition. I also
> added short brief of them to psql document.
>
> BTW, while working around \d command, I noticed that we can avoid
> variable width (# of columns) query result, which is used to fetch
> column information, with using NULL as placeholder (and it has already
> been used partially). I think that it would enhance maintainability
> little, so I've separated this fix to another patch
> avoid_variable_width_result.patch. The main patch
> per_column_option_v2.patch assumes that this fix has been applied.
>
> [1] http://developer.postgresql.org/pgdocs/postgres/app-psql.html
>
>>> Here I found an inconsistency about privilege to see generic options
>>> (not only column but also FDW and server et al). The
>>> information_schema.*_options only shows options which are associated to
>>> objects that current user can access, but \de*+ doesn't have such
>>> restriction. \de* commands should be fixed to hide forbidden objects?
>>
>> It's less important whether \de* is consistent with information_schema
>> in this regard than it is whether it is consistent with other psql
>> backslash commands, e.g. \dv or \db or \dC. AFAIK those commands do
>> not filter by privilege.
>
> Agreed, I'll leave \de* to show results unconditionally.
>
>>> 1) Is "generic options" proper term to mean FDW-specific option
>>> associated to a FDW object? It's used in the SQL/MED standard, but
>>> seems not popular... "FDW option" would be better than "generic option"?
>>
>> I think FDW option is much clearer.
>
> So do I, but I didn't touch them because "generic option" appears in
> many documents, source files including comments and psql's \d* output.
> Most of them have been there since 8.4. Is it acceptable to change them
> to "FDW option", at least for only documents?
>
> OTOH, psql's \d* commands use "Options" as column header of FDW options
> and reloptions. I also left them because I thought that this would not
> cause misunderstanding.
>
> Regards,
> --
> Shigeru Hanada
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>