public inbox for [email protected]
help / color / mirror / Atom feedFrom: Bruce Momjian <[email protected]>
To: Bruno Bonfils <[email protected]>
Cc: [email protected]
Subject: Re: About #13489, array dimensions and CREATE TABLE ... LIKE
Date: Fri, 8 Sep 2023 17:10:51 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
On Wed, Apr 19, 2023 at 11:35:29AM +0200, Bruno Bonfils wrote:
> Hello there,
>
> A few years ago, someone reported a bug (#13489) about attndims, which
> returned a false value on an array on a table created by CREATE TABLE
> <cloned_table> (LIKE <original_table> INCLUDING ALL),
>
> example:
>
> CREATE TABLE test (data integer, data_array integer[];
> CREATE TABLE test_clone (LIKE test INCLUDING ALL);
>
> SELECT attndims FROM pg_attribute WHERE attrelid = 'test'::regclass AND
> attname = 'data_array';
>
> returns 1
>
> but
>
> SELECT attndims FROM pg_attribute WHERE attrelid = 'test_clone'::regclass AND
> attname = 'data_array';
>
> returns 0
>
> However, according to the documentation https://www.postgresql.org/docs/15/catalog-pg-attribute.html,
> since data_array is an array I expected the returned value should be
> greater than 0
I did a lot of research on this and found out a few things. First,
CREATE TABLE is a complex command that gets its column names, types,
type modifiers, and array dimensions from a a variety of places:
* Specified literally
* Gotten from LIKE
* Gotten from queries
What you found is that we don't pass the array dimensions properly with
LIKE. As the code is written, it can only get dimensions that are
literally specified in the query. What I was able to do in the attached
patch is to pass the array dimensions to the ColumnDef structure, which
is picked up by LIKE, and optionally use that if no dimensions are
specified in the query.
I am not sure how I feel about the patch. We don't seem to record array
dimensionality well --- we don't record the dimension constants and we
don't enforce the dimensionality either, and psql doesn't even show the
dimensionality we do record in pg_attribute, which looks like another
bug. (I think the SQL function format_type() would need to pass in the
array dimensionality to fix this):
CREATE TABLE test (data integer, data_array integer[5][5]);
CREATE TABLE test_clone (LIKE test INCLUDING ALL);
SELECT attndims FROM pg_attribute WHERE attrelid = 'test'::regclass AND
attname = 'data_array';
attndims
----------
2
SELECT attndims FROM pg_attribute WHERE attrelid = 'test_clone'::regclass AND
attname = 'data_array';
attndims
----------
--> 2
INSERT INTO test VALUES (1, '{1}');
INSERT INTO test VALUES (1, '{{1},{2}}');
INSERT INTO test VALUES (1, '{{1},{2},{3}}');
\d test
Table "public.test"
Column | Type | Collation | Nullable | Default
------------+-----------+-----------+----------+---------
data | integer | | |
--> data_array | integer[] | | |
SELECT * FROM test;
data | data_array
------+---------------
--> 1 | {1}
1 | {{1},{2}}
--> 1 | {{1},{2},{3}}
Is it worth applying this patch and improving psql? Are there other
missing pieces that could be easily improved.
However, we already document that array dimensions are for documentation
purposes only, so the fact we don't update pg_attribute, and don't
display the dimensions properly, could be considered acceptable:
https://www.postgresql.org/docs/devel/arrays.html#ARRAYS-DECLARATION
The current implementation does not enforce the declared number of
dimensions either. Arrays of a particular element type are all
considered to be of the same type, regardless of size or number of
dimensions. So, declaring the array size or number of dimensions in
CREATE TABLE is simply documentation; it does not affect run-time
behavior.
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?
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
Attachments:
[text/x-diff] ndims.diff (10.1K, ../[email protected]/2-ndims.diff)
download | inline diff:
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 7c5c390503..57e38a7b8e 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -830,7 +830,11 @@ BuildDescForRelation(List *schema)
aclcheck_error_type(aclresult, atttypid);
attcollation = GetColumnDefCollation(NULL, entry, atttypid);
+
attdim = list_length(entry->typeName->arrayBounds);
+ /* If no bounds were specified, use the ColumnDef value. */
+ if (attdim == 0)
+ attdim = entry->ndims;
if (attdim > PG_INT16_MAX)
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index e91920ca14..39bf62e776 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -188,7 +188,7 @@ create_ctas_nodata(List *tlist, IntoClause *into)
col = makeColumnDef(colname,
exprType((Node *) tle->expr),
exprTypmod((Node *) tle->expr),
- exprCollation((Node *) tle->expr));
+ 0, exprCollation((Node *) tle->expr));
/*
* It's possible that the column is of a collatable type but the
@@ -497,6 +497,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
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 8a2c671b66..13755fa416 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2748,7 +2748,8 @@ MergeAttributes(List *schema, 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 */
@@ -7059,6 +7060,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
errmsg("too many array dimensions"));
attribute.attndims = list_length(colDef->typeName->arrayBounds);
attribute.atttypmod = typmod;
+ attribute.attndims = colDef->ndims;
attribute.attbyval = tform->typbyval;
attribute.attalign = tform->typalign;
if (colDef->storage_name)
diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
index 9bd77546b9..1f87ada072 100644
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -70,7 +70,7 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
ColumnDef *def = makeColumnDef(tle->resname,
exprType((Node *) tle->expr),
exprTypmod((Node *) tle->expr),
- exprCollation((Node *) tle->expr));
+ 0, exprCollation((Node *) tle->expr));
/*
* It's possible that the column is of a collatable type but the
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 0e7e6e46d9..c5e124e068 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -490,12 +490,14 @@ 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->ndims = ndims;
n->inhcount = 0;
n->is_local = true;
n->is_not_null = false;
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 864ea9b0d5..94a4c0d249 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1921,7 +1921,7 @@ addRangeTableEntryForFunction(ParseState *pstate,
attrname,
attrtype,
attrtypmod,
- 0);
+ n->ndims);
TupleDescInitEntryCollation(tupdesc,
(AttrNumber) i,
attrcollation);
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 55c315f0e2..d20ed1a4a5 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1078,7 +1078,9 @@ 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);
+fprintf(stderr, "transformTableLikeClause ndims %d\n", attribute->attndims);
/*
* For constraints, ONLY the not-null constraint is inherited by the
@@ -1624,7 +1626,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/backend/utils/fmgr/funcapi.c b/src/backend/utils/fmgr/funcapi.c
index 24683bb608..03213a5ef7 100644
--- a/src/backend/utils/fmgr/funcapi.c
+++ b/src/backend/utils/fmgr/funcapi.c
@@ -971,7 +971,7 @@ resolve_polymorphic_tupdesc(TupleDesc tupdesc, oidvector *declared_args,
NameStr(att->attname),
poly_actuals.anyelement_type,
-1,
- 0);
+ att->attndims);
TupleDescInitEntryCollation(tupdesc, i + 1, anycollation);
break;
case ANYARRAYOID:
@@ -979,7 +979,7 @@ resolve_polymorphic_tupdesc(TupleDesc tupdesc, oidvector *declared_args,
NameStr(att->attname),
poly_actuals.anyarray_type,
-1,
- 0);
+ att->attndims);
TupleDescInitEntryCollation(tupdesc, i + 1, anycollation);
break;
case ANYRANGEOID:
@@ -987,7 +987,7 @@ resolve_polymorphic_tupdesc(TupleDesc tupdesc, oidvector *declared_args,
NameStr(att->attname),
poly_actuals.anyrange_type,
-1,
- 0);
+ att->attndims);
/* no collation should be attached to a range type */
break;
case ANYMULTIRANGEOID:
@@ -995,7 +995,7 @@ resolve_polymorphic_tupdesc(TupleDesc tupdesc, oidvector *declared_args,
NameStr(att->attname),
poly_actuals.anymultirange_type,
-1,
- 0);
+ att->attndims);
/* no collation should be attached to a multirange type */
break;
case ANYCOMPATIBLEOID:
@@ -1004,7 +1004,7 @@ resolve_polymorphic_tupdesc(TupleDesc tupdesc, oidvector *declared_args,
NameStr(att->attname),
anyc_actuals.anyelement_type,
-1,
- 0);
+ att->attndims);
TupleDescInitEntryCollation(tupdesc, i + 1, anycompatcollation);
break;
case ANYCOMPATIBLEARRAYOID:
@@ -1012,7 +1012,7 @@ resolve_polymorphic_tupdesc(TupleDesc tupdesc, oidvector *declared_args,
NameStr(att->attname),
anyc_actuals.anyarray_type,
-1,
- 0);
+ att->attndims);
TupleDescInitEntryCollation(tupdesc, i + 1, anycompatcollation);
break;
case ANYCOMPATIBLERANGEOID:
@@ -1020,7 +1020,7 @@ resolve_polymorphic_tupdesc(TupleDesc tupdesc, oidvector *declared_args,
NameStr(att->attname),
anyc_actuals.anyrange_type,
-1,
- 0);
+ att->attndims);
/* no collation should be attached to a range type */
break;
case ANYCOMPATIBLEMULTIRANGEOID:
@@ -1028,7 +1028,7 @@ resolve_polymorphic_tupdesc(TupleDesc tupdesc, oidvector *declared_args,
NameStr(att->attname),
anyc_actuals.anymultirange_type,
-1,
- 0);
+ att->attndims);
/* no collation should be attached to a multirange type */
break;
default:
diff --git a/src/include/nodes/makefuncs.h b/src/include/nodes/makefuncs.h
index 3180703005..27f2a2f42a 100644
--- a/src/include/nodes/makefuncs.h
+++ b/src/include/nodes/makefuncs.h
@@ -74,7 +74,7 @@ extern TypeName *makeTypeNameFromNameList(List *names);
extern TypeName *makeTypeNameFromOid(Oid typeOid, int32 typmod);
extern ColumnDef *makeColumnDef(const char *colname,
- Oid typeOid, int32 typmod, Oid collOid);
+ Oid typeOid, int32 typmod, int16 ndims, Oid collOid);
extern FuncExpr *makeFuncExpr(Oid funcid, Oid rettype, List *args,
Oid funccollid, Oid inputcollid, CoercionForm fformat);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index fef4c714b8..7797b19466 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -719,6 +719,7 @@ typedef struct ColumnDef
NodeTag type;
char *colname; /* name of column */
TypeName *typeName; /* type of column */
+ int16 ndims; /* array dimensions */
char *compression; /* compression method for column */
int inhcount; /* number of times column is inherited */
bool is_local; /* column has local (non-inherited) def'n */
view thread (7+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: About #13489, array dimensions and CREATE TABLE ... LIKE
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox