Refactoring of gram.y for distribution clauses

This refactoring allows the usage of clauses TO NODE, TO GROUP and
DISTRIBUTE BY in a more flexible way by decoupling empty extensions.
This is particularly useful to plug-in those clauses to other existing
SQL of Postgres.
A slight grammar modification is being introduced in this commit, which
is the addition of brackets when defining a node list. This has consequences
on the following queries:
- EXECUTE DIRECT
- CLEAN CONNECTION
- CREATE TABLE (subset of nodes)
- CREATE TABLE AS
- CREATE NODE GROUP
This simple modification allows the use of TO NODE/TO GROUP extensions
when they are listed in a list of commands separated by commas.
By the way, this modification really lowers the possibility of
shift-reduce conflicts in bison when implementing new features in XC
using existing clauses.

7 files changed, 42 insertions, 27 deletions

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 198976009d..89833349ad 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -363,7 +363,7 @@ SPI_execute_direct(const char *remote_sql, char *nodename)
 	initStringInfo(&execdirect);
 
 	/* This string is never used. It is just passed to fill up spierrcontext.arg */
-	appendStringInfo(&execdirect, "EXECUTE DIRECT ON %s '%s'",
+	appendStringInfo(&execdirect, "EXECUTE DIRECT ON (%s) '%s'",
 	                               nodename, remote_sql);
 
 	stmt->node_names = list_make1(makeString(nodename));
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index cf57d7abe3..c87fdbf3d9 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -252,7 +252,7 @@ static void processCASbits(int cas_bits, int location, const char *constrType,
 %type <list>	createdb_opt_list alterdb_opt_list copy_opt_list
 				transaction_mode_list
 				create_extension_opt_list alter_extension_opt_list
-				pgxcnode_list
+				pgxcnode_list pgxcnodes
 %type <defelt>	createdb_opt_item alterdb_opt_item copy_opt_item
 				transaction_mode_item
 				create_extension_opt_item alter_extension_opt_item
@@ -480,8 +480,8 @@ static void processCASbits(int cas_bits, int location, const char *constrType,
 %type <str>		opt_existing_window_name
 /* PGXC_BEGIN */
 %type <str>		opt_barrier_id
-%type <distby>	OptDistributeBy
-%type <subclus> OptSubCluster
+%type <distby>	OptDistributeBy OptDistributeByInternal
+%type <subclus> OptSubCluster OptSubClusterInternal
 /* PGXC_END */
 
 
@@ -3070,7 +3070,11 @@ DistributeByHash: DISTRIBUTE BY
 			| DISTRIBUTE BY HASH
 		;
 
-OptDistributeBy: DistributeByHash '(' name ')'
+OptDistributeBy: OptDistributeByInternal			{ $$ = $1; }
+			| /* EMPTY */							{ $$ = NULL; }
+		;
+
+OptDistributeByInternal: DistributeByHash '(' name ')'
 				{
 					DistributeBy *n = makeNode(DistributeBy);
 					n->disttype = DISTTYPE_HASH;
@@ -3098,11 +3102,14 @@ OptDistributeBy: DistributeByHash '(' name ')'
 					n->colname = NULL;
 					$$ = n;
 				}
-			| /*EMPTY*/								{ $$ = NULL; }
 		;
 
-OptSubCluster:
-			TO NODE pgxcnode_list
+OptSubCluster: OptSubClusterInternal				{ $$ = $1; }
+			| /* EMPTY */							{ $$ = NULL; }
+		;
+
+OptSubClusterInternal:
+			TO NODE pgxcnodes
 				{
 					PGXCSubCluster *n = makeNode(PGXCSubCluster);
 					n->clustertype = SUBCLUSTER_NODE;
@@ -3116,7 +3123,6 @@ OptSubCluster:
 					n->members = list_make1(makeString($3));
 					$$ = n;
 				}
-			| /* EMPTY */							{ $$ = NULL; }
 		;
 /* PGXC_END */
 
@@ -8064,8 +8070,12 @@ pgxcnode_name:
 pgxcgroup_name:
 			ColId							{ $$ = $1; };
 
+pgxcnodes:
+			'(' pgxcnode_list ')'			{ $$ = $2; }
+		;
+
 pgxcnode_list:
-			pgxcnode_list ',' pgxcnode_name				{ $$ = lappend($1, makeString($3)); }
+			pgxcnode_list ',' pgxcnode_name		{ $$ = lappend($1, makeString($3)); }
 			| pgxcnode_name						{ $$ = list_make1(makeString($1)); }
 		;
 
@@ -8110,11 +8120,11 @@ DropNodeStmt: DROP NODE pgxcnode_name
 /*****************************************************************************
  *
  *		QUERY:
- *				CREATE NODE GROUP groupname WITH node1,...,nodeN
+ *				CREATE NODE GROUP groupname WITH (node1,...,nodeN)
  *
  *****************************************************************************/
 
-CreateNodeGroupStmt: CREATE NODE GROUP_P pgxcgroup_name WITH pgxcnode_list
+CreateNodeGroupStmt: CREATE NODE GROUP_P pgxcgroup_name WITH pgxcnodes
 				{
 					CreateGroupStmt *n = makeNode(CreateGroupStmt);
 					n->group_name = $4;
@@ -8225,11 +8235,11 @@ explain_option_arg:
 /*****************************************************************************
  *
  *		QUERY:
- *				EXECUTE DIRECT ON nodename [, ... ] query
+ *				EXECUTE DIRECT ON ( nodename [, ... ] ) query
  *
  *****************************************************************************/
 
-ExecDirectStmt: EXECUTE DIRECT ON pgxcnode_list DirectStmt
+ExecDirectStmt: EXECUTE DIRECT ON pgxcnodes DirectStmt
 				{
 					ExecDirectStmt *n = makeNode(ExecDirectStmt);
 					n->node_names = $4;
@@ -8251,13 +8261,13 @@ DirectStmt:
  *
  *		QUERY:
  *
- *		CLEAN CONNECTION TO { COORDINATOR nodename | NODE nodename | ALL {FORCE} }
+ *		CLEAN CONNECTION TO { COORDINATOR ( nodename ) | NODE ( nodename ) | ALL {FORCE} }
  *				[ FOR DATABASE dbname ]
  *				[ TO USER username ]
  *
  *****************************************************************************/
 
-CleanConnStmt: CLEAN CONNECTION TO COORDINATOR pgxcnode_list CleanConnDbName CleanConnUserName
+CleanConnStmt: CLEAN CONNECTION TO COORDINATOR pgxcnodes CleanConnDbName CleanConnUserName
 				{
 					CleanConnStmt *n = makeNode(CleanConnStmt);
 					n->is_coord = true;
@@ -8267,7 +8277,7 @@ CleanConnStmt: CLEAN CONNECTION TO COORDINATOR pgxcnode_list CleanConnDbName Cle
 					n->username = $7;
 					$$ = (Node *)n;
 				}
-				| CLEAN CONNECTION TO NODE pgxcnode_list CleanConnDbName CleanConnUserName
+				| CLEAN CONNECTION TO NODE pgxcnodes CleanConnDbName CleanConnUserName
 				{
 					CleanConnStmt *n = makeNode(CleanConnStmt);
 					n->is_coord = false;
diff --git a/src/backend/pgxc/locator/locator.c b/src/backend/pgxc/locator/locator.c
index 8c44f70308..68174c1298 100644
--- a/src/backend/pgxc/locator/locator.c
+++ b/src/backend/pgxc/locator/locator.c
@@ -920,10 +920,12 @@ RelationLocInfo *
 GetRelationLocInfo(Oid relid)
 {
 	RelationLocInfo *ret_loc_info = NULL;
-
 	Relation	rel = relation_open(relid, AccessShareLock);
 
-	if (rel && rel->rd_locator_info)
+	/* Relation needs to be valid */
+	Assert(rel->rd_isvalid);
+
+	if (rel->rd_locator_info)
 		ret_loc_info = CopyRelationLocInfo(rel->rd_locator_info);
 
 	relation_close(rel, AccessShareLock);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 8894a93005..c864e1486d 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -4197,7 +4197,7 @@ get_utility_query_def(Query *query, deparse_context *context)
 			switch (stmt->subcluster->clustertype)
 			{
 				case SUBCLUSTER_NODE:
-					appendStringInfo(buf, " TO NODE");
+					appendStringInfo(buf, " TO NODE (");
 
 					/* Add node members */
 					Assert(stmt->subcluster->members);
@@ -4207,6 +4207,7 @@ get_utility_query_def(Query *query, deparse_context *context)
 						if (cell->next)
 							appendStringInfo(buf, ",");
 					}
+					appendStringInfo(buf, ")");
 					break;
 
 				case SUBCLUSTER_GROUP:
diff --git a/src/pl/plpgsql/src/plpgsql--1.0.sql b/src/pl/plpgsql/src/plpgsql--1.0.sql
index 0f09df0fc8..f93fcd12e4 100644
--- a/src/pl/plpgsql/src/plpgsql--1.0.sql
+++ b/src/pl/plpgsql/src/plpgsql--1.0.sql
@@ -26,7 +26,7 @@ DECLARE
 		--Get all the node names
 		query_str_nodes := 'SELECT node_name FROM pgxc_node WHERE node_type = ''D''';
 		FOR row_name IN EXECUTE(query_str_nodes) LOOP
-			query_str := 'EXECUTE DIRECT ON ' || row_name.node_name || ' ''SELECT gid FROM pg_prepared_xact()''';
+			query_str := 'EXECUTE DIRECT ON (' || row_name.node_name || ') ''SELECT gid FROM pg_prepared_xact()''';
 			FOR row_data IN EXECUTE(query_str) LOOP
 				return next row_data.gid;
 			END LOOP;
diff --git a/src/test/regress/expected/xc_create_function.out b/src/test/regress/expected/xc_create_function.out
index 642100a641..41bbcdd256 100644
--- a/src/test/regress/expected/xc_create_function.out
+++ b/src/test/regress/expected/xc_create_function.out
@@ -16,12 +16,12 @@ declare
 	tmp_node	int;
 	num_nodes	int;
 begin
-	nodenames_query := 'SELECT node_name FROM pgxc_node WHERE node_type = ''D'''; 
+	nodenames_query := 'SELECT node_name FROM pgxc_node WHERE node_type = ''D''';
 	cr_command := 'CREATE TABLE ' || tab_schema || ' DISTRIBUTE BY ' || distribution || ' TO NODE ';
 	for nodename in execute nodenames_query loop
 		nodes := array_append(nodes, nodename);
 	end loop;
-	nodenames := '';
+	nodenames := '(';
 	sep := '';
 	num_nodes := array_length(nodes, 1);
 	foreach node in array nodenums loop
@@ -35,6 +35,7 @@ begin
 		nodenames := nodenames || sep || nodes[tmp_node];
 		sep := ', ';
 	end loop;
+	nodenames := nodenames || ')';
 	cr_command := cr_command || nodenames;
 	if (cmd_suffix is not null) then
 		cr_command := cr_command  || ' ' || cmd_suffix;
@@ -69,7 +70,7 @@ DECLARE
 	r		pg_prepared_xacts%rowtype;
 BEGIN
 	nodename := (SELECT get_xc_node_name(nodenum));
-	qry := 'EXECUTE DIRECT ON ' || nodename || ' ' || chr(39) || 'SELECT * FROM pg_prepared_xacts' || chr(39);
+	qry := 'EXECUTE DIRECT ON (' || nodename || ') ' || chr(39) || 'SELECT * FROM pg_prepared_xacts' || chr(39);
 
 	FOR r IN EXECUTE qry LOOP
 		IF r.gid = txn_id THEN
diff --git a/src/test/regress/sql/xc_create_function.sql b/src/test/regress/sql/xc_create_function.sql
index 8f9fd42cde..1c8e2350eb 100644
--- a/src/test/regress/sql/xc_create_function.sql
+++ b/src/test/regress/sql/xc_create_function.sql
@@ -17,12 +17,12 @@ declare
 	tmp_node	int;
 	num_nodes	int;
 begin
-	nodenames_query := 'SELECT node_name FROM pgxc_node WHERE node_type = ''D'''; 
+	nodenames_query := 'SELECT node_name FROM pgxc_node WHERE node_type = ''D''';
 	cr_command := 'CREATE TABLE ' || tab_schema || ' DISTRIBUTE BY ' || distribution || ' TO NODE ';
 	for nodename in execute nodenames_query loop
 		nodes := array_append(nodes, nodename);
 	end loop;
-	nodenames := '';
+	nodenames := '(';
 	sep := '';
 	num_nodes := array_length(nodes, 1);
 	foreach node in array nodenums loop
@@ -36,6 +36,7 @@ begin
 		nodenames := nodenames || sep || nodes[tmp_node];
 		sep := ', ';
 	end loop;
+	nodenames := nodenames || ')';
 	cr_command := cr_command || nodenames;
 	if (cmd_suffix is not null) then
 		cr_command := cr_command  || ' ' || cmd_suffix;
@@ -72,7 +73,7 @@ DECLARE
 	r		pg_prepared_xacts%rowtype;
 BEGIN
 	nodename := (SELECT get_xc_node_name(nodenum));
-	qry := 'EXECUTE DIRECT ON ' || nodename || ' ' || chr(39) || 'SELECT * FROM pg_prepared_xacts' || chr(39);
+	qry := 'EXECUTE DIRECT ON (' || nodename || ') ' || chr(39) || 'SELECT * FROM pg_prepared_xacts' || chr(39);
 
 	FOR r IN EXECUTE qry LOOP
 		IF r.gid = txn_id THEN