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

On Fri, 11 Jul 2025 at 16:31, Peter Smith <smithpb2250@gmail.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
65536with 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