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

On Mon, Aug 11, 2025 at 7:39 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
>
> 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.

Fixed in v18.

>
> 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.
>

Fixed in v18.

> 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?
>

Yeah, the README "3.3.1 Ad-hoc hooks" already says that the plan is to
try to see if we can convert these ad-hoc VCI hooks to instead use IAM
callback methods wherever possible.

> 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)
>

Fixed in v18.

======
Kind Regards,
Peter Smith.
Fujitsu Australia