Re: [WIP]Vertical Clustered Index (columnar store extension) - take2

On Fri, Aug 08, 2025 at 06:08:27PM +1000, Peter Smith wrote:
> Here are the latest v17 patches.
>
> Changes include:
>
> PATCH 0002.
> - Rebase to fix compile error, result of recent master change
> - Bugfix for an unreported EXPLAIN ANALYZE error
> - Bugfix for an unreported double pfree
> - Code cleanup (ran pgindent; corrected ~100 typos; removed empty
> coverage comments)
>

1.
+static struct
+{
+ int transfn_oid; /* Transition function's funcoid. Arrays are
+ * sorted in ascending order */
+ Oid transtype; /* Transition data type */
+ PGFunction merge_trans; /* Function pointer set required for parallel
+ * aggregation for each transfn_oid */
+ vci_aggtranstype_kind kind; /* If transtype is INTERNALOID, its details */
+} trans_funcs_table[] = {
+ {F_FLOAT4_ACCUM, 1022, merge_floatX_accum, VCI_AGG_NOT_INTERNAL}, /* 208 */
+ {F_FLOAT8_ACCUM, 1022, merge_floatX_accum, VCI_AGG_NOT_INTERNAL}, /* 222 */
+ {F_INT8INC, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 1833 */
+ {F_NUMERIC_ACCUM, 2281, numeric_combine, VCI_AGG_NUMERIC_AGG_STATE}, /* 1834 */
+ {F_INT2_ACCUM, 2281, numeric_poly_combine, VCI_AGG_POLY_NUM_AGG_STATE}, /* 1836 */
+ {F_INT4_ACCUM, 2281, numeric_poly_combine, VCI_AGG_POLY_NUM_AGG_STATE}, /* 1835 */
+ {F_INT8_ACCUM, 2281, numeric_combine, VCI_AGG_NUMERIC_AGG_STATE}, /* 1836 */
+ {F_INT2_SUM, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 1840 */
+ {F_INT4_SUM, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 1841 */
+ {F_INTERVAL_AVG_COMBINE, 2281, merge_interval_avg_accum, VCI_AGG_NOT_INTERNAL}, /* 3325 */
+ {F_INT2_AVG_ACCUM, 1016, merge_intX_accum, VCI_AGG_NOT_INTERNAL}, /* 1962 */
+ {F_INT4_AVG_ACCUM, 1016, merge_intX_accum, VCI_AGG_NOT_INTERNAL}, /* 1963 */
+ {F_INT8INC_ANY, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 2804 */
+ {F_INT8_AVG_ACCUM, 2281, int8_avg_combine, VCI_AGG_POLY_AVG_NUM_AGG_STATE}, /* 2746 */
+ {F_NUMERIC_AVG_ACCUM, 2281, numeric_avg_combine, VCI_AGG_AVG_NUMERIC_AGG_STATE}, /* 2858 */
+};

The comments state that this is sorted in ascending order, but the code doesn't
follow that rule. While the current linear search works, a future change to
binary search could cause problems.

2.
+static void
+CheckIndexedRelationKind(Relation rel)
+{
+ char relKind = get_rel_relkind(RelationGetRelid(rel));
+
+ if (relKind == RELKIND_MATVIEW)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("access method \"%s\" does not support index on materialized view", VCI_STRING)));
+
+ if (rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("access method \"%s\" does not support index on temporary table", VCI_STRING)));
+}

Would it be possible to use rel->rd_rel->relkind directly? This might avoid
the overhead of a function call.

3.
The discussion on add_index_delete_hook [1] makes me wonder if an Index Access
Method callback could serve the same purpose. This also raises the question:
would we then need an index update callback as well?

3.
Here are some typos.

a)
@@ -475,7 +477,7 @@ vci_scan_EndCustomPlan(CustomScanState *node)

default:
/* LCOV_EXCL_START */
- elog(PANIC, "Should not reach hare");
+ elog(PANIC, "Should not reach here");
/* LCOV_EXCL_STOP */
break;
}
b)
@@ -543,7 +545,7 @@ vci_create_relation(const char *rel_identifier, Relation indexRel, IndexInfo *in
TupleDescInitEntry(new_tupdesc, (AttrNumber) 1, "bindata", BYTEAOID, -1, 0); /* */
break;

- /* TIC-CRID */
+ /* TID-CRID */
case VCI_RELTYPE_TIDCRID:
natts = 1;
new_tupdesc = CreateTemplateTupleDesc(natts); /* no Oid */

c)
@@ -1065,7 +1065,7 @@ vci_inner_build(Relation heapRel, Relation indexRel, IndexInfo *indexInfo)
/*
* Put or Copy page into INIT_FORK.
* If valid page is given, that page will be putted into INIT_FORK.
- * If Invalid page (NULL pointer) is given, MAIN_FORK page well be copied.
+ * If invalid page (NULL pointer) is given, MAIN_FORK page well be copied.
*/
static void
vci_putInitPage(Oid oid, Page page, BlockNumber blkno)

[1] https://www.postgresql.org/message-id/OS7PR01MB119642862AA1CE536549D08CFEA40A%40OS7PR01MB11964.jpnprd01.prod.outlook.com

--
Best regards,
Japin Li
ChengDu WenWu Information Technology Co., LTD.