schema-vars-review.patch

diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml
index 2d6555497c..bd4733c0bb 100644
--- a/doc/src/sgml/advanced.sgml
+++ b/doc/src/sgml/advanced.sgml
@@ -714,24 +714,16 @@ SELECT name, elevation
 
    <para>
     Schema variables are database objects that can hold a value.
-    Schema variables, like relations, exist within a schema and their access is
+    Schema variables, like relations, exist within a schema and the access is
     controlled via <command>GRANT</command> and <command>REVOKE</command> commands.
-    The schema variable can be created by the <command>CREATE VARIABLE</command> command.
+    A schema variable can be created by the <command>CREATE VARIABLE</command> command.
    </para>
 
    <para>
-    The value of a schema variable is local to the current session. Retrieving
-    a variable's value returns either a NULL or a default value, unless its value
-    is set to something else in the current session with a LET command. The content
-    of a variable is not transactional. This is the same as in regular variables
-    in PL languages.
-   </para>
-
-   <para>
-    Schema variables are retrieved by the <command>SELECT</command> SQL command.
-    Their value is set with the <command>LET</command> SQL command.
+    The value of a schema variable is set with the <command>LET</command> SQL command.
     While schema variables share properties with tables, their value cannot be updated
-    with an <command>UPDATE</command> command.
+    with an <command>UPDATE</command> command. The value of a schema variable may be
+    retrieved by the <command>SELECT</command> SQL command.
 <programlisting>
 CREATE VARIABLE var1 AS date;
 LET var1 = current_date;
@@ -747,6 +739,15 @@ LET current_user_id = (SELECT id FROM users WHERE usename = session_user);
 SELECT current_user_id;
 </programlisting>
    </para>
+
+   <para>
+    The value of a schema variable is local to the current session. Retrieving
+    a variable's value returns either a <literal>NULL</literal> or a default value,
+    unless its value is set to something else in the current session using the
+    <command>LET</command> command. The content of a variable is not transactional.
+    This is the same as regular variables in PL languages.
+   </para>
+
   </sect1>
 
 
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 179fa873dc..400724456f 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -13972,8 +13972,8 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
   </indexterm>
 
   <para>
-   The table <structname>pg_variable</structname> holds metadata
-   of schema variables.
+   The table <structname>pg_variable</structname> provides information
+   about schema variables.
   </para>
 
   <table>
@@ -14029,8 +14029,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
        <structfield>vartypmod</structfield> records type-specific data
        supplied at variable creation time (for example, the maximum
        length of a <type>varchar</type> column).  It is passed to
-       type-specific input 
-       functions and length coercion functions.
+       type-specific input functions and length coercion functions.
        The value will generally be -1 for types that do not need <structfield>vartypmod</structfield>.
       </entry>
      </row>
@@ -14075,6 +14074,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
       <entry><type>char</type></entry>
       <entry></entry>
       <entry>
+       Action performed at end of transaction:
        <literal>n</literal> = no action, <literal>d</literal> = drop the variable,
        <literal>r</literal> = reset the variable to its default value.
       </entry>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index e494161741..fb2ef96d0d 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5954,7 +5954,7 @@ $$ LANGUAGE plpgsql STRICT IMMUTABLE;
    </sect3>
 
    <sect3>
-    <title><command>Global variables and constants</command></title>
+    <title><command>Schema variables and constants</command></title>
 
     <para>
      The <application>PL/pgSQL</application> language has no packages,
diff --git a/doc/src/sgml/ref/discard.sgml b/doc/src/sgml/ref/discard.sgml
index 698b2c07fd..7d6ad2dd49 100644
--- a/doc/src/sgml/ref/discard.sgml
+++ b/doc/src/sgml/ref/discard.sgml
@@ -81,7 +81,7 @@ DISCARD { ALL | PLANS | SEQUENCES | TEMPORARY | TEMP | VARIABLES }
      <para>
       Resets the value of all schema variables. If a variable
       is later reused, it is re-initialized to either
-      NULL or its default value.
+      <literal>NULL</literal> or its default value.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 8d1c02b871..715208af23 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -4763,7 +4763,7 @@ pg_variable_aclmask(Oid var_oid, Oid roleid, AclMode mask, AclMaskHow how)
 		return mask;
 
 	/*
-	 * Must get the type's tuple from pg_type
+	 * Must get the variables's tuple from pg_variable
 	 */
 	tuple = SearchSysCache1(VARIABLEOID, ObjectIdGetDatum(var_oid));
 	if (!HeapTupleIsValid(tuple))
@@ -4774,7 +4774,7 @@ pg_variable_aclmask(Oid var_oid, Oid roleid, AclMode mask, AclMaskHow how)
 	varForm = (Form_pg_variable) GETSTRUCT(tuple);
 
 	/*
-	 * Now get the type's owner and ACL from the tuple
+	 * Now get the variable's owner and ACL from the tuple
 	 */
 	ownerId = varForm->varowner;
 
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 4ec76d4e03..f504701515 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1878,8 +1878,8 @@ find_expr_references_walker(Node *node,
 	{
 		Param	   *param = (Param *) node;
 
+		/* A variable parameter depends on the schema variable */
 		if (param->paramkind == PARAM_VARIABLE)
-			/* A variable parameter depends on the schema variable */
 			add_object_address(OCLASS_VARIABLE, param->paramvarid, 0,
 							   context->addrs);
 
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 02c5de7f73..789f41876c 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -828,7 +828,6 @@ VariableIsVisible(Oid varid)
 	return visible;
 }
 
-
 /*
  * TypenameGetTypid
  *		Wrapper for binary compatibility.
@@ -2909,6 +2908,8 @@ TSConfigIsVisible(Oid cfgid)
 
 /*
  * When we know a variable name, then we can find variable simply
+ *
+ * FIXME seems incomplete
  */
 Oid
 LookupVariable(const char *nspname, const char *varname, bool missing_ok)
@@ -2995,6 +2996,9 @@ NamesFromList(List *names)
  * Returns oid of not ambigonuous variable specified by qualified path
  * or InvalidOid. When the path is ambigonuous, then not_uniq flag is
  * is true.
+ *
+ * XXX Do we need the not_uniq flag, when we return InvalidOid anyway?
+ * XXX Maybe use not_unique, it's not much longer.
  */
 Oid
 IdentifyVariable(List *names, char **attrname, bool *not_uniq)
diff --git a/src/backend/commands/schemavariable.c b/src/backend/commands/schemavariable.c
index 15e0567199..5426e5fa21 100644
--- a/src/backend/commands/schemavariable.c
+++ b/src/backend/commands/schemavariable.c
@@ -64,6 +64,10 @@ static List *on_commits = NIL;
  * implementation of DROP can be simple, because although DROP VARIABLE
  * can be reverted, the content of variable can be lost. In this example,
  * DROP VARIABLE is same like reset variable.
+ *
+ * XXX Seems misplaced? There is no example here.
+ * XXX Also, is it really true that discarding the value even if DROP
+ * VARIABLE gets reverted makes sense?
  */
 
 typedef struct SchemaVariableData
@@ -108,6 +112,9 @@ static void clean_cache_variable(Oid varid);
  * Save info about necessity to clean hash table, because some
  * schema variable was dropped. Don't do here more, recheck
  * needs to be in transaction state.
+ *
+ * XXX Not sure "needs to be in transaction state" means? Perhaps
+ * we can do the cleanup only at transaction end?
  */
 static void
 InvalidateSchemaVariableCacheCallback(Datum arg, int cacheid, uint32 hashvalue)
@@ -126,41 +133,53 @@ InvalidateSchemaVariableCacheCallback(Datum arg, int cacheid, uint32 hashvalue)
 static void
 clean_cache_callback(XactEvent event, void *arg)
 {
+	HASH_SEQ_STATUS status;
+	SchemaVariable svar;
+
+	/* If no cleanup needed, we're done. */
+	if (!clean_cache_req)
+		return;
+
+	/* XXX How can this happen after having clean_cache_req=true? */
+	if (!schemavarhashtab)
+		return;
+
 	/*
-	 * should continue only in transaction time, when syscache is available.
+	 * XXX Can we even get here when not in a transaction? Isn't that really
+	 * an error? Maybe should be assert or elog(ERROR).
 	 */
-	if (clean_cache_req && IsTransactionState())
-	{
-		HASH_SEQ_STATUS status;
-		SchemaVariable svar;
+	if (!IsTransactionState())
+		return;
 
-		if (!schemavarhashtab)
-			return;
+	hash_seq_init(&status, schemavarhashtab);
 
-		hash_seq_init(&status, schemavarhashtab);
+	/*
+	 * Walk the current contents of the hash table, and check if the
+	 * schema variable still exists in the catalog. If not, remove it.
+	 *
+	 * XXX Try using a variable still used by another session.
+	 */
+	while ((svar = (SchemaVariable) hash_seq_search(&status)) != NULL)
+	{
+		HeapTuple	tp;
 
-		while ((svar = (SchemaVariable) hash_seq_search(&status)) != NULL)
+		tp = SearchSysCache1(VARIABLEOID, ObjectIdGetDatum(svar->varid));
+		if (!HeapTupleIsValid(tp))
 		{
-			HeapTuple	tp;
+			elog(DEBUG1, "variable %d is removed from cache", svar->varid);
 
-			tp = SearchSysCache1(VARIABLEOID, ObjectIdGetDatum(svar->varid));
-			if (!HeapTupleIsValid(tp))
-			{
-				elog(DEBUG1, "variable %d is removed from cache", svar->varid);
+			free_schema_variable(svar, true);
 
-				free_schema_variable(svar, true);
+			remove_variable_on_commit_actions(svar->varid, VARIABLE_EOX_DROP);
 
-				remove_variable_on_commit_actions(svar->varid, VARIABLE_EOX_DROP);
-
-				(void) hash_search(schemavarhashtab, (void *) &svar->varid,
-								   HASH_REMOVE, NULL);
-			}
-			else
-				ReleaseSysCache(tp);
+			(void) hash_search(schemavarhashtab, (void *) &svar->varid,
+							   HASH_REMOVE, NULL);
 		}
-
-		clean_cache_req = false;
+		else
+			ReleaseSysCache(tp);
 	}
+
+	clean_cache_req = false;
 }
 
 /*
@@ -202,12 +221,19 @@ create_schema_variable_hashtable(void)
 }
 
 /*
- * Fast drop complete content of schema variables
+ * Fast drop complete content of schema variables hash table.
  */
 void
 ResetSchemaVariableCache(void)
 {
-	/* Remove temporal schema variables */
+	/*
+	 * Remove temporary schema variables.
+	 *
+	 * XXX This does not *remove* the variables, does it? If the variable
+	 * used ON COMMIT RESET then it'll still be there.
+	 *
+	 * XXX Is it correct to call this with "isCommit=true"?
+	 */
 	AtPreEOXact_SchemaVariable_on_commit_actions(true);
 
 	/* Destroy hash table and reset related memory context */
@@ -228,6 +254,9 @@ ResetSchemaVariableCache(void)
  * current value for possible future usage on rollback.
  *
  * When force is true, then release current and possibly archived value.
+ *
+ * XXX The "force" flag is not really used.
+ * XXX Can't the value be something more complex?
  */
 static void
 free_schema_variable(SchemaVariable svar, bool force)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 44e42c6f69..5922e6fc34 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -5955,7 +5955,7 @@ copyObjectImpl(const void *from)
 			break;
 
 		default:
-			elog(ERROR, "urecognized node type: %d", (int) nodeTag(from));
+			elog(ERROR, "unrecognized node type: %d", (int) nodeTag(from));
 			retval = 0;			/* keep compiler quiet */
 			break;
 	}
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b18528ffbc..30d9662148 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4761,7 +4761,9 @@ substitute_actual_parameters_mutator(Node *node,
 {
 	if (node == NULL)
 		return NULL;
-	if (IsA(node, Param) &&((Param *) node)->paramkind != PARAM_VARIABLE)
+
+	/* XXX Explain why this does not work with PARAM_VARIABLE? */
+	if (IsA(node, Param) && ((Param *) node)->paramkind != PARAM_VARIABLE)
 	{
 		Param	   *param = (Param *) node;
 
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 8971cf7e80..34cbaa9100 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1290,6 +1290,10 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
 	 * Transform targetlist. Second usage of this transformation
 	 * is for Let statement. In this case we would to allow DEFAULT
 	 * keyword - we specify EXPR_KIND_LET_TARGET.
+	 *
+	 * XXX This is really hard to understand. What if p_expr_kind is
+	 * EXPR_KIND_LET_TARGET, why should it matter if parentParseState
+	 * is NULL?
 	 */
 	target_exprkind =
 		(pstate->p_expr_kind != EXPR_KIND_LET_TARGET ||
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index fb5391d173..b56ff4f69d 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4717,6 +4717,7 @@ OptSchemaVarDefExpr: DEFAULT b_expr					{ $$ = $2; }
 			| /* EMPTY */							{ $$ = NULL; }
 		;
 
+/* XXX This seems a bit strange. */
 OnEOXActionOption:  ON COMMIT DROP					{ $$ = VARIABLE_EOX_DROP; }
 			| ON TRANSACTION END_P RESET			{ $$ = VARIABLE_EOX_RESET; }
 			| /*EMPTY*/								{ $$ = VARIABLE_EOX_NOOP; }
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 6aaaa5bb29..31b5b55a09 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -281,7 +281,7 @@ transformExprRecurse(ParseState *pstate, Node *expr)
 			 * processed it rather than passing it to transformExpr().
 			 */
 		case T_SetToDefault:
-			Assert(false);
+			Assert(false);	/* FIXME left here by accident? */
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
 					 errmsg("DEFAULT is not allowed in this context"),
@@ -772,6 +772,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
 			break;
 	}
 
+	/* XXX Why is this before the p_post_columnref_hook block? Why not as part of
+	 * the varid block right after it? */
 	varid = IdentifyVariable(cref->fields, &attrname, &not_unique);
 
 	if (not_unique)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a8b727898b..5f1b386b55 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4735,7 +4735,7 @@ getVariables(Archive *fout)
 	int			i,
 				ntups;
 
-	if (fout->remoteVersion < 130000)
+	if (fout->remoteVersion < 150000)
 		return;
 
 	acl_subquery = createPQExpBuffer();