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

Hi Timur.

Thanks for the patches you provided. My replies are inline below.

On Sat, Aug 16, 2025 at 3:45 AM Timur Magomedov
<t(dot)magomedov(at)postgrespro(dot)ru> wrote:
>
> On Thu, 2025-08-14 at 11:23 +1000, Peter Smith wrote:
> > Here are the latest v18* patches.
> >
>
> Hi Peter!
> I've reworked my recent patch [1] so it is now based on v18 and is
> divided into several simpler patches. Here they are plus one additional
> patch.
>
> 0001-Fixed-comment-and-guard-name-in-vci_pg_copy.h.patch
> Looks like vci_pg_copy.h was renamed from vci_numeric.h but file name
> comment and define guard name were not updated. Fixed it.

In v19 I intend to merge vci_pg_copy.h into postgresql_copy.h, so this is moot.

>
> 0002-Removed-vci_set_merge_and_copy_trans_funcs.patch
> Found that vci_set_merge_and_copy_trans_funcs() is not used anywhere,
> removed it alogn with the code that was only called inside it.
> trans_funcs_table[] now only contains single transfn_oid field, others
> (unused) are removed.
>
> 0003-Replaced-linear-search-by-switch-case.patch
> Replaced linear search inside trans_funcs_table array to more optimal
> switch-case.

Thanks, these 0002 and 0003 changes will be in v19 patches which I'm
hoping to post tomorrow or the day after.

>
> 0004-Removed-worker-name-check-in-lock.c.patch
> This is one I'm not sure about.
> Found that changes in Postgres core lock.c file check for "backend="
> substring in background worker name. There is also a comment in
> vci_ros_daemon.c mentioning bgw_name checks of LockAquire(). Names
> don't match however. So as far as I understand the check for "backend="
> in name is always false since no code in VCI sets bgw_name to something
> similar.
> This is either forgotten feature that can be easily fixed by removing
> bgw_name checks, either some bug, either my misunderstanding.
> For the first case, here is a patch that removes bgw_name checks in
> lock.c. It makes core patch a bit smaller and not touching lock.c at
> all (Yay!).
>

There is code in the function vci_LaunchROSControlWorker() which does
a sprintf to assign the worker.bgw_name, but I also do not see how
LockAcquire can have a bgw_name containing string “backend=”.
Frankly, I expected the patch 0001 code should say more like strstr(…,
“vci:”) because otherwise it does not seem VCI specific. Indeed, if it
was checking “vci:” then the comment in storage/vci_ros_daemon.c would
make sense to me.

So, I agree that the Acquire/ReleaseLock code seems like it might be
unreachable, OTOH, I am reluctant to remove it without understanding
more about what was the intended purpose in the first place. Checking…

======
Kind Regards,
Peter Smith.
Fujitsu Australia