[yugabyte#2272] YSQL: Creating system views during YSQL cluster upgrade

Summary:
Support `CREATE VIEW` during YSQL upgrade imitating the behaviour of `yb_system_views.sql` processing by initdb.
This has two key differences with `CREATE TABLE`:
* View is defined through a rewrite rule (entry in `pg_rewrite`), which has its own OID allocated in [10000, 16384) range (from `FirstBootstrapObjectId` until `FirstNormalObjectId`). To specify it, new reloption `rewrite_rule_oid` was introduced.
* Since it's an SQL script and not BKI definition, reloptions can now be specified and persisted. As such, temporary helper options `table_oid`, `row_type_oid` and `rewrite_rule_oid` are removed before persisting.

Additionally, `yb_non_ddl_txn_for_sys_tables_allowed` development flag caused `CREATE VIEW` to fail with a read restart error because cache refresh operation was done in the wrong transaction.  Changed it to affect WRITE operations only.

Test Plan: ybd --java-test org.yb.pgsql.TestYsqlUpgrade

Reviewers: dmitry, mihnea, jason

Reviewed By: jason

Subscribers: zyu, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D12128

Author: frozenspider

java/yb-pgsql/src/test/java/org/yb/pgsql/TestYsqlUpgrade.java

@@ -438,6 +438,97 @@ public void sharedRelsIndexesWork() throws Exception {
     }
   }
 
+  /** Create a system view just like pg_stats and verify they look the same. */
+  @Test
+  public void creatingSystemViewsIsLikeInitdb() throws Exception {
+    TableInfo origTi = new TableInfo("pg_stats", 12081L, 12082L, 12083L,
+        Arrays.asList());
+
+    TableInfo newTi = new TableInfo("pg_stats_2", newSysOid(), newSysOid(), newSysOid(),
+        Arrays.asList());
+
+    try (Connection conn = getConnectionBuilder().withDatabase(customDbName).connect();
+         Statement stmtA = conn.createStatement();
+         Statement stmtB = conn.createStatement()) {
+      enableSystemRelsModification(stmtA);
+
+      String createViewSql = "CREATE VIEW pg_catalog." + newTi.name + " WITH ("
+          + "  table_oid = " + newTi.oid
+          + ", row_type_oid = " + newTi.typeOid
+          + ", rewrite_rule_oid = " + newTi.ruleOid
+          + ", security_barrier = true"
+          + ") AS"
+          + " SELECT"
+          + "     nspname AS schemaname,"
+          + "     relname AS tablename,"
+          + "     attname AS attname,"
+          + "     stainherit AS inherited,"
+          + "     stanullfrac AS null_frac,"
+          + "     stawidth AS avg_width,"
+          + "     stadistinct AS n_distinct,"
+          + "     CASE"
+          + "         WHEN stakind1 = 1 THEN stavalues1"
+          + "         WHEN stakind2 = 1 THEN stavalues2"
+          + "         WHEN stakind3 = 1 THEN stavalues3"
+          + "         WHEN stakind4 = 1 THEN stavalues4"
+          + "         WHEN stakind5 = 1 THEN stavalues5"
+          + "     END AS most_common_vals,"
+          + "     CASE"
+          + "         WHEN stakind1 = 1 THEN stanumbers1"
+          + "         WHEN stakind2 = 1 THEN stanumbers2"
+          + "         WHEN stakind3 = 1 THEN stanumbers3"
+          + "         WHEN stakind4 = 1 THEN stanumbers4"
+          + "         WHEN stakind5 = 1 THEN stanumbers5"
+          + "     END AS most_common_freqs,"
+          + "     CASE"
+          + "         WHEN stakind1 = 2 THEN stavalues1"
+          + "         WHEN stakind2 = 2 THEN stavalues2"
+          + "         WHEN stakind3 = 2 THEN stavalues3"
+          + "         WHEN stakind4 = 2 THEN stavalues4"
+          + "         WHEN stakind5 = 2 THEN stavalues5"
+          + "     END AS histogram_bounds,"
+          + "     CASE"
+          + "         WHEN stakind1 = 3 THEN stanumbers1[1]"
+          + "         WHEN stakind2 = 3 THEN stanumbers2[1]"
+          + "         WHEN stakind3 = 3 THEN stanumbers3[1]"
+          + "         WHEN stakind4 = 3 THEN stanumbers4[1]"
+          + "         WHEN stakind5 = 3 THEN stanumbers5[1]"
+          + "     END AS correlation,"
+          + "     CASE"
+          + "         WHEN stakind1 = 4 THEN stavalues1"
+          + "         WHEN stakind2 = 4 THEN stavalues2"
+          + "         WHEN stakind3 = 4 THEN stavalues3"
+          + "         WHEN stakind4 = 4 THEN stavalues4"
+          + "         WHEN stakind5 = 4 THEN stavalues5"
+          + "     END AS most_common_elems,"
+          + "     CASE"
+          + "         WHEN stakind1 = 4 THEN stanumbers1"
+          + "         WHEN stakind2 = 4 THEN stanumbers2"
+          + "         WHEN stakind3 = 4 THEN stanumbers3"
+          + "         WHEN stakind4 = 4 THEN stanumbers4"
+          + "         WHEN stakind5 = 4 THEN stanumbers5"
+          + "     END AS most_common_elem_freqs,"
+          + "     CASE"
+          + "         WHEN stakind1 = 5 THEN stanumbers1"
+          + "         WHEN stakind2 = 5 THEN stanumbers2"
+          + "         WHEN stakind3 = 5 THEN stanumbers3"
+          + "         WHEN stakind4 = 5 THEN stanumbers4"
+          + "         WHEN stakind5 = 5 THEN stanumbers5"
+          + "     END AS elem_count_histogram"
+          + " FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)"
+          + "      JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)"
+          + "      LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)"
+          + " WHERE NOT attisdropped"
+          + " AND has_column_privilege(c.oid, a.attnum, 'select')"
+          + " AND (c.relrowsecurity = false OR NOT row_security_active(c.oid));";
+
+      LOG.info("Executing '{}'", createViewSql);
+      stmtA.execute(createViewSql);
+
+      assertTablesAreSimilar(origTi, newTi, stmtA, stmtB);
+    }
+  }
+
   //
   // Helpers
   //
@@ -487,6 +578,7 @@ private void assertTablesAreSimilar(
           .map(r -> expectRelfilenodeMismatch ? excluded(r, "relfilenode") : r)
           .map(r -> replaced(r, newTi.oid, origTi.oid))
           .map(r -> replaced(r, newTi.typeOid, origTi.typeOid))
+          .map(r -> origTi.ruleOid != 0 ? replaced(r, newTi.ruleOid, origTi.ruleOid) : r)
           .map(r -> replacedString(r, newTi.name, origTi.name))
           .map(r -> {
             for (int i = 0; i < newTi.indexes.size(); ++i) {
@@ -498,8 +590,8 @@ private void assertTablesAreSimilar(
     };
 
     {
-      // pg_class and pg_type (for rel and indexes)
-      String sql = "SELECT cl.oid, cl.*, tp.oid, tp.* FROM pg_class cl"
+      // pg_class and pg_type (for rel and indexes), as well as pg_get_viewdef (view query)
+      String sql = "SELECT cl.oid, cl.*, tp.oid, tp.*, pg_get_viewdef(cl.oid) FROM pg_class cl"
           + " LEFT JOIN pg_type tp ON cl.oid = tp.typrelid"
           + " WHERE cl.oid = %d ORDER By cl.oid, tp.oid";
       {
@@ -690,12 +782,20 @@ private class TableInfo {
     public final String name;
     public final long oid;
     public final long typeOid;
+    /** Only when applicable, set to 0 otherwise. */
+    public final long ruleOid;
     public final List<Pair<String, Long>> indexes;
 
     public TableInfo(String name, long oid, long typeOid, List<Pair<String, Long>> indexes) {
+      this(name, oid, typeOid, 0L, indexes);
+    }
+
+    public TableInfo(String name, long oid, long typeOid, long ruleOid,
+        List<Pair<String, Long>> indexes) {
       this.name = name;
       this.oid = oid;
       this.typeOid = typeOid;
+      this.ruleOid = ruleOid;
       this.indexes = Collections.unmodifiableList(new ArrayList<>(indexes));
     }
   }

src/postgres/src/backend/access/common/reloptions.c

@@ -392,6 +392,17 @@ static relopt_int intRelOpts[] =
 		1 /* parse_utilcmd takes care of OID >= FirstNormalObjectId for user tables */,
 		INT_MAX
 	},
+	{
+		{
+			"rewrite_rule_oid",
+			"Postgres rewrite rule oid for the new rewrite rule defined for this view.",
+			RELOPT_KIND_VIEW,
+			AccessExclusiveLock
+		},
+		InvalidOid,
+		1,
+		FirstNormalObjectId - 1 /* we don't expect this for non-system views */
+	},
 	/* list terminator */
 	{{NULL}}
 };
@@ -827,6 +838,24 @@ add_string_reloption(bits32 kinds, const char *name, const char *desc, const cha
 Datum
 transformRelOptions(Datum oldOptions, List *defList, const char *namspace,
 					char *validnsps[], bool ignoreOids, bool isReset)
+{
+	return ybTransformRelOptions(oldOptions, defList, namspace, validnsps,
+								 ignoreOids, isReset,
+								 false /* ybIgnoreYsqlUpgradeViewOptions */);
+}
+
+/*
+ * See above for transformRelOptions description.
+ * 
+ * If ybIgnoreYsqlUpgradeViewOptions is specified, ignore "table_oid",
+ * "row_type_oid" and "rewrite_rule_oid" options. This is needed in
+ * YSQL upgrade mode, where we use them to simulate initdb-like behaviour
+ * for creating relations but don't want them to be persisted.
+ */
+Datum
+ybTransformRelOptions(Datum oldOptions, List *defList, const char *namspace,
+					  char *validnsps[], bool ignoreOids, bool isReset,
+					  bool ybIgnoreYsqlUpgradeOptions)
 {
 	Datum		result;
 	ArrayBuildState *astate;
@@ -940,6 +969,12 @@ transformRelOptions(Datum oldOptions, List *defList, const char *namspace,
 			if (ignoreOids && strcmp(def->defname, "oids") == 0)
 				continue;
 
+			if (ybIgnoreYsqlUpgradeOptions &&
+				(strcmp(def->defname, "table_oid") == 0 ||
+				 strcmp(def->defname, "row_type_oid") == 0 ||
+				 strcmp(def->defname, "rewrite_rule_oid") == 0))
+				continue;
+
 			/* ignore if not in the same namespace */
 			if (namspace == NULL)
 			{
@@ -1475,7 +1510,9 @@ view_reloptions(Datum reloptions, bool validate)
 		{"security_barrier", RELOPT_TYPE_BOOL,
 		offsetof(ViewOptions, security_barrier)},
 		{"check_option", RELOPT_TYPE_STRING,
-		offsetof(ViewOptions, check_option_offset)}
+		offsetof(ViewOptions, check_option_offset)},
+		{"rewrite_rule_oid", RELOPT_TYPE_BOOL,
+		offsetof(ViewOptions, rewrite_rule_oid)},
 	};
 
 	options = parseRelOptions(reloptions, validate, RELOPT_KIND_VIEW, &numoptions);

src/postgres/src/backend/catalog/catalog.c

@@ -51,6 +51,7 @@
 
 /* YB includes. */
 #include "access/htup_details.h"
+#include "catalog/pg_rewrite.h"
 #include "utils/syscache.h"
 #include "pg_yb_utils.h"
 
@@ -693,3 +694,55 @@ GetRowTypeOidFromRelOptions(List *relOptions)
 
 	return InvalidOid;
 }
+
+/*
+ * GetRewriteRuleOidFromRelOptions
+ *		Scans through relOptions for any 'rewrite_rule_oid' options, and ensures
+ *		that oid is available. Returns that oid, or InvalidOid if unspecified.
+ */
+Oid
+GetRewriteRuleOidFromRelOptions(List *relOptions)
+{
+	ListCell   *opt_cell;
+	Oid			rewrite_rule_oid;
+	Relation	pg_rewrite_desc;
+	ScanKeyData	skey[1];
+	SysScanDesc	rcscan;
+	HeapTuple	tuple;
+
+	foreach(opt_cell, relOptions)
+	{
+		DefElem *def = (DefElem *) lfirst(opt_cell);
+		if (strcmp(def->defname, "rewrite_rule_oid") == 0)
+		{
+			rewrite_rule_oid = strtol(defGetString(def), NULL, 10);
+			if (OidIsValid(rewrite_rule_oid))
+			{
+				/* Check there's no such rule yet. */
+				pg_rewrite_desc = heap_open(RewriteRelationId, RowExclusiveLock);
+
+				ScanKeyInit(&skey[0],
+							ObjectIdAttributeNumber,
+							BTEqualStrategyNumber, F_OIDEQ,
+							ObjectIdGetDatum(rewrite_rule_oid));
+
+				rcscan = systable_beginscan(pg_rewrite_desc, RewriteOidIndexId,
+							true, NULL, 1, skey);
+
+				tuple = systable_getnext(rcscan);
+
+				if (HeapTupleIsValid(tuple))
+					ereport(ERROR,
+							(errcode(ERRCODE_DUPLICATE_OBJECT),
+							 errmsg("rewrite rule OID %d is in use", rewrite_rule_oid)));
+
+				systable_endscan(rcscan);
+				heap_close(pg_rewrite_desc, RowExclusiveLock);
+
+				return rewrite_rule_oid;
+			}
+		}
+	}
+
+	return InvalidOid;
+}

src/postgres/src/backend/catalog/heap.c

@@ -1206,17 +1206,29 @@ heap_create_with_catalog(const char *relname,
 	}
 
 	/*
+	 * YSQL upgrade notes:
+	 * -------------------
 	 * At this point, reloptions no longer affects the relation
 	 * creation process, the only remaining use for them is to be
-	 * stored in pg_class.
+	 * stored in pg_class. ybTransformRelOptions has already
+	 * removed all temporary reloptions.
 	 *
-	 * For YSQL upgrade, we want system relations to not have
-	 * reloptions stored to imitate BKI processing, so we can safely
-	 * remove them now.
+	 * We want system tables to not have any reloptions stored to
+	 * imitate BKI processing, so we don't allow any reloption not
+	 * removed by ybTransformRelOptions.
+	 *
+	 * System views, on the other hand, are defined in
+	 * yb_system_views.sql rather than via BKI, so they are allowed
+	 * to have non-temporary reloptions.
 	 */
-	if (is_system && IsYsqlUpgrade)
+	if (IsYsqlUpgrade && is_system && relkind != RELKIND_VIEW
+		&& reloptions != (Datum) 0)
 	{
-		reloptions = (Datum) 0;
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+				 errmsg("unsuppored reloptions were used for a system table "
+				        "during YSQL upgrade"),
+				 errhint("Only a small subset is allowed due to BKI restrictions.")));
 	}
 
 	/*

src/postgres/src/backend/commands/createas.c

@@ -146,7 +146,8 @@ create_ctas_internal(List *attrList, IntoClause *into)
 		/* StoreViewQuery scribbles on tree, so make a copy */
 		Query	   *query = (Query *) copyObject(into->viewQuery);
 
-		StoreViewQuery(intoRelationAddr.objectId, query, false);
+		StoreViewQuery(intoRelationAddr.objectId, query, false,
+					   InvalidOid /* yb_rule_id */);
 		CommandCounterIncrement();
 	}
 

src/postgres/src/backend/commands/tablecmds.c

@@ -658,6 +658,10 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 				ereport(ERROR,
 						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 						 errmsg("shared relations can not be partitioned")));
+			else if (relkind != RELKIND_RELATION)
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("only ordinary tables may be shared")));
 
 			relisshared = true;
 
@@ -692,11 +696,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		}
 	}
 
-	if (IsYsqlUpgrade && relkind == RELKIND_VIEW && IsSystemNamespace(namespaceId))
-		ereport(ERROR,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("creating system views during YSQL upgrade is not yet supported")));
-
 	/*
 	 * Select tablegroup to use. If not specified, InvalidOid.
 	 * Disallow mixing of COLOCATED=true/false syntax and TABLEGROUP. Cannot use tablegroups
@@ -758,8 +757,8 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	/*
 	 * Parse and validate reloptions, if any.
 	 */
-	reloptions = transformRelOptions((Datum) 0, stmt->options, NULL, validnsps,
-									 true, false);
+	reloptions = ybTransformRelOptions((Datum) 0, stmt->options, NULL, validnsps,
+									   true, false, IsYsqlUpgrade);
 
 	if (relkind == RELKIND_VIEW)
 		(void) view_reloptions(reloptions, true);