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

On Mon, Aug 11, 2025 at 05:39:01PM +0800, Japin Li 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)
> >
> 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.
PFA the other typos.

2.
I found it skip vci query context initialization in vci_intialize_query_context()
if full page writes is disabled, Could you explain why we need full page write
enabled for VCI?

3.
Both vci_ros.h and vci_ros.c have a comment about accessing the VCI main relation
header, but they are slightly different. Could we sync them and keep only one?

It seems the comment is outdated, as the functions vci_KeepReadingMainRelHeader()
and vci_KeepWritingMainRelHeader() do not exist in the current VCI implementation.

4.
+/**
+ * This function is assumed when the VCI index is newly built, and
+ * it converts all the data in the relation of PostgreSQL into ROS.
+ */
+uint64
+vci_ConvertWos2RosForBuild(Relation mainRel,
+ Size workareaSize,
+ IndexInfo *indexInfo)
...
+ result = (uint64) table_index_build_scan(comContext.heapRel,
+ mainRel,
+ indexInfo,
+ true, /* allow syncscan */
+ true,
+ vci_build_callback,
+ (void *) &comContext, NULL)

Perhaps we can use a double return type to avoid type casting since
other places also use double.

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