Re: not null validation option in contrib/file_fdw - Mailing list pgsql-hackers

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@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Etsuro Fujita
> Sent: Monday, April 16, 2012 4:09 PM
> To: 'Andrew Dunstan'; 'Shigeru HANADA'
> Cc: pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] not null validation option in contrib/file_fdw
> 
> Thank you for the review.
> 
> > -----Original Message-----
> > From: pgsql-hackers-owner@postgresql.org
> > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Andrew
> > Dunstan
> > Sent: Friday, April 13, 2012 9:16 PM
> > To: Shigeru HANADA
> > Cc: Etsuro Fujita; pgsql-hackers@postgresql.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@postgresql.org) To
> > make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
> 
> 
> 
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make
> changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers