Fix UPDATE to account BR trigger changes.

jeevanchalke · jeevanchalke · commit 1c82d1055fa8 · 2020-10-05T15:23:27.000+05:30
Earlier, only columns that are present in the UPDATE query got updated at the MySQL side. However, in any column value which is not present in the UPDATE query but modified in the BEFORE ROW trigger was not updated at MySQL side. Fix that by marking all columns as updatable in such cases. The fix for this was given by Francois Payette through pull request #194 on GitHub, which is further revised by Suraj Kharage. Also, updating a row-identifier column is not supported. However, this was broken when the BR update trigger is trying to update that. Fix that by comparing old and new value for the row-identifier column and throwing an error if they are not the same. Reported on GitHub through issues #193 by Francois Payette. FDW-193, Suraj Kharage, reviewed by Vaibhav Dalvi and me.
diff --git a/expected/dml.out b/expected/dml.out
@@ -14,7 +14,7 @@ CREATE USER MAPPING FOR public SERVER mysql_svr
 -- Create foreign tables
 CREATE FOREIGN TABLE f_mysql_test(a int, b int)
   SERVER mysql_svr OPTIONS (dbname 'mysql_fdw_regress', table_name 'mysql_test');
-CREATE FOREIGN TABLE fdw126_ft1(stu_id int, stu_name varchar(255))
+CREATE FOREIGN TABLE fdw126_ft1(stu_id int, stu_name varchar(255), stu_dept int)
   SERVER mysql_svr OPTIONS (dbname 'mysql_fdw_regress1', table_name 'student');
 CREATE FOREIGN TABLE fdw126_ft2(stu_id int, stu_name varchar(255))
   SERVER mysql_svr OPTIONS (table_name 'student');
@@ -28,6 +28,8 @@ CREATE FOREIGN TABLE fdw126_ft6(stu_id int, stu_name varchar(255))
   SERVER mysql_svr OPTIONS (table_name 'mysql_fdw_regress1.student');
 CREATE FOREIGN TABLE f_empdata(emp_id int, emp_dat bytea)
   SERVER mysql_svr OPTIONS (dbname 'mysql_fdw_regress', table_name 'empdata');
+CREATE FOREIGN TABLE fdw193_ft1(stu_id varchar(10), stu_name varchar(255), stu_dept int)
+  SERVER mysql_svr OPTIONS (dbname 'mysql_fdw_regress1', table_name 'student1');
 -- Operation on blob data.
 INSERT INTO f_empdata VALUES (1, decode ('01234567', 'hex'));
 SELECT count(*) FROM f_empdata ORDER BY 1;
@@ -56,7 +58,7 @@ SELECT emp_id, emp_dat FROM f_empdata ORDER BY 1;
 -- the operation on foreign table created for tables in mysql_fdw_regress
 -- MySQL database.  Below operations will be performed for foreign table
 -- created for table in mysql_fdw_regress1 MySQL database.
-INSERT INTO fdw126_ft1 VALUES(1, 'One');
+INSERT INTO fdw126_ft1 VALUES(1, 'One', 101);
 UPDATE fdw126_ft1 SET stu_name = 'one' WHERE stu_id = 1;
 DELETE FROM fdw126_ft1 WHERE stu_id = 1;
 -- Select on f_mysql_test foreign table which is created for mysql_test table
@@ -116,9 +118,105 @@ ANALYZE f_empdata(emp_id);
 WARNING:  skipping "f_empdata" --- cannot analyze this foreign table
 VACUUM ANALYZE f_empdata;
 WARNING:  skipping "f_empdata" --- cannot vacuum non-tables or special system tables
+-- Verify the before update trigger which modifies the column value which is not
+-- part of update statement.
+CREATE FUNCTION before_row_update_func() RETURNS TRIGGER AS $$
+BEGIN
+  NEW.stu_name := NEW.stu_name || ' trigger updated!';
+	RETURN NEW;
+  END
+$$ language plpgsql;
+CREATE TRIGGER before_row_update_trig
+BEFORE UPDATE ON fdw126_ft1
+FOR EACH ROW EXECUTE PROCEDURE before_row_update_func();
+INSERT INTO fdw126_ft1 VALUES(1, 'One', 101);
+EXPLAIN (verbose, costs off)
+UPDATE fdw126_ft1 SET stu_dept = 201 WHERE stu_id = 1;
+                                                             QUERY PLAN                                                              
+-------------------------------------------------------------------------------------------------------------------------------------
+ Update on public.fdw126_ft1
+   ->  Foreign Scan on public.fdw126_ft1
+         Output: stu_id, stu_name, 201, stu_id, fdw126_ft1.*
+         Local server startup cost: 10
+         Remote query: SELECT `stu_id`, `stu_name`, `stu_dept` FROM `mysql_fdw_regress1`.`student` WHERE ((`stu_id` = 1)) FOR UPDATE
+(5 rows)
+
+UPDATE fdw126_ft1 SET stu_dept = 201 WHERE stu_id = 1;
+SELECT * FROM fdw126_ft1 ORDER BY stu_id;
+ stu_id |       stu_name       | stu_dept 
+--------+----------------------+----------
+      1 | One trigger updated! |      201
+(1 row)
+
+-- Throw an error when target list has row identifier column.
+UPDATE fdw126_ft1 SET stu_dept = 201, stu_id = 10  WHERE stu_id = 1;
+ERROR:  row identifier column update is not supported
+-- Throw an error when before row update trigger modify the row identifier
+-- column (int column) value.
+CREATE OR REPLACE FUNCTION before_row_update_func() RETURNS TRIGGER AS $$
+BEGIN
+  NEW.stu_name := NEW.stu_name || ' trigger updated!';
+  NEW.stu_id = 20;
+  RETURN NEW;
+  END
+$$ language plpgsql;
+UPDATE fdw126_ft1 SET stu_dept = 301 WHERE stu_id = 1;
+ERROR:  row identifier column update is not supported
+-- Verify the before update trigger which modifies the column value which is
+-- not part of update statement.
+CREATE OR REPLACE FUNCTION before_row_update_func() RETURNS TRIGGER AS $$
+BEGIN
+  NEW.stu_name := NEW.stu_name || ' trigger updated!';
+  RETURN NEW;
+  END
+$$ language plpgsql;
+CREATE TRIGGER before_row_update_trig1
+BEFORE UPDATE ON fdw193_ft1
+FOR EACH ROW EXECUTE PROCEDURE before_row_update_func();
+INSERT INTO fdw193_ft1 VALUES('aa', 'One', 101);
+EXPLAIN (verbose, costs off)
+UPDATE fdw193_ft1 SET stu_dept = 201 WHERE stu_id = 'aa';
+                                                               QUERY PLAN                                                                
+-----------------------------------------------------------------------------------------------------------------------------------------
+ Update on public.fdw193_ft1
+   ->  Foreign Scan on public.fdw193_ft1
+         Output: stu_id, stu_name, 201, stu_id, fdw193_ft1.*
+         Local server startup cost: 10
+         Remote query: SELECT `stu_id`, `stu_name`, `stu_dept` FROM `mysql_fdw_regress1`.`student1` WHERE ((`stu_id` = 'aa')) FOR UPDATE
+(5 rows)
+
+UPDATE fdw193_ft1 SET stu_dept = 201 WHERE stu_id = 'aa';
+SELECT * FROM fdw193_ft1 ORDER BY stu_id;
+ stu_id |       stu_name       | stu_dept 
+--------+----------------------+----------
+ aa     | One trigger updated! |      201
+(1 row)
+
+-- Throw an error when before row update trigger modify the row identifier
+-- column (varchar column) value.
+CREATE OR REPLACE FUNCTION before_row_update_func() RETURNS TRIGGER AS $$
+BEGIN
+  NEW.stu_name := NEW.stu_name || ' trigger updated!';
+  NEW.stu_id = 'bb';
+  RETURN NEW;
+  END
+$$ language plpgsql;
+UPDATE fdw193_ft1 SET stu_dept = 301 WHERE stu_id = 'aa';
+ERROR:  row identifier column update is not supported
+-- Verify the NULL assignment scenario.
+CREATE OR REPLACE FUNCTION before_row_update_func() RETURNS TRIGGER AS $$
+BEGIN
+  NEW.stu_name := NEW.stu_name || ' trigger updated!';
+  NEW.stu_id = NULL;
+  RETURN NEW;
+  END
+$$ language plpgsql;
+UPDATE fdw193_ft1 SET stu_dept = 401 WHERE stu_id = 'aa';
+ERROR:  row identifier column update is not supported
 -- Cleanup
 DELETE FROM fdw126_ft1;
 DELETE FROM f_empdata;
+DELETE FROM fdw193_ft1;
 DROP FOREIGN TABLE f_mysql_test;
 DROP FOREIGN TABLE fdw126_ft1;
 DROP FOREIGN TABLE fdw126_ft2;
@@ -127,6 +225,8 @@ DROP FOREIGN TABLE fdw126_ft4;
 DROP FOREIGN TABLE fdw126_ft5;
 DROP FOREIGN TABLE fdw126_ft6;
 DROP FOREIGN TABLE f_empdata;
+DROP FOREIGN TABLE fdw193_ft1;
+DROP FUNCTION before_row_update_func();
 DROP USER MAPPING FOR public SERVER mysql_svr;
 DROP SERVER mysql_svr;
 DROP EXTENSION mysql_fdw;
diff --git a/mysql_fdw.c b/mysql_fdw.c
@@ -26,6 +26,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "access/htup_details.h"
 #include "access/sysattr.h"
 #include "access/reloptions.h"
 #if PG_VERSION_NUM >= 120000
@@ -48,9 +49,11 @@
 #include "parser/parsetree.h"
 #include "storage/ipc.h"
 #include "utils/builtins.h"
+#include "utils/datum.h"
 #include "utils/guc.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
+#include "utils/syscache.h"
 
 /* Declarations for dynamic loading */
 PG_MODULE_MAGIC;
@@ -215,6 +218,7 @@ static int interactive_timeout = INTERACTIVE_TIMEOUT;
 static void mysql_error_print(MYSQL *conn);
 static void mysql_stmt_error_print(MySQLFdwExecState *festate,
 								   const char *msg);
+static List *getUpdateTargetAttrs(RangeTblEntry *rte);
 
 /*
  * mysql_load_library function dynamically load the mysql's library
@@ -1186,11 +1190,32 @@ mysqlPlanForeignModify(PlannerInfo *root,
 				(errcode(ERRCODE_FDW_UNABLE_TO_CREATE_EXECUTION),
 				 errmsg("first column of remote table must be unique for INSERT/UPDATE/DELETE operation")));
 
-	if (operation == CMD_INSERT)
+	/*
+	 * In an INSERT, we transmit all columns that are defined in the foreign
+	 * table.  In an UPDATE, if there are BEFORE ROW UPDATE triggers on the
+	 * foreign table, we transmit all columns like INSERT; else we transmit
+	 * only columns that were explicitly targets of the UPDATE, so as to avoid
+	 * unnecessary data transmission.  (We can't do that for INSERT since we
+	 * would miss sending default values for columns not listed in the source
+	 * statement, and for UPDATE if there are BEFORE ROW UPDATE triggers since
+	 * those triggers might change values for non-target columns, in which
+	 * case we would miss sending changed values for those columns.)
+	 */
+	if (operation == CMD_INSERT ||
+		(operation == CMD_UPDATE &&
+		 rel->trigdesc &&
+		 rel->trigdesc->trig_update_before_row))
 	{
 		TupleDesc	tupdesc = RelationGetDescr(rel);
 		int			attnum;
 
+		/*
+		 * If it is an UPDATE operation, check for row identifier column in
+		 * target attribute list by calling getUpdateTargetAttrs().
+		 */
+		if (operation == CMD_UPDATE)
+			getUpdateTargetAttrs(rte);
+
 		for (attnum = 1; attnum <= tupdesc->natts; attnum++)
 		{
 			Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum - 1);
@@ -1201,27 +1226,7 @@ mysqlPlanForeignModify(PlannerInfo *root,
 	}
 	else if (operation == CMD_UPDATE)
 	{
-#if PG_VERSION_NUM >= 90500
-		Bitmapset  *tmpset = bms_copy(rte->updatedCols);
-#else
-		Bitmapset  *tmpset = bms_copy(rte->modifiedCols);
-#endif
-		AttrNumber	col;
-
-		while ((col = bms_first_member(tmpset)) >= 0)
-		{
-			col += FirstLowInvalidHeapAttributeNumber;
-			if (col <= InvalidAttrNumber)	/* shouldn't happen */
-				elog(ERROR, "system-column update is not supported");
-
-			/*
-			 * We also disallow updates to the first column
-			 */
-			if (col == 1)		/* shouldn't happen */
-				elog(ERROR, "row identifier column update is not supported");
-
-			targetAttrs = lappend_int(targetAttrs, col);
-		}
+		targetAttrs = getUpdateTargetAttrs(rte);
 		/* We also want the rowid column to be available for the update */
 		targetAttrs = lcons_int(1, targetAttrs);
 	}
@@ -1437,6 +1442,10 @@ mysqlExecForeignUpdate(EState *estate,
 	Datum		value;
 	int			n_params;
 	bool	   *isnull;
+	Datum		new_value;
+	HeapTuple	tuple;
+	Form_pg_attribute attr;
+	bool		found_row_id_col = false;
 
 	n_params = list_length(fmstate->retrieved_attrs);
 
@@ -1449,9 +1458,16 @@ mysqlExecForeignUpdate(EState *estate,
 		int			attnum = lfirst_int(lc);
 		Oid			type;
 
-		/* first attribute cannot be in target list attribute */
+		/*
+		 * The first attribute cannot be in the target list attribute.  Set the
+		 * found_row_id_col to true once we find it so that we can fetch the
+		 * value later.
+		 */
 		if (attnum == 1)
+		{
+			found_row_id_col = true;
 			continue;
+		}
 
 		type = TupleDescAttr(slot->tts_tupleDescriptor, attnum - 1)->atttypid;
 		value = slot_getattr(slot, attnum, (bool *) (&isnull[bindnum]));
@@ -1461,9 +1477,62 @@ mysqlExecForeignUpdate(EState *estate,
 		bindnum++;
 	}
 
-	/* Get the id that was passed up as a resjunk column */
+	/*
+	 * Since we add a row identifier column in the target list always, so
+	 * found_row_id_col flag should be true.
+	 */
+	if (!found_row_id_col)
+		elog(ERROR, "missing row identifier column value in UPDATE");
+
+	new_value = slot_getattr(slot, 1, &is_null);
+
+	/*
+	 * Get the row identifier column value that was passed up as a resjunk
+	 * column and compare that value with the new value to identify if that
+	 * value is changed.
+	 */
 	value = ExecGetJunkAttribute(planSlot, 1, &is_null);
-	typeoid = get_atttype(foreignTableId, 1);
+
+	tuple = SearchSysCache2(ATTNUM,
+							ObjectIdGetDatum(foreignTableId),
+							Int16GetDatum(1));
+	if (!HeapTupleIsValid(tuple))
+		elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+			 1, foreignTableId);
+
+	attr = (Form_pg_attribute) GETSTRUCT(tuple);
+	typeoid = attr->atttypid;
+
+	if (DatumGetPointer(new_value) != NULL && DatumGetPointer(value) != NULL)
+	{
+		Datum		n_value = new_value;
+		Datum 		o_value = value;
+
+		/* If the attribute type is varlena then need to detoast the datums. */
+		if (attr->attlen == -1)
+		{
+			n_value = PointerGetDatum(PG_DETOAST_DATUM(new_value));
+			o_value = PointerGetDatum(PG_DETOAST_DATUM(value));
+		}
+
+		if (!datumIsEqual(o_value, n_value, attr->attbyval, attr->attlen))
+			ereport(ERROR,
+					(errcode(ERRCODE_FDW_UNABLE_TO_CREATE_EXECUTION),
+					 errmsg("row identifier column update is not supported")));
+
+		/* Free memory if it's a copy made above */
+		if (DatumGetPointer(n_value) != DatumGetPointer(new_value))
+			pfree(DatumGetPointer(n_value));
+		if (DatumGetPointer(o_value) != DatumGetPointer(value))
+			pfree(DatumGetPointer(o_value));
+	}
+	else if (!(DatumGetPointer(new_value) == NULL &&
+			   DatumGetPointer(value) == NULL))
+		ereport(ERROR,
+				(errcode(ERRCODE_FDW_UNABLE_TO_CREATE_EXECUTION),
+				 errmsg("row identifier column update is not supported")));
+
+	ReleaseSysCache(tuple);
 
 	/* Bind qual */
 	mysql_bind_sql_var(typeoid, bindnum, value, mysql_bind_buffer, &is_null);
@@ -2009,3 +2078,37 @@ mysql_stmt_error_print(MySQLFdwExecState *festate, const char *msg)
 			break;
 	}
 }
+
+/*
+ * getUpdateTargetAttrs
+ * 		Returns the list of attribute numbers of the columns being updated.
+ */
+static List *
+getUpdateTargetAttrs(RangeTblEntry *rte)
+{
+	List	   *targetAttrs = NIL;
+
+#if PG_VERSION_NUM >= 90500
+	Bitmapset  *tmpset = bms_copy(rte->updatedCols);
+#else
+	Bitmapset  *tmpset = bms_copy(rte->modifiedCols);
+#endif
+	AttrNumber	col;
+
+	while ((col = bms_first_member(tmpset)) >= 0)
+	{
+		col += FirstLowInvalidHeapAttributeNumber;
+		if (col <= InvalidAttrNumber)	/* shouldn't happen */
+			elog(ERROR, "system-column update is not supported");
+
+		/* We also disallow updates to the first column */
+		if (col == 1)
+			ereport(ERROR,
+				(errcode(ERRCODE_FDW_UNABLE_TO_CREATE_EXECUTION),
+				 errmsg("row identifier column update is not supported")));
+
+		targetAttrs = lappend_int(targetAttrs, col);
+	}
+
+	return targetAttrs;
+}
diff --git a/mysql_init.sh b/mysql_init.sh
@@ -29,14 +29,16 @@ mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress -e
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress1 -e "DROP TABLE IF EXISTS student;"
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress1 -e "DROP TABLE IF EXISTS numbers;"
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress -e "DROP TABLE IF EXISTS enum_t1;"
+mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress1 -e "DROP TABLE IF EXISTS student1;"
 
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress -e "CREATE TABLE mysql_test(a int primary key, b int);"
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress -e "INSERT INTO mysql_test(a,b) VALUES (1,1);"
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress -e "CREATE TABLE empdata (emp_id int, emp_dat blob, PRIMARY KEY (emp_id));"
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress -e "CREATE TABLE numbers (a int PRIMARY KEY, b varchar(255));"
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress -e "CREATE TABLE test_tbl1 (c1 INT primary key, c2 VARCHAR(10), c3 CHAR(9), c4 MEDIUMINT, c5 DATE, c6 DECIMAL(10,5), c7 INT, c8 SMALLINT);"
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress -e "CREATE TABLE test_tbl2 (c1 INT primary key, c2 TEXT, c3 TEXT);"
-mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress1 -e "CREATE TABLE student (stu_id int PRIMARY KEY, stu_name text);"
+mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress1 -e "CREATE TABLE student (stu_id int PRIMARY KEY, stu_name text, stu_dept int);"
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress1 -e "CREATE TABLE numbers (a int, b varchar(255));"
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress -e "CREATE TABLE enum_t1 (id int PRIMARY KEY, size ENUM('small', 'medium', 'large'));"
 mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress -e "INSERT INTO enum_t1 VALUES (1, 'small'),(2, 'medium'),(3, 'medium');"
+mysql -h $MYSQL_HOST -u $MYSQL_USER_NAME -P $MYSQL_PORT -D mysql_fdw_regress1 -e "CREATE TABLE student1 (stu_id varchar(10) PRIMARY KEY, stu_name text, stu_dept int);"
diff --git a/sql/dml.sql b/sql/dml.sql
