Support for remote DELETE planning and remove restrictions for UPDATE

This adds general support for DELETE query planning in standard planner
in case Postgres-XC planner falls back to standard planner. Now all the
DML plans follow the same logic by adding remoteQuery nodes to plan steps
of ModifyTable plans.
UPDATE has its restrictions removed by adding the CTID value for tuple
selection on remote nodes as a parameter in WHERE clause and in parameter
list associated with extended protocol of remote query.

2 files changed, 130 insertions, 223 deletions

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 66ebf02849..9889f3470a 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -313,12 +313,18 @@ ExecDelete(ItemPointer tupleid,
 	HTSU_Result result;
 	ItemPointerData update_ctid;
 	TransactionId update_xmax;
+#ifdef PGXC
+	PlanState  *resultRemoteRel = NULL;
+#endif
 
 	/*
 	 * get information on the (current) result relation
 	 */
 	resultRelInfo = estate->es_result_relation_info;
 	resultRelationDesc = resultRelInfo->ri_RelationDesc;
+#ifdef PGXC
+	resultRemoteRel = estate->es_result_remoterel;
+#endif
 
 	/* BEFORE ROW DELETE Triggers */
 	if (resultRelInfo->ri_TrigDesc &&
@@ -363,6 +369,14 @@ ExecDelete(ItemPointer tupleid,
 		 * mode transactions.
 		 */
 ldelete:;
+#ifdef PGXC
+		if (IS_PGXC_COORDINATOR && resultRemoteRel)
+		{
+			ExecRemoteQueryStandard(resultRelationDesc, (RemoteQueryState *)resultRemoteRel, planSlot);
+		}
+		else
+		{
+#endif
 		result = heap_delete(resultRelationDesc, tupleid,
 							 &update_ctid, &update_xmax,
 							 estate->es_output_cid,
@@ -414,6 +428,10 @@ ldelete:;
 		 * take care of it later.  We can't delete index tuples immediately
 		 * anyway, since the tuple is still visible to other transactions.
 		 */
+
+#ifdef PGXC
+		}
+#endif
 	}
 
 	if (canSetTag)
@@ -579,7 +597,7 @@ lreplace:;
 #ifdef PGXC
 		if (IS_PGXC_COORDINATOR && resultRemoteRel)
 		{
-			ExecRemoteQueryStandard(resultRelationDesc, (RemoteQueryState *)resultRemoteRel, slot);
+			ExecRemoteQueryStandard(resultRelationDesc, (RemoteQueryState *)resultRemoteRel, planSlot);
 		}
 		else
 		{
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 8a42c7176d..8a57816bd8 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -5367,12 +5367,6 @@ findReferencedVars(List *parent_vars, Plan *plan, List **out_tlist, Relids *out_
  *
  * For every target relation, add a remote query node to carry out remote
  * operations.
- *
- * !!PGXCTODO We should also fix the create_remotedelete_plan to work on similar
- * lines. Right now, it seems to assume only one result relation, which
- * certainly does not look true for inherited tables. But then, we can work on
- * this (and fix this comment) when we test and add support for inherited
- * tables
  */
 Plan *
 create_remoteinsert_plan(PlannerInfo *root, Plan *topplan)
@@ -5584,6 +5578,8 @@ create_remoteupdate_plan(PlannerInfo *root, Plan *topplan)
 		bool			is_set_printed = false;	/* Control of SET generation */
 		bool			is_where_printed = false;	/* Control of WHERE generation */
 		RemoteQuery	   *fstep;		/* Plan step generated */
+		ListCell	   *elt;
+		TargetEntry	   *tle_saved;
 
 		ttab = rt_fetch(resultRelationIndex, parse->rtable);
 
@@ -5599,22 +5595,6 @@ create_remoteupdate_plan(PlannerInfo *root, Plan *topplan)
 		/* Get number of arrtibutes */
 		natts = get_relnatts(ttab->relid);
 
-		/*
-		 * PGXCTODO: Allow use of remote UPDATE in case all the columns are
-		 * updated at once. In this case, create_remotequery_plan refers to
-		 * remote tuples with only the ctid which is not used by this part of
-		 * planning. CTID needs to be coupled with the node name of where
-		 * the tuple is located to avoid inconsistency due to similar CTIDs
-		 * among PGXC remote nodes.
-		 * This has no impact on distributed table as distribution column
-		 * cannot be updated yet, so only replicated tables are impacted now.
-		 */
-		if (list_length(parse->targetList) == natts + 1)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("Postgres-XC does not support remote UPDATE on all columns"),
-					 errdetail("At least one column reference to a remote value is necessary")));
-
 		/* Create query buffers */
 		buf = makeStringInfo();		/* For SET clause */
 		buf2 = makeStringInfo();	/* For WHERE clause */
@@ -5633,11 +5613,9 @@ create_remoteupdate_plan(PlannerInfo *root, Plan *topplan)
 		else
 			appendStringInfo(buf, "UPDATE %s.%s SET ", quote_identifier(nspname),
 							 quote_identifier(ttab->relname));
-		fstep = make_remotequery(NIL, ttab, NIL, ttab->relid);
-		fstep->is_temp = IsTempTable(ttab->relid);
 
-		/* All the parameters are the tuple attributes */
-		att_types = (Oid *) palloc0 (sizeof (Oid) * natts);
+		/* All the parameters are the tuple attributes and tuple CTID */
+		att_types = (Oid *) palloc0 (sizeof (Oid) * (natts + 1));
 
 		/*
 		 * Populate the SET and WHERE clauses with parameters.
@@ -5658,9 +5636,7 @@ create_remoteupdate_plan(PlannerInfo *root, Plan *topplan)
 			if (HeapTupleIsValid(tp))
 			{
 				Form_pg_attribute att_tup = (Form_pg_attribute) GETSTRUCT(tp);
-				ListCell *elt;
 				bool is_found = false;
-				TargetEntry *tle_saved;
 
 				/*
 				 * Attributes in the target list are inserted in the SET clause directly
@@ -5670,14 +5646,17 @@ create_remoteupdate_plan(PlannerInfo *root, Plan *topplan)
 				foreach(elt, parse->targetList)
 				{
 					TargetEntry *tle = lfirst(elt);
-					tle_saved = tle;
 					if (strcmp(tle->resname, NameStr(att_tup->attname)) == 0)
 					{
+						tle_saved = tle;
 						is_found = true;
 						break;
 					}
 				}
 
+				/* We need a target entry here */
+				Assert(tle_saved);
+
 				/*
 				 * If attribute is found in target list, add it to SET clause and
 				 * create associated parameter. If it is not found, this attribute
@@ -5742,6 +5721,32 @@ create_remoteupdate_plan(PlannerInfo *root, Plan *topplan)
 					 att, ttab->relid);
 		}
 
+		/* Now have a look at target list and find CTID in list */
+		tle_saved = NULL;
+		foreach(elt, parse->targetList)
+		{
+			TargetEntry *tle = lfirst(elt);
+			if (strcmp(tle->resname, "ctid") == 0)
+			{
+				tle_saved = tle;
+				break;
+			}
+		}
+
+		/* CTID has to be in target list */
+		Assert(tle_saved);
+
+		/* Add CTID as last parameter in query... */
+		if (!is_where_printed)
+			appendStringInfo(buf2, " WHERE ctid = $%d ", natts + 1);
+		else
+			appendStringInfo(buf2, "AND ctid = $%d ", natts + 1);
+		/* ... And in target list for remote query */
+		att_types[natts] = TIDOID;
+		tle_saved = makeTargetEntry(tle_saved->expr, natts + 1,
+									"ctid", false);
+		tlist = lappend(tlist, tle_saved);
+
 		/* Finish building the query by gathering SET and WHERE clauses */
 		appendStringInfo(buf, "%s", buf2->data);
 
@@ -5760,7 +5765,7 @@ create_remoteupdate_plan(PlannerInfo *root, Plan *topplan)
 		fstep->exec_nodes->nodeList = NULL;
 		fstep->exec_nodes->en_relid = ttab->relid;
 		fstep->exec_nodes->accesstype = RELATION_ACCESS_UPDATE;
-		SetRemoteStatementName((Plan *) fstep, NULL, natts, att_types, 0);
+		SetRemoteStatementName((Plan *) fstep, NULL, natts + 1, att_types, 0);
 		pfree(buf->data);
 		pfree(buf2->data);
 		pfree(buf);
@@ -5775,234 +5780,118 @@ create_remoteupdate_plan(PlannerInfo *root, Plan *topplan)
 /*
  * create_remotedelete_plan()
  *
- * Builds up a final node of the plan executing DELETE command.
- *
- * If target table is on coordinator (like catalog tables) the plan is left
- * unchanged and delete will be handled using standard postgres procedure.
- *
- * If topmost node of the plan is a RemoteQuery the step query looks like
- * SELECT ctid FROM target_table WHERE condition, and we should convert it to
- * DELETE FROM target_table WHERE condition.
- *
- * In correlated case the step query looks like
- * SELECT target_table.ctid FROM target_table, other_tables WHERE condition, and
- * we should convert it to DELETE FROM target_table USING other_tables WHERE condition.
- *
- * XXX Is it ever possible if the topmost node is not a RemoteQuery?
+ * For every target relation, add a remote query node to carry out remote
+ * operations. The tuple to be deleted is selected depending on the target
+ * list of given plan, generating parametrized WHERE clause in consequence.
  */
 Plan *
 create_remotedelete_plan(PlannerInfo *root, Plan *topplan)
 {
-	Query 		   *parse = root->parse;
-	RangeTblEntry  *ttab;
-	RelationLocInfo *rel_loc_info;
-	RemoteQuery	   *fstep;
-	StringInfo		buf;
-	Oid				nspid;
-	char		   *nspname;
-	Var 		   *ctid;
-
-	/* Get target table */
-	ttab = (RangeTblEntry *) list_nth(parse->rtable, parse->resultRelation - 1);
-	/* Bad relation ? */
-	if (ttab == NULL || ttab->rtekind != RTE_RELATION)
-		return topplan;
-
-	/* Get location info of the target table */
-	rel_loc_info = GetRelationLocInfo(ttab->relid);
-	if (rel_loc_info == NULL)
-		return topplan;
-
-	buf = makeStringInfo();
-
-	/* Compose DELETE FROM target_table */
-	nspid = get_rel_namespace(ttab->relid);
-	nspname = get_namespace_name(nspid);
+	ModifyTable *mt = (ModifyTable *)topplan;
+	ListCell	*l;
 
-	appendStringInfo(buf, "DELETE FROM %s.%s", quote_identifier(nspname),
-					 quote_identifier(ttab->relname));
+	/* We expect to work only on ModifyTable node */
+	if (!IsA(topplan, ModifyTable))
+		elog(ERROR, "Unexpected node type: %d", topplan->type);
 
-	/* See if we can push down DELETE */
-	if (IsA(topplan, RemoteQuery))
+	/*
+	 * For every result relation, build a remote plan to execute remote delete.
+	 */
+	foreach(l, mt->resultRelations)
 	{
-		char *query;
-
-		fstep = (RemoteQuery *) topplan;
-		query = fstep->sql_statement;
-
-		if (strncmp(query, "SELECT ctid", 11) == 0)
-		{
-			/*
-			 * Single table case
-			 * We need to find position of the WHERE keyword in the string and
-			 * append to the buffer part of original string starting from the
-			 * position found. It is possible WHERE clause is absent (DELETE ALL)
-			 * In this case buffer already has new step query
-			 */
-			char *where = strstr(query, " WHERE ");
-			if (where)
-				appendStringInfoString(buf, where);
-		}
-		else
-		{
-			/*
-			 * Multi table case
-			 * Assuming the RemoteQuery is created in create_remotejoin_plan().
-			 * If the final RemoteQuery is for correlated delete outer_statement
-			 * is just a SELECT FROM target_table, outer_statement is correlated
-			 * part and we can put it into USING clause.
-			 * Join type should be plain jon (comma-separated list) and all
-			 * conditions are in WHERE clause.
-			 * No GROUP BY or ORDER BY clauses expected.
-			 * If create_remotejoin_plan is modified the code below should be
-			 * revisited.
-			 */
-			/*
-			 * In expressions target table is referenced as outer_alias, append
-			 * alias name before USING clause
-			 */
-			appendStringInfo(buf, " %s USING ", fstep->outer_alias);
-
-			/* Make up USING clause */
-			appendStringInfo(buf, "(%s) %s ", fstep->inner_statement, fstep->inner_alias);
+		Index			resultRelationIndex = lfirst_int(l);
+		Query			*parse = root->parse;
+		RangeTblEntry   *ttab;
+		RelationLocInfo *rel_loc_info;
+		StringInfo		buf;
+		Oid			    nspid;		/* Relation namespace Oid */
+		char		   *nspname;	/* Relation namespace name */
+		int				nparams;	/* Attribute used is CTID */
+		Oid			   *param_types;	/* Types of query parameters */
+		RemoteQuery	   *fstep;		/* Plan step generated */
+		bool			is_where_created = false;
+		ListCell	   *elt;
+		int				count = 1;
 
-			/* Append WHERE clause */
-			appendStringInfoString(buf, fstep->join_condition);
-		}
+		ttab = rt_fetch(resultRelationIndex, parse->rtable);
 
-		/* Replace step query */
-		pfree(fstep->sql_statement);
-		fstep->sql_statement = pstrdup(buf->data);
+		/* Bad relation ? */
+		if (ttab == NULL || ttab->rtekind != RTE_RELATION)
+			continue;
 
-		/* Set combine_type, it is COMBINE_TYPE_NONE for SELECT */
-		fstep->combine_type = rel_loc_info->locatorType == LOCATOR_TYPE_REPLICATED ?
-								  COMBINE_TYPE_SAME : COMBINE_TYPE_SUM;
-		fstep->read_only = false;
+		/* Get location info of the target table */
+		rel_loc_info = GetRelationLocInfo(ttab->relid);
+		if (rel_loc_info == NULL)
+			continue;
 
-		pfree(buf->data);
-		pfree(buf);
+		/* Create query buffers */
+		buf = makeStringInfo();
 
-		return topplan;
-	}
+		/* Compose UPDATE target_table */
+		nspid = get_rel_namespace(ttab->relid);
+		nspname = get_namespace_name(nspid);
 
-	/*
-	 * Top plan will return CTIDs and we should delete tuples with these CTIDs
-	 * on the nodes. To determine target node
-	 */
-	fstep = make_remotequery(NIL, ttab, NIL, ttab->relid);
+		/* Parameters are defined by target list */
+		nparams = list_length(parse->targetList);
+		param_types = (Oid *) palloc0(sizeof(Oid) * nparams);
 
-	if (rel_loc_info->locatorType == LOCATOR_TYPE_REPLICATED)
-	{
 		/*
-		 * For replicated case we need two extra steps. One is to determine
-		 * all values by CTID on the node from which the tuple has come, next
-		 * is to remove all rows with these values on all nodes
+		 * Do not qualify with namespace for TEMP tables. The schema name may
+		 * vary on each node.
 		 */
-		RemoteQuery	   *xstep;
-		List 		   *xtlist = NIL;
-		StringInfo 		xbuf = makeStringInfo();
-		int 			natts = get_relnatts(ttab->relid);
-		int 			att;
-
-		appendStringInfoString(xbuf, "SELECT ");
-		appendStringInfoString(buf, " WHERE");
+		if (IsTempTable(ttab->relid))
+			appendStringInfo(buf, "DELETE FROM %s ",
+							 quote_identifier(ttab->relname));
+		else
+			appendStringInfo(buf, "DELETE FROM %s.%s ", quote_identifier(nspname),
+							 quote_identifier(ttab->relname));
 
-		/*
-		 * Populate projections of the extra SELECT step and WHERE clause of
-		 * the final DELETE step
-		 */
-		for (att = 1; att <= natts; att++)
+		/* Generate WHERE clause for each target list item */
+		foreach(elt, parse->targetList)
 		{
-			TargetEntry *tle;
-			Var *expr;
-			HeapTuple tp;
+			TargetEntry *tle = lfirst(elt);
 
-			tp = SearchSysCache(ATTNUM,
-								ObjectIdGetDatum(ttab->relid),
-								Int16GetDatum(att),
-								0, 0);
-			if (HeapTupleIsValid(tp))
+			/* Set the clause if necessary */
+			if (!is_where_created)
 			{
-				Form_pg_attribute att_tup = (Form_pg_attribute) GETSTRUCT(tp);
-
-				/* Add comma before all except first attributes */
-				if (att > 1)
-				{
-					appendStringInfoString(xbuf, ", ");
-					appendStringInfoString(buf, " AND");
-				}
-				appendStringInfoString(xbuf, NameStr(att_tup->attname));
-				appendStringInfo(buf, " %s = $%d", NameStr(att_tup->attname), att);
-
-				expr = makeVar(att, att, att_tup->atttypid,
-							   att_tup->atttypmod, InvalidOid, 0);
-				tle = makeTargetEntry((Expr *) expr, att,
-									  NameStr(att_tup->attname), false);
-				xtlist = lappend(xtlist, tle);
-				ReleaseSysCache(tp);
+				is_where_created = true;
+				appendStringInfoString(buf, "WHERE ");
 			}
 			else
-				elog(ERROR, "cache lookup failed for attribute %d of relation %u",
-					 att, ttab->relid);
+				appendStringInfoString(buf, "AND ");
+
+			appendStringInfo(buf, "%s = $%d ",
+							 quote_identifier(tle->resname),
+							 count);
+
+			/* Associate type of parameter */
+			param_types[count - 1] = exprType((Node *) tle->expr);
+			count++;
 		}
 
-		/* Complete SELECT command */
-		appendStringInfo(xbuf, " FROM %s.%s WHERE ctid = $1",
-						 quote_identifier(nspname),
-						 quote_identifier(ttab->relname));
-
-		/* Build up the extra select step */
-		xstep = make_remotequery(xtlist, ttab, NIL, ttab->relid);
-		innerPlan(xstep) = topplan;
-		xstep->sql_statement = pstrdup(xbuf->data);
-		xstep->read_only = true;
-		xstep->exec_nodes = makeNode(ExecNodes);
-		xstep->exec_nodes->baselocatortype = rel_loc_info->locatorType;
-		xstep->exec_nodes->tableusagetype = TABLE_USAGE_TYPE_USER;
-		xstep->exec_nodes->primarynodelist = NULL;
-		xstep->exec_nodes->nodeList = rel_loc_info->nodeList;
-		xstep->exec_nodes->en_relid = ttab->relid;
-		xstep->exec_nodes->accesstype = RELATION_ACCESS_READ;
-
-		/* First and only target entry of topplan is ctid, reference it */
-		ctid = makeVar(INNER, 1, TIDOID, -1, InvalidOid, 0);
-		xstep->exec_nodes->en_expr = (Expr *) ctid;
-
-		pfree(xbuf->data);
-		pfree(xbuf);
-
-		/* Build up the final delete step */
-		innerPlan(fstep) = (Plan *) xstep;
-		fstep->sql_statement = pstrdup(buf->data);
-		fstep->combine_type = COMBINE_TYPE_SAME;
-		fstep->read_only = false;
-		fstep->exec_nodes = GetRelationNodes(rel_loc_info, 0, UNKNOWNOID, RELATION_ACCESS_UPDATE);
-	}
-	else
-	{
-		/* Build up the final delete step */
-		innerPlan(fstep) = topplan;
-		appendStringInfoString(buf, " WHERE ctid = $1");
+		/* Finish by building the plan step */
+		fstep = make_remotequery(parse->targetList, ttab, NIL, ttab->relid);
+		fstep->is_temp = IsTempTable(ttab->relid);
 		fstep->sql_statement = pstrdup(buf->data);
-		fstep->combine_type = COMBINE_TYPE_SUM;
+		fstep->combine_type = COMBINE_TYPE_NONE;
+
 		fstep->read_only = false;
 		fstep->exec_nodes = makeNode(ExecNodes);
+		fstep->exec_nodes = GetRelationNodes(rel_loc_info, 0, UNKNOWNOID, RELATION_ACCESS_UPDATE);
 		fstep->exec_nodes->baselocatortype = rel_loc_info->locatorType;
 		fstep->exec_nodes->tableusagetype = TABLE_USAGE_TYPE_USER;
 		fstep->exec_nodes->primarynodelist = NULL;
 		fstep->exec_nodes->nodeList = NULL;
 		fstep->exec_nodes->en_relid = ttab->relid;
 		fstep->exec_nodes->accesstype = RELATION_ACCESS_UPDATE;
+		SetRemoteStatementName((Plan *) fstep, NULL, nparams, param_types, 0);
+		pfree(buf->data);
+		pfree(buf);
 
-		/* First and only target entry of topplan is ctid, reference it */
-		ctid = makeVar(INNER, 1, TIDOID, -1, InvalidOid, 0);
-		fstep->exec_nodes->en_expr = (Expr *) ctid;
+		mt->remote_plans = lappend(mt->remote_plans, fstep);
 	}
 
-	pfree(buf->data);
-	pfree(buf);
-
-	return (Plan *) fstep;
+	return topplan;
 }
 
 /*