Compatible with Oracle's sys_guid function#844
Conversation
WalkthroughA new function, Changes
Sequence Diagram(s)sequenceDiagram
participant SQL_User
participant PostgreSQL
participant uuid-ossp Extension (C)
SQL_User->>PostgreSQL: SELECT sys_guid();
PostgreSQL->>uuid-ossp Extension (C): Call ora_sys_guid()
uuid-ossp Extension (C)->>uuid-ossp Extension (C): Generate 16-byte UUID (OSS/OSSP/E2FS/arc4random)
uuid-ossp Extension (C)-->>PostgreSQL: Return bytea UUID
PostgreSQL-->>SQL_User: Return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested labels
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
contrib/uuid-ossp/sql/ivy_uuid_ossp.sql (1)
81-83: Add a length assertion to validate RAW(16)-equivalent output.Small enhancement to ensure the function returns 16 bytes as intended.
Add below this block:
SELECT length(sys_guid()) = 16;contrib/uuid-ossp/uuid-ossp--1.1.sql (1)
56-59: Mark sys_guid() as PARALLEL SAFE for consistency and planner latitude.Other volatile generators here (e.g., uuid_generate_v4) are PARALLEL SAFE. sys_guid() is similarly side-effect free and safe in parallel contexts.
-CREATE OR REPLACE FUNCTION pg_catalog.sys_guid() - RETURNS bytea -AS 'MODULE_PATHNAME', 'ora_sys_guid' -VOLATILE STRICT LANGUAGE C; +CREATE OR REPLACE FUNCTION pg_catalog.sys_guid() + RETURNS bytea +AS 'MODULE_PATHNAME', 'ora_sys_guid' +VOLATILE STRICT LANGUAGE C PARALLEL SAFE;contrib/uuid-ossp/uuid-ossp--1.0--1.1.sql (1)
17-20: Align with main script: make sys_guid() PARALLEL SAFE in the upgrade path too.Keep the upgrade and base scripts consistent.
-CREATE OR REPLACE FUNCTION pg_catalog.sys_guid() - RETURNS bytea -AS 'MODULE_PATHNAME', 'ora_sys_guid' -VOLATILE STRICT LANGUAGE C; +CREATE OR REPLACE FUNCTION pg_catalog.sys_guid() + RETURNS bytea +AS 'MODULE_PATHNAME', 'ora_sys_guid' +VOLATILE STRICT LANGUAGE C PARALLEL SAFE;contrib/uuid-ossp/uuid-ossp.c (1)
105-107: Avoid duplicating constants for UUID lengthPrefer reusing the existing UUID length constant to prevent drift.
Apply:
-#define SYS_GUID_LENGTH 16 +#define SYS_GUID_LENGTH UUID_LEN
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
GNUmakefile.in(3 hunks)contrib/ivorysql_ora/preload_ora_misc.sql(1 hunks)contrib/uuid-ossp/sql/ivy_uuid_ossp.sql(1 hunks)contrib/uuid-ossp/uuid-ossp--1.0--1.1.sql(1 hunks)contrib/uuid-ossp/uuid-ossp--1.1.sql(1 hunks)contrib/uuid-ossp/uuid-ossp.c(3 hunks)
🔇 Additional comments (4)
contrib/uuid-ossp/sql/ivy_uuid_ossp.sql (1)
81-83: LGTM: adds a minimal sanity check for sys_guid() randomness.Comparable to the existing v4 inequality test. Good.
contrib/uuid-ossp/uuid-ossp--1.1.sql (1)
56-59: default_version correctly set to ’1.1’Verified in contrib/uuid-ossp/uuid-ossp.control:
- default_version = '1.1'
No further action needed.
contrib/uuid-ossp/uuid-ossp.c (2)
121-121: Registration looks goodFunction is properly registered with PG_FUNCTION_INFO_V1.
580-587: Ensure RFC 4122 version/variant in fallback and avoid extra memcpyThe fallback currently returns 16 random bytes without setting version/variant bits, so it’s not a proper v4 UUID. Also, you can fill the result buffer directly.
Apply:
-#else - int i; - unsigned char byte_array[SYS_GUID_LENGTH]; - for (i = 0; i < SYS_GUID_LENGTH; i++) { - byte_array[i] = (unsigned char)arc4random(); - } - memcpy(VARDATA(result), byte_array, SYS_GUID_LENGTH); -#endif +#else + unsigned char *bytes = (unsigned char *) VARDATA(result); + for (int i = 0; i < SYS_GUID_LENGTH; i++) + bytes[i] = (unsigned char) arc4random(); + /* Set version (4) and variant (RFC 4122) */ + bytes[6] = (unsigned char)((bytes[6] & 0x0F) | 0x40); + bytes[8] = (unsigned char)((bytes[8] & 0x3F) | 0x80); +#endifOptional: if available in your portability layer, arc4random_buf(bytes, SYS_GUID_LENGTH) would be simpler and faster.
Likely an incorrect or invalid review comment.
|
|
||
| -- | ||
| -- Plugin uuid-ossp | ||
| -- | ||
| CREATE EXTENSION IF NOT EXISTS "uuid-ossp"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don't CREATE EXTENSION while search_path is set to sys (objects will be installed into sys).
At this point search_path=sys; CREATE EXTENSION without SCHEMA will place extension objects (e.g., uuid_generate_*) into sys, not public. That can break lookups/tests expecting these functions on the default path and pollutes the sys schema.
Recommend creating the extension with search_path reset to default (public), then restore sys:
--
-- Plugin uuid-ossp
--
-CREATE EXTENSION IF NOT EXISTS "uuid-ossp";
+RESET search_path; -- ensure extension installs into public
+CREATE EXTENSION IF NOT EXISTS "uuid-ossp";
+SET search_path TO sys; -- restore Oracle compatibility pathAlternatively (if the extension is relocatable), you can force it into public:
- CREATE EXTENSION IF NOT EXISTS "uuid-ossp" SCHEMA public;
Run to confirm relocatable status and avoid the SCHEMA clause if false, and to ensure default_version is 1.1:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate control file and check 'relocatable' and 'default_version'
fd -a uuid-ossp.control | tee /dev/stderr
if fd -a uuid-ossp.control >/dev/null; then
rg -n "^(relocatable|default_version)\s*=" $(fd -a uuid-ossp.control)
else
echo "uuid-ossp.control not found under contrib/uuid-ossp" >&2
fiLength of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the uuid-ossp control file
control_file=$(fd -a uuid-ossp.control || true)
if [[ -z "$control_file" ]]; then
echo "uuid-ossp.control not found in the repo" >&2
exit 1
fi
echo "Found control file: $control_file"
# Print relocatable and default_version settings
rg -n "^(relocatable|default_version)\s*=" "$control_file"Length of output: 488
Ensure uuid-ossp Installs into the Public Schema
The CREATE EXTENSION call currently runs with search_path=sys, so all objects (e.g., uuid_generate_*) land in the sys schema. To avoid polluting sys and breaking clients/tests that expect these functions in the default path, install the extension into public.
We’ve confirmed from contrib/uuid-ossp/uuid-ossp.control that
default_version = '1.1'relocatable = true
You can address this in one of two ways:
• Reset the search_path around the CREATE:
--
-- Plugin uuid-ossp
--
- CREATE EXTENSION IF NOT EXISTS "uuid-ossp";
+ RESET search_path; -- ensure objects go into public
+ CREATE EXTENSION IF NOT EXISTS "uuid-ossp";
+ SET search_path TO sys; -- restore Oracle-compat path• Or, since the extension is relocatable, force it into public:
CREATE EXTENSION IF NOT EXISTS "uuid-ossp" SCHEMA public;Pick the approach that best fits your code-style guidelines.
🤖 Prompt for AI Agents
In contrib/ivorysql_ora/preload_ora_misc.sql around lines 16 to 20, the CREATE
EXTENSION for uuid-ossp currently installs objects into the sys schema due to
the search_path setting. To fix this, modify the CREATE EXTENSION statement to
explicitly install the extension into the public schema by adding "SCHEMA
public" to the command, ensuring the uuid_generate_* functions and related
objects reside in the public schema and avoid polluting sys.
| #ifdef HAVE_UUID_OSSP | ||
| uuid_t *uuid; | ||
| uuid_rc_t rc; | ||
| uuid = get_cached_uuid_t(0); | ||
| rc = uuid_make(uuid, UUID_MAKE_V4, NULL, NULL); | ||
| if (rc != UUID_RC_OK) { | ||
| pguuid_complain(rc); | ||
| } | ||
| memcpy(VARDATA(result), (unsigned char *)uuid, SYS_GUID_LENGTH); | ||
|
|
There was a problem hiding this comment.
Bug: OSSP uuid_t is an opaque handle; memcpy does not yield the UUID bytes
With OSSP, uuid_t is not a 16-byte binary UUID. You must export the generated UUID to binary via uuid_export; memcpy-ing from the uuid_t object produces invalid results.
Apply:
#ifdef HAVE_UUID_OSSP
- uuid_t *uuid;
- uuid_rc_t rc;
- uuid = get_cached_uuid_t(0);
- rc = uuid_make(uuid, UUID_MAKE_V4, NULL, NULL);
- if (rc != UUID_RC_OK) {
- pguuid_complain(rc);
- }
- memcpy(VARDATA(result), (unsigned char *)uuid, SYS_GUID_LENGTH);
+ uuid_t *uuid;
+ uuid_rc_t rc;
+ void *ptr = VARDATA(result);
+ size_t len = SYS_GUID_LENGTH;
+
+ uuid = get_cached_uuid_t(0);
+ rc = uuid_make(uuid, UUID_MAKE_V4, NULL, NULL);
+ if (rc != UUID_RC_OK)
+ pguuid_complain(rc);
+ rc = uuid_export(uuid, UUID_FMT_BIN, &ptr, &len);
+ if (rc != UUID_RC_OK || len != SYS_GUID_LENGTH)
+ pguuid_complain(rc);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifdef HAVE_UUID_OSSP | |
| uuid_t *uuid; | |
| uuid_rc_t rc; | |
| uuid = get_cached_uuid_t(0); | |
| rc = uuid_make(uuid, UUID_MAKE_V4, NULL, NULL); | |
| if (rc != UUID_RC_OK) { | |
| pguuid_complain(rc); | |
| } | |
| memcpy(VARDATA(result), (unsigned char *)uuid, SYS_GUID_LENGTH); | |
| #ifdef HAVE_UUID_OSSP | |
| uuid_t *uuid; | |
| uuid_rc_t rc; | |
| void *ptr = VARDATA(result); | |
| size_t len = SYS_GUID_LENGTH; | |
| uuid = get_cached_uuid_t(0); | |
| rc = uuid_make(uuid, UUID_MAKE_V4, NULL, NULL); | |
| if (rc != UUID_RC_OK) | |
| pguuid_complain(rc); | |
| rc = uuid_export(uuid, UUID_FMT_BIN, &ptr, &len); | |
| if (rc != UUID_RC_OK || len != SYS_GUID_LENGTH) | |
| pguuid_complain(rc); |
🤖 Prompt for AI Agents
In contrib/uuid-ossp/uuid-ossp.c around lines 565 to 574, the code incorrectly
uses memcpy to copy the UUID from the opaque uuid_t handle, which does not
contain raw UUID bytes. To fix this, replace the memcpy call with a call to
uuid_export to convert the uuid_t to a 16-byte binary UUID before copying it
into the result buffer.
| include $(top_builddir)/src/Makefile.global | ||
|
|
||
| $(call recurse,all install,src config contrib/ivorysql_ora contrib/ora_btree_gist contrib/ora_btree_gin) | ||
| $(call recurse,all install,src config contrib/ivorysql_ora contrib/uuid-ossp contrib/ora_btree_gist contrib/ora_btree_gin) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify no duplicate recursion/builds for contrib/uuid-ossp.
You added contrib/uuid-ossp to top-level recurse targets alongside contrib. If contrib already enumerates uuid-ossp, this could double-build/clean/test. If not, this is fine.
If uuid-ossp is already in contrib’s SUBDIRS, consider removing the explicit contrib/uuid-ossp entries here to avoid duplicate recursion. Otherwise, keep as-is.
Also applies to: 48-48, 98-98
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Does contrib/Makefile include uuid-ossp?
fd -a contrib/Makefile
if fd -a contrib/Makefile >/dev/null; then
rg -n "SUBDIRS|DIRS" contrib/Makefile -A2 -B2
rg -n "uuid-ossp" contrib/Makefile || echo "uuid-ossp not listed in contrib/Makefile"
else
echo "contrib/Makefile not found"
fiLength of output: 1067
Remove duplicate recursion for contrib/uuid-ossp
The contrib/Makefile already adds uuid-ossp to SUBDIRS/ALWAYS_SUBDIRS, so having it explicitly in the top‐level recurse call will double-build/clean/test. Please remove the extra entries at lines 11, 48, and 98 in GNUmakefile.in.
- GNUmakefile.in:11,48,98
-$(call recurse,all install,src config contrib/ivorysql_ora contrib/uuid-ossp contrib/ora_btree_gist contrib/ora_btree_gin) +$(call recurse,all install,src config contrib/ivorysql_ora contrib/ora_btree_gist contrib/ora_btree_gin)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $(call recurse,all install,src config contrib/ivorysql_ora contrib/uuid-ossp contrib/ora_btree_gist contrib/ora_btree_gin) | |
| $(call recurse,all install,src config contrib/ivorysql_ora contrib/ora_btree_gist contrib/ora_btree_gin) |
🤖 Prompt for AI Agents
In GNUmakefile.in at lines 11, 48, and 98, the contrib/uuid-ossp directory is
redundantly included in the recurse calls, causing duplicate build, clean, and
test actions. Remove the explicit mentions of contrib/uuid-ossp from these lines
to avoid double-processing since it is already handled by the contrib/Makefile's
SUBDIRS/ALWAYS_SUBDIRS settings.
|
|
||
| SELECT uuid_generate_v4() <> uuid_generate_v4(); | ||
|
|
||
| SELECT sys_guid() <> sys_guid(); No newline at end of file |
There was a problem hiding this comment.
You need also change corresponding expected/ivy_uuid_ossp.out, otherwise the regression test would fail.
| include $(top_builddir)/src/Makefile.global | ||
|
|
||
| $(call recurse,all install,src config contrib/ivorysql_ora contrib/ora_btree_gist contrib/ora_btree_gin) | ||
| $(call recurse,all install,src config contrib/ivorysql_ora contrib/uuid-ossp contrib/ora_btree_gist contrib/ora_btree_gin) |
There was a problem hiding this comment.
Could you clarify why this file was modified？
There was a problem hiding this comment.
Hi, @OreoYang
The modification of this line is to automatically compile the uuid-ossp plugin with the aim of automatically loading the plugin for using the sys_guid function.
If "uuid - ossp" is removed, the following error message will be reported when executing initdb, indicating that uuid - ossp has not been compiled correctly.
There was a problem hiding this comment.
Hello @Carlos-Chong200 @OreoYang
Question has raised in the past about UUID function "in-core" or not (on psql-hackers). OSSP was on the topic.
It's a bit surprising to have dependency on contrib/extensions for initdb, does it make sense to make requirement closer to postgres kernel by moving required functions outside of contrib/ ?
There was a problem hiding this comment.
Hi, Cedric,
In addition to uuid extension, there are other extensions that are created in initdb, like plisql, ivorysql_ora.
This is IvorySQL's design and we try to develop Oracle compatibility features in extensions for flexibility.
| include $(top_builddir)/src/Makefile.global | ||
|
|
||
| $(call recurse,all install,src config contrib/ivorysql_ora contrib/ora_btree_gist contrib/ora_btree_gin) | ||
| $(call recurse,all install,src config contrib/ivorysql_ora contrib/uuid-ossp contrib/ora_btree_gist contrib/ora_btree_gin) |
Summary by CodeRabbit
New Features
sys_guid(), which generates a 16-byte binary UUID value.uuid-osspextension is now automatically installed if not present during certain setup scripts.Improvements
uuid-osspmodule.Tests
sys_guid()produce different values.