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

On 03/02/2014 10:06 PM, Ian Lawrence Barwick wrote:
> 2014-03-02 8:26 GMT+09:00 Andrew Dunstan <andrew@dunslane.net>:
>> On 01/29/2014 10:59 AM, Ian Lawrence Barwick wrote:
>>> 2014/1/29 Ian Lawrence Barwick <barwick@gmail.com>:
>>>> 2014-01-29 Andrew Dunstan <andrew@dunslane.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?
>
>


Yes, you're right. Will fix.

cheers

andrew