From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | Timur Magomedov <t(dot)magomedov(at)postgrespro(dot)ru> |
Cc: | Tomas Vondra <tomas(at)vondra(dot)me>, "Aya Iwata (Fujitsu)" <iwata(dot)aya(at)fujitsu(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: [WIP]Vertical Clustered Index (columnar store extension) - take2 |
Date: | 2025-07-01 03:17:54 |
Message-ID: | CAHut+Pvrf9tWO9X3img0Eca35_zsc-ESxCp5HSqCxZfmAm4hbA@mail.gmail.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sat, Jun 28, 2025 at 12:24 AM Timur Magomedov
<t(dot)magomedov(at)postgrespro(dot)ru> wrote:
...
> Thanks for updates.
> I was trying to move some code from Postgres core patch to contrib/vci.
> Started with moving VCI options from StdRdOptions struct and
> reloptions.c, reloptions.h files into contrib/vci like Tomas suggested
> earlier. Getting new relopt kind using add_reloption_kind() instead of
> hardcoded RELOPT_KIND_VCI and so on, just like in contrib/bloom.
>
> During this work I've found that those VCI-specific options,
> "vci_column_ids" and "vci_dropped_column_ids", are only read in case
> vci_IsExtendedToMoreThan32Columns() is true. And
> vci_IsExtendedToMoreThan32Columns() itself checks if vci_column_ids
> option is non-empty string to return true. AFAIK no one sets those
> options and vci_IsExtendedToMoreThan32Columns() always returns false.
>
> There are comments in code mentioning that one can create VCI index
> with more than 32 columns using vci_create(). However, there is no such
> a function anywhere. I guess it is now impossible to create VCI index
> with more than 32 (INDEX_MAX_KEYS) columns. Maybe the options were used
> to store column information for indexes created with vci_create() but
> this function has gone.
>
> So, giving the ideas above, can we remove those vci_column_ids,
> vci_dropped_column_ids options and all the code that assumes
> vci_IsExtendedToMoreThan32Columns() to be true? This would make core
> patch a bit shorter, specifically there will be no changes in
> reloptions.{c,h} files and no changes in StdRdOptions struct in rel.h.
>
Hi Timur.
Thanks for reporting this! Your idea seems correct to me.
AFAICT, those relops were intended for supporting >32 vci indexes,
which is only possible when the VCI index is created using
“vci_create()” function, but that function does not exist in the VCI
implementation posted to OSS. So, I agree with you – since those
relops are not currently needed they can just be removed. I will
confirm this understanding with the original patch devs and, if
correct, I will address this in a future patch version.
======
Kind Regards,
Peter Smith.
Fujitsu Australia.
From | Date | Subject | |
---|---|---|---|
Next Message | John Naylor | 2025-07-01 03:20:07 | implicit casts from void* |
Previous Message | Richard Guo | 2025-07-01 02:57:44 | Re: Pathify RHS unique-ification for semijoin planning |