Revert misguided change to postgres_fdw FOR UPDATE/SHARE code.

tglsfdc · tglsfdc · commit 8ec8760fc87e · 2014-12-12T12:41:49.000-05:00
In commit 462bd95, I changed postgres_fdw to rely on get_plan_rowmark() instead of get_parse_rowmark(). I still think that's a good idea in the long run, but as Etsuro Fujita pointed out, it doesn't work today because planner.c forces PlanRowMarks to have markType = ROW_MARK_COPY for all foreign tables. There's no urgent reason to change this in the back branches, so let's just revert that part of yesterday's commit rather than trying to design a better solution under time pressure. Also, add a regression test case showing what postgres_fdw does with FOR UPDATE/SHARE. I'd blithely assumed there was one already, else I'd have realized yesterday that this code didn't work.
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -237,6 +237,39 @@ SELECT * FROM ft1 t1 WHERE t1.c1 = 101 AND t1.c6 = '1' AND t1.c7 >= '1';
  101 |  1 | 00101 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1  | 1          | foo
 (1 row)
 
+-- with FOR UPDATE/SHARE
+EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE c1 = 101 FOR UPDATE;
+                                                   QUERY PLAN                                                   
+----------------------------------------------------------------------------------------------------------------
+ LockRows
+   Output: c1, c2, c3, c4, c5, c6, c7, c8, t1.*
+   ->  Foreign Scan on public.ft1 t1
+         Output: c1, c2, c3, c4, c5, c6, c7, c8, t1.*
+         Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" = 101)) FOR UPDATE
+(5 rows)
+
+SELECT * FROM ft1 t1 WHERE c1 = 101 FOR UPDATE;
+ c1  | c2 |  c3   |              c4              |            c5            | c6 |     c7     | c8  
+-----+----+-------+------------------------------+--------------------------+----+------------+-----
+ 101 |  1 | 00101 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1  | 1          | foo
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE c1 = 102 FOR SHARE;
+                                                  QUERY PLAN                                                   
+---------------------------------------------------------------------------------------------------------------
+ LockRows
+   Output: c1, c2, c3, c4, c5, c6, c7, c8, t1.*
+   ->  Foreign Scan on public.ft1 t1
+         Output: c1, c2, c3, c4, c5, c6, c7, c8, t1.*
+         Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" = 102)) FOR SHARE
+(5 rows)
+
+SELECT * FROM ft1 t1 WHERE c1 = 102 FOR SHARE;
+ c1  | c2 |  c3   |              c4              |            c5            | c6 |     c7     | c8  
+-----+----+-------+------------------------------+--------------------------+----+------------+-----
+ 102 |  2 | 00102 | Sat Jan 03 00:00:00 1970 PST | Sat Jan 03 00:00:00 1970 | 2  | 2          | foo
+(1 row)
+
 -- aggregate
 SELECT COUNT(*) FROM ft1 t1;
  count 
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
@@ -822,7 +822,7 @@ postgresGetForeignPlan(PlannerInfo *root,
 	}
 	else
 	{
-		PlanRowMark *rc = get_plan_rowmark(root->rowMarks, baserel->relid);
+		RowMarkClause *rc = get_parse_rowmark(root->parse, baserel->relid);
 
 		if (rc)
 		{
@@ -835,18 +835,15 @@ postgresGetForeignPlan(PlannerInfo *root,
 			 * complete information about, and (b) it wouldn't work anyway on
 			 * older remote servers.  Likewise, we don't worry about NOWAIT.
 			 */
-			switch (rc->markType)
+			switch (rc->strength)
 			{
-				case ROW_MARK_EXCLUSIVE:
-				case ROW_MARK_NOKEYEXCLUSIVE:
-					appendStringInfoString(&sql, " FOR UPDATE");
-					break;
-				case ROW_MARK_SHARE:
-				case ROW_MARK_KEYSHARE:
+				case LCS_FORKEYSHARE:
+				case LCS_FORSHARE:
 					appendStringInfoString(&sql, " FOR SHARE");
 					break;
-				default:
-					/* nothing needed */
+				case LCS_FORNOKEYUPDATE:
+				case LCS_FORUPDATE:
+					appendStringInfoString(&sql, " FOR UPDATE");
 					break;
 			}
 		}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -151,6 +151,11 @@ SELECT * FROM ft1 WHERE false;
 -- with WHERE clause
 EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = 101 AND t1.c6 = '1' AND t1.c7 >= '1';
 SELECT * FROM ft1 t1 WHERE t1.c1 = 101 AND t1.c6 = '1' AND t1.c7 >= '1';
+-- with FOR UPDATE/SHARE
+EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE c1 = 101 FOR UPDATE;
+SELECT * FROM ft1 t1 WHERE c1 = 101 FOR UPDATE;
+EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE c1 = 102 FOR SHARE;
+SELECT * FROM ft1 t1 WHERE c1 = 102 FOR SHARE;
 -- aggregate
 SELECT COUNT(*) FROM ft1 t1;
 -- join two tables

Original file line number	Diff line number	Diff line change
`@@ -822,7 +822,7 @@ postgresGetForeignPlan(PlannerInfo *root,`
`822`	`822`	`}`
`823`	`823`	`else`
`824`	`824`	`{`
`825`		`- PlanRowMark *rc = get_plan_rowmark(root->rowMarks, baserel->relid);`
	`825`	`+ RowMarkClause *rc = get_parse_rowmark(root->parse, baserel->relid);`
`826`	`826`
`827`	`827`	`if (rc)`
`828`	`828`	`{`
`@@ -835,18 +835,15 @@ postgresGetForeignPlan(PlannerInfo *root,`
`835`	`835`	`* complete information about, and (b) it wouldn't work anyway on`
`836`	`836`	`* older remote servers. Likewise, we don't worry about NOWAIT.`
`837`	`837`	`*/`
`838`		`- switch (rc->markType)`
	`838`	`+ switch (rc->strength)`
`839`	`839`	`{`
`840`		`- case ROW_MARK_EXCLUSIVE:`
`841`		`- case ROW_MARK_NOKEYEXCLUSIVE:`
`842`		`- appendStringInfoString(&sql, " FOR UPDATE");`
`843`		`- break;`
`844`		`- case ROW_MARK_SHARE:`
`845`		`- case ROW_MARK_KEYSHARE:`
	`840`	`+ case LCS_FORKEYSHARE:`
	`841`	`+ case LCS_FORSHARE:`
`846`	`842`	`appendStringInfoString(&sql, " FOR SHARE");`
`847`	`843`	`break;`
`848`		`- default:`
`849`		`- /* nothing needed */`
	`844`	`+ case LCS_FORNOKEYUPDATE:`
	`845`	`+ case LCS_FORUPDATE:`
	`846`	`+ appendStringInfoString(&sql, " FOR UPDATE");`
`850`	`847`	`break;`
`851`	`848`	`}`
`852`	`849`	`}`