Re: [WIP]Vertical Clustered Index (columnar store extension) - take2 - Mailing list pgsql-hackers

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.