public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 06/17] avoid shadow vars: basebackup_target.c: ttype 7+ messages / 4 participants [nested] [flat]
* [PATCH 06/17] avoid shadow vars: basebackup_target.c: ttype @ 2022-08-16 23:51 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Justin Pryzby @ 2022-08-16 23:51 UTC (permalink / raw) backpatch to v15 commit e4ba69f3f4a1b997aa493cc02e563a91c0f35b87 Author: Robert Haas <[email protected]> Date: Tue Mar 15 13:22:04 2022 -0400 Allow extensions to add new backup targets. --- src/backend/backup/basebackup_target.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/backup/basebackup_target.c b/src/backend/backup/basebackup_target.c index 83928e32055..8d10fe15530 100644 --- a/src/backend/backup/basebackup_target.c +++ b/src/backend/backup/basebackup_target.c @@ -73,7 +73,7 @@ BaseBackupAddTarget(char *name, /* Search the target type list for an existing entry with this name. */ foreach(lc, BaseBackupTargetTypeList) { - BaseBackupTargetType *ttype = lfirst(lc); + ttype = lfirst(lc); if (strcmp(ttype->name, name) == 0) { -- 2.17.1 --uTRFFR9qmiCqR05s Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0007-avoid-shadow-vars-parse_jsontable.c-jtc.patch" ^ permalink raw reply [nested|flat] 7+ messages in thread
* About #13489 @ 2023-04-19 09:35 Bruno Bonfils <[email protected]> 2023-09-08 21:10 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Bruno Bonfils @ 2023-04-19 09:35 UTC (permalink / raw) To: [email protected] 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 Thanks (tested on PostgreSQL 15.2 (Debian 15.2-1.pgdg110+1)) ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: About #13489, array dimensions and CREATE TABLE ... LIKE 2023-04-19 09:35 About #13489 Bruno Bonfils <[email protected]> @ 2023-09-08 21:10 ` Bruce Momjian <[email protected]> 2023-11-21 01:33 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Bruce Momjian @ 2023-09-08 21:10 UTC (permalink / raw) To: Bruno Bonfils <[email protected]>; +Cc: [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 */ ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: About #13489, array dimensions and CREATE TABLE ... LIKE 2023-04-19 09:35 About #13489 Bruno Bonfils <[email protected]> 2023-09-08 21:10 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> @ 2023-11-21 01:33 ` Bruce Momjian <[email protected]> 2023-11-21 02:04 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Tom Lane <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Bruce Momjian @ 2023-11-21 01:33 UTC (permalink / raw) To: Bruno Bonfils <[email protected]>; +Cc: pgsql-hackers; [email protected]; Alexey Bashtanov <[email protected]>; Bruno Bonfils <[email protected]> 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 <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. Attachments: [text/x-diff] ndims2.diff (7.7K, ../../[email protected]/2-ndims2.diff) download | inline diff: 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); ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: About #13489, array dimensions and CREATE TABLE ... LIKE 2023-04-19 09:35 About #13489 Bruno Bonfils <[email protected]> 2023-09-08 21:10 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> 2023-11-21 01:33 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> @ 2023-11-21 02:04 ` Tom Lane <[email protected]> 2023-11-21 02:07 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Tom Lane @ 2023-11-21 02:04 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: Bruno Bonfils <[email protected]>; pgsql-hackers; [email protected]; Alexey Bashtanov <[email protected]> Bruce Momjian <[email protected]> writes: > I would like to apply this patch to master because I think our current > deficiencies in this area are unacceptable. I do not think this is a particularly good idea, because it creates the impression in a couple of places that we track this data, when we do not really do so to any meaningful extent. > An alternate approach would > be to remove pg_attribute.attndims so we don't even try to preserve > dimensionality. I could get behind that, perhaps. It looks like we're not using the field in any meaningful way, and we could simplify TupleDescInitEntry and perhaps some other APIs. regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: About #13489, array dimensions and CREATE TABLE ... LIKE 2023-04-19 09:35 About #13489 Bruno Bonfils <[email protected]> 2023-09-08 21:10 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> 2023-11-21 01:33 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> 2023-11-21 02:04 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Tom Lane <[email protected]> @ 2023-11-21 02:07 ` Bruce Momjian <[email protected]> 2023-11-21 02:13 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Tom Lane <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Bruce Momjian @ 2023-11-21 02:07 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Bruno Bonfils <[email protected]>; pgsql-hackers; [email protected]; Alexey Bashtanov <[email protected]> On Mon, Nov 20, 2023 at 09:04:21PM -0500, Tom Lane wrote: > Bruce Momjian <[email protected]> writes: > > I would like to apply this patch to master because I think our current > > deficiencies in this area are unacceptable. > > I do not think this is a particularly good idea, because it creates > the impression in a couple of places that we track this data, when > we do not really do so to any meaningful extent. Okay, I thought we could get by without tracking the CREATE TABLE AS case, but it is inconsistent. My patch just makes it less inconsistent. > > An alternate approach would > > be to remove pg_attribute.attndims so we don't even try to preserve > > dimensionality. > > I could get behind that, perhaps. It looks like we're not using the > field in any meaningful way, and we could simplify TupleDescInitEntry > and perhaps some other APIs. So should I work on that patch or do you want to try? I think we should do something. -- Bruce Momjian <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: About #13489, array dimensions and CREATE TABLE ... LIKE 2023-04-19 09:35 About #13489 Bruno Bonfils <[email protected]> 2023-09-08 21:10 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> 2023-11-21 01:33 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> 2023-11-21 02:04 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Tom Lane <[email protected]> 2023-11-21 02:07 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> @ 2023-11-21 02:13 ` Tom Lane <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2023-11-21 02:13 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: Bruno Bonfils <[email protected]>; pgsql-hackers; Alexey Bashtanov <[email protected]> Bruce Momjian <[email protected]> writes: > On Mon, Nov 20, 2023 at 09:04:21PM -0500, Tom Lane wrote: >> Bruce Momjian <[email protected]> writes: >>> An alternate approach would >>> be to remove pg_attribute.attndims so we don't even try to preserve >>> dimensionality. >> I could get behind that, perhaps. It looks like we're not using the >> field in any meaningful way, and we could simplify TupleDescInitEntry >> and perhaps some other APIs. > So should I work on that patch or do you want to try? I think we should > do something. Let's wait for some other opinions, first ... regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2023-11-21 02:13 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-08-16 23:51 [PATCH 06/17] avoid shadow vars: basebackup_target.c: ttype Justin Pryzby <[email protected]> 2023-04-19 09:35 About #13489 Bruno Bonfils <[email protected]> 2023-09-08 21:10 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> 2023-11-21 01:33 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> 2023-11-21 02:04 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Tom Lane <[email protected]> 2023-11-21 02:07 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Bruce Momjian <[email protected]> 2023-11-21 02:13 ` Re: About #13489, array dimensions and CREATE TABLE ... LIKE Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox