(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
