Re: per-column generic option
От | Kohei KaiGai |
---|---|
Тема | Re: per-column generic option |
Дата | |
Msg-id | CADyhKSXE_Ze6uprbw2ixn25N5O3vLV1oWbJDpe8iGe1Ftei=cg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: per-column generic option (Shigeru Hanada <shigeru.hanada@gmail.com>) |
Ответы |
Re: per-column generic option
|
Список | pgsql-hackers |
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.hanada@gmail.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@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@kaigai.gr.jp>
В списке pgsql-hackers по дате отправления:
Следующее
От: Martijn van OosterhoutДата:
Сообщение: Re: Deriving release notes from git commit messages