On Fri, Sep 8, 2023 at 05:10:51PM -0400, Bruce Momjian wrote:
> I knew we only considered the array dimension sizes to be documentation
> _in_ the query, but I thought we at least properly displayed the number
> of dimensions specified at creation when we described the table in psql,
> but it seems we don't do that either.
>
> A big question is why we even bother to record the dimensions in
> pg_attribute if is not accurate for LIKE and not displayed to the user
> in a meaningful way by psql.
>
> I think another big question is whether the structure we are using to
> supply the column information to BuildDescForRelation is optimal. The
> typmod that has to be found for CREATE TABLE uses:
>
> typenameTypeIdAndMod(NULL, entry->typeName, &atttypid, &atttypmod);
>
> which calls typenameTypeIdAndMod() -> typenameType() -> LookupTypeName()
> -> LookupTypeNameExtended() -> typenameTypeMod(). This seems very
> complicated because the ColumnDef, at least in the LIKE case, already
> has the value. Is there a need to revisit how we handle type such
> cases?
(Bug report moved to hackers, previous bug reporters added CCs.)
I looked at this some more and found more fundamental problems. We have
pg_attribute.attndims which does record the array dimensionality:
CREATE TABLE test (data integer, data_array integer[5][5]);
SELECT attndims
FROM pg_attribute
WHERE attrelid = 'test'::regclass AND
attname = 'data_array';
attndims
----------
2
The first new problem I found is that we don't dump the dimensionality:
$ pg_dump test
...
CREATE TABLE public.test (
data integer,
--> data_array integer[]
);
and psql doesn't display the dimensionality:
\d test
Table "public.test"
Column | Type | Collation | Nullable | Default
------------+-----------+-----------+----------+---------
data | integer | | |
--> data_array | integer[] | | |
A report from 2015 reports that CREATE TABLE ... LIKE and CREATE TABLE
... AS doesn't propagate the dimensionality:
https://www.postgresql.org/message-id/flat/20150707072942.1186.98151%40wrigleys.postgresql.org
and this thread from 2018 supplied a fix:
https://www.postgresql.org/message-id/flat/7862e882-8b9a-0c8e-4a38-40ad374d3634%40brandwatch.com
though in my testing it only fixes LIKE, not CREATE TABLE ... AS. This
report from April of this year also complains about LIKE:
https://www.postgresql.org/message-id/flat/ZD%2B14YZ4IUue8Rhi%40gendo.asyd.net
Here is the output from master for LIKE:
CREATE TABLE test2 (LIKE test);
SELECT attndims
FROM pg_attribute
WHERE attrelid = 'test2'::regclass AND
attname = 'data_array';
attndims
----------
--> 0
and this is the output for CREATE TABLE ... AS:
CREATE TABLE test3 AS SELECT * FROM test;
SELECT attndims
FROM pg_attribute
WHERE attrelid = 'test3'::regclass AND
attname = 'data_array';
attndims
----------
--> 0
The attached patch fixes pg_dump:
$ pg_dump test
...
CREATE TABLE public.test2 (
data integer,
--> data_array integer[][]
);
It uses repeat() at the SQL level rather then modifying format_type() at
the SQL or C level. It seems format_type() is mostly used to get the
type name, e.g. int4[], rather than the column definition so I added
brackets at the call site. I used a similar fix for psql output:
\d test
Table "public.test"
Column | Type | Collation | Nullable | Default
------------+-------------+-----------+----------+---------
data | integer | | |
--> data_array | integer[][] | | |
The 2018 patch from Alexey Bashtanov fixes the LIKE case:
CREATE TABLE test2 (LIKE test);
\d test2
Table "public.test2"
Column | Type | Collation | Nullable | Default
------------+-------------+-----------+----------+---------
data | integer | | |
--> data_array | integer[][] | | |
It does not fix CREATE TABLE ... AS because it looks like fixing that
would require adding an ndims column to Var for WITH NO DATA and adding
ndims to TupleDesc for WITH DATA. I am not sure if that overhead is
warrented to fix this item. I have added C comments where they should
be added.
I would like to apply this patch to master because I think our current
deficiencies in this area are unacceptable. An alternate approach would
be to remove pg_attribute.attndims so we don't even try to preserve
dimensionality.
--
Bruce Momjian <br...@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index e91920ca14..56ea7f4eca 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -185,9 +185,13 @@ create_ctas_nodata(List *tlist, IntoClause *into)
else
colname = tle->resname;
+ /*
+ * Var doesn't have an ndims structure member, so we don't
+ * have access to the original attndims, equals 0.
+ */
col = makeColumnDef(colname,
exprType((Node *) tle->expr),
- exprTypmod((Node *) tle->expr),
+ exprTypmod((Node *) tle->expr), 0,
exprCollation((Node *) tle->expr));
/*
@@ -494,9 +498,15 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
else
colname = NameStr(attribute->attname);
+ /*
+ * TupleDescData doesn't have an ndims structure member, so
+ * we don't have access to the original attndims;
+ * attribute->attndims equals 0.
+ */
col = makeColumnDef(colname,
attribute->atttypid,
attribute->atttypmod,
+ attribute->attndims,
attribute->attcollation);
/*
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 47acdf5166..f497cfad46 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -177,15 +177,15 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
switch (i)
{
case SEQ_COL_LASTVAL:
- coldef = makeColumnDef("last_value", INT8OID, -1, InvalidOid);
+ coldef = makeColumnDef("last_value", INT8OID, -1, 0, InvalidOid);
value[i - 1] = Int64GetDatumFast(seqdataform.last_value);
break;
case SEQ_COL_LOG:
- coldef = makeColumnDef("log_cnt", INT8OID, -1, InvalidOid);
+ coldef = makeColumnDef("log_cnt", INT8OID, -1, 0, InvalidOid);
value[i - 1] = Int64GetDatum((int64) 0);
break;
case SEQ_COL_CALLED:
- coldef = makeColumnDef("is_called", BOOLOID, -1, InvalidOid);
+ coldef = makeColumnDef("is_called", BOOLOID, -1, 0, InvalidOid);
value[i - 1] = BoolGetDatum(false);
break;
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 323d9bf870..99d406416d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2845,7 +2845,8 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
* No, create a new inherited column
*/
def = makeColumnDef(attributeName, attribute->atttypid,
- attribute->atttypmod, attribute->attcollation);
+ attribute->atttypmod, attribute->attndims,
+ attribute->attcollation);
def->inhcount = 1;
def->is_local = false;
/* mark attnotnull if parent has it and it's not NO INHERIT */
diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
index 9bd77546b9..f99b9e003f 100644
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -69,7 +69,7 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
{
ColumnDef *def = makeColumnDef(tle->resname,
exprType((Node *) tle->expr),
- exprTypmod((Node *) tle->expr),
+ exprTypmod((Node *) tle->expr), 0,
exprCollation((Node *) tle->expr));
/*
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index c6fb571982..32f819d84b 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -479,6 +479,24 @@ makeTypeNameFromOid(Oid typeOid, int32 typmod)
n->typeOid = typeOid;
n->typemod = typmod;
n->location = -1;
+
+ return n;
+}
+
+/*
+ * makeTypeNameFromOid -
+ * build a TypeName node to represent a type already known by OID/typmod/ndims
+ */
+TypeName *
+makeTypeNameWithNdimsFromOid(Oid typeOid, int32 typmod, int16 ndims)
+{
+ int i;
+ TypeName *n = makeTypeNameFromOid(typeOid, typmod);
+
+ n->arrayBounds = NIL;
+ for (i = 0; i < ndims; i++)
+ n->arrayBounds = lcons_int(-1, n->arrayBounds);
+
return n;
}
@@ -490,12 +508,13 @@ makeTypeNameFromOid(Oid typeOid, int32 typmod)
* Other properties are all basic to start with.
*/
ColumnDef *
-makeColumnDef(const char *colname, Oid typeOid, int32 typmod, Oid collOid)
+makeColumnDef(const char *colname, Oid typeOid, int32 typmod, int16 ndims,
+ Oid collOid)
{
ColumnDef *n = makeNode(ColumnDef);
n->colname = pstrdup(colname);
- n->typeName = makeTypeNameFromOid(typeOid, typmod);
+ n->typeName = makeTypeNameWithNdimsFromOid(typeOid, typmod, ndims);
n->inhcount = 0;
n->is_local = true;
n->is_not_null = false;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index cf0d432ab1..43f3b358eb 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1078,7 +1078,8 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
* Create a new column definition
*/
def = makeColumnDef(NameStr(attribute->attname), attribute->atttypid,
- attribute->atttypmod, attribute->attcollation);
+ attribute->atttypmod, attribute->attndims,
+ attribute->attcollation);
/*
* For constraints, ONLY the not-null constraint is inherited by the
@@ -1615,7 +1616,8 @@ transformOfType(CreateStmtContext *cxt, TypeName *ofTypename)
continue;
n = makeColumnDef(NameStr(attr->attname), attr->atttypid,
- attr->atttypmod, attr->attcollation);
+ attr->atttypmod, attr->attndims,
+ attr->attcollation);
n->is_from_type = true;
cxt->columns = lappend(cxt->columns, n);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 34fd0a86e9..dd571e250b 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8471,7 +8471,12 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
"a.attlen,\n"
"a.attalign,\n"
"a.attislocal,\n"
- "pg_catalog.format_type(t.oid, a.atttypmod) AS atttypname,\n"
+ "(pg_catalog.format_type(t.oid, a.atttypmod) || "
+ /*
+ * format_type() supplies one "[]" for arrays,
+ * and we add if needed.
+ */
+ " repeat('[]', a.attndims - 1)) AS atttypname,\n"
"array_to_string(a.attoptions, ', ') AS attoptions,\n"
"CASE WHEN a.attcollation <> t.typcollation "
"THEN a.attcollation ELSE 0 END AS attcollation,\n"
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 5077e7b358..9eda1d72a4 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1848,7 +1848,8 @@ describeOneTableDetails(const char *schemaname,
cols = 0;
printfPQExpBuffer(&buf, "SELECT a.attname");
attname_col = cols++;
- appendPQExpBufferStr(&buf, ",\n pg_catalog.format_type(a.atttypid, a.atttypmod)");
+ /* format_type() supplies one "[]" for arrays, and we add if needed. */
+ appendPQExpBufferStr(&buf, ",\n pg_catalog.format_type(a.atttypid, a.atttypmod) || repeat('[]', a.attndims - 1)");
atttype_col = cols++;
if (show_column_details)
diff --git a/src/include/nodes/makefuncs.h b/src/include/nodes/makefuncs.h
index 3180703005..f723edb38d 100644
--- a/src/include/nodes/makefuncs.h
+++ b/src/include/nodes/makefuncs.h
@@ -72,9 +72,10 @@ extern RangeVar *makeRangeVar(char *schemaname, char *relname, int location);
extern TypeName *makeTypeName(char *typnam);
extern TypeName *makeTypeNameFromNameList(List *names);
extern TypeName *makeTypeNameFromOid(Oid typeOid, int32 typmod);
+extern TypeName *makeTypeNameWithNdimsFromOid(Oid typeOid, int32 typmod, int16 ndims);
-extern ColumnDef *makeColumnDef(const char *colname,
- Oid typeOid, int32 typmod, Oid collOid);
+extern ColumnDef *makeColumnDef(const char *colname, Oid typeOid,
+ int32 typmod, int16 ndims, Oid collOid);
extern FuncExpr *makeFuncExpr(Oid funcid, Oid rettype, List *args,
Oid funccollid, Oid inputcollid, CoercionForm fformat);
