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

On Wed, 2025-06-25 at 13:13 +1000, Peter Smith wrote:
> Hi Timur, Thanks for your feedback!

Hello Peter!

> Hmm. IIUC the “supported types” code was written this way for the
> following reason. Out of an over-abundance (?) of caution, the
> original patch authors wanted to call the function
> 'vci_check_supported_types' as a compatibility check, to discover
> up-front (during CREATE EXTENSION) if any of the current PostgreSQL
> OID definitions differ in any way since when the VCI extension was
> built.
> e.g.
> - if vci build supports the type oid, but the oid value in current
> PostgreSQL no longer exists then raise an error
> - if vci build supports the type oid but the oid now has a different
> meaning (typname mismatch) then raise an error
>
> In other words, even though these type OIDs are in a range that is
> supposed to be stable/unlikely to change, I think original VCI
> developers were deliberately cautious about any chance of OID values
> changing in later PostgreSQL versions.

This check is really necessary if types are maintained as plain OID
numbers since they theoretically can change. Using preprocessor
constants, for example, BOOLOID instead of "16", make it simpler to
survive actual number change.

> So, for the compatibility checking we still need to keep that
> function
> 'vci_check_supported_types', and therefore we also still need to keep
> the struct because that is where the known oid/name mapping (at time
> of VCI extension build) is held which is used for the checking.
>
> The current code is written so that when building a new VCI you only
> need to execute:
> "SELECT oid, typname FROM pg_type WHERE typnamespace = 11 AND
> typrelid
> = 0 AND typelem = 0 ORDER BY oid;"
> for the current PostgreSQL then substitute in the necessary
> true/false
> for support.

Using query to get numbers and storing those numbers in struct instead
of using defines like <TYPE_NAME>OID needs some runtime checks indeed.

> I agree this means the resulting vci_is_supported_type() is not as
> efficient as your implementation, but OTOH it is perhaps easier to
> maintain and check against new PostgreSQL releases?
>
> Also, you'll encounter all the same problems with the supported
> functions logic of vci_supported_funcs.c -- those have similar logic,
> so maybe it is better to keep them similar despite inefficiencies?

I agree, having similar logic in both vci_supported_funcs.c and
vci_supported_types.c is good for code readability.

> FYI, I have re-executed the SQL
> SELECT oid, typname FROM pg_type WHERE typnamespace = 11 AND typrelid
> = 0 AND typelem = 0 ORDER BY oid;
> and this exposed a few changes since whenever this
> vci_supported_type_table[] was last generated many years ago.
>
> ~~
>
> AFAICT we could implement the vci_supported_type_table[] to *only*
> include the types where supported is “true”. That would be more
> efficient than now because then the entire array would only have
> 20ish
> elements, but comes at a cost of perhaps being less easy to compare
> with the executed SQL result.
>
> Thoughts?

Looks like having both "false" and "true" cases inside structs is done
for better maintainability. I.e. in case we only have "true"-valued
types we can't say if some type is absent because it is decided to be
unsupported or because it was not examined yet.

> Also, while the vci_supported_type_table[]  lookup is needed for the
> compatibility check logic, I guess we could implement the
> vci_is_supported_type()  function exactly the same as your patch
> switch code, to be called for the runtime checking (during CREATE
> INDEX). That would be more efficient at runtime, but comes at a cost
> of then having 2 functions to maintain.
>
> Thoughts?

I think having two functions could make maintainability worse.

Maybe one day those checks will be rewritten using some smart runtime
logic in C, not compile-time list of types that were previously queried
using SQL script.
So I don't insist on my patch, it was just a proposal.
It has less code but probably we can do even better or at least stick
to current code running SQL query to get actual list of types from time
to time.

> ~~~
>
> So, for now, I have only done the following:
> - Updated vci_supported_type_table[] according to the current SQL
> result.
> - Notice that “name” (NAMEOID) no longer qualifies as a VCI supported
> type (because typelem is not 0 anymore) so the test code was modified
> to remove the name column “c04”.
> - In passing, I removed the costings from the EXPLAIN test output
>
> >
> > 0002-Updated-supported-funcs-SQL-for-recent-PostgreSQL.patch
> > I wonder if it is possible to rewrite vci_supported_funcs.c in a
> > similar way. There is a vci_supported_funcs.sql which I updated so
> > it
> > can be executed at master version of PostgreSQL.
>
> OK. I included your changes in v9 and also added some IF EXISTS so
> the
> .sql file can be run repeatedly without error. I also changed the
> matching comment in the supported_funcs.c code where the SQL of this
> file was described.

Thanks!

>
> > I don't know if it is intentional, but safe_types list from
> > vci_supported_funcs.sql have some differences to the types list
> > from
> > vci_supported_types.c. Specifically, it doesn't include varchar,
> > varbit
> > and uuid.
>
> Thanks for reporting this! TBH, I also expected these lists should be
> the same.
>
> I found multiple inconsistencies:
> -          Some items are in safe_types table but NOT in the
> vci_supported_type_table[]
> -          Some Items are in the vci_supported_type_table[] but are
> NOT in the safe_types table
>
> Furthermore (and maybe partly caused by content of safe_types), the
> results of running vci_support_func.sql also differs quite a lot also
> from the vci_supported_func_table [] (in vci_supported_funcs.c),
> which
> also does not seem right to me.
> -          Some items are in the SQL script results but are NOT in
> the
> vci_supported_func_table []
> -          Some Items are in the vci_supported_func_table [] but are
> NOT in the SQL script results
>
> So, this remains an open problem. I am investigating if there is any
> history/explanation of these differences. IIUC, then I hope later to
> fix the safe_types, and then also fix the vci_supported_func_table[]
> based on the result of that SQL script.

Thanks! Evolutionary updating supported types and supported functions
lists looks good to me for now.

--
Regards,
Timur Magomedov