Thread

1. NOT NULL violation error handling in file_fdw

   Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2012-03-12T10:21:06Z

   According to the following documentation on IterateForeignScan() in
   50.2. Foreign Data Wrapper Callback Routines, I have created a patch to
   support the error handling in file_fdw.  Please find attached a patch.
   
       Note that PostgreSQL's executor doesn't care whether the rows
       returned violate the NOT NULL constraints which were defined
       on the foreign table columns - but the planner does care, and
       may optimize queries incorrectly if NULL values are present
       in a column declared not to contain them. If a NULL value is
       encountered when the user has declared that none should be
       present, it may be appropriate to raise an error (just as you
       would need to do in the case of a data type mismatch).
   
   Best regards,
   Etsuro Fujita
   

2. Re: NOT NULL violation error handling in file_fdw

   Shigeru Hanada <shigeru.hanada@gmail.com> — 2012-03-13T06:53:03Z

   (2012/03/12 19:21), Etsuro Fujita wrote:
   > According to the following documentation on IterateForeignScan() in
   > 50.2. Foreign Data Wrapper Callback Routines, I have created a patch to
   > support the error handling in file_fdw.  Please find attached a patch.
   > 
   >      Note that PostgreSQL's executor doesn't care whether the rows
   >      returned violate the NOT NULL constraints which were defined
   >      on the foreign table columns - but the planner does care, and
   >      may optimize queries incorrectly if NULL values are present
   >      in a column declared not to contain them. If a NULL value is
   >      encountered when the user has declared that none should be
   >      present, it may be appropriate to raise an error (just as you
   >      would need to do in the case of a data type mismatch).
   
   Interesting.  This patch could be applied cleanly, and it catches first
   record which violates NOT NULL constraint.  I have some comments for the
   patch.
   
   I worry performance degradation caused by checking NOT NULL constraints
   for every row, though such overhead might be hidden by disk I/O.  Do you
   have any result of performance testing?  Users might want to disable NOT
   NULL checking for already-validated files.
   
   In addition to performance issue, IMHO exporting
   ExecBuildSlotValueDescription needs more consideration.  Have you
   examined calling ExecConstraints instead of copying NOT NULL check
   codes?  It requires fully-built ResultRelInfo, and it also checks CHECK
   constraints which have not been supported on foreign tables, but it
   seems the standard way to apply constraints on a tuple.  If you don't
   want to check CHECK constraints, another possible idea is to add new
   external function ExecNotNull (or something) and move NOT NULL checking
   codes from ExecConstraints, and call it from fileIterateForeignScan and
   ExecConstraints.
   
   Anyway, please add this patch to Commit Fest App for tracking.
   https://commitfest.postgresql.org/action/commitfest_view?id=14
   
   -- 
   Shigeru Hanada
   
   

3. Re: NOT NULL violation error handling in file_fdw

   Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2012-03-13T09:13:08Z

   (2012/03/13 15:53), Shigeru HANADA wrote:
   > (2012/03/12 19:21), Etsuro Fujita wrote:
   >> According to the following documentation on IterateForeignScan() in
   >> 50.2. Foreign Data Wrapper Callback Routines, I have created a patch to
   >> support the error handling in file_fdw.  Please find attached a patch.
   
   > Interesting.  This patch could be applied cleanly, and it catches first
   > record which violates NOT NULL constraint.  I have some comments for the
   > patch.
   
   Thank you for the review.
   
   > I worry performance degradation caused by checking NOT NULL constraints
   > for every row, though such overhead might be hidden by disk I/O.  Do you
   > have any result of performance testing?  Users might want to disable NOT
   > NULL checking for already-validated files.
   
   I don't have any numbers for now.  OK I'll check it.
   
   > In addition to performance issue, IMHO exporting
   > ExecBuildSlotValueDescription needs more consideration.  Have you
   > examined calling ExecConstraints instead of copying NOT NULL check
   > codes?  It requires fully-built ResultRelInfo, and it also checks CHECK
   > constraints which have not been supported on foreign tables, but it
   > seems the standard way to apply constraints on a tuple.
   
   Yes, I thought the use of ExecConstraints().  But I feel that it is an
   overkill.
   
   > If you don't
   > want to check CHECK constraints, another possible idea is to add new
   > external function ExecNotNull (or something) and move NOT NULL checking
   > codes from ExecConstraints, and call it from fileIterateForeignScan and
   > ExecConstraints.
   
   I think that it is a good idea.  I'll do it at the next version of the
   patch.
   
   > Anyway, please add this patch to Commit Fest App for tracking.
   > https://commitfest.postgresql.org/action/commitfest_view?id=14
   
   Done.
   
   Best regards,
   Etsuro Fujita