Re: not null validation option in contrib/file_fdw

I updated the patch. Attached is an updated version of the patch.

Changes:
* fix a bug in fileGetOptions()
* rename the validation option and its code to "validate_data_file"
* clean up

Best regards,
Etsuro Fujita

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
> Sent: Monday, April 16, 2012 4:09 PM
> To: 'Andrew Dunstan'; 'Shigeru HANADA'
> Cc: pgsql-hackers(at)postgresql(dot)org
> Subject: Re: [HACKERS] not null validation option in contrib/file_fdw
>
> Thank you for the review.
>
> > -----Original Message-----
> > From: pgsql-hackers-owner(at)postgresql(dot)org
> > [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Andrew
> > Dunstan
> > Sent: Friday, April 13, 2012 9:16 PM
> > To: Shigeru HANADA
> > Cc: Etsuro Fujita; pgsql-hackers(at)postgresql(dot)org
> > Subject: Re: [HACKERS] not null validation option in contrib/file_fdw
> >
> >
> >
> > On 04/13/2012 07:21 AM, Shigeru HANADA wrote:
> > > (2012/04/13 16:59), Etsuro Fujita wrote:
> > >> I updated the patch added to CF 2012-Next [1]. Attached is the
> > >> updated version of the patch.
> > > I applied the patch and ran regression tests of file_fdw, and I got
> > > SIGSEGV X-(
> > >
> > > The failure occurs in fileGetOptions, and it is caused by
> > > list_delete_cell used in foreach loop; ListCell points delete target
> > > has been free-ed in list_delete_cell, but foreach accesses it to get
> > > next element.
> > >
> > > Some of backend functions which use list_delete_cell in loop use "for"
> > > loop instead of foreach, and other functions exit the loop after
> > > calling list_delete_cell. Since we can't stop searching non-COPY
> > > options until meeting the end of the options list, we would need to
> > > choose former ("for" loop), or create another list which contains
> > > only valid COPY options and return it via other_options parameter.
> > >
> >
> > Yes, the code in fileGetOptions() appears to be bogus.
>
> Sorry, I will fix it.
>
> > Also, "validate" is a terrible name for the option (and in the code)
> IMNSHO.
> > It's far too generic. "validate_not_null" or some such would surely be
> > better.
>
> I thought it would be used for not only NOT NULL but also CHECK and
foreign
> key constraints. That is, when a user sets the option to 'true', file_fdw
> verifies that each tuple meets all kinds of constraints. So, how about
> "validate_data_file" or simply "validate_file"?
>
> Best regards,
> Etsuro Fujita
>
> > cheers
> >
> > andrew
> >
> >
> > --
> > 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
>
>
>
> --
> 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