Hello!

According to the docs[1], one may use DEFAULT keyword while inserting
into generated columns (stored and identity). However, currently it
works only for a single VALUES sublist with DEFAULT for a generated column
but not for the case when VALUES RTE is used. This is not being tested
and it is broken.

I am attaching two patches. One for tests and another one with the
proposed changes based on ideas from Andrew on IRC. So if all good there
goes the credit where credit is due. If patch is no good, then it is
likely my misunderstanding how to put words into code :-)

This is my only second patch to PostgreSQL (the first one was rejected)
so don't be too harsh :-) It may not be perfect but I am open for a
feedback and this is just to get the ball rolling and to let the
community know about this issue.

Before you ask why would I want to insert DEFAULTs ... well, there are
ORMs[2] that still need to be patched and current situation contradicts
documentation[1].

Footnotes:
[1]  https://www.postgresql.org/docs/devel/ddl-generated-columns.html

[2]  https://github.com/rails/rails/pull/39368#issuecomment-670351379

--
Mikhail
From d606c4f952a6080dff6fb621ea034bfce2865f7b Mon Sep 17 00:00:00 2001
From: Mikhail Titov <m...@gmx.us>
Date: Wed, 12 Aug 2020 22:42:37 -0500
Subject: [PATCH 1/2] Test DEFAULT in VALUES RTE for generated columns

---
 src/test/regress/expected/generated.out |  9 +++++++++
 src/test/regress/expected/identity.out  | 13 +++++++++----
 src/test/regress/sql/generated.sql      |  4 ++++
 src/test/regress/sql/identity.sql       |  1 +
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index 7ccc3c65ed..31525ef2a6 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -90,6 +90,15 @@ CREATE TABLE gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT AS (a *
 ERROR:  for a generated column, GENERATED ALWAYS must be specified
 LINE 1: ...E gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT...
                                                              ^
+-- test VALUES RTE with defaults
+INSERT INTO gtest0 VALUES (1, DEFAULT), (2, DEFAULT);
+SELECT * FROM gtest0;
+ a | b
+---+----
+ 1 | 55
+ 2 | 55
+(2 rows)
+
 INSERT INTO gtest1 VALUES (1);
 INSERT INTO gtest1 VALUES (2, DEFAULT);
 INSERT INTO gtest1 VALUES (3, 33);  -- error
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 7ac9df767f..ca27b7ff73 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -76,6 +76,7 @@ INSERT INTO itest3 DEFAULT VALUES;
 INSERT INTO itest3 DEFAULT VALUES;
 INSERT INTO itest4 DEFAULT VALUES;
 INSERT INTO itest4 DEFAULT VALUES;
+INSERT INTO itest4 (a, b) VALUES (DEFAULT, 'foo'), (DEFAULT, 'bar');
 SELECT * FROM itest1;
  a | b
 ---+---
@@ -99,10 +100,12 @@ SELECT * FROM itest3;

 SELECT * FROM itest4;
  a | b
----+---
+---+-----
  1 |
  2 |
-(2 rows)
+ 3 | foo
+ 4 | bar
+(4 rows)

 -- VALUES RTEs
 INSERT INTO itest3 VALUES (DEFAULT, 'a');
@@ -211,11 +214,13 @@ ALTER TABLE itest4 ALTER COLUMN a DROP NOT NULL;
 INSERT INTO itest4 DEFAULT VALUES;
 SELECT * FROM itest4;
  a | b
----+---
+---+-----
  1 |
  2 |
+ 3 | foo
+ 4 | bar
    |
-(3 rows)
+(5 rows)

 -- check that sequence is removed
 SELECT sequence_name FROM itest4_a_seq;
diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql
index 4cff1279c7..18bb1d3c78 100644
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -40,6 +40,10 @@ CREATE TABLE gtest_err_7d (a int PRIMARY KEY, b int GENERATED ALWAYS AS (generat
 -- GENERATED BY DEFAULT not allowed
 CREATE TABLE gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT AS (a * 2) STORED);

+-- test VALUES RTE with defaults
+INSERT INTO gtest0 VALUES (1, DEFAULT), (2, DEFAULT);
+SELECT * FROM gtest0;
+
 INSERT INTO gtest1 VALUES (1);
 INSERT INTO gtest1 VALUES (2, DEFAULT);
 INSERT INTO gtest1 VALUES (3, 33);  -- error
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 1bf2a976eb..b3d99583c2 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -47,6 +47,7 @@ INSERT INTO itest3 DEFAULT VALUES;
 INSERT INTO itest3 DEFAULT VALUES;
 INSERT INTO itest4 DEFAULT VALUES;
 INSERT INTO itest4 DEFAULT VALUES;
+INSERT INTO itest4 (a, b) VALUES (DEFAULT, 'foo'), (DEFAULT, 'bar');

 SELECT * FROM itest1;
 SELECT * FROM itest2;
--
2.28.0.windows.1

From 7a187f698d31638c65da10a71e97060793a29c7f Mon Sep 17 00:00:00 2001
From: Mikhail Titov <m...@gmx.us>
Date: Wed, 12 Aug 2020 21:07:22 -0500
Subject: [PATCH 2/2] Allow DEFAULT in VALUES RTE for generated columns

---
 src/backend/parser/parse_relation.c  | 50 ++++++++++++++++++++++------
 src/backend/rewrite/rewriteHandler.c | 13 +++++++-
 2 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index b875a50646..da26863d22 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -2981,19 +2981,47 @@ expandNSItemVars(ParseNamespaceItem *nsitem,
 		if (colname[0])
 		{
 			Var		   *var;
+			ListCell   *c;
+			Node	   *std = NULL;
+			bool		all_default = true;

 			Assert(nscol->p_varno > 0);
-			var = makeVar(nscol->p_varno,
-						  nscol->p_varattno,
-						  nscol->p_vartype,
-						  nscol->p_vartypmod,
-						  nscol->p_varcollid,
-						  sublevels_up);
-			/* makeVar doesn't offer parameters for these, so set by hand: */
-			var->varnosyn = nscol->p_varnosyn;
-			var->varattnosyn = nscol->p_varattnosyn;
-			var->location = location;
-			result = lappend(result, var);
+			foreach(c, nsitem->p_rte->values_lists)
+			{
+				List *row = (List*) lfirst(c);
+				std = list_nth_node(Node, row, colindex);
+				if (!IsA(std, SetToDefault))
+				{
+					all_default = false;
+					break;
+				}
+			}
+
+			if (all_default && std)
+			{
+				/*
+				 * If all nodes for the column are SetToDefault, keep the marker
+				 * for later INSERT VALUES list rewriting where we remove
+				 * TLE and force NULL constant values node.
+				 * We cannot just remove values column
+				 * as attribute numbering will be off.
+				 */
+				result = lappend(result, std);
+			}
+			else
+			{
+				var = makeVar(nscol->p_varno,
+							  nscol->p_varattno,
+							  nscol->p_vartype,
+							  nscol->p_vartypmod,
+							  nscol->p_varcollid,
+							  sublevels_up);
+				/* makeVar doesn't offer parameters for these, so set by hand: */
+				var->varnosyn = nscol->p_varnosyn;
+				var->varattnosyn = nscol->p_varattnosyn;
+				var->location = location;
+				result = lappend(result, var);
+			}
 			if (colnames)
 				*colnames = lappend(*colnames, colnameval);
 		}
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index fe777c3103..f885fe7405 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1373,8 +1373,19 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
 				Form_pg_attribute att_tup;
 				Node	   *new_expr;

+				/*
+				 * TLE was likely already removed for this generated column.
+				 * We do not have a good way to check it now,
+				 * but attrno should not be 0 anyway.
+				 */
 				if (attrno == 0)
-					elog(ERROR, "cannot set value in column %d to DEFAULT", i);
+				{
+					SetToDefault *std = (SetToDefault*) col;
+					new_expr = (Node*) makeNullConst(std->typeId, std->typeMod, std->collation);
+					newList = lappend(newList, new_expr);
+					continue;
+				}
+
 				att_tup = TupleDescAttr(target_relation->rd_att, attrno - 1);

 				if (!force_nulls && !att_tup->attisdropped)
--
2.28.0.windows.1

Reply via email to