#632: [YSQL] Re-enable default values for columns in PostgreSQL tables

Summary:
Support for default values for columns in PostgreSQL tables was disabled in an earlier revision due
to lack of support to update sys catalog tables. The support is added in this revision and default
values are now enabled.

This revision also includes a small bugfix in ybcam.c to unregister only temp snapshot in scans.

Test Plan:
```
postgres=# create table t (a int primary key, b int default 1);
CREATE TABLE
postgres=# \d t
                 Table "public.t"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           |          |
 b      | integer |           |          | 1
postgres=# select oid from pg_class where relname = 't';
  oid
-------
 26384

postgres=# select * from pg_attribute where attrelid = 26384;
 attrelid | attname  | atttypid | attstattarget | attlen | attnum | attndims | attcacheoff | atttypmod | attbyval | attstorage | attalign | attnotnull | atthasdef | attidentity | attisdropped | attislocal | attinhcount | attcollation | attacl | attoptions | attfdwoptions
----------+----------+----------+---------------+--------+--------+----------+-------------+-----------+----------+------------+----------+------------+-----------+-------------+--------------+------------+-------------+--------------+--------+------------+---------------
    26384 | a        |       23 |            -1 |      4 |      1 |        0 |          -1 |        -1 | t        | p          | i        | f          | f         |             | f            | t          |           0 |            0 |        |            |
    26384 | b        |       23 |            -1 |      4 |      2 |        0 |          -1 |        -1 | t        | p          | i        | f          | t         |             | f            | t          |           0 |            0 |        |            |
    26384 | cmax     |       29 |             0 |      4 |     -6 |        0 |          -1 |        -1 | t        | p          | i        | t          | f         |             | f            | t          |           0 |            0 |        |            |
    26384 | cmin     |       29 |             0 |      4 |     -4 |        0 |          -1 |        -1 | t        | p          | i        | t          | f         |             | f            | t          |           0 |            0 |        |            |
    26384 | ctid     |       27 |             0 |      6 |     -1 |        0 |          -1 |        -1 | f        | p          | s        | t          | f         |             | f            | t          |           0 |            0 |        |            |
    26384 | tableoid |       26 |             0 |      4 |     -7 |        0 |          -1 |        -1 | t        | p          | i        | t          | f         |             | f            | t          |           0 |            0 |        |            |
    26384 | xmax     |       28 |             0 |      4 |     -5 |        0 |          -1 |        -1 | t        | p          | i        | t          | f         |             | f            | t          |           0 |            0 |        |            |
    26384 | xmin     |       28 |             0 |      4 |     -3 |        0 |          -1 |        -1 | t        | p          | i        | t          | f         |             | f            | t          |           0 |            0 |        |            |
```

Reviewers: mikhail, neil, hector, mihnea

Reviewed By: hector, mihnea

Subscribers: mihnea, yql

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

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

@@ -794,7 +794,7 @@ protected List<Row> setupSimpleTable(String tableName) throws SQLException {
 
   @Override
   public int getTestMethodTimeoutSec() {
-    return nonTsanVsTsan(360, 1080);
+    return nonTsanVsTsan(720, 1080);
   }
 
 }

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

@@ -167,7 +167,7 @@ public void testSelectByTextKey() throws Exception {
   // TODO disabled until we support UPDATE for syscatalog tables.
   // Currently the "atthasdef" column in the pg_attribute table starts as false and is updated
   // _after_ processing and adding default values (and constraints) to pg_attrdef.
-  @Ignore
+  @Test
   public void testDefaultValues() throws Exception {
     try (Statement statement = connection.createStatement()) {
       statement.execute("CREATE TABLE testdefaultvaluetable " +

src/postgres/src/backend/access/heap/heapam.c

@@ -74,6 +74,7 @@
 #include "utils/syscache.h"
 #include "utils/tqual.h"
 
+#include "executor/ybcModifyTable.h"
 #include "pg_yb_utils.h"
 
 /* GUC variable */
@@ -6284,8 +6285,7 @@ heap_inplace_update(Relation relation, HeapTuple tuple)
 
 	if (IsYugaByteEnabled())
 	{
-		YBC_LOG_WARNING("Ignoring in-place update for %s.",
-		                RelationGetRelationName(relation));
+		YBCUpdateSysCatalogTuple(relation, tuple);
 		return;
 	}
 

src/postgres/src/backend/access/ybc/ybcam.c

@@ -59,7 +59,7 @@ setup_ybcscan_from_scankey(Relation relation,
 	/*
 	 * Set up the where clause condition (for the YB scan).
 	 */
-	for (int i  = 0; i < nkeys; i++)
+	for (int i = 0; i < nkeys; i++)
 	{
 		if (key[i].sk_attno == InvalidOid)
 			break;
@@ -92,7 +92,7 @@ setup_ybcscan_from_scankey(Relation relation,
 			/* This must be an OID column. */
 			lhs->vartype = OIDOID;
 		}
-		cond->args    = lappend(cond->args, lhs);
+		cond->args = lappend(cond->args, lhs);
 
 		/* Set up value (rhs) */
 		Const *rhs = makeNode(Const);
@@ -129,10 +129,7 @@ setup_ybcscan_from_scankey(Relation relation,
 
 
 static bool
-tuple_matches_key(HeapTuple tup,
-                  TupleDesc tupdesc,
-                  int nkeys,
-                  ScanKey key)
+tuple_matches_key(HeapTuple tup, TupleDesc tupdesc, int nkeys, ScanKey key)
 {
 	for (int i = 0; i < nkeys; i++)
 	{
@@ -178,7 +175,7 @@ HeapScanDesc ybc_heap_beginscan(Relation relation,
 
 SysScanDesc ybc_systable_beginscan(Relation relation,
                                    Oid indexId,
-                                   bool first_time,
+                                   bool indexOK,
                                    Snapshot snapshot,
                                    int nkeys,
                                    ScanKey key)
@@ -187,9 +184,9 @@ SysScanDesc ybc_systable_beginscan(Relation relation,
 
 	/* Set up Postgres sys table scan description */
 	SysScanDesc scan_desc = (SysScanDesc) palloc0(sizeof(SysScanDescData));
-	scan_desc->heap_rel = relation;
-	scan_desc->snapshot = snapshot;
-	scan_desc->ybscan   = ybScan;
+	scan_desc->heap_rel   = relation;
+	scan_desc->snapshot   = snapshot;
+	scan_desc->ybscan     = ybScan;
 
 	return scan_desc;
 }

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

@@ -232,12 +232,9 @@ CatalogTupleUpdate(Relation heapRel, ItemPointer otid, HeapTuple tup)
 
 	if (IsYugaByteEnabled())
 	{
-		if (!YBIsInitDbModeEnvVarSet())
-		{
-			YBC_LOG_WARNING("Ignoring unsupported tuple update for rel %s tupid %d",
-							RelationGetRelationName(heapRel),
-							HeapTupleGetOid(tup));
-		}
+		YBCUpdateSysCatalogTuple(heapRel, tup);
+		/* Update the local cache automatically */
+		SetSysCacheTuple(heapRel, tup);
 		return;
 	}
 
@@ -264,9 +261,9 @@ CatalogTupleUpdateWithInfo(Relation heapRel, ItemPointer otid, HeapTuple tup,
 {
 	if (IsYugaByteEnabled())
 	{
-		YBC_LOG_WARNING("Ignoring unsupported tuple update for rel %s tupid %d",
-		                RelationGetRelationName(heapRel),
-		                HeapTupleGetOid(tup));
+		YBCUpdateSysCatalogTuple(heapRel, tup);
+		/* Update the local cache automatically */
+		SetSysCacheTuple(heapRel, tup);
 		return;
 	}
 

src/postgres/src/backend/executor/nodeModifyTable.c

@@ -961,10 +961,11 @@ ExecUpdate(ModifyTableState *mtstate,
 	}
 	else if (IsYugaByteEnabled())
 	{
-		if (!IsYBRelation(resultRelationDesc)) {
+		if (!IsYBRelation(resultRelationDesc))
+		{
 			ereport(ERROR,
-							(errcode(ERRCODE_UNDEFINED_OBJECT),
-							 errmsg("This relational object does not exist in YugaByte database")));
+					(errcode(ERRCODE_UNDEFINED_OBJECT),
+					 errmsg("This relational object does not exist in YugaByte database")));
 		}
 		YBCExecuteUpdate(resultRelationDesc, resultRelInfo, planSlot, tuple);
 		if (resultRelInfo->ri_projectReturning)

src/postgres/src/backend/executor/ybcModifyTable.c

@@ -62,7 +62,7 @@ bool IsRealYBColumn(Relation rel, int attrNum)
  * Get the type ID of a real or virtual attribute (column).
  * Returns InvalidOid if the attribute number is invalid.
  */
-Oid GetTypeId(int attrNum, TupleDesc tupleDesc)
+static Oid GetTypeId(int attrNum, TupleDesc tupleDesc)
 {
 	switch (attrNum)
 	{
@@ -89,29 +89,18 @@ Oid GetTypeId(int attrNum, TupleDesc tupleDesc)
 }
 
 /*
- * Insert a tuple into the a relation's backing YugaByte table.
+ * Get primary key columns as bitmap of a table.
  */
-Oid YBCExecuteInsert(Relation rel, TupleDesc tupleDesc, HeapTuple tuple)
+static Bitmapset *GetTablePrimaryKey(Relation rel)
 {
 	Oid            dboid         = YBCGetDatabaseOid(rel);
 	Oid            relid         = RelationGetRelid(rel);
 	AttrNumber     minattr       = FirstLowInvalidHeapAttributeNumber + 1;
 	int            natts         = RelationGetNumberOfAttributes(rel);
 	Bitmapset      *pkey         = NULL;
-	YBCPgStatement ybc_stmt      = NULL;
 	YBCPgTableDesc ybc_tabledesc = NULL;
 
-	/* Generate a new oid for this row if needed */
-	if (rel->rd_rel->relhasoids)
-	{
-		if (!OidIsValid(HeapTupleGetOid(tuple)))
-			HeapTupleSetOid(tuple, GetNewOid(rel));
-	}
-
-	/*
-	 * Get the primary key columns 'pkey' from YugaByte. Used below to
-	 * check that values for all primary key columns are given (not null)
-	 */
+	/* Get the primary key columns 'pkey' from YugaByte. */
 	HandleYBStatus(YBCPgGetTableDesc(ybc_pg_session, dboid, relid, &ybc_tabledesc));
 	for (AttrNumber attnum = minattr; attnum <= natts; attnum++)
 	{
@@ -132,12 +121,33 @@ Oid YBCExecuteInsert(Relation rel, TupleDesc tupleDesc, HeapTuple tuple)
 		}
 	}
 	HandleYBStatus(YBCPgDeleteTableDesc(ybc_tabledesc));
-	ybc_tabledesc = NULL;
+
+	return pkey;
+}
+
+/*
+ * Insert a tuple into the a relation's backing YugaByte table.
+ */
+Oid YBCExecuteInsert(Relation rel, TupleDesc tupleDesc, HeapTuple tuple)
+{
+	Oid            dboid    = YBCGetDatabaseOid(rel);
+	Oid            relid    = RelationGetRelid(rel);
+	AttrNumber     minattr  = FirstLowInvalidHeapAttributeNumber + 1;
+	int            natts    = RelationGetNumberOfAttributes(rel);
+	Bitmapset      *pkey    = GetTablePrimaryKey(rel);
+	YBCPgStatement ybc_stmt = NULL;
+
+	/* Generate a new oid for this row if needed */
+	if (rel->rd_rel->relhasoids)
+	{
+		if (!OidIsValid(HeapTupleGetOid(tuple)))
+			HeapTupleSetOid(tuple, GetNewOid(rel));
+	}
 
 	/* Create the INSERT request and add the values from the tuple. */
 	HandleYBStatus(YBCPgNewInsert(ybc_pg_session, dboid, relid, &ybc_stmt));
 	bool is_null = false;
-	for (AttrNumber attnum  = minattr; attnum <= natts; attnum++)
+	for (AttrNumber attnum = minattr; attnum <= natts; attnum++)
 	{
 		/* Skip virtual (system) columns */
 		if (!IsRealYBColumn(rel, attnum))
@@ -267,7 +277,8 @@ void YBCExecuteUpdate(Relation rel,
 	/* Look for ybctid. */
 	int idx;
 	Datum ybctid = 0;
-	Form_pg_attribute *attrs = slot->tts_tupleDescriptor->attrs;
+	Form_pg_attribute *attrs = tupleDesc->attrs;
+
 	for (idx = 0; idx < slot->tts_nvalid; idx++)
 	{
 		if (strcmp(NameStr(attrs[idx]->attname), "ybctid") == 0 &&	!slot->tts_isnull[idx])
@@ -296,7 +307,69 @@ void YBCExecuteUpdate(Relation rel,
 
 	/* Assign new values to columns for updating the current row. */
 	Form_pg_attribute *table_attrs = rel->rd_att->attrs;
-	for (idx = 0; idx < rel->rd_att->natts; idx++) {
+	for (idx = 0; idx < rel->rd_att->natts; idx++)
+	{
+		AttrNumber attnum = table_attrs[idx]->attnum;
+
+		bool is_null = false;
+		Datum d = heap_getattr(tuple, attnum, tupleDesc, &is_null);
+		YBCPgExpr ybc_expr = YBCNewConstant(update_stmt, table_attrs[idx]->atttypid, d, is_null);
+		HandleYBStmtStatus(YBCPgDmlAssignColumn(update_stmt, attnum, ybc_expr), update_stmt);
+	}
+
+	/* Execute the statement */
+	HandleYBStmtStatus(YBCPgExecUpdate(update_stmt), update_stmt);
+	HandleYBStatus(YBCPgDeleteStatement(update_stmt));
+	update_stmt = NULL;
+}
+
+void YBCUpdateSysCatalogTuple(Relation rel, HeapTuple tuple)
+{
+	Oid            dboid       = YBCGetDatabaseOid(rel);
+	Oid            relid       = RelationGetRelid(rel);
+	TupleDesc      tupleDesc   = RelationGetDescr(rel);
+	YBCPgStatement update_stmt = NULL;
+
+	/* Create update statement. */
+	HandleYBStatus(YBCPgNewUpdate(ybc_pg_session, dboid, relid, &update_stmt));
+
+	AttrNumber minattr = FirstLowInvalidHeapAttributeNumber + 1;
+	int        natts   = RelationGetNumberOfAttributes(rel);
+	Bitmapset  *pkey   = GetTablePrimaryKey(rel);
+
+	for (AttrNumber attnum = minattr; attnum <= natts; attnum++)
+	{
+		/* Skip non-primary-key columns */
+		if (!bms_is_member(attnum - minattr, pkey))
+		{
+			continue;
+		}
+
+		Oid   type_id = GetTypeId(attnum, tupleDesc);
+		Datum datum   = 0;
+		bool  is_null = false;
+
+		datum = heap_getattr(tuple, attnum, tupleDesc, &is_null);
+
+		/* Check not-null constraint on primary key early */
+		if (is_null)
+		{
+			HandleYBStatus(YBCPgDeleteStatement(update_stmt));
+			ereport(ERROR,
+					(errcode(ERRCODE_NOT_NULL_VIOLATION), errmsg(
+							"Missing/null value for primary key column")));
+		}
+
+		/* Add the column value to the update request */
+		YBCPgExpr ybc_expr = YBCNewConstant(update_stmt, type_id, datum, is_null);
+		HandleYBStmtStatus(YBCPgDmlBindColumn(update_stmt, attnum, ybc_expr), update_stmt);
+	}
+
+	/* Assign new values to columns for updating the current row. */
+	int idx;
+	Form_pg_attribute *table_attrs = rel->rd_att->attrs;
+	for (idx = 0; idx < rel->rd_att->natts; idx++)
+	{
 		AttrNumber attnum = table_attrs[idx]->attnum;
 
 		bool is_null = false;
@@ -305,6 +378,12 @@ void YBCExecuteUpdate(Relation rel,
 		HandleYBStmtStatus(YBCPgDmlAssignColumn(update_stmt, attnum, ybc_expr), update_stmt);
 	}
 
+	/*
+	 * Invalidate the cache now so if there is an error with delete we will
+	 * re-query to get the correct state from the master.
+	 */
+	CacheInvalidateHeapTuple(rel, tuple, NULL);
+
 	/* Execute the statement */
 	HandleYBStmtStatus(YBCPgExecUpdate(update_stmt), update_stmt);
 	HandleYBStatus(YBCPgDeleteStatement(update_stmt));

src/postgres/src/backend/parser/gram.y

@@ -3568,7 +3568,6 @@ ColConstraintElem:
 				}
 			| DEFAULT b_expr
 				{
-					parser_ybc_not_support(@1, "DEFAULT column value");
 					Constraint *n = makeNode(Constraint);
 					n->contype = CONSTR_DEFAULT;
 					n->location = @1;

src/postgres/src/backend/utils/cache/relcache.c

@@ -558,7 +558,8 @@ RelationBuildTupleDesc(Relation relation)
 
 		if (attp->atthasdef)
 		{
-			attrdef = (AttrDefault *)
+			if (attrdef == NULL)
+				attrdef = (AttrDefault *)
 					MemoryContextAllocZero(CacheMemoryContext,
 					                       relation->rd_rel->relnatts *
 					                       sizeof(AttrDefault));
@@ -710,6 +711,7 @@ RelationBuildRuleLock(Relation relation)
 									  RewriteRelRulenameIndexId,
 									  true, NULL,
 									  1, &key);
+
 	while (HeapTupleIsValid(rewrite_tuple = systable_getnext(rewrite_scan)))
 	{
 		Form_pg_rewrite rewrite_form = (Form_pg_rewrite) GETSTRUCT(rewrite_tuple);

src/postgres/src/include/access/ybcam.h

@@ -51,9 +51,9 @@ typedef struct YbSysScanDescData *YbSysScanDesc;
  * We ignore the index id and always do a regular YugaByte scan (Postgres
  * would do either heap scan or index scan depending on the params).
  */
-extern SysScanDesc ybc_systable_beginscan(Relation heapRelation,
+extern SysScanDesc ybc_systable_beginscan(Relation relation,
                                           Oid indexId,
-                                          bool first_time,
+                                          bool indexOK,
                                           Snapshot snapshot,
                                           int nkeys,
                                           ScanKey key);

src/postgres/src/include/executor/ybcModifyTable.h

@@ -23,13 +23,18 @@
 #ifndef YBCMODIFYTABLE_H
 #define YBCMODIFYTABLE_H
 
+#include "nodes/execnodes.h"
+#include "executor/tuptable.h"
+
 extern Oid YBCExecuteInsert(Relation rel, TupleDesc tupleDesc, HeapTuple tuple);
 
-void YBCExecuteDelete(Relation rel, ResultRelInfo *resultRelInfo, TupleTableSlot *slot);
+extern void YBCExecuteDelete(Relation rel, ResultRelInfo *resultRelInfo, TupleTableSlot *slot);
+
+extern void YBCDeleteSysCatalogTuple(Relation rel, HeapTuple tuple, Bitmapset *pkey);
 
-void YBCDeleteSysCatalogTuple(Relation rel, HeapTuple tuple, Bitmapset *pkey);
+extern void YBCExecuteUpdate(Relation rel, ResultRelInfo *resultRelInfo, TupleTableSlot *slot,
+							 HeapTuple tuple);
 
-void YBCExecuteUpdate(Relation rel, ResultRelInfo *resultRelInfo, TupleTableSlot *slot,
-											HeapTuple tuple);
+extern void YBCUpdateSysCatalogTuple(Relation rel, HeapTuple tuple);
 
 #endif							/* YBCMODIFYTABLE_H */

src/postgres/src/test/regress/yb_serial_schedule

@@ -17,8 +17,7 @@ test: yb_int4_array
 test: yb_float4
 test: yb_float8
 test: yb_timestamp
-# Disable test because default values are disabled.
-# test: yb_insert
+test: yb_insert
 test: yb_schema
 #
 ####################################################################################################