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

On Mon, Aug 11, 2025 at 7:39 PM Japin Li <japinli@hotmail.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