Fix crash that partition table has no encoding attributes for new columns (apache#820)

gfphoenix78 · web-flow · commit 976e4c82897d · 2024-12-31T14:11:00.000+08:00
The encoding attributes are not added for new columns added by alter
table. The parent hasn't any information on the new columns. Its
encoding is determined by the storage type, or default encoding.
diff --git a/src/backend/access/aocs/aocsam_handler.c b/src/backend/access/aocs/aocsam_handler.c
@@ -2342,6 +2342,7 @@ aoco_transform_column_encoding_clauses(Relation rel, List *aocoColumnEncoding,
 	bool foundCompressTypeNone = false;
 	char *cmplevel = NULL;
 	bool foundBlockSize = false;
+	bool hasAttrs;
 	char *arg;
 	List *retList = list_copy(aocoColumnEncoding);
 	DefElem *el;
@@ -2352,6 +2353,14 @@ aoco_transform_column_encoding_clauses(Relation rel, List *aocoColumnEncoding,
 	char	   *compresstype = NULL;
 	NameData	compresstype_nd;
 
+	/*
+	 * The relam of partition table may be ao table, but partition table
+	 * has no entry in pg_appendonly. It shouldn't fetch encoding options
+	 * from here for partition tables. See details in function
+	 * transformColumnEncoding.
+	 */
+	hasAttrs = rel && rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE;
+
 	foreach(lc, aocoColumnEncoding)
 	{
 		dl = (DefElem *) lfirst(lc);
@@ -2388,7 +2397,7 @@ aoco_transform_column_encoding_clauses(Relation rel, List *aocoColumnEncoding,
 	 * table setting in pg_appendonly is preferred over default
 	 * options in GUC gp_default_storage_option.
 	 */
-	if (rel)
+	if (hasAttrs)
 	{
 		GetAppendOnlyEntryAttributes(RelationGetRelid(rel),
 									 &blocksize,
@@ -2399,7 +2408,7 @@ aoco_transform_column_encoding_clauses(Relation rel, List *aocoColumnEncoding,
 		compresstype = NameStr(compresstype_nd);
 	}
 
-	if (!foundCompressType && rel && compresstype[0])
+	if (!foundCompressType && hasAttrs && compresstype[0])
 	{
 		el = makeDefElem("compresstype", (Node *) makeString(pstrdup(compresstype)), -1);
 		retList = lappend(retList, el);
diff --git a/src/backend/access/common/reloptions_gp.c b/src/backend/access/common/reloptions_gp.c
@@ -1689,9 +1689,10 @@ transformColumnEncoding(const TableAmRoutine *tam, Relation rel, List *colDefs,
 					encoding = copyObject(deflt->encoding);
 				else if (!explicitOnly)
 				{
-					if (parentenc)
+					ColumnReferenceStorageDirective *parent_col_encoding;
+					parent_col_encoding = find_crsd(d->colname, parentenc);
+					if (parent_col_encoding)
 					{
-						ColumnReferenceStorageDirective *parent_col_encoding = find_crsd(d->colname, parentenc);
 						encoding = transformStorageEncodingClause(parent_col_encoding->encoding, true);
 					}
 					else if (d->typeName) {
@@ -1701,7 +1702,6 @@ transformColumnEncoding(const TableAmRoutine *tam, Relation rel, List *colDefs,
 							encoding = tam->transform_column_encoding_clauses(rel, encoding, true, true);
 					}
 					if (!encoding && createDefaultOne) {
-						Assert(tam == GetTableAmRoutineByAmId(rel->rd_rel->relam));
 						encoding = default_column_encoding_clause(rel);
 					}
 				}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
@@ -8579,7 +8579,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	add_column_datatype_dependency(myrelid, newattnum, attribute.atttypid);
 	add_column_collation_dependency(myrelid, newattnum, attribute.attcollation);
 
-	if (OidIsValid(rel->rd_rel->relam) && rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+	if (OidIsValid(rel->rd_rel->relam))
 	{
 		List *enc;
 		const TableAmRoutine *tam = GetTableAmRoutineByAmId(rel->rd_rel->relam);
diff --git a/src/backend/gpopt/translate/CTranslatorDXLToPlStmt.cpp b/src/backend/gpopt/translate/CTranslatorDXLToPlStmt.cpp
@@ -4042,7 +4042,7 @@ CTranslatorDXLToPlStmt::TranslateDXLSequence(
 Plan *
 CTranslatorDXLToPlStmt::TranslateDXLDynTblScan(
 	const CDXLNode *dyn_tbl_scan_dxlnode, CDXLTranslateContext *output_context,
-	CDXLTranslationContextArray *ctxt_translation_prev_siblings)
+	CDXLTranslationContextArray * /*ctxt_translation_prev_siblings*/)
 {
 	// translate table descriptor into a range table entry
 	CDXLPhysicalDynamicTableScan *dyn_tbl_scan_dxlop =
diff --git a/src/test/regress/expected/uao_ddl/gp_partition_tables_column.out b/src/test/regress/expected/uao_ddl/gp_partition_tables_column.out
@@ -177,5 +177,58 @@ select relname, relkind, amname, reloptions from pg_class c left join pg_am am o
  pt2_ao_column_1_prt_11 | r       | ao_column | 
 (1 row)
 
+create table gppt_ao_column.alter_part_tab1 (id SERIAL, a1 int, a2 char(5), a3 text)
+  WITH (appendonly=true, orientation=column, compresstype=zlib)
+  partition by list(a2) subpartition by range(a1)
+  subpartition template (
+    default subpartition subothers,
+    subpartition sp1 start(1) end(9) with(appendonly=true,orientation=column,compresstype=rle_type),
+    subpartition sp2 start(11) end(20) with(appendonly=true,orientation=column,compresstype=zstd))
+   (partition p1 values('val1') , partition p2 values('val2'));
+create index on gppt_ao_column.alter_part_tab1(a1);
+create index on gppt_ao_column.alter_part_tab1 using bitmap(a3);
+alter table gppt_ao_column.alter_part_tab1 add column a4 numeric default 5.5;
+update gppt_ao_column.alter_part_tab1 set a4 = a1 % 2;
+ALTER TABLE gppt_ao_column.alter_part_tab1 ADD partition p31 values(1);
+\d+ gppt_ao_column.alter_part_tab1
+                                                                   Partitioned table "gppt_ao_column.alter_part_tab1"
+ Column |     Type     | Collation | Nullable |                   Default                   | Storage  | Stats target | Compression Type | Compression Level | Block Size | Description 
+--------+--------------+-----------+----------+---------------------------------------------+----------+--------------+------------------+-------------------+------------+-------------
+ id     | integer      |           | not null | nextval('alter_part_tab1_id_seq'::regclass) | plain    |              | zlib             | 1                 | 32768      | 
+ a1     | integer      |           |          |                                             | plain    |              | zlib             | 1                 | 32768      | 
+ a2     | character(5) |           |          |                                             | extended |              | zlib             | 1                 | 32768      | 
+ a3     | text         |           |          |                                             | extended |              | zlib             | 1                 | 32768      | 
+ a4     | numeric      |           |          | 5.5                                         | main     |              | none             | 0                 | 32768      | 
+Partition key: LIST (a2)
+Indexes:
+    "alter_part_tab1_a1_idx" btree (a1)
+    "alter_part_tab1_a3_idx" bitmap (a3)
+Partitions: alter_part_tab1_1_prt_p1 FOR VALUES IN ('val1 '), PARTITIONED,
+            alter_part_tab1_1_prt_p2 FOR VALUES IN ('val2 '), PARTITIONED,
+            alter_part_tab1_1_prt_p31 FOR VALUES IN ('1    '), PARTITIONED
+Distributed by: (id)
+Options: compresstype=zlib
+
+\d+ gppt_ao_column.alter_part_tab1_1_prt_p31
+                                                              Partitioned table "gppt_ao_column.alter_part_tab1_1_prt_p31"
+ Column |     Type     | Collation | Nullable |                   Default                   | Storage  | Stats target | Compression Type | Compression Level | Block Size | Description 
+--------+--------------+-----------+----------+---------------------------------------------+----------+--------------+------------------+-------------------+------------+-------------
+ id     | integer      |           | not null | nextval('alter_part_tab1_id_seq'::regclass) | plain    |              | zlib             | 1                 | 32768      | 
+ a1     | integer      |           |          |                                             | plain    |              | zlib             | 1                 | 32768      | 
+ a2     | character(5) |           |          |                                             | extended |              | zlib             | 1                 | 32768      | 
+ a3     | text         |           |          |                                             | extended |              | zlib             | 1                 | 32768      | 
+ a4     | numeric      |           |          | 5.5                                         | main     |              | none             | 0                 | 32768      | 
+Partition of: alter_part_tab1 FOR VALUES IN ('1    ')
+Partition constraint: ((a2 IS NOT NULL) AND (a2 = '1    '::character(5)))
+Partition key: RANGE (a1)
+Indexes:
+    "alter_part_tab1_1_prt_p31_a1_idx" btree (a1)
+    "alter_part_tab1_1_prt_p31_a3_idx" bitmap (a3)
+Partitions: alter_part_tab1_1_prt_p31_2_prt_sp1 FOR VALUES FROM (1) TO (9),
+            alter_part_tab1_1_prt_p31_2_prt_sp2 FOR VALUES FROM (11) TO (20),
+            alter_part_tab1_1_prt_p31_2_prt_subothers DEFAULT
+Distributed by: (id)
+Options: compresstype=zlib
+
 reset default_table_access_method;
 reset search_path;
diff --git a/src/test/regress/greenplum_schedule b/src/test/regress/greenplum_schedule
@@ -230,7 +230,7 @@ test: uao_ddl/alter_ao_table_setstorage_column uao_ddl/alter_ao_table_col_ddl_ro
 test: uao_ddl/alter_ao_part_exch_column uao_ddl/alter_ao_part_tables_row uao_ddl/create_ao_table_500cols_column uao_ddl/alter_ao_part_tables_column
 
 test: uao_ddl/alter_drop_allcol_row uao_ddl/alter_drop_allcol_column uao_ddl/alter_rollback_row uao_ddl/alter_rollback_column uao_ddl/uao_allalter_row uao_ddl/uao_allalter_column
-
+test: uao_ddl/gp_partition_tables_column uao_ddl/gp_partition_tables_row
 # These tests use gp_select_invisible and VACUUM, and will get confused if there are
 # concurrent transactions holding back the global xmin.
 
diff --git a/src/test/regress/sql/uao_ddl/gp_partition_tables_column.sql b/src/test/regress/sql/uao_ddl/gp_partition_tables_column.sql
@@ -58,5 +58,23 @@ ALTER TABLE gppt_ao_column.pt2_ao_column ADD PARTITION START ('2027-01-01') INCL
 \d+ gppt_ao_column.pt2_ao_column_1_prt_11
 select relname, relkind, amname, reloptions from pg_class c left join pg_am am on c.relam=am.oid where relname='pt2_ao_column_1_prt_11';
 
+create table gppt_ao_column.alter_part_tab1 (id SERIAL, a1 int, a2 char(5), a3 text)
+  WITH (appendonly=true, orientation=column, compresstype=zlib)
+  partition by list(a2) subpartition by range(a1)
+  subpartition template (
+    default subpartition subothers,
+    subpartition sp1 start(1) end(9) with(appendonly=true,orientation=column,compresstype=rle_type),
+    subpartition sp2 start(11) end(20) with(appendonly=true,orientation=column,compresstype=zstd))
+   (partition p1 values('val1') , partition p2 values('val2'));
+
+create index on gppt_ao_column.alter_part_tab1(a1);
+create index on gppt_ao_column.alter_part_tab1 using bitmap(a3);
+alter table gppt_ao_column.alter_part_tab1 add column a4 numeric default 5.5;
+update gppt_ao_column.alter_part_tab1 set a4 = a1 % 2;
+ALTER TABLE gppt_ao_column.alter_part_tab1 ADD partition p31 values(1);
+\d+ gppt_ao_column.alter_part_tab1
+\d+ gppt_ao_column.alter_part_tab1_1_prt_p31
+
+
 reset default_table_access_method;
 reset search_path;

Original file line number	Diff line number	Diff line change
`@@ -1689,9 +1689,10 @@ transformColumnEncoding(const TableAmRoutine tam, Relation rel, List colDefs,`
`1689`	`1689`	`encoding = copyObject(deflt->encoding);`
`1690`	`1690`	`else if (!explicitOnly)`
`1691`	`1691`	`{`
`1692`		`- if (parentenc)`
	`1692`	`+ ColumnReferenceStorageDirective *parent_col_encoding;`
	`1693`	`+ parent_col_encoding = find_crsd(d->colname, parentenc);`
	`1694`	`+ if (parent_col_encoding)`
`1693`	`1695`	`{`
`1694`		`- ColumnReferenceStorageDirective *parent_col_encoding = find_crsd(d->colname, parentenc);`
`1695`	`1696`	`encoding = transformStorageEncodingClause(parent_col_encoding->encoding, true);`
`1696`	`1697`	`}`
`1697`	`1698`	`else if (d->typeName) {`
`@@ -1701,7 +1702,6 @@ transformColumnEncoding(const TableAmRoutine tam, Relation rel, List colDefs,`
`1701`	`1702`	`encoding = tam->transform_column_encoding_clauses(rel, encoding, true, true);`
`1702`	`1703`	`}`
`1703`	`1704`	`if (!encoding && createDefaultOne) {`
`1704`		`- Assert(tam == GetTableAmRoutineByAmId(rel->rd_rel->relam));`
`1705`	`1705`	`encoding = default_column_encoding_clause(rel);`
`1706`	`1706`	`}`
`1707`	`1707`	`}`
Original file line number	Diff line number	Diff line change
`@@ -8579,7 +8579,7 @@ ATExecAddColumn(List *wqueue, AlteredTableInfo tab, Relation rel,`
`8579`	`8579`	`add_column_datatype_dependency(myrelid, newattnum, attribute.atttypid);`
`8580`	`8580`	`add_column_collation_dependency(myrelid, newattnum, attribute.attcollation);`
`8581`	`8581`
`8582`		`- if (OidIsValid(rel->rd_rel->relam) && rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)`
	`8582`	`+ if (OidIsValid(rel->rd_rel->relam))`
`8583`	`8583`	`{`
`8584`	`8584`	`List *enc;`
`8585`	`8585`	`const TableAmRoutine *tam = GetTableAmRoutineByAmId(rel->rd_rel->relam);`
Original file line number	Diff line number	Diff line change
`@@ -4042,7 +4042,7 @@ CTranslatorDXLToPlStmt::TranslateDXLSequence(`
`4042`	`4042`	`Plan *`
`4043`	`4043`	`CTranslatorDXLToPlStmt::TranslateDXLDynTblScan(`
`4044`	`4044`	`const CDXLNode dyn_tbl_scan_dxlnode, CDXLTranslateContext output_context,`
`4045`		`- CDXLTranslationContextArray *ctxt_translation_prev_siblings)`
	`4045`	`+ CDXLTranslationContextArray * /ctxt_translation_prev_siblings/)`
`4046`	`4046`	`{`
`4047`	`4047`	`// translate table descriptor into a range table entry`
`4048`	`4048`	`CDXLPhysicalDynamicTableScan *dyn_tbl_scan_dxlop =`