Hello Peter,
I've tried running VCI, the idea looks great to me. Thank you and
Iwata-san for working on this feature. Looked at the source code of VCI
module, the patch is huge but here are some ideas I hope could be
useful for community. Made some patches on top of v4 that can be
squashed into future iteration of VCI module.
0001-Removed-unnecessary-preload-libraries-checks.patch
Please correct me if I'm wrong but there is no need for adding VCI to
"session_preload_libraries" in case it is in "shared_preload_libraries"
already.
0002-Meson-fixes.patch
Fixed some ghost files in Meson build, added VCI to
contrib/meson.build.
0003-Removed-unused-OS-specific-CAS-functions.patch
Having own VCI-specific CAS looks scary to me, fortunately looks like
it was unused and can be removed.
0004-Fixed-segfault-in-case-there-is-no-hottable.patch
The most exciting one, faced it while playing around with queries from
ClickBench benchmark. Segfault was caused by accessing
aggstate->hottable even in case aggstate->hottable is NULL, using
aggstate->hashtable instead fixes this.
Also there are some ideas of making the VCI module patch smaller and
simpler for review:
First of all, we have this hottable optimization which is enabled but
surrounded by "#ifdef VCI_ENABLE_HOT_HASH_TABLE ... #endif". I think
this could be a separate patch on top of VCI module patch, say "Hot
hashtable optimization".
Second, there is some OS-specific code that looks like used only in
case ENABLE_WOSROS_TRANS_DEFERRAL is defined. This feature is unclear
to me, probably it can be better reviewed if separated to another
"Deferral of WOS->ROS transforamtion" patch that would introduce new
GUC and some OS-specific code. So we can be sure other VCI code has no
OS-specific parts.
--
Regards,
Timur Magomedov
From 60f4ab928e2b50c831f97f585520950a8f3ddeef Mon Sep 17 00:00:00 2001
From: Timur Magomedov <t.magome...@postgrespro.ru>
Date: Fri, 30 May 2025 18:41:10 +0300
Subject: [PATCH 1/4] Removed unnecessary preload libraries checks
---
contrib/vci/vci.conf | 1 -
contrib/vci/vci_main.c | 69 ------------------------------------------
2 files changed, 70 deletions(-)
diff --git a/contrib/vci/vci.conf b/contrib/vci/vci.conf
index 776dd7b5cc6..017c481c68f 100644
--- a/contrib/vci/vci.conf
+++ b/contrib/vci/vci.conf
@@ -1,5 +1,4 @@
shared_preload_libraries = 'vci'
-session_preload_libraries = 'vci'
max_worker_processes = 20
vci.table_rows_threshold = 0
vci.cost_threshold = 0
diff --git a/contrib/vci/vci_main.c b/contrib/vci/vci_main.c
index 10624e1d3f0..0919ecfe10f 100644
--- a/contrib/vci/vci_main.c
+++ b/contrib/vci/vci_main.c
@@ -59,60 +59,6 @@ ProcessUtility_hook_type process_utility_prev = NULL;
static shmem_request_hook_type prev_shmem_request_hook = NULL;
static void vci_shmem_request(void);
-/*
- * @description check whether 'vci' is contained in the specified guc.
- *
- * This function is written based on load_libraries
- * in src/backend/utils/init/miscinit.c
- *
- * @param[in] str specified string by guc.
- * @param[in] guc parameter.
- * @return true is 'vci' is contained in the parameter specified by guc.
- */
-static bool
-has_vci_lib_str(const char *str_)
-{
- char *rawstring;
- List *elemlist;
- ListCell *l;
- bool result = false;
-
- if (str_ == NULL || str_[0] == '\0')
- return result; /* nothing to do */
-
- /* Need a modifiable copy of string */
- rawstring = pstrdup(str_);
-
- /* Parse string into list of identifiers */
- if (!SplitIdentifierString(rawstring, ',', &elemlist))
- {
- /* syntax error in list */
- pfree(rawstring);
- list_free(elemlist);
- return result;
- }
-
-#ifdef WIN32 /* WIN32 */
- foreach(l, elemlist)
-#else /* WIN32 */
- foreach(l, elemlist)
-#endif
- {
- char *tok = (char *) lfirst(l);
-
- if (!strcmp(VCI_STRING, tok))
- {
- result = true;
- break;
- }
- }
-
- pfree(rawstring);
- list_free(elemlist);
-
- return result;
-}
-
/**
* _PG_init: Entry point of this module.
* It is called when the module is loaded.
@@ -120,7 +66,6 @@ has_vci_lib_str(const char *str_)
void
_PG_init(void)
{
-
pg_bindtextdomain(TEXTDOMAIN);
if (!process_shared_preload_libraries_in_progress)
@@ -131,20 +76,6 @@ _PG_init(void)
return; /* LCOV_EXCL_LINE */
}
- /*
- * Check if both shared_preload_libraries and session_preload_libraries
- * have the string "vci".
- */
- if ((!has_vci_lib_str(shared_preload_libraries_string)) ||
- (!has_vci_lib_str(session_preload_libraries_string)))
- {
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("\"%s\" must be registered in shared_preload_libraries and session_preload_libraries",
- VCI_STRING)));
- return; /* LCOV_EXCL_LINE */
- }
-
vci_read_guc_variables();
if (!IsPostmasterEnvironment)
--
2.43.0
From e146873c4c94d51689953cca512767ef23348ecf Mon Sep 17 00:00:00 2001
From: Timur Magomedov <t.magome...@postgrespro.ru>
Date: Fri, 30 May 2025 19:36:53 +0300
Subject: [PATCH 2/4] Meson fixes
---
contrib/meson.build | 1 +
contrib/vci/meson.build | 14 +++++++++++---
contrib/vci/port/meson.build | 6 +-----
contrib/vci/storage/meson.build | 1 -
4 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/contrib/meson.build b/contrib/meson.build
index ed30ee7d639..d8bd5c855a5 100644
--- a/contrib/meson.build
+++ b/contrib/meson.build
@@ -70,4 +70,5 @@ subdir('tsm_system_time')
subdir('unaccent')
subdir('uuid-ossp')
subdir('vacuumlo')
+subdir('vci')
subdir('xml2')
diff --git a/contrib/vci/meson.build b/contrib/vci/meson.build
index f56d90c4aef..fd3dc103278 100644
--- a/contrib/vci/meson.build
+++ b/contrib/vci/meson.build
@@ -21,8 +21,6 @@ vci_sources += vci_storage_sources
vci_sources += vci_utils_sources
-vci_sources += vci_prewarm_sources
-
if host_system == 'solaris'
ldflags += ['-lc -lkstat']
endif
@@ -61,4 +59,14 @@ install_data(
kwargs:contrib_data_args,
)
-subdir('po', if_found: libintl)
+tests += {
+ 'name': 'vci',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'vci',
+ ],
+ 'regress_args': ['--temp-config', files('vci.conf')],
+ }
+}
diff --git a/contrib/vci/port/meson.build b/contrib/vci/port/meson.build
index de943cff85e..916447a92ad 100644
--- a/contrib/vci/port/meson.build
+++ b/contrib/vci/port/meson.build
@@ -4,13 +4,9 @@ vci_port_linux_sources = files(
'vci_linux.c',
)
-vci_port_posix_sources = files(
- 'vci_posix.c',
-)
-
vci_port_sources = []
-elif host_system == 'linux'
+if host_system == 'linux'
vci_port_sources += vci_port_linux_sources
endif
diff --git a/contrib/vci/storage/meson.build b/contrib/vci/storage/meson.build
index e49dfa80964..711e2b4608f 100644
--- a/contrib/vci/storage/meson.build
+++ b/contrib/vci/storage/meson.build
@@ -14,7 +14,6 @@ vci_storage_sources = files(
'vci_ros.c',
'vci_ros_command.c',
'vci_ros_daemon.c',
- 'vci_ros_minmax.c',
'vci_tidcrid.c',
'vci_wos.c',
'vci_xact.c',
--
2.43.0
From e8d30b7e89dcf789ceab75bf13f3956658a8ea09 Mon Sep 17 00:00:00 2001
From: Timur Magomedov <t.magome...@postgrespro.ru>
Date: Fri, 30 May 2025 20:08:15 +0300
Subject: [PATCH 3/4] Removed unused OS-specific CAS functions
---
contrib/vci/include/vci_mem.h | 1 -
contrib/vci/include/vci_port_linux.h | 7 -------
contrib/vci/include/vci_port_solaris.h | 6 ------
contrib/vci/include/vci_port_windows.h | 26 --------------------------
contrib/vci/include/vci_ros.h | 13 -------------
5 files changed, 53 deletions(-)
diff --git a/contrib/vci/include/vci_mem.h b/contrib/vci/include/vci_mem.h
index 4225682a71f..c36c21d9e2f 100644
--- a/contrib/vci/include/vci_mem.h
+++ b/contrib/vci/include/vci_mem.h
@@ -19,7 +19,6 @@
#include "utils/palloc.h"
#include "vci.h"
-#include "vci_port.h"
#include "vci_ros.h"
#include "vci_memory_entry.h"
diff --git a/contrib/vci/include/vci_port_linux.h b/contrib/vci/include/vci_port_linux.h
index a6ed0f2e3e7..edefb70ad19 100644
--- a/contrib/vci/include/vci_port_linux.h
+++ b/contrib/vci/include/vci_port_linux.h
@@ -25,13 +25,6 @@
#define VCI_NAME_MAX NAME_MAX
#define VCI_PATH_MAX PATH_MAX
-static inline int64
-vci_CompareAndSwap64(volatile int64 *ptr, int64 oldValue, int64 newValue)
-{
- /* GCC build-in function */
- return __sync_val_compare_and_swap(ptr, oldValue, newValue);
-}
-
/**
* values in /proc/diskstats
* For more detail, see https://www.kernel.org/doc/Documentation/iostats.txt
diff --git a/contrib/vci/include/vci_port_solaris.h b/contrib/vci/include/vci_port_solaris.h
index a2eecf753aa..cf2878fd139 100644
--- a/contrib/vci/include/vci_port_solaris.h
+++ b/contrib/vci/include/vci_port_solaris.h
@@ -24,10 +24,4 @@
#define VCI_FULLPATH_FMT "%s/%s"
#define VCI_MOUNTS_FILE "/etc/mnttab"
-static inline int64
-vci_CompareAndSwap64(volatile int64 *ptr, int64 oldValue, int64 newValue)
-{
- return atomic_cas_64(ptr, oldValue, newValue);
-}
-
#endif /* VCI_PORT_SOLARIS_H */
diff --git a/contrib/vci/include/vci_port_windows.h b/contrib/vci/include/vci_port_windows.h
index 40d28ea6662..241f602b49d 100644
--- a/contrib/vci/include/vci_port_windows.h
+++ b/contrib/vci/include/vci_port_windows.h
@@ -21,30 +21,4 @@
#endif /* #if defined(_MSC_VER) ||
* defined(__BORLANDC__) */
-#ifdef __GCC__
-static inline int64
-vci_CompareAndSwap64(volatile int64 *ptr, int64 oldValue, int64 newValue)
-{
- /* GCC build-in function */
- return __sync_val_compare_and_swap(ptr, oldValue, newValue);
-}
-#endif /* #ifdef __GCC__ */
-
-#if defined(_MSC_VER) || defined(__BORLANDC__)
-static inline int64
-vci_CompareAndSwap64(volatile int64 *ptr, int64 oldValue, int64 newValue)
-{
- /* Visual studio built-in function */
- return _InterlockedCompareExchange64(ptr, oldValue, newValue);
-}
-#endif /* #if defined(_MSC_VER) ||
- * defined(__BORLANDC__) */
-
-extern bool vci_ReserveSharedMemoryRegion(HANDLE hProcess, void **addr_p, Size size);
-
-extern pid_t vci_getpid(void);
-#ifndef getpid
-#define getpid vci_getpid
-#endif
-
#endif /* VCI_PORT_WINDOWS_H */
diff --git a/contrib/vci/include/vci_ros.h b/contrib/vci/include/vci_ros.h
index bfafede1af6..a92b3864559 100644
--- a/contrib/vci/include/vci_ros.h
+++ b/contrib/vci/include/vci_ros.h
@@ -1067,19 +1067,6 @@ Buffer
const void *data_,
Size size);
-static inline void
-vci_SetValueAtomically64(volatile int64 *ptr, int64 val)
-{
- int64 oldValue;
- int64 chkValue;
-
- do
- {
- oldValue = *ptr;
- chkValue = vci_CompareAndSwap64(ptr, oldValue, val);
- } while (oldValue != chkValue);
-}
-
typedef struct vci_meta_item_scanner
{
bool inited;
--
2.43.0
From bd9ca0b4a2a0caceb3d62f938240e31a2ba4075f Mon Sep 17 00:00:00 2001
From: Timur Magomedov <t.magome...@postgrespro.ru>
Date: Fri, 30 May 2025 20:52:41 +0300
Subject: [PATCH 4/4] Fixed segfault in case there is no hottable
---
contrib/vci/executor/vci_agg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/vci/executor/vci_agg.c b/contrib/vci/executor/vci_agg.c
index 1c558de7dc6..269c249efaa 100644
--- a/contrib/vci/executor/vci_agg.c
+++ b/contrib/vci/executor/vci_agg.c
@@ -623,7 +623,7 @@ lookup_hash_entry_vector(VciAggState *aggstate,
hashslot,
&isnew,
NULL);
- hashcxt = aggstate->hottable->tablecxt;
+ hashcxt = aggstate->hashtable->tablecxt;
#ifdef VCI_ENABLE_HOT_HASH_TABLE
found_hot_table:
#endif
--
2.43.0
