Reviewing files that changed from the base of the PR and between 05f115e and 3a230f2.

• src/cli/src/bench.rs (1 hunks)
• src/common/meta/src/error.rs (3 hunks)
• src/common/meta/src/kv_backend/etcd.rs (3 hunks)
• src/common/meta/src/kv_backend/memory.rs (2 hunks)
• src/common/meta/src/kv_backend/postgres.rs (14 hunks)
• src/common/meta/src/kv_backend/test.rs (10 hunks)
• src/common/meta/src/rpc/store.rs (4 hunks)
• src/log-store/src/raft_engine/backend.rs (3 hunks)
• src/meta-srv/src/bootstrap.rs (1 hunks)
• src/meta-srv/src/election/postgres.rs (9 hunks)

• GitHub Check: Toml Check
• GitHub Check: Check License Header
• GitHub Check: Clippy
• GitHub Check: Build GreptimeDB binaries (ubuntu-20.04)
• GitHub Check: Build GreptimeDB binary (profile-CI) (ubuntu-20.04)

src/common/meta/src/kv_backend/postgres.rs (1)

88-133: Enhancement: Introduction of SqlTemplateFactory Improves Modularity

The addition of SqlTemplateFactory and SqlTemplateSet encapsulates SQL template generation, enhancing code modularity and readability. This refactoring simplifies the creation and management of SQL statements and reduces redundancy.

src/meta-srv/src/election/postgres.rs (3)

333-337: Improved Function Signatures by Using &str Instead of &String

Changing the parameter types from &String to &str in methods like get_value_with_lease simplifies the code and aligns with Rust best practices by reducing unnecessary indirection.

---

Line range hint 65-70: Ensure Safe SQL Concatenation in CAS_WITH_EXPIRE_TIME

In the CAS_WITH_EXPIRE_TIME SQL statement, concatenation is performed using parameters:

v=convert_to($3 || $4 || TO_CHAR(CURRENT_TIMESTAMP + INTERVAL '1 second' * $5, 'YYYY-MM-DD HH24:MI:SS.MS'), 'UTF8')

Please ensure that the concatenation of parameters is secure against SQL injection and that all parameters ($3, $4, $5) are properly sanitized, especially if they include any untrusted input.

✅ Verification successful

SQL Concatenation in CAS_WITH_EXPIRE_TIME is Secure

The implementation is safe against SQL injection as it properly uses PostgreSQL's parameterized queries with type-safe parameter binding through Rust's ToSql trait. The string concatenation occurs at the database level after parameter binding, and the timestamp formatting uses a hardcoded format string.

🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found

---

Script:

#!/bin/bash
# Find the complete implementation containing these SQL statements
ast-grep --pattern 'const CAS_WITH_EXPIRE_TIME: &str = r#"'

# Find where these constants are used
rg "CAS_WITH_EXPIRE_TIME" -A 5

Length of output: 982

---

54-57: Verify Safe Handling of SQL Parameter Concatenation

In the PUT_IF_NOT_EXISTS_WITH_EXPIRE_TIME SQL statement, values are concatenated using the || operator with parameters $2, $3, and a formatted date:

VALUES($1, convert_to($2 || $3 || TO_CHAR(CURRENT_TIMESTAMP + INTERVAL '1 second' * $4, 'YYYY-MM-DD HH24:MI:SS.MS'), 'UTF8'))

While parameterization is used, please verify that concatenating parameters in this manner does not introduce SQL injection vulnerabilities, especially if any of the parameters ($2, $3, $4) can contain untrusted data.

✅ Verification successful

SQL Parameter Concatenation is Secure

The implementation is safe from SQL injection as it:

• Uses proper parameter binding via Postgres's client library
• Performs string concatenation within Postgres after parameter binding, not through string interpolation
• Has strongly typed parameters (bytes for key, fixed separator constant, and numeric TTL)

🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found

---

Script:

#!/bin/bash
# Find the complete SQL statement and its usage
rg -A 10 -B 10 "PUT_IF_NOT_EXISTS_WITH_EXPIRE_TIME" src/

# Look for the function/method that uses this SQL statement
ast-grep --pattern 'const PUT_IF_NOT_EXISTS_WITH_EXPIRE_TIME: &str'

Length of output: 4267

src/cli/src/bench.rs (1)

81-83: Update to PgStore::with_url Reflects API Changes

The call to PgStore::with_url has been updated to include the table_name parameter:

PgStore::with_url(postgres_addr, "greptime_metakv", 128)

This change aligns with the updated method signature in PgStore. Please ensure that all usages of with_url are updated accordingly throughout the codebase to prevent compilation errors.

src/common/meta/src/kv_backend/memory.rs (1)

358-358: LGTM! Improved memory efficiency in tests.

The changes correctly modify the test functions to use references instead of taking ownership, which is more efficient and aligns with Rust's ownership model.

Also applies to: 379-379, 386-386, 391-397

src/common/meta/src/kv_backend/etcd.rs (1)

594-594: LGTM! Consistent test improvements.

The changes correctly:

1. Use references in test functions for better memory efficiency
2. Add proper cleanup after tests

Also applies to: 621-622, 631-632, 639-644

src/common/meta/src/kv_backend/test.rs (2)

64-75: LGTM! Improved error reporting.

The enhanced assertion message now includes the prefix in UTF-8 format, which will help with debugging test failures.

---

355-355: LGTM! Consistent use of references in test functions.

The changes consistently modify function signatures to use &impl KvBackend instead of taking ownership, which:

1. Improves memory efficiency
2. Follows Rust's ownership best practices
3. Maintains consistency across the test suite

Also applies to: 359-359, 410-410, 414-414, 454-454, 479-479, 509-509, 538-538, 578-578, 618-618

src/log-store/src/raft_engine/backend.rs (1)

647-647: LGTM! Improved efficiency by passing backend by reference.

The changes consistently update test function calls to pass the backend by reference instead of by value, which is more efficient and aligns with Rust's ownership model.

Also applies to: 674-674, 683-683

src/common/meta/src/error.rs (2)

652-654: LGTM! Enhanced error context for Postgres execution failures.

Including the SQL statement in the error message improves debuggability by providing more context about the failing operation.

---

812-821: LGTM! Added helper method to identify Postgres serialization errors.

The is_serialization_error method provides a clean way to check for specific Postgres serialization failures, which is useful for handling transient errors in transaction retries.

🧰 Tools

🪛 GitHub Check: Check (ubuntu-20.04)

[failure] 817-817:
failed to resolve: use of undeclared crate or module tokio_postgres

---

[failure] 815-815:
no variant named PostgresTransaction found for enum error::Error

src/common/meta/src/rpc/store.rs (2)

269-269: LGTM! Added Default trait implementations.

Adding the Default trait to these response types improves ergonomics by allowing them to be created with default values.

Also applies to: 428-428, 512-512

---

757-768: LGTM! Enhanced DeleteRangeResponse API.

The new constructor and setter methods provide a more ergonomic way to create and modify DeleteRangeResponse instances.