Re: Review: Patch FORCE_NULL option for copy COPY in CSV mode

2014-03-02 8:26 GMT+09:00 Andrew Dunstan <andrew(at)dunslane(dot)net>:
>
> On 01/29/2014 10:59 AM, Ian Lawrence Barwick wrote:
>>
>> 2014/1/29 Ian Lawrence Barwick <barwick(at)gmail(dot)com>:
>>>
>>> 2014-01-29 Andrew Dunstan <andrew(at)dunslane(dot)net>:
>>>>
>>>> On 01/28/2014 05:55 AM, Ian Lawrence Barwick wrote:
>>>>>
>>>>>
>>>>> Hi Payal
>>>>>
>>>>> Many thanks for the review, and my apologies for not getting back to
>>>>> you earlier.
>>>>>
>>>>> Updated version of the patch attached with suggested corrections.
>>>>
>>>> On a very quick glance, I see that you have still not made adjustments
>>>> to
>>>> contrib/file_fdw to accommodate this new option. I don't see why this
>>>> COPY
>>>> option should be different in that respect.
>>>
>>> Hmm, that idea seems to have escaped me completely. I'll get onto it
>>> forthwith.
>>
>> Striking while the keyboard is hot... version with contrib/file_fdw
>> modifications
>> attached.
>>
>>

> I have reviewed this. Generally it's good, but the author has made a
> significant error - the idea is not to force a quoted empty string to null,
> but to force a quoted null string to null, whatever the null string might
> be. The default case has these the same, but if you specify a non-empty null
> string they aren't.

The author slaps himself on the forehead while regretting he was temporally
constricted when dealing with the patch and never thought to look beyond
the immediate use case.

Thanks for the update, much appreciated.

> That difference actually made the file_fdw regression results plain wrong,
> in my view, in that they expected a quoted empty string to be turned to null
> even when the null string was something else.
>
> I've adjusted this and the docs and propose to apply the attached patch in
> the next day or two unless there are any objections.

Unless I'm overlooking something, output from "SELECT * FROM text_csv;"
in 'output/file_fdw.source' still needs updating?

Regards

Ian Barwick