Fix contrib/postgres_fdw's handling of column defaults.

Adopt the position that only locally-defined defaults matter.  Any defaults
defined in the remote database do not affect insertions performed through
a foreign table (unless they are for columns not known to the foreign
table).  While it'd arguably be more useful to permit remote defaults to be
used, making that work in a consistent fashion requires far more work than
seems possible for 9.3.

contrib/postgres_fdw/deparse.c

@@ -77,7 +77,7 @@ static void deparseReturningList(StringInfo buf, PlannerInfo *root,
 					 List *returningList);
 static void deparseColumnRef(StringInfo buf, int varno, int varattno,
 				 PlannerInfo *root);
-static void deparseRelation(StringInfo buf, Oid relid);
+static void deparseRelation(StringInfo buf, Relation rel);
 static void deparseStringLiteral(StringInfo buf, const char *val);
 static void deparseExpr(StringInfo buf, Expr *expr, PlannerInfo *root);
 static void deparseVar(StringInfo buf, Var *node, PlannerInfo *root);
@@ -387,7 +387,7 @@ deparseSelectSql(StringInfo buf,
 	 * Construct FROM clause
 	 */
 	appendStringInfoString(buf, " FROM ");
-	deparseRelation(buf, RelationGetRelid(rel));
+	deparseRelation(buf, rel);
 
 	heap_close(rel, NoLock);
 }
@@ -499,18 +499,16 @@ appendWhereClause(StringInfo buf,
  * deparse remote INSERT statement
  */
 void
-deparseInsertSql(StringInfo buf, PlannerInfo *root, Index rtindex,
+deparseInsertSql(StringInfo buf, PlannerInfo *root,
+				 Index rtindex, Relation rel,
 				 List *targetAttrs, List *returningList)
 {
-	RangeTblEntry *rte = planner_rt_fetch(rtindex, root);
-	Relation	rel = heap_open(rte->relid, NoLock);
-	TupleDesc	tupdesc = RelationGetDescr(rel);
 	AttrNumber	pindex;
 	bool		first;
 	ListCell   *lc;
 
 	appendStringInfoString(buf, "INSERT INTO ");
-	deparseRelation(buf, rte->relid);
+	deparseRelation(buf, rel);
 
 	if (targetAttrs)
 	{
@@ -520,9 +518,6 @@ deparseInsertSql(StringInfo buf, PlannerInfo *root, Index rtindex,
 		foreach(lc, targetAttrs)
 		{
 			int			attnum = lfirst_int(lc);
-			Form_pg_attribute attr = tupdesc->attrs[attnum - 1];
-
-			Assert(!attr->attisdropped);
 
 			if (!first)
 				appendStringInfoString(buf, ", ");
@@ -552,26 +547,22 @@ deparseInsertSql(StringInfo buf, PlannerInfo *root, Index rtindex,
 
 	if (returningList)
 		deparseReturningList(buf, root, rtindex, rel, returningList);
-
-	heap_close(rel, NoLock);
 }
 
 /*
  * deparse remote UPDATE statement
  */
 void
-deparseUpdateSql(StringInfo buf, PlannerInfo *root, Index rtindex,
+deparseUpdateSql(StringInfo buf, PlannerInfo *root,
+				 Index rtindex, Relation rel,
 				 List *targetAttrs, List *returningList)
 {
-	RangeTblEntry *rte = planner_rt_fetch(rtindex, root);
-	Relation	rel = heap_open(rte->relid, NoLock);
-	TupleDesc	tupdesc = RelationGetDescr(rel);
 	AttrNumber	pindex;
 	bool		first;
 	ListCell   *lc;
 
 	appendStringInfoString(buf, "UPDATE ");
-	deparseRelation(buf, rte->relid);
+	deparseRelation(buf, rel);
 	appendStringInfoString(buf, " SET ");
 
 	pindex = 2;					/* ctid is always the first param */
@@ -579,9 +570,6 @@ deparseUpdateSql(StringInfo buf, PlannerInfo *root, Index rtindex,
 	foreach(lc, targetAttrs)
 	{
 		int			attnum = lfirst_int(lc);
-		Form_pg_attribute attr = tupdesc->attrs[attnum - 1];
-
-		Assert(!attr->attisdropped);
 
 		if (!first)
 			appendStringInfoString(buf, ", ");
@@ -595,30 +583,22 @@ deparseUpdateSql(StringInfo buf, PlannerInfo *root, Index rtindex,
 
 	if (returningList)
 		deparseReturningList(buf, root, rtindex, rel, returningList);
-
-	heap_close(rel, NoLock);
 }
 
 /*
  * deparse remote DELETE statement
  */
 void
-deparseDeleteSql(StringInfo buf, PlannerInfo *root, Index rtindex,
+deparseDeleteSql(StringInfo buf, PlannerInfo *root,
+				 Index rtindex, Relation rel,
 				 List *returningList)
 {
-	RangeTblEntry *rte = planner_rt_fetch(rtindex, root);
-
 	appendStringInfoString(buf, "DELETE FROM ");
-	deparseRelation(buf, rte->relid);
+	deparseRelation(buf, rel);
 	appendStringInfoString(buf, " WHERE ctid = $1");
 
 	if (returningList)
-	{
-		Relation	rel = heap_open(rte->relid, NoLock);
-
 		deparseReturningList(buf, root, rtindex, rel, returningList);
-		heap_close(rel, NoLock);
-	}
 }
 
 /*
@@ -653,12 +633,11 @@ deparseReturningList(StringInfo buf, PlannerInfo *root,
 void
 deparseAnalyzeSizeSql(StringInfo buf, Relation rel)
 {
-	Oid			relid = RelationGetRelid(rel);
 	StringInfoData relname;
 
 	/* We'll need the remote relation name as a literal. */
 	initStringInfo(&relname);
-	deparseRelation(&relname, relid);
+	deparseRelation(&relname, rel);
 
 	appendStringInfo(buf, "SELECT pg_catalog.pg_relation_size(");
 	deparseStringLiteral(buf, relname.data);
@@ -718,7 +697,7 @@ deparseAnalyzeSql(StringInfo buf, Relation rel)
 	 * Construct FROM clause
 	 */
 	appendStringInfoString(buf, " FROM ");
-	deparseRelation(buf, relid);
+	deparseRelation(buf, rel);
 }
 
 /*
@@ -771,7 +750,7 @@ deparseColumnRef(StringInfo buf, int varno, int varattno, PlannerInfo *root)
  * Similarly, schema_name FDW option overrides schema name.
  */
 static void
-deparseRelation(StringInfo buf, Oid relid)
+deparseRelation(StringInfo buf, Relation rel)
 {
 	ForeignTable *table;
 	const char *nspname = NULL;
@@ -779,7 +758,7 @@ deparseRelation(StringInfo buf, Oid relid)
 	ListCell   *lc;
 
 	/* obtain additional catalog information. */
-	table = GetForeignTable(relid);
+	table = GetForeignTable(RelationGetRelid(rel));
 
 	/*
 	 * Use value of FDW options if any, instead of the name of object itself.
@@ -799,9 +778,9 @@ deparseRelation(StringInfo buf, Oid relid)
 	 * that doesn't seem worth the trouble.
 	 */
 	if (nspname == NULL)
-		nspname = get_namespace_name(get_rel_namespace(relid));
+		nspname = get_namespace_name(RelationGetNamespace(rel));
 	if (relname == NULL)
-		relname = get_rel_name(relid);
+		relname = RelationGetRelationName(rel);
 
 	appendStringInfo(buf, "%s.%s",
 					 quote_identifier(nspname), quote_identifier(relname));

contrib/postgres_fdw/postgres_fdw.c

@@ -1023,6 +1023,8 @@ postgresPlanForeignModify(PlannerInfo *root,
 						  int subplan_index)
 {
 	CmdType		operation = plan->operation;
+	RangeTblEntry *rte = planner_rt_fetch(resultRelation, root);
+	Relation	rel;
 	StringInfoData sql;
 	List	   *targetAttrs = NIL;
 	List	   *returningList = NIL;
@@ -1030,15 +1032,33 @@ postgresPlanForeignModify(PlannerInfo *root,
 	initStringInfo(&sql);
 
 	/*
-	 * Construct a list of the columns that are to be assigned during INSERT
-	 * or UPDATE.  We should transmit only these columns, for performance and
-	 * to respect any DEFAULT values the remote side may have for other
-	 * columns.  (XXX this will need some re-thinking when we support default
-	 * expressions for foreign tables.)
+	 * Core code already has some lock on each rel being planned, so we can
+	 * use NoLock here.
 	 */
-	if (operation == CMD_INSERT || operation == CMD_UPDATE)
+	rel = heap_open(rte->relid, NoLock);
+
+	/*
+	 * In an INSERT, we transmit all columns that are defined in the foreign
+	 * table.  In an UPDATE, 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.)
+	 */
+	if (operation == CMD_INSERT)
+	{
+		TupleDesc	tupdesc = RelationGetDescr(rel);
+		int			attnum;
+
+		for (attnum = 1; attnum <= tupdesc->natts; attnum++)
+		{
+			Form_pg_attribute attr = tupdesc->attrs[attnum - 1];
+
+			if (!attr->attisdropped)
+				targetAttrs = lappend_int(targetAttrs, attnum);
+		}
+	}
+	else if (operation == CMD_UPDATE)
 	{
-		RangeTblEntry *rte = planner_rt_fetch(resultRelation, root);
 		Bitmapset  *tmpset = bms_copy(rte->modifiedCols);
 		AttrNumber	col;
 
@@ -1063,21 +1083,24 @@ postgresPlanForeignModify(PlannerInfo *root,
 	switch (operation)
 	{
 		case CMD_INSERT:
-			deparseInsertSql(&sql, root, resultRelation,
+			deparseInsertSql(&sql, root, resultRelation, rel,
 							 targetAttrs, returningList);
 			break;
 		case CMD_UPDATE:
-			deparseUpdateSql(&sql, root, resultRelation,
+			deparseUpdateSql(&sql, root, resultRelation, rel,
 							 targetAttrs, returningList);
 			break;
 		case CMD_DELETE:
-			deparseDeleteSql(&sql, root, resultRelation, returningList);
+			deparseDeleteSql(&sql, root, resultRelation, rel,
+							 returningList);
 			break;
 		default:
 			elog(ERROR, "unexpected operation: %d", (int) operation);
 			break;
 	}
 
+	heap_close(rel, NoLock);
+
 	/*
 	 * Build the fdw_private list that will be available to the executor.
 	 * Items in the list must match enum FdwModifyPrivateIndex, above.

contrib/postgres_fdw/postgres_fdw.h

@@ -53,11 +53,14 @@ extern void appendWhereClause(StringInfo buf,
 				  PlannerInfo *root,
 				  List *exprs,
 				  bool is_first);
-extern void deparseInsertSql(StringInfo buf, PlannerInfo *root, Index rtindex,
+extern void deparseInsertSql(StringInfo buf, PlannerInfo *root,
+				 Index rtindex, Relation rel,
 				 List *targetAttrs, List *returningList);
-extern void deparseUpdateSql(StringInfo buf, PlannerInfo *root, Index rtindex,
+extern void deparseUpdateSql(StringInfo buf, PlannerInfo *root,
+				 Index rtindex, Relation rel,
 				 List *targetAttrs, List *returningList);
-extern void deparseDeleteSql(StringInfo buf, PlannerInfo *root, Index rtindex,
+extern void deparseDeleteSql(StringInfo buf, PlannerInfo *root,
+				 Index rtindex, Relation rel,
 				 List *returningList);
 extern void deparseAnalyzeSizeSql(StringInfo buf, Relation rel);
 extern void deparseAnalyzeSql(StringInfo buf, Relation rel);

contrib/postgres_fdw/sql/postgres_fdw.sql

@@ -63,23 +63,23 @@ CREATE FOREIGN TABLE ft1 (
 	c4 timestamptz,
 	c5 timestamp,
 	c6 varchar(10),
-	c7 char(10),
+	c7 char(10) default 'ft1',
 	c8 user_enum
 ) SERVER loopback;
 ALTER FOREIGN TABLE ft1 DROP COLUMN c0;
 
 CREATE FOREIGN TABLE ft2 (
-	c0 int,
 	c1 int NOT NULL,
 	c2 int NOT NULL,
+	cx int,
 	c3 text,
 	c4 timestamptz,
 	c5 timestamp,
 	c6 varchar(10),
-	c7 char(10),
+	c7 char(10) default 'ft2',
 	c8 user_enum
 ) SERVER loopback;
-ALTER FOREIGN TABLE ft2 DROP COLUMN c0;
+ALTER FOREIGN TABLE ft2 DROP COLUMN cx;
 
 -- ===================================================================
 -- tests for validator
@@ -286,9 +286,9 @@ INSERT INTO ft2 (c1,c2,c3) VALUES (1104,204,'ddd'), (1105,205,'eee');
 UPDATE ft2 SET c2 = c2 + 300, c3 = c3 || '_update3' WHERE c1 % 10 = 3;
 UPDATE ft2 SET c2 = c2 + 400, c3 = c3 || '_update7' WHERE c1 % 10 = 7 RETURNING *;
 EXPLAIN (verbose, costs off)
-UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9'
+UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
   FROM ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 9;
-UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9'
+UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
   FROM ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 9;
 DELETE FROM ft2 WHERE c1 % 10 = 5 RETURNING *;
 EXPLAIN (verbose, costs off)
@@ -296,8 +296,7 @@ DELETE FROM ft2 USING ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 2;
 DELETE FROM ft2 USING ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 2;
 SELECT c1,c2,c3,c4 FROM ft2 ORDER BY c1;
 
--- Test that defaults and triggers on remote table work as expected
-ALTER TABLE "S 1"."T 1" ALTER c6 SET DEFAULT '(^-^;)';
+-- Test that trigger on remote table works as expected
 CREATE OR REPLACE FUNCTION "S 1".F_BRTRIG() RETURNS trigger AS $$
 BEGIN
     NEW.c3 = NEW.c3 || '_trig_update';