Re: force_not_null option support for file_fdw

I tried to review this patch.

It seems to me its implementation is reasonable and enough simple.
All the works of this patch is pick-up "force_not_null" option from
pg_attribute.attfdwoptions and transform its data structure into suitable
form to the existing BeginCopyFrom().
So, I'd almost like to mark this patch "Ready for Committer".

Here are only two points I'd like to comment on.

+ tuple = SearchSysCache2(ATTNUM,
+ RelationGetRelid(rel),
+ Int16GetDatum(attnum));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("cache lookup failed for attribute %d of
relation %u",
+ attnum, RelationGetRelid(rel))));

The tuple should be always found unless we have any bugs that makes
mismatch between pg_class.relnatts and actual number of attributes.
So, it is a case to use elog(), instead of ereport() with error code.

One other point is diffset of regression test, when I run `make check
-C contrib/file_fdw'.
Do we have something changed corresponding to COPY TO/FROM statement
since 8th-August to now?

*** /home/kaigai/repo/sepgsql/contrib/file_fdw/expected/file_fdw.out
2011-09-04 20:36:23.670981921 +0200
--- /home/kaigai/repo/sepgsql/contrib/file_fdw/results/file_fdw.out
2011-09-04 20:36:51.202989681 +0200
***************
*** 118,126 ****
word1 | word2
-------+-------
123 | 123
ABC | ABC
NULL |
- abc | abc
(4 rows)

-- basic query tests
--- 118,126 ----
word1 | word2
-------+-------
123 | 123
+ abc | abc
ABC | ABC
NULL |
(4 rows)

-- basic query tests

======================================================================

Thanks,

2011年8月8日9:14 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> Hi,
>
> I propose to support force_not_null option for file_fdw too.
>
> In 9.1 development cycle, file_fdw had been implemented with exported
> COPY FROM routines, but only force_not_null option has not been
> supported yet.
>
> Originally, in COPY FROM, force_not_null is specified as a list of
> column which is not matched against null-string. Now per-column FDW
> option is available, so I implemented force_not_null options as boolean
> value for each column to avoid parsing column list in file_fdw.
>
> True means that the column is not matched against null-string, and it's
> equivalent to specify the column's name in force_not_null option of COPY
> FROM. Default value is false.
>
> The patch includes changes for code, document and regression tests. Any
> comments/questions are welcome.
>
> 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>