out_pgsql: refactor plugin logic to follow new config maps and threads#11700
Open
sxd wants to merge 2 commits intofluent:masterfrom
Open
out_pgsql: refactor plugin logic to follow new config maps and threads#11700sxd wants to merge 2 commits intofluent:masterfrom
sxd wants to merge 2 commits intofluent:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConfiguration now uses a flb_config_map; plugin builds and prepares a single parameterized INSERT at init; flush decodes msgpack with flb_log_event_decoder, formats timestamps and JSON bodies, executes per-event PQexecPrepared inside an explicit transaction on a single PGconn; connection pool removed; helpers and unit tests added. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Plugin
participant Decoder as flb_log_event_decoder
participant LibPQ as libpq (PGconn)
participant DB as PostgreSQL
Plugin->>Decoder: decode msgpack batch → events
loop per event
Decoder-->>Plugin: event (timestamp, body)
Plugin->>Plugin: format timestamp & body JSON
Plugin->>LibPQ: PQexecPrepared(insert_stmt, params)
LibPQ->>DB: execute prepared statement
DB-->>LibPQ: result (OK / ERROR)
LibPQ-->>Plugin: result status
end
alt all events succeeded
Plugin->>LibPQ: execute "COMMIT"
else any error
Plugin->>LibPQ: execute "ROLLBACK"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b39a245f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/out_pgsql/pgsql.c`:
- Around line 430-431: The branch is only checking for == 1 but
pgsql_next_connection() can return -1 on failure, so change the condition to
check for non-zero: replace the if (pgsql_next_connection(ctx) == 1) {
FLB_OUTPUT_RETURN(FLB_RETRY); } with a non-zero check (if
(pgsql_next_connection(ctx) != 0) { FLB_OUTPUT_RETURN(FLB_RETRY); }) to ensure
failures (e.g., -1) also return FLB_RETRY before calling PQstatus() on
ctx->conn_current.
- Around line 468-500: Wrap the entire chunk processing loop in an explicit
PostgreSQL transaction: before entering the flb_log_event_decoder_next loop call
PQexec(ctx->pg_conn, "BEGIN") (or use the existing helper if any), on successful
completion after the loop call COMMIT, and on any failure path (any non-zero
return from pgsql_insert_record, json == NULL, or decoder_result !=
FLB_EVENT_DECODER_SUCCESS) call ROLLBACK before returning FLB_RETRY or
translating the decoder result; update the error paths that call flb_plg_error,
pgsql_free_body_json, flb_log_event_decoder_destroy, and FLB_OUTPUT_RETURN so
they perform ROLLBACK using the same pg connection, ensuring
pgsql_insert_record, flb_log_event_decoder_get_last_result and
pgsql_translate_decoder_result remain intact.
In `@tests/internal/pgsql.c`:
- Around line 45-210: Add two unit tests exercising cb_pgsql_flush failure
paths: (1) a mid-batch insert failure test that builds a minimal struct
flb_pgsql_config and a fake/initialized struct flb_pgsql_conn where the
connection is made to fail (simulate a failing PG exec state) then call
cb_pgsql_flush and assert it returns FLB_RETRY and that reconnect/reprepare
behavior is triggered (e.g., connection marked for reconnect or ctx.conn_current
moved); (2) a decoder terminal error test that drives the event decoder to
return FLB_EVENT_DECODER_ERROR_WRONG_ROOT_TYPE during iteration and assert
cb_pgsql_flush returns FLB_ERROR (or the expected terminal code) and that
resources are cleaned up. Use existing test helpers/assertions (TEST_CHECK) and
reference cb_pgsql_flush, struct flb_pgsql_config, struct flb_pgsql_conn, and
FLB_EVENT_DECODER_ERROR_WRONG_ROOT_TYPE/FLB_RETRY to locate where to hook the
simulated failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 514328fe-44be-4e01-aa2a-07f1194ddc64
📒 Files selected for processing (5)
plugins/out_pgsql/pgsql.cplugins/out_pgsql/pgsql.hplugins/out_pgsql/pgsql_connections.ctests/internal/CMakeLists.txttests/internal/pgsql.c
sxd
force-pushed
the
out_pgsql_to_new_model
branch
from
1b39a24 to
be8d652
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/out_pgsql/pgsql_connections.c`:
- Around line 53-63: PQstatus() is being called on conn without checking for
NULL after PQsetdbLogin; update the connection handling in the block that calls
PQsetdbLogin(ctx->db_hostname, ctx->db_port, ctx->conn_options, NULL,
ctx->db_name, ctx->db_user, ctx->db_passwd) to first test if conn == NULL and
handle that error path (log via pgsql_log_conn_error or a suitable logger and
avoid calling PQstatus/PQfinish on a NULL pointer), then only call
PQstatus(conn) and PQfinish(conn) when conn is non-NULL to prevent null pointer
dereference.
In `@plugins/out_pgsql/pgsql.c`:
- Around line 485-493: The decoder init block currently treats any
flb_log_event_decoder_init() failure as FLB_RETRY; change it to call
pgsql_translate_decoder_result(decoder_result) and use its returned FLB_* code
so terminal decoder errors map to FLB_ERROR instead of always retrying.
Concretely, after calling flb_log_event_decoder_init(&log_decoder, (char *)
event_chunk->data, event_chunk->size) and getting decoder_result !=
FLB_EVENT_DECODER_SUCCESS, replace the unconditional FLB_RETRY return with a
translated return value from pgsql_translate_decoder_result(decoder_result)
while keeping the flb_plg_error(ctx->ins, ...) log call intact; this ensures
decoder_result is mapped consistently with the post-loop handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83d5dff7-164e-4fd2-b641-7172f1610175
📒 Files selected for processing (5)
plugins/out_pgsql/pgsql.cplugins/out_pgsql/pgsql.hplugins/out_pgsql/pgsql_connections.ctests/internal/CMakeLists.txttests/internal/pgsql.c
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/internal/CMakeLists.txt
sxd
force-pushed
the
out_pgsql_to_new_model
branch
from
be8d652 to
431d069
Compare
sxd
force-pushed
the
out_pgsql_to_new_model
branch
from
fbd5809 to
75e4133
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/internal/pgsql.c (1)
575-587: Make this a real precision regression test.This case uses
.500000000, which is exactly the kind of timestamp that hides the floating-point conversion path above. Using a value such as.123456789would turn this into a regression test for precision loss inpgsql_format_timestamp().As per coding guidelines, "Add or update tests for behavior changes, especially protocol parsing and encoder/decoder paths".Suggested test tweak
- flb_time_set(×tamp, 1700000000, 500000000); + flb_time_set(×tamp, 1700000000, 123456789); result = pgsql_format_timestamp(buffer, sizeof(buffer), ×tamp); TEST_CHECK(result > 0); - TEST_CHECK(strcmp(buffer, "1700000000.500000000") == 0); + TEST_CHECK(strcmp(buffer, "1700000000.123456789") == 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/internal/pgsql.c` around lines 575 - 587, The test test_pgsql_format_timestamp currently uses a fractional part of .500000000 which masks floating-point conversion issues; update the test to use a non-trivial fractional value (e.g. .123456789) so pgsql_format_timestamp is exercised for precision loss, set flb_time_set(×tamp, 1700000000, 123456789), call pgsql_format_timestamp(buffer, sizeof(buffer), ×tamp) and assert the result is >0 and strcmp(buffer, "1700000000.123456789") == 0 to catch any regression in precision handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/out_pgsql/pgsql.c`:
- Around line 48-75: The insert path currently converts event times via
flb_time_to_double in pgsql_format_timestamp and binds the lossy double as $2;
change the query and binding to preserve exact timestamps instead—either (A)
update pgsql_build_insert_query to accept a text timestamp parameter (bind the
formatted ISO8601 timestamp string as $2 and use the DB's parse function, e.g.,
VALUES ($1, $2::timestamptz, $3::jsonb)) and adjust pgsql_format_timestamp to
emit the full-text timestamp, or (B) modify pgsql_build_insert_query to accept
two integer parameters (sec and nsec) and construct the timestamp in SQL (e.g.,
DATE '1970-01-01' + ($2::bigint * INTERVAL '1 second' + $3::bigint * INTERVAL '1
nanosecond')), updating pgsql_format_timestamp to bind sec and nsec instead of
flb_time_to_double; pick one approach and update the prepared statement,
parameter positions, and bindings accordingly.
---
Nitpick comments:
In `@tests/internal/pgsql.c`:
- Around line 575-587: The test test_pgsql_format_timestamp currently uses a
fractional part of .500000000 which masks floating-point conversion issues;
update the test to use a non-trivial fractional value (e.g. .123456789) so
pgsql_format_timestamp is exercised for precision loss, set
flb_time_set(×tamp, 1700000000, 123456789), call
pgsql_format_timestamp(buffer, sizeof(buffer), ×tamp) and assert the result
is >0 and strcmp(buffer, "1700000000.123456789") == 0 to catch any regression in
precision handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da7795e7-2974-4f76-9f5c-84bc04e764df
📒 Files selected for processing (5)
plugins/out_pgsql/pgsql.cplugins/out_pgsql/pgsql.hplugins/out_pgsql/pgsql_connections.ctests/internal/CMakeLists.txttests/internal/pgsql.c
✅ Files skipped from review due to trivial changes (1)
- tests/internal/CMakeLists.txt
Contributor
|
Hi, thanks for your contribution. We need to address this type of linter error: Usually, we split this type of commits into two of the commits at least. CurrentIdealCould you address this linter failure? |
sxd
force-pushed
the
out_pgsql_to_new_model
branch
from
75e4133 to
335b912
Compare
The plugin was too old and required an update as follow: * Use of the config map for the config options * Decoder for the log events * Use of the internal instances to avoid having a pool of connections Signed-off-by: Jonathan Gonzalez V. <jonathan.abdiel@gmail.com>
Signed-off-by: Jonathan Gonzalez V. <jonathan.abdiel@gmail.com>
sxd
force-pushed
the
out_pgsql_to_new_model
branch
from
335b912 to
5c95a0d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The plugin was too old and required an update as follow:
test_local_out_pgsql.valgrind.log
Also, some unit tests were added to make it easy to maintain the plugin
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests