NOT NULL violation error handling in file_fdw

Started by Etsuro Fujitaalmost 14 years ago3 messages

fujita.etsuro@lab.ntt.co.jp

almost 14 years ago

1 attachment(s)

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

Attachments:

file_fdw_notnull_v1.patchtext/plain; name=file_fdw_notnull_v1.patchDownload

*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***************
*** 502,507 **** fileIterateForeignScan(ForeignScanState *node)
--- 502,510 ----
  {
  	FileFdwExecutionState *festate = (FileFdwExecutionState *) node->fdw_state;
  	TupleTableSlot *slot = node->ss.ss_ScanTupleSlot;
+ 	Relation	rel = node->ss.ss_currentRelation;
+ 	TupleConstr *constr = rel->rd_att->constr;
+ 	bool		has_not_null = (constr != NULL) ? constr->has_not_null : false;
  	bool		found;
  	ErrorContextCallback errcontext;
  
***************
*** 528,534 **** fileIterateForeignScan(ForeignScanState *node)
--- 531,556 ----
  						 slot->tts_values, slot->tts_isnull,
  						 NULL);
  	if (found)
+ 	{
  		ExecStoreVirtualTuple(slot);
+ 		if (has_not_null)
+ 		{
+ 			int			natts = rel->rd_att->natts;
+ 			int			attrChk;
+ 
+ 			for (attrChk = 1; attrChk <= natts; attrChk++)
+ 			{
+ 				if (rel->rd_att->attrs[attrChk - 1]->attnotnull &&
+ 					slot_attisnull(slot, attrChk))
+ 					ereport(ERROR,
+ 							(errcode(ERRCODE_NOT_NULL_VIOLATION),
+ 							 errmsg("null value in column \"%s\" violates not-null constraint",
+ 									NameStr(rel->rd_att->attrs[attrChk - 1]->attname)),
+ 							 errdetail("Failing row contains %s.",
+ 									   ExecBuildSlotValueDescription(slot, 64))));
+ 			}
+ 		}
+ 	}
  
  	/* Remove error callback. */
  	error_context_stack = errcontext.previous;
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***************
*** 86,93 **** static void ExecutePlan(EState *estate, PlanState *planstate,
  			DestReceiver *dest);
  static bool ExecCheckRTEPerms(RangeTblEntry *rte);
  static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt);
- static char *ExecBuildSlotValueDescription(TupleTableSlot *slot,
- 										   int maxfieldlen);
  static void EvalPlanQualStart(EPQState *epqstate, EState *parentestate,
  				  Plan *planTree);
  static void OpenIntoRel(QueryDesc *queryDesc);
--- 86,91 ----
***************
*** 1604,1610 **** ExecConstraints(ResultRelInfo *resultRelInfo,
   * here since heap field values could be very long, whereas index entries
   * typically aren't so wide.
   */
! static char *
  ExecBuildSlotValueDescription(TupleTableSlot *slot, int maxfieldlen)
  {
  	StringInfoData buf;
--- 1602,1608 ----
   * here since heap field values could be very long, whereas index entries
   * typically aren't so wide.
   */
! char *
  ExecBuildSlotValueDescription(TupleTableSlot *slot, int maxfieldlen)
  {
  	StringInfoData buf;
*** a/src/include/executor/executor.h
--- b/src/include/executor/executor.h
***************
*** 184,189 **** extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
--- 184,191 ----
  extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
  extern void ExecConstraints(ResultRelInfo *resultRelInfo,
  				TupleTableSlot *slot, EState *estate);
+ extern char *ExecBuildSlotValueDescription(TupleTableSlot *slot,
+ 										   int maxfieldlen);
  extern ExecRowMark *ExecFindRowMark(EState *estate, Index rti);
  extern ExecAuxRowMark *ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist);
  extern TupleTableSlot *EvalPlanQual(EState *estate, EPQState *epqstate,

Shigeru HANADA

shigeru.hanada@gmail.com

almost 14 years ago

In reply to: Etsuro Fujita (#1)

Re: NOT NULL violation error handling in file_fdw

(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

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

almost 14 years ago

In reply to: Shigeru HANADA (#2)

Re: NOT NULL violation error handling in file_fdw

(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