From ba52100f4b5e5b16215ee26bb1a7a549f50362df Mon Sep 17 00:00:00 2001 From: wuhao Date: Fri, 17 Mar 2023 11:21:46 +0800 Subject: [PATCH 1/7] Add macro RelationIsNonblockRelation to expand code path like AO/CO The builtin types of tables, in kernel, are standard heap table and AO/CO. The custom table AM will fall one of the two code paths. RelationIsAppendOptimized will only choose the builtin AO/CO relations. Some custom table AM, like PAX, will run the same code path as AO/CO. RelationIsNonblockRelation looks replacable by `!RelationIsHeap`, but they have different meanings. RelationIsNonblockRelation expand the relation type to run the code path like AO/CO. `!RelationIsHeap` emphasizes NOT heap relation. --- src/backend/commands/analyze.c | 5 +++-- src/backend/commands/trigger.c | 4 ++-- src/backend/executor/nodeModifyTable.c | 13 ++++++++----- src/backend/optimizer/util/appendinfo.c | 2 +- src/backend/utils/adt/dbsize.c | 4 ++-- src/include/utils/rel.h | 18 ++++++++++++++++++ 6 files changed, 34 insertions(+), 12 deletions(-) diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index 467818814b2..212fe3a3fc2 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -1621,8 +1621,9 @@ acquire_sample_rows(Relation onerel, int elevel, * GPDB_12_MERGE_FIXME: BlockNumber is uint32 and Number of tuples is uint64. * That means that after row number UINT_MAX we will never analyze the table. */ - if (RelationIsAppendOptimized(onerel)) + if (RelationIsNonblockRelation(onerel)) { + /* AO/CO/PAX use non-fixed block layout */ BlockNumber pages; double tuples; double allvisfrac; @@ -1653,7 +1654,7 @@ acquire_sample_rows(Relation onerel, int elevel, * because Blocks is not same as Heap tables. * Set prefetch_maximum to zero seems the easiest way to bypass. */ - if (RelationIsAppendOptimized(onerel)) + if (RelationIsNonblockRelation(onerel)) { prefetch_maximum = 0; } diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 82b12c3fa97..f60aa299fc0 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -715,7 +715,7 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, } /* Check GPDB limitations */ - if (RelationIsAppendOptimized(rel) && + if (RelationIsNonblockRelation(rel) && TRIGGER_FOR_ROW(tgtype) && !stmt->isconstraint) { @@ -3105,7 +3105,7 @@ GetTupleForTrigger(EState *estate, Relation relation = relinfo->ri_RelationDesc; /* these should be rejected when you try to create such triggers, but let's check */ - if (RelationIsAppendOptimized(relation)) + if (RelationIsNonblockRelation(relation)) elog(ERROR, "UPDATE and DELETE triggers are not supported on append-only tables"); Assert(RelationIsHeap(relation)); diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 8bac299db77..0e76ca1f710 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1463,7 +1463,7 @@ ldelete:; * triggers on an UPDATE that moves tuples from one partition to another. * Should we follow that example with cross-segment UPDATEs too? */ - if (!RelationIsAppendOptimized(resultRelationDesc) && !splitUpdate) + if (!RelationIsNonblockRelation(resultRelationDesc) && !splitUpdate) { ExecARDeleteTriggers(estate, resultRelInfo, tupleid, oldtuple, ar_delete_trig_tcs); @@ -1949,7 +1949,7 @@ lreplace:; * AO case, as visimap update within same command happens at end * of command. */ - if (!RelationIsAppendOptimized(resultRelationDesc) && + if (!RelationIsNonblockRelation(resultRelationDesc) && tmfd.cmax != estate->es_output_cid) ereport(ERROR, (errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION), @@ -2075,7 +2075,7 @@ lreplace:; /* AFTER ROW UPDATE Triggers */ /* GPDB: AO and AOCO tables don't support triggers */ - if (!RelationIsAppendOptimized(resultRelationDesc)) + if (!RelationIsNonblockRelation(resultRelationDesc)) ExecARUpdateTriggers(estate, resultRelInfo, tupleid, oldtuple, slot, recheckIndexes, mtstate->operation == CMD_INSERT ? @@ -2664,8 +2664,11 @@ ExecModifyTable(PlanState *pstate) * We need to fix this problem by redesigning AO/AOCS storage format or * making the update plan is consistent whether it generated by pg optimizer * or ORCA optimizer in the future. + * + * PAX_STORAGE_FIXME(gongxun):we reuse the logic of the AO table to implement ExecUpdate, + * If there is a better implementation, we need to revert it */ - if (operation == CMD_UPDATE && !RelationIsHeap(resultRelInfo->ri_RelationDesc) && + if (operation == CMD_UPDATE && RelationIsNonblockRelation(resultRelInfo->ri_RelationDesc) && AttributeNumberIsValid(resultRelInfo->ri_WholeRowNo)) { /* ri_WholeRowNo refers to a wholerow attribute */ @@ -3137,7 +3140,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) elog(ERROR, "could not find junk ctid column"); /* extra GPDB junk columns for update AO table */ - if (operation == CMD_UPDATE && !RelationIsHeap(resultRelInfo->ri_RelationDesc)) + if (operation == CMD_UPDATE && RelationIsNonblockRelation(resultRelInfo->ri_RelationDesc)) { resultRelInfo->ri_WholeRowNo = ExecFindJunkAttributeInTlist(subplan->targetlist, "wholerow"); diff --git a/src/backend/optimizer/util/appendinfo.c b/src/backend/optimizer/util/appendinfo.c index f44f94ad265..40b49d4d1b3 100644 --- a/src/backend/optimizer/util/appendinfo.c +++ b/src/backend/optimizer/util/appendinfo.c @@ -953,7 +953,7 @@ add_row_identity_columns(PlannerInfo *root, Index rtindex, * redesigning AO/AOCS storage format or making the update plan is * consistent whether it generated by pg optimizer or ORCA optimizer. */ - if (commandType == CMD_UPDATE && !RelationIsHeap(target_relation)) + if (commandType == CMD_UPDATE && RelationIsNonblockRelation(target_relation)) { var = makeVar(rtindex, InvalidAttrNumber, diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c index cc3ad270d4f..3d331afa0b3 100644 --- a/src/backend/utils/adt/dbsize.c +++ b/src/backend/utils/adt/dbsize.c @@ -463,8 +463,8 @@ calculate_relation_size(Relation rel, ForkNumber forknum) if (relation_size_hook) return (*relation_size_hook) (rel, forknum); - /* Call into the tableam api for AO/AOCO relations */ - if (RelationIsAppendOptimized(rel)) + /* Call into the tableam api if the table is not heap, i.e. AO/CO/PAX relations. */ + if (RelationIsNonblockRelation(rel)) return table_relation_size(rel, forknum); relationpath = relpathbackend(rel->rd_node, rel->rd_backend, forknum); diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index 3b2f4ab2922..7536754726e 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -20,6 +20,7 @@ #include "fmgr.h" #include "access/tupdesc.h" #include "access/xlog.h" +#include "catalog/pg_am.h" #include "catalog/pg_appendonly.h" #include "catalog/pg_class.h" #include "catalog/pg_index.h" @@ -516,6 +517,23 @@ typedef struct ViewOptions #define RelationIsAppendOptimized(relation) \ AMHandlerIsAO((relation)->rd_amhandler) +/* + * CAUTION: this macro is a violation of the absraction that table AM and + * index AM interfaces provide. Use of this macro is discouraged. If + * table/index AM API falls short for your use case, consider enhancing the + * interface. + * + * RelationIsNonblockRelation looks replacable by `!RelationIsHeap`, but + * they have different meanings. RelationIsNonblockRelation expand the + * relation type to run the code path like AO/CO. `!RelationIsHeap` + * emphasizes NOT heap relation. + * + * RelationIsNonblockRelation + * True iff relation(table) should run the code path as AO/CO + */ +#define RelationIsNonblockRelation(relation) \ + ((relation)->rd_tableam && (relation)->rd_rel->relam != HEAP_TABLE_AM_OID) + /* * RelationIsBitmapIndex * True iff relation is a bitmap index From 4f24e94dcd6ea63d05902d44e0469f14c1f1ea60 Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Mon, 25 Dec 2023 13:42:42 +0800 Subject: [PATCH 2/7] exclude index or partition table --- src/include/utils/rel.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index 7536754726e..fe70148d408 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -532,7 +532,8 @@ typedef struct ViewOptions * True iff relation(table) should run the code path as AO/CO */ #define RelationIsNonblockRelation(relation) \ - ((relation)->rd_tableam && (relation)->rd_rel->relam != HEAP_TABLE_AM_OID) + ((relation)->rd_rel->relkind == RELKIND_RELATION && \ + (relation)->rd_rel->relam != HEAP_TABLE_AM_OID) /* * RelationIsBitmapIndex From 6fc4419ec20dcafe552f8392dc6e624735eecfa8 Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Wed, 27 Dec 2023 08:32:44 +0800 Subject: [PATCH 3/7] fix test --- src/backend/utils/adt/dbsize.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c index 3d331afa0b3..10f44aa6cc7 100644 --- a/src/backend/utils/adt/dbsize.c +++ b/src/backend/utils/adt/dbsize.c @@ -464,7 +464,7 @@ calculate_relation_size(Relation rel, ForkNumber forknum) return (*relation_size_hook) (rel, forknum); /* Call into the tableam api if the table is not heap, i.e. AO/CO/PAX relations. */ - if (RelationIsNonblockRelation(rel)) + if (RelationIsAppendOptimized(rel)) return table_relation_size(rel, forknum); relationpath = relpathbackend(rel->rd_node, rel->rd_backend, forknum); From 0bb1293b2fb3018fbe04a07e91ca6e76f10c8d8e Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Wed, 27 Dec 2023 08:33:34 +0800 Subject: [PATCH 4/7] fix test --- src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp | 2 +- src/include/utils/rel.h | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp b/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp index bd2baf2bd6e..effd787f186 100644 --- a/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp +++ b/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp @@ -2445,7 +2445,7 @@ CTranslatorRelcacheToDXL::RetrieveRelStorageType(Relation rel) // Why use the magic number 7014 instead of the macro definition? // Just to make it look like it doesn't make sense, // so others will notice that the logic needs to be refactored - case 7014: + case PAX_AM_OID: case AO_COLUMN_TABLE_AM_OID: rel_storage_type = IMDRelation::ErelstorageAppendOnlyCols; break; diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index fe70148d408..e74fb71fa8f 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -517,6 +517,11 @@ typedef struct ViewOptions #define RelationIsAppendOptimized(relation) \ AMHandlerIsAO((relation)->rd_amhandler) +/* + * FIXME: CBDB should not know the am oid of PAX. We put here because the kernel + * can't distinguish the PAX and renamed heap(heap_psql) in test `psql`. + */ +#define PAX_AM_OID 7014 /* * CAUTION: this macro is a violation of the absraction that table AM and * index AM interfaces provide. Use of this macro is discouraged. If @@ -532,8 +537,8 @@ typedef struct ViewOptions * True iff relation(table) should run the code path as AO/CO */ #define RelationIsNonblockRelation(relation) \ - ((relation)->rd_rel->relkind == RELKIND_RELATION && \ - (relation)->rd_rel->relam != HEAP_TABLE_AM_OID) + (RelationIsAppendOptimized(relation) || \ + (relation)->rd_rel->relam == PAX_AM_OID) /* * RelationIsBitmapIndex From 2edd1ce77b676d611b36f665d18e06bf15a1afc7 Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Wed, 27 Dec 2023 09:12:10 +0800 Subject: [PATCH 5/7] fix dbsize --- src/backend/utils/adt/dbsize.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c index 10f44aa6cc7..3d331afa0b3 100644 --- a/src/backend/utils/adt/dbsize.c +++ b/src/backend/utils/adt/dbsize.c @@ -464,7 +464,7 @@ calculate_relation_size(Relation rel, ForkNumber forknum) return (*relation_size_hook) (rel, forknum); /* Call into the tableam api if the table is not heap, i.e. AO/CO/PAX relations. */ - if (RelationIsAppendOptimized(rel)) + if (RelationIsNonblockRelation(rel)) return table_relation_size(rel, forknum); relationpath = relpathbackend(rel->rd_node, rel->rd_backend, forknum); From 810e61bc1a39e06980c6bd48a1fa623d40f1cd0d Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Wed, 27 Dec 2023 10:08:01 +0800 Subject: [PATCH 6/7] preserve oid for pax --- src/include/catalog/duplicate_oids | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/include/catalog/duplicate_oids b/src/include/catalog/duplicate_oids index 10268e9195a..490e5b9c0bd 100755 --- a/src/include/catalog/duplicate_oids +++ b/src/include/catalog/duplicate_oids @@ -32,6 +32,9 @@ my $oids = Catalog::FindAllOidsFromHeaders(@input_files); my %oidcounts; +# preserved oid for PAX table AM +$oidcounts{7014}++; + foreach my $oid (@{$oids}) { $oidcounts{$oid}++; From 7c7855bc9774e7e1a90fe5b52013d0c0ff24972a Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Wed, 27 Dec 2023 10:28:51 +0800 Subject: [PATCH 7/7] change oid to 7047 --- src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp | 6 +++--- src/include/catalog/duplicate_oids | 2 +- src/include/utils/rel.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp b/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp index effd787f186..6db71fa1527 100644 --- a/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp +++ b/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp @@ -2435,14 +2435,14 @@ CTranslatorRelcacheToDXL::RetrieveRelStorageType(Relation rel) break; // FIXME: need to add support for custom table am!!! // - // Why 7014 here? + // Why 7047 here? // Because we defined a custom table am using columnar storage, // the orca optimizer does not support am other than HEAP/AO/AOCS. At present, // there is no way to extend orca to support custom table am. So here we use - // the am_id(7014) we assigned to the custom table am, and let the orca optimizer + // the am_id(7047) we assigned to the custom table am, and let the orca optimizer // treat this columnar storage format as AOCS to generate an execution plan // - // Why use the magic number 7014 instead of the macro definition? + // Why use the magic number 7047 instead of the macro definition? // Just to make it look like it doesn't make sense, // so others will notice that the logic needs to be refactored case PAX_AM_OID: diff --git a/src/include/catalog/duplicate_oids b/src/include/catalog/duplicate_oids index 490e5b9c0bd..b4e4c064a26 100755 --- a/src/include/catalog/duplicate_oids +++ b/src/include/catalog/duplicate_oids @@ -33,7 +33,7 @@ my $oids = Catalog::FindAllOidsFromHeaders(@input_files); my %oidcounts; # preserved oid for PAX table AM -$oidcounts{7014}++; +$oidcounts{7047}++; foreach my $oid (@{$oids}) { diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index e74fb71fa8f..4df458425eb 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -521,7 +521,7 @@ typedef struct ViewOptions * FIXME: CBDB should not know the am oid of PAX. We put here because the kernel * can't distinguish the PAX and renamed heap(heap_psql) in test `psql`. */ -#define PAX_AM_OID 7014 +#define PAX_AM_OID 7047 /* * CAUTION: this macro is a violation of the absraction that table AM and * index AM interfaces provide. Use of this macro is discouraged. If