Re: per-column generic option

Hanada-san,

I checked the per-column generic option patch.
Right now, I have nothing to comment on anymore.
So, it should be reviewed by committers.

Thanks,

2011年6月27日16:47 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> (2011/06/26 18:34), Kohei KaiGai wrote:
>> I checked your patch.
>
> Thanks for the review! Please find attached a revised 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.)
>
> I've rebased against current HEAD, and reverted catversion.h.
>
>> * 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.
>
> IIRC someone has objected against storing FDW options in
> reloptions/attoptions, but I couldn't find such post. I'll follow the
> discussion again.
>
> IMHO, though at present I don't have clear proof, separating FDW options
> from access method options seems better than merging them, but I should
> learn more about AM mechanism to clarify this issue. Please check other
> issues first.
>
>> * 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.
>
> Fixed in the way you've recommended, and tested against 8.4. I should
> have noticed that same technique is used in some other places...
>
> BTW, I also have found an unnecessary FIXME comment and removed it.
> Please see the line 2845 of src/backend/catalog/heap.c
> (InsertPgAttributeTuple) for the correction.
>
> Regards,
> --
> Shigeru Hanada
>
>

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