preview capabilities by tomas-villagesql · Pull Request #418 · villagesql/villagesql-server

tomas-villagesql · 2026-05-01T16:05:05Z

No description provided.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dbentley-vsql · 2026-05-01T18:00:59Z

+// capability struct at populate time.
+// Intended for built-in capabilities registered at startup and for
+// extension-provided capabilities.
+void register_capability(std::string name, uint32_t version, void *vtable,


Do we need to expose this yet (vs. just register_builtin_capabilities)?

moved it out

dbentley-vsql · 2026-05-01T18:02:01Z

+  "version": "0.0.1",
+  "description": "VillageSQL test extension for the preview capability system",
+  "author": "VillageSQL Community",
+  "license": "GPL-2.0"


TODO that we want to require preview capabilities to be listed here?

I added a TODO where we populate the capabilities to verify they are the same as what is in the manifest

dbentley-vsql · 2026-05-01T18:02:47Z

-// FuncCount, TypeCount, SysVarCount, and StatusVarCount are explicit template
-// parameters so that array sizes are compile-time constants without relying on
-// VLAs.
+// FuncCount, TypeCount, SysVarCount, StatusVarCount, and


Probably worth switching from enumerating each to just describing this in general at this point.

changed to generic comment

dbentley-vsql · 2026-05-01T18:04:52Z

+  // of the extension (use a string literal).
+  const char *name;
+  // Version of the capability struct the extension was compiled against.
+  uint32_t version;


I'm a bit uneasy about adding yet another version field before we need it.

Isn't this the point of the preview feature? It is versioned independently of the rest of the ABI; I asked about this in your doc.

removed version

dbentley-vsql · 2026-05-01T18:42:20Z

+VEF_GENERATE_ENTRY_POINTS(
+    make_extension()
+        .func(make_func<&ping_impl>("ping").returns(INT).build())
+        .preview_require_ping(g_ping))


I thought it would be:

preview_require(g_ping)

(i.e., one method for all preview capabilities and one for preview require capabilities)

I like that, but the current design followed what was outlined in the slack thread.

dbentley-vsql · 2026-05-01T18:44:16Z

+
+namespace {
+
+// Registry key: capability name + version.


Some of this is related to capabilities generally (vs. specifically to preview_capabilities).

split it up

malone-at-work

I will need to take another pass.

malone-at-work · 2026-05-01T18:40:03Z

    return true;
  }

+  // Populate any preview capabilities the extension requires.


I think that this should go after at least line 891

malone-at-work · 2026-05-01T21:01:22Z

+  // of the extension (use a string literal).
+  const char *name;
+  // Version of the capability struct the extension was compiled against.
+  uint32_t version;


Isn't this the point of the preview feature? It is versioned independently of the rest of the ABI; I asked about this in your doc.

malone-at-work · 2026-05-01T21:09:33Z

+
+    // Copy the vtable into the extension's capability struct. Both are the
+    // same ABI type and the registered size matches sizeof(that type).
+    memcpy(req.capability, it->second.vtable, it->second.vtable_size);


This feels awkward. How does the extension know when this is safe to access?

Could the vef_register_result_t have a func pointer for each element:

vef_register_capability(void *vtable)

Or maybe one that then gets called with each of the vtables and it dispatches them?

malone-at-work · 2026-05-04T01:14:03Z

+
+struct CapabilityKey {
+  std::string name;
+  uint32_t version;


I thought only preview capabilities would have a different version. Let's discuss versions Monday morning.

removed version

on_load now takes const vef_register_arg_t * (server context, extensible). on_unload takes no arguments (cleanup only, no server context needed). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

malone-at-work · 2026-05-04T18:05:35Z

+
+// Returns a compile-time hash of T's fully qualified type name via
+// __PRETTY_FUNCTION__. Used to verify that the capability ABI struct type
+// an extension was compiled against matches the type the server registered.


AFAICT - This doesn't include anything about the structure of the type, just the name; i.e. you could change the functions in the interface, and this would not change:

> cat t.cc #include <iostream> #include <string_view> template <typename T> constexpr std::string_view abi_hash_this() { return __PRETTY_FUNCTION__; } struct Foo { int foo_A(); int foo_B(int); }; int main() { std::cout << abi_hash_this<Foo>() << std::endl; return 0; } > clang++ t.cc -o t > ./t std::string_view abi_hash_this() [T = Foo]

I asked Claude if there are C++ features that could help with this problem, C++26 seems nice, but I don't think we can use it yet:

C++26 static reflection (P2996) is exactly what's needed here. C++26: std::meta (P2996) Accepted into C++26, this lets you enumerate a struct's non-static data members at compile time: #include <meta> template <typename T> constexpr size_t layout_hash() { size_t h = sizeof(T); // Iterate every non-static data member automatically for... (auto member : std::meta::nonstatic_data_members_of(^T)) { h = (h ^ std::meta::offset_of(member)) * 1099511628211ULL; h = (h ^ std::meta::size_of(member)) * 1099511628211ULL; } return h; } This would catch adding/removing fields, reordering them, and changing their types — all automatically, with no per-struct boilerplate. Clang has experimental support behind -freflection today, but it's not production-ready and the syntax is still slightly in flux. Practical today: Boost.PFR (C++17/20) For aggregate structs (plain C structs like vef_preview_ping_t qualify), Boost.PFR can enumerate fields without macros or annotations: #include <boost/pfr.hpp> template <typename T> constexpr size_t layout_hash() { size_t h = sizeof(T); // Hashes the size of each field in order — catches count and type changes boost::pfr::for_each_field(T{}, [&h](auto& field) { h = (h ^ sizeof(field)) * 1099511628211ULL; }); return h; } Limitation: PFR gives you field values/types but not offsetof, so it catches field count and size changes but misses reordering of fields with the same type (e.g. swapping two uint32_ts). It also can't be constexpr in all cases. Summary ┌───────────────────────────────┬─────────────────────┬─────────────────┬───────────────┐ │ Approach │ Catches size change │ Catches reorder │ Available now │ ├───────────────────────────────┼─────────────────────┼─────────────────┼───────────────┤ │ Current (__PRETTY_FUNCTION__) │ No │ No │ Yes │ ├───────────────────────────────┼─────────────────────┼─────────────────┼───────────────┤ │ + sizeof(T) │ Yes │ No │ Yes │ ├───────────────────────────────┼─────────────────────┼─────────────────┼───────────────┤ │ Boost.PFR │ Mostly │ No │ Yes │ ├───────────────────────────────┼─────────────────────┼─────────────────┼───────────────┤ │ C++26 std::meta │ Yes │ Yes │ Experimental │ └───────────────────────────────┴─────────────────────┴─────────────────┴───────────────┘ For VillageSQL, the sizeof(T) improvement is the pragmatic near-term fix. C++26 reflection is the right long-term answer once it stabilizes — and since the ABI struct headers are simple C aggregates, they'd be ideal candidates for it.

I have a new proposal via Claude (though maybe this macros should have PREVIEW in the name?):

❯ What about a C++ macro or something that hashes the text of the struct? ⏺ Yes — the preprocessor # (stringify) operator can do this. Since fnv1a_hash is already constexpr, you can hash the literal text of the struct body at compile time: #define VEF_DEFINE_ABI(TypeName, ...) \ typedef struct { __VA_ARGS__ } TypeName; \ namespace villagesql::detail { \ template <> \ constexpr size_t abi_schema_hash<TypeName>() { \ constexpr const char s[] = #__VA_ARGS__; \ return fnv1a_hash(s, sizeof(s) - 1); \ } \ } // Usage: VEF_DEFINE_ABI(vef_preview_ping_t, vef_ping_fn ping; ) #__VA_ARGS__ captures "vef_ping_fn ping;" as a string literal at compile time. Add a field, reorder, change a type — the string changes, the hash changes. The real risk: clang-format. Macro arguments are generally left alone by clang-format, but if it ever reformats the whitespace inside (e.g. normalizes spacing around ;), the hash silently changes and extensions built before the reformat break at load time, even though the ABI is identical. You'd want a // clang-format off guard around uses of the macro.

going with the sizeof(T) improvement — it catches the practically important cases (field additions/removals, type size changes) without external dependencies. Let me know if you think it is worth going further

malone-at-work · 2026-05-05T12:32:25Z

+  const char *name;
+  // Called by the server with the capability vtable pointer if registered.
+  // The extension assigns the vtable to its own capability struct.
+  void (*receive)(void *vtable);


Why do we need receive AND onload? Can't we just use 1?

receive is internal plumbing, on_load is a user facing hook. Not used currently, but the thread PR will be using on_unload to clean up

malone-at-work · 2026-05-05T12:40:00Z

+static bool check_allow_preview_extensions(sys_var *, THD *, set_var *var) {
+  const bool new_value = var->save_result.ulonglong_value != 0;
+
+  // TODO(villagesql-beta): There is a TOCTOU race between this check and the


This comment applies to the false branch, can you move it down to that branch?

malone-at-work · 2026-05-05T12:42:18Z

LGTM, my only concern is do we need both onload and receive. I think we should pass the vtable in onload.

dbentley-vsql · 2026-05-05T15:29:03Z

+
+}  // namespace
+
+void register_capability(std::string name, void *vtable, size_t abi_type_hash) {


Is it important to expose this if it's only used by register_builtin_capabilities?

dbentley-vsql · 2026-05-05T15:29:13Z

+  g_registry[std::move(name)] = {vtable, abi_type_hash};
+}
+
+void unregister_capability(const std::string &name) { g_registry.erase(name); }


Do we ever need to unregister a capability?

we call it, but it is a no-op currently, future use

dbentley-vsql · 2026-05-05T15:29:51Z

+
+// TODO(villagesql-beta): Verify that the capabilities declared in
+// vef_registration_t match those listed in the extension's manifest.
+bool populate_capabilities(const vef_registration_t *reg,


Is it hard to do? Shoudl we have a 0.0.5 TODO target, cause I don't want to ship without this?

changed TODO

dbentley-vsql · 2026-05-05T16:06:38Z

+#include "villagesql/sdk/include/villagesql/detail/capability_hash.h"
+#include "villagesql/services/preview/ping.h"
+
+bool vsql_allow_preview_extensions = false;


Maybe "allow_preview_capabilities", because it's the capabilities that are in preview, not the extensions themselves? (I could imagine us offering preview extensions that are only using fully-baked capabilities)

keeping it as is for now

dbentley-vsql · 2026-05-05T16:07:31Z

+// Register all server built-in capabilities. Called once at server startup.
+void register_builtin_capabilities();
+
+// Populate capabilities declared in a vef_registration_t.


I'd mention "for one extension", to clarify the difference here.

dbentley-vsql · 2026-05-05T16:09:14Z

+  return h;
+}
+
+// Returns a compile-time hash of T's fully qualified type name and size via


I'm not sure the hash buys us that much? It's just the len of the vtable, not hashing the functions' signatures themselves?

yes, does a bit more now with Mike suggestion

malone-at-work · 2026-05-05T18:40:18Z

 } vef_status_var_desc_t;

+// Forward declaration so vef_required_capability_t can reference it.
+typedef struct vef_registration_t vef_registration_t;


We no longer need this forward declaration.

malone-at-work · 2026-05-05T18:43:12Z

+
+  bool available() const { return abi_.ping != nullptr; }
+
+  // Public so that cap_receive() can access abi_ via a pointer.


Not necessarily in this PR, but we should make the cap_receive() a friend or something in future capabilities.

tomas-villagesql requested review from dbentley-vsql and malone-at-work May 1, 2026 16:05

tomas-villagesql force-pushed the tomas/preview-abi branch from 7346d76 to 0886266 Compare May 1, 2026 16:36

first cut

4fea333

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

tomas-villagesql force-pushed the tomas/preview-abi branch from 0886266 to 4fea333 Compare May 1, 2026 17:34

dbentley-vsql reviewed May 1, 2026

View reviewed changes

malone-at-work reviewed May 1, 2026

View reviewed changes

tomas-villagesql requested review from dbentley-vsql and malone-at-work May 2, 2026 07:38

address review comments

fba36d9

tomas-villagesql force-pushed the tomas/preview-abi branch from 8fddfec to fba36d9 Compare May 2, 2026 10:38

malone-at-work reviewed May 4, 2026

View reviewed changes

v2

555cade

tomas-villagesql requested a review from malone-at-work May 4, 2026 16:33

tomas-villagesql and others added 2 commits May 4, 2026 18:37

preview: update on_load/on_unload signatures

4d2e91f

on_load now takes const vef_register_arg_t * (server context, extensible). on_unload takes no arguments (cleanup only, no server context needed). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

preview: add vsql_allow_preview_extensions sys var

a6996b0

malone-at-work reviewed May 4, 2026

View reviewed changes

tomas-villagesql requested a review from malone-at-work May 4, 2026 19:41

added sizof(T) to hash

d625ec7

tomas-villagesql force-pushed the tomas/preview-abi branch from bd0c92d to d625ec7 Compare May 5, 2026 07:38

malone-at-work reviewed May 5, 2026

View reviewed changes

malone-at-work mentioned this pull request May 5, 2026

[Server]: Preview Unstable Features #424

Open

dbentley-vsql reviewed May 5, 2026

View reviewed changes

villagesql-adam mentioned this pull request May 5, 2026

sdk: make 'vsql' top-level namespace #431

Merged

review comments

6ce9777

tomas-villagesql force-pushed the tomas/preview-abi branch from a4b2dfd to 6ce9777 Compare May 5, 2026 18:36

tomas-villagesql enabled auto-merge (squash) May 5, 2026 18:37

malone-at-work reviewed May 5, 2026

View reviewed changes

malone-at-work approved these changes May 5, 2026

View reviewed changes

dbentley-vsql approved these changes May 5, 2026

View reviewed changes

tomas-villagesql merged commit 639a1ea into main May 5, 2026
3 checks passed

tomas-villagesql deleted the tomas/preview-abi branch May 5, 2026 18:52

github-actions Bot locked and limited conversation to collaborators May 5, 2026


		} // namespace

		void register_capability(std::string name, void *vtable, size_t abi_type_hash) {


		bool available() const { return abi_.ping != nullptr; }

		// Public so that cap_receive() can access abi_ via a pointer.

Conversation

tomas-villagesql commented May 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malone-at-work left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malone-at-work commented May 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!