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

On Fri, 11 Jul 2025 at 16:31, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> Hi. Here is the latest patch set v12*
>
> Main differences are:
>
> Patch 0001 (core)
> - removed SizeOfIptrData macro, as reported by Tomas [1] and Japin [2]
>
> Patch 0002 (vci module)
> - Made fixes so the "ROS Control Worker" (for background WOS->ROS
> transfer) can now launch ok.
>

Hi, Peter

1.
I'm curious if you encountered the following warning during compilation:

/home/japin/Codes/pg/master/build/../contrib/vci/include/vci_ros.h:745:9: warning: result of comparison of constant 65536 with expression of type 'OffsetNumber' (aka 'unsigned short') is always true [-Wtautological-constant-out-of-range-c
ompare]
745 | return vci_MakeUint64FromBlockNumberAndOffset(blockNumber, item->ip_posid);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/japin/Codes/pg/master/build/../contrib/vci/include/vci_ros.h:639:19: note: expanded from macro 'vci_MakeUint64FromBlockNumberAndOffset'
638 | (AssertMacro((0 <= (offset)) \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
639 | && ((offset) < (1U << (BITS_PER_BYTE * sizeof(OffsetNumber))))), \
| ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/japin/Codes/pg/master/build/../src/include/c.h:868:12: note: expanded from macro 'AssertMacro'
868 | ((void) ((condition) || \
| ^~~~~~~~~
1 warning generated.

Since the offset is unsigned, we can infer it's always non-negative. Did I miss
anything?

2.
I've noticed that InitPageCoreWithoutLock() consistently requires a lock.
Given this, I propose merging it with vci_InitPageCore() and having the caller
handle buffer locking.

3.
In the README, 'TID' seems to have conflicting definitions:
Transaction ID (2.1) vs. tuple physical identifier (2.3.1).

Could you confirm the intended meaning? Suggest using 'XID' for Transaction ID
if my understanding is correct.

4.
-1: TID relation (maps CRID to original TID)
-5: TID-CRID mapping table

I'm trying to understand the distinctions here. Based on the definition in
vci_tidcrid.h, it seems plausible to use just one relation for the mapping,
suggesting a potential redundancy.

/*
* TID-CRID pair used for TIDCRID update list
*/
typedef struct vcis_tidcrid_pair_item
{
ItemPointerData page_item_id; /* TID on the original relation */
vcis_Crid crid; /* CRID */
} vcis_tidcrid_pair_item_t;

How they are different? I see the code in vci_tidcrid.c

5.
Typo in README.
- Each extent can have its own independent compression dictionary or all
extents can share a comon dictionary
--> s/comon/common/g

--
Regards,
Japin Li