Thanks for the review.
(2011/09/05 3:55), Kohei KaiGai wrote:
> 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.
Oh, I've missed that other similar errors use elog()...
Fixed.
> 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?
I don't know about such change, and src/backend/command/copy.c has not
been touched since Feb 23.
> *** /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
>
> ======================================================================
In my usual environment that test passed, but finally I've reproduced
the failure with setting $LC_COLLATE to "es_ES.UTF-8". Do you have set
any $LC_COLLATE in your test environment?
Regards,
--
Shigeru Hanada
