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.
diff --git a/contrib/vci/executor/vci_aggmergetranstype.c b/contrib/vci/executor/vci_aggmergetranstype.c
index 4c136e8660a..a516e59c310 100644
--- a/contrib/vci/executor/vci_aggmergetranstype.c
+++ b/contrib/vci/executor/vci_aggmergetranstype.c
@@ -8,6 +8,8 @@
*
* IDENTIFICATION
* contrib/vci/executor/vci_aggmergetranstype.c
+ *
+ *-------------------------------------------------------------------------
*/
#include "postgres.h"
@@ -74,19 +76,19 @@ static struct
} 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_INT8INC, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 1219 */
+ {F_NUMERIC_ACCUM, 2281, numeric_combine, VCI_AGG_NUMERIC_AGG_STATE}, /* 1833 */
+ {F_INT2_ACCUM, 2281, numeric_poly_combine, VCI_AGG_POLY_NUM_AGG_STATE}, /* 1834 */
{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_INT8INC_ANY, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 2804 */
{F_NUMERIC_AVG_ACCUM, 2281, numeric_avg_combine, VCI_AGG_AVG_NUMERIC_AGG_STATE}, /* 2858 */
+ {F_INTERVAL_AVG_COMBINE, 2281, merge_interval_avg_accum, VCI_AGG_NOT_INTERNAL}, /* 3325 */
};
/**
diff --git a/contrib/vci/executor/vci_scan.c b/contrib/vci/executor/vci_scan.c
index 5f9d32b0ec8..c43b1ada36a 100644
--- a/contrib/vci/executor/vci_scan.c
+++ b/contrib/vci/executor/vci_scan.c
@@ -1,13 +1,15 @@
/*-------------------------------------------------------------------------
-*
-* vci_scan.c
-* Routines to handle VCI Scan nodes
-*
+ *
+ * vci_scan.c
+ * Routines to handle VCI Scan nodes
+ *
* Portions Copyright (c) 2025, PostgreSQL Global Development Group
*
* IDENTIFICATION
* contrib/vci/executor/vci_scan.c
-*/
+ *
+ *-------------------------------------------------------------------------
+ */
#include "postgres.h"
#include "access/relscan.h"
@@ -113,16 +115,16 @@ vci_scan_BeginCustomPlan(CustomScanState *node, EState *estate, int eflags)
break;
}
-/*
- * create state structure
- */
+ /*
+ * create state structure
+ */
scanstate->is_subextent_grain = scan->is_subextent_grain;
scanstate->vci.css.ss.ps.state = estate;
-/* create expression context for node */
+ /* create expression context for node */
ExecAssignExprContext(estate, &scanstate->vci.css.ss.ps);
-/* initialize child expressions */
+ /* initialize child expressions */
scanstate->vci.css.ss.ps.qual =
VciExecInitQual(scan->vci.cscan.scan.plan.qual, &scanstate->vci.css.ss.ps,
initexpr);
@@ -161,11 +163,11 @@ vci_scan_BeginCustomPlan(CustomScanState *node, EState *estate, int eflags)
break;
}
-/* tuple table initialization */
+ /* tuple table initialization */
ExecInitScanTupleSlot(estate, &scanstate->vci.css.ss, scanDesc, &TTSOpsMinimalTuple);
ExecInitResultTupleSlotTL(&scanstate->vci.css.ss.ps, &TTSOpsMinimalTuple);
-/* ExecAssignScanProjectionInfo() ???? */
+ /* ExecAssignScanProjectionInfo() ???? */
if (scan->scan_mode == VCI_SCAN_MODE_COLUMN_STORE)
{
vci_scan_BeginCustomPlan_postprocess_enabling_vp(scan, scanstate);
@@ -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;
}
diff --git a/contrib/vci/storage/vci_index.c b/contrib/vci/storage/vci_index.c
index 80511803ecc..ea213d2b17a 100644
--- a/contrib/vci/storage/vci_index.c
+++ b/contrib/vci/storage/vci_index.c
@@ -7,6 +7,8 @@
*
* IDENTIFICATION
* contrib/vci/storage/vci_index.c
+ *
+ *-------------------------------------------------------------------------
*/
#include "postgres.h"
@@ -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 */
@@ -671,9 +673,7 @@ GenRelName(Relation rel, int16 columnId, char suffix)
static void
CheckIndexedRelationKind(Relation rel)
{
- char relKind = get_rel_relkind(RelationGetRelid(rel));
-
- if (relKind == RELKIND_MATVIEW)
+ if (rel->rd_rel->relkind == RELKIND_MATVIEW)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("access method \"%s\" does not support index on materialized view", VCI_STRING)));
@@ -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)
