Master branch: add display ivorysql version in psql welcome banner and select(). #940
Master branch: add display ivorysql version in psql welcome banner and select(). #940gaoxueyu merged 3 commits intoIvorySQL:masterfrom
Conversation
WalkthroughAdds a PACKAGE_IVORYSQL_VERSION ("5beta1") to build configs and headers and embeds it into PG_VERSION_STR, causing runtime/version outputs (e.g., psql banner and version()) to display the IvorySQL version. Changes are declarative and formatting-only in client code. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant AC as Configure/Build
participant Header as Generated header (pg_config.h)
participant psql as psql binary
participant User as Stdout/User
Note over AC,Header: Configure/build time
AC->>Header: define PACKAGE_IVORYSQL_VERSION = "5beta1"
Header-->>psql: compiled-in macro available
Note over psql,User: Runtime (psql start / version())
psql->>psql: assemble PG_VERSION_STR with IvorySQL macro
psql->>User: print "PostgreSQL X (IvorySQL 5beta1) on ...", shown in banner/version output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
meson.build (1)
31-31: Avoid hardcoding IvorySQL version; centralize or make it configurable.Hardcoding '5beta1' risks divergence from autoconf. Prefer a single source of truth or a meson option (e.g., -Divorysql_version=…).
Example (meson side):
- Define an option in meson_options.txt:
option('ivorysql_version', type: 'string', value: '5beta1')- Use it here:
ivorysql_version = get_option('ivorysql_version')src/include/pg_config.h.in (1)
630-632: Style nit: match comment formatting used elsewhere.Use a space after /* and before */ for consistency with nearby comments.
-/*IvorySQL version as a string*/ +/* IvorySQL version as a string */src/bin/psql/command.c (3)
1041-1041: Superfluous blank line.This blank line after
getDbCompatibleMode(pset.db);appears unnecessary and doesn't match the surrounding code style. Consider removing it.
5542-5542: Remove extraneous blank line.This blank line in
exec_command_parserappears to be accidental and doesn't improve readability. Consider removing it for consistency with the surrounding code.
5748-5761: Inconsistent variable declaration placement.The variable declarations
char wbuff[32];are now separated from the rest of the code block with blank lines. This formatting change doesn't improve readability and is inconsistent with the rest of the function. Consider reverting to a more compact style that matches the surrounding code.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
configure(1 hunks)configure.ac(1 hunks)meson.build(3 hunks)src/bin/psql/command.c(7 hunks)src/include/pg_config.h.in(1 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
- configure
🧰 Additional context used
🧬 Code graph analysis (1)
src/bin/psql/command.c (1)
src/backend/utils/mmgr/mcxt.c (1)
pstrdup(1702-1706)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: build (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
🔇 Additional comments (3)
meson.build (1)
158-158: Good: macro exported for headers.Defining PACKAGE_IVORYSQL_VERSION via set_quoted is correct for pg_config.h generation under meson.
src/bin/psql/command.c (2)
45-45: LGTM: Oracle scanner header included.The include for
ora_psqlscanslash.his appropriately placed alongside the relatedpsqlscanslash.hheader and is used consistently in the Oracle mode code paths.
297-298: LGTM: Consistent Oracle mode handling.The Oracle mode branch correctly mirrors the PostgreSQL mode pattern, maintaining consistency in the codebase.
| AC_DEFINE_UNQUOTED(PG_VERSION_STR, | ||
| ["PostgreSQL $PG_VERSION on $host, compiled by $cc_string, `expr $ac_cv_sizeof_void_p \* 8`-bit"], | ||
| ["PostgreSQL $PG_VERSION (IvorySQL $PACKAGE_IVORYSQL_VERSION) on $host, compiled by $cc_string, `expr $ac_cv_sizeof_void_p \* 8`-bit"], | ||
| [A string containing the version number, platform, and C compiler]) |
There was a problem hiding this comment.
🧩 Analysis chain
Define PACKAGE_IVORYSQL_VERSION for autoconf builds; currently likely undefined in pg_config.h.
You use $PACKAGE_IVORYSQL_VERSION in PG_VERSION_STR, but there’s no AC_DEFINE to emit a C macro for it. Given pg_config.h.in now declares #undef PACKAGE_IVORYSQL_VERSION, autoconf builds will leave it undefined.
Add a definition near other version metadata (after PG_VERSION):
AC_DEFINE_UNQUOTED(PG_VERSION, "$PG_VERSION", [PostgreSQL version as a string])
+
+# IvorySQL version as a string (exported to pg_config.h)
+AC_DEFINE_UNQUOTED(PACKAGE_IVORYSQL_VERSION, ["$PACKAGE_IVORYSQL_VERSION"], [IvorySQL version as a string])Also ensure $PACKAGE_IVORYSQL_VERSION is set (AC_SUBST or default). For consistency with meson, consider a single source (e.g., m4_define or a VERSION file) and plumb both build systems from it.
---
🏁 Script executed:
```shell
#!/bin/bash
# Extract lines 2545-2547 from configure.ac and surrounding context
sed -n '2540,2550p' configure.ac
Length of output: 477
🏁 Script executed:
#!/bin/bash
# Search for AC_DEFINE_UNQUOTED or AC_DEFINE related to PACKAGE_IVORYSQL_VERSION
rg -n 'AC_DEFINE.*PACKAGE_IVORYSQL_VERSION' configure.acLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for AC_SUBST assignments for PACKAGE_IVORYSQL_VERSION
rg -n 'AC_SUBST.*PACKAGE_IVORYSQL_VERSION|PACKAGE_IVORYSQL_VERSION.*=' configure.acLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check pg_config.h.in for #undef PACKAGE_IVORYSQL_VERSION
rg -n 'PACKAGE_IVORYSQL_VERSION' src/include/pg_config.h.inLength of output: 35
🏁 Script executed:
#!/bin/bash
# Search entire configure.ac for where PACKAGE_IVORYSQL_VERSION is set
rg -n -C 3 'PACKAGE_IVORYSQL_VERSION' configure.ac | head -100Length of output: 379
🏁 Script executed:
#!/bin/bash
# Look for VERSION file or m4_define for ivorysql version
fd -e 'VERSION' -o -name '*version*' | grep -i ivoryLength of output: 230
🏁 Script executed:
#!/bin/bash
# Search the first 500 lines of configure.ac for PACKAGE_IVORYSQL_VERSION
head -500 configure.ac | rg -n 'PACKAGE_IVORYSQL_VERSION'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for all AC_SUBST calls in configure.ac
rg -n 'AC_SUBST' configure.ac | head -20Length of output: 645
🏁 Script executed:
#!/bin/bash
# Check if configure.in or configure.ac.in exists and contains PACKAGE_IVORYSQL_VERSION
find . -maxdepth 2 -name 'configure.*' -type fLength of output: 14
🏁 Script executed:
#!/bin/bash
# Search for where PACKAGE_IVORYSQL_VERSION might be exported or used in other build files
rg -n 'PACKAGE_IVORYSQL_VERSION' --type-list | head -5Length of output: 389
🏁 Script executed:
#!/bin/bash
# Look for meson.build or other version definitions
fd -e 'meson.build' -o -name 'meson.options' | xargs rg -l 'ivorysql.*version|PACKAGE_IVORYSQL_VERSION' 2>/dev/null | head -5Length of output: 323
🏁 Script executed:
#!/bin/bash
# Check if there's a VERSION file or similar at root
ls -la | grep -i versionLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for IVORYSQL_VERSION (without PACKAGE_ prefix)
rg -n 'IVORYSQL_VERSION' configure.ac | head -20Length of output: 159
🏁 Script executed:
#!/bin/bash
# Search meson.build for PACKAGE_IVORYSQL_VERSION and related version handling
rg -n -C 5 'PACKAGE_IVORYSQL_VERSION|ivorysql.*version' meson.buildLength of output: 1154
🏁 Script executed:
#!/bin/bash
# Check if PACKAGE_IVORYSQL_VERSION is set as a shell variable assignment before line 2546
sed -n '1,2545p' configure.ac | rg -n 'PACKAGE_IVORYSQL_VERSION\s*='Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for where version variables are typically set in configure.ac (near top, often AC_INIT)
head -100 configure.ac | rg -n 'AC_INIT|VERSION|version'Length of output: 1047
🏁 Script executed:
#!/bin/bash
# Check for any shell variable assignments involving version at the top
head -150 configure.acLength of output: 4850
Define PACKAGE_IVORYSQL_VERSION in configure.ac for autoconf builds; currently undefined in pg_config.h.
The variable $PACKAGE_IVORYSQL_VERSION is used in PG_VERSION_STR (line 2546), but no AC_DEFINE_UNQUOTED exists for it in configure.ac. While meson.build properly defines it (line 158: cdata.set_quoted('PACKAGE_IVORYSQL_VERSION', ivorysql_version)), the autoconf build leaves it undefined in pg_config.h, causing inconsistency and potential breakage for any C code referencing the macro.
Add near the PG_VERSION definition (around line 43):
AC_DEFINE_UNQUOTED(PG_VERSION, "$PG_VERSION", [PostgreSQL version as a string])
+AC_DEFINE_UNQUOTED(PACKAGE_IVORYSQL_VERSION, ["5beta1"], [IvorySQL version as a string])Also initialize PACKAGE_IVORYSQL_VERSION as a shell variable at the top of configure.ac (or extract from a source like meson.build to maintain single source of truth).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In configure.ac around lines 2545-2547, PG_VERSION_STR references
PACKAGE_IVORYSQL_VERSION but that macro is not defined for autoconf builds; add
an AC_DEFINE_UNQUOTED(PACKAGE_IVORYSQL_VERSION, ...) alongside the PG_VERSION
definition so PACKAGE_IVORYSQL_VERSION is written into pg_config.h, and at the
top of configure.ac (around line 43) initialize a shell variable
PACKAGE_IVORYSQL_VERSION (or extract it from meson.build/single source of truth)
so the configure script can substitute the value into the AC_DEFINE; ensure the
variable is set before the AC_DEFINE and use a quoted string appropriate for
AC_DEFINE_UNQUOTED so C code sees the same value as meson builds.
| cdata.set_quoted('PG_VERSION_STR', | ||
| 'PostgreSQL (IvorySQL @6@) @0@ on @1@-@2@, compiled by @3@-@4@, @5@-bit'.format( | ||
| pg_version, host_machine.cpu_family(), host_system, | ||
| cc.get_id(), cc.version(), cdata.get('SIZEOF_VOID_P') * 8, cdata.get('PACKAGE_IVORYSQL_VERSION') | ||
| cc.get_id(), cc.version(), cdata.get('SIZEOF_VOID_P') * 8, ivorysql_version | ||
| ) | ||
| ) |
There was a problem hiding this comment.
PG_VERSION_STR order mismatch with configure; put PostgreSQL version before IvorySQL.
configure.ac emits “PostgreSQL $PG_VERSION (IvorySQL …) …”, while meson currently formats “PostgreSQL (IvorySQL …) $PG_VERSION …”. This will produce inconsistent version() output depending on build system.
Fix by swapping placeholders:
- cdata.set_quoted('PG_VERSION_STR',
- 'PostgreSQL (IvorySQL @6@) @0@ on @1@-@2@, compiled by @3@-@4@, @5@-bit'.format(
- pg_version, host_machine.cpu_family(), host_system,
- cc.get_id(), cc.version(), cdata.get('SIZEOF_VOID_P') * 8, ivorysql_version
- )
- )
+ cdata.set_quoted('PG_VERSION_STR',
+ 'PostgreSQL @0@ (IvorySQL @6@) on @1@-@2@, compiled by @3@-@4@, @5@-bit'.format(
+ pg_version, host_machine.cpu_family(), host_system,
+ cc.get_id(), cc.version(), cdata.get('SIZEOF_VOID_P') * 8, ivorysql_version
+ )
+ )📝 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.
| cdata.set_quoted('PG_VERSION_STR', | |
| 'PostgreSQL (IvorySQL @6@) @0@ on @1@-@2@, compiled by @3@-@4@, @5@-bit'.format( | |
| pg_version, host_machine.cpu_family(), host_system, | |
| cc.get_id(), cc.version(), cdata.get('SIZEOF_VOID_P') * 8, cdata.get('PACKAGE_IVORYSQL_VERSION') | |
| cc.get_id(), cc.version(), cdata.get('SIZEOF_VOID_P') * 8, ivorysql_version | |
| ) | |
| ) | |
| cdata.set_quoted('PG_VERSION_STR', | |
| 'PostgreSQL @0@ (IvorySQL @6@) on @1@-@2@, compiled by @3@-@4@, @5@-bit'.format( | |
| pg_version, host_machine.cpu_family(), host_system, | |
| cc.get_id(), cc.version(), cdata.get('SIZEOF_VOID_P') * 8, ivorysql_version | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In meson.build around lines 2979 to 2984, the PG_VERSION_STR places the IvorySQL
version before the PostgreSQL version, which mismatches configure.ac; swap the
placeholders so the PostgreSQL version (pg_version) appears immediately after
"PostgreSQL" and IvorySQL version (ivorysql_version) appears in the parenthesis,
updating the .format() argument order to match the intended string layout.
| /* For version match, only print psql banner on startup. */ | ||
| else if (in_startup) | ||
| printf("%s (%s)\n", pset.progname, PG_VERSION); | ||
| printf("%s(%s), %s(%s)\n", pset.progname, PG_VERSION, "ivorysql", PACKAGE_IVORYSQL_VERSION); |
There was a problem hiding this comment.
🧩 Analysis chain
Fix formatting and use constants for the startup banner.
The banner format string has spacing issues and uses a hardcoded string:
- Missing space between program name and version:
%s(%s)should be%s (%s) - The string
"ivorysql"is hardcoded and should be defined as a constant for maintainability
Apply this diff to improve the format:
- printf("%s(%s), %s(%s)\n", pset.progname, PG_VERSION, "ivorysql", PACKAGE_IVORYSQL_VERSION);
+ printf("%s (%s), %s (%s)\n", pset.progname, PG_VERSION, "ivorysql", PACKAGE_IVORYSQL_VERSION);Additionally, consider defining a constant like PACKAGE_IVORYSQL_NAME for "ivorysql" to avoid hardcoding the product name.
Please verify that PACKAGE_IVORYSQL_VERSION is properly defined in the build configuration (likely in pg_config.h.in):
🏁 Script executed:
#!/bin/bash
# Verify PACKAGE_IVORYSQL_VERSION is defined in configuration headers
rg -n "PACKAGE_IVORYSQL_VERSION" --type=c --type=hLength of output: 186
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for other version string definitions and usage patterns
rg -n "PACKAGE_IVORYSQL" --type=c --type=h -B2 -A2
Length of output: 622
🏁 Script executed:
#!/bin/bash
# Check for similar version/banner printf patterns
rg -n 'printf.*%s.*%s.*VERSION' --type=c -B1 -A1Length of output: 1446
🏁 Script executed:
#!/bin/bash
# Look for IVORYSQL-related constants
rg -n "ivorysql" --type=c --type=h -i | head -20Length of output: 1907
Fix formatting to match PostgreSQL conventions for the startup banner.
The spacing issue is valid: PostgreSQL tools consistently use a space before the version in parentheses (e.g., pgbench uses "%s (%s)"), but the current code omits the space. Additionally, hardcoding "ivorysql" reduces maintainability.
Apply this change:
- printf("%s(%s), %s(%s)\n", pset.progname, PG_VERSION, "ivorysql", PACKAGE_IVORYSQL_VERSION);
+ printf("%s (%s), %s (%s)\n", pset.progname, PG_VERSION, "ivorysql", PACKAGE_IVORYSQL_VERSION);Consider defining a constant for the product name to avoid hardcoding "ivorysql".
📝 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.
| printf("%s(%s), %s(%s)\n", pset.progname, PG_VERSION, "ivorysql", PACKAGE_IVORYSQL_VERSION); | |
| printf("%s (%s), %s (%s)\n", pset.progname, PG_VERSION, "ivorysql", PACKAGE_IVORYSQL_VERSION); |
🤖 Prompt for AI Agents
In src/bin/psql/command.c around line 4435, the startup banner omits the
conventional space before the version parenthesis and hardcodes the product
name; change the format string to include a space before each version (e.g., "%s
(%s), %s (%s)\n") and replace the literal "ivorysql" with a product-name
constant (define PRODUCT_NAME and use it instead) so the banner prints as
pset.progname (PG_VERSION), PRODUCT_NAME (PACKAGE_IVORYSQL_VERSION).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
configure.ac (2)
33-34: Version duplication between build systems requires maintenance coordination.The IvorySQL version is hardcoded here as '5beta1' and also appears in meson.build. When bumping versions, both locations must be updated consistently. Consider extracting to a shared VERSION file that both build systems can read, or document the duplication clearly in release procedures.
43-43: Critical fix implemented correctly; minor style refinements suggested.This AC_DEFINE_UNQUOTED properly addresses the past review's critical concern by defining PACKAGE_IVORYSQL_VERSION for C code. The shell variable from line 34 is correctly expanded here.
Optional improvements:
- Remove space before comma:
"$PACKAGE_IVORYSQL_VERSION" ,→"$PACKAGE_IVORYSQL_VERSION",- Update description for accuracy:
[IvorySQL version number]→[IvorySQL version as a string](matches line 48's pattern)- For consistency with line 48, consider removing square brackets around first argument (both styles work, but consistency helps)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
configure.ac(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: contrib_regression
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: pg_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
🔇 Additional comments (2)
configure.ac (2)
27-27: LGTM - Copyright notice added correctly.The IvorySQL copyright follows the same format as the PostgreSQL copyright above it.
2550-2552: Version string correctly enhanced with IvorySQL branding.The updated PG_VERSION_STR properly incorporates the IvorySQL version alongside the PostgreSQL version, achieving the PR's objective of displaying "PostgreSQL 18beta1 (IvorySQL 5beta1) on ..." in psql and version() output.
…compatibility with extensions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/bin/psql/command.c (1)
5746-5762: Inconsistent return statement style.The return statements at lines 5748, 5750, and 5752 use parentheses around the return expression (
return (pstrdup(...))), while line 5760 does not (return pstrdup(wbuff)). In C, parentheses around return values are unnecessary and the inconsistency reduces readability.Apply this diff to make the style consistent:
if (popt->topt.expanded_header_width_type == PRINT_XHEADER_FULL) - return (pstrdup("full")); + return pstrdup("full"); else if (popt->topt.expanded_header_width_type == PRINT_XHEADER_COLUMN) - return (pstrdup("column")); + return pstrdup("column"); else if (popt->topt.expanded_header_width_type == PRINT_XHEADER_PAGE) - return (pstrdup("page")); + return pstrdup("page");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/bin/psql/command.c(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/bin/psql/command.c (1)
src/backend/utils/mmgr/mcxt.c (1)
pstrdup(1702-1706)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: oracle_pg_regression (ubuntu-latest)
- GitHub Check: oracle_regression (ubuntu-latest)
- GitHub Check: meson_build (ubuntu-latest)
- GitHub Check: contrib_regression
- GitHub Check: pg_regression (ubuntu-latest)
Summary by CodeRabbit
New Features
Chores