Skip to content

feat: fix backward compatibility pg 14 for postgres trigger #5851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

dieriba
Copy link
Contributor

@dieriba dieriba commented Jun 1, 2025

Important

Fixes PostgreSQL 14 compatibility for triggers by updating backend logic and frontend UI to handle limitations and provide better error handling.

  • Backend:
    • Add get_postgres_version_internal() and get_postgres_version() in handler.rs to fetch PostgreSQL version.
    • Modify create_pg_publication() and create_logical_replication_slot() in mod.rs to handle PostgreSQL 14 limitations.
    • Update set_postgres_trigger_config() in capture.rs to support basic and advanced modes.
    • Add error handling for unsupported features in PostgreSQL 14 in mod.rs.
  • Frontend:
    • Update PostgresTriggerEditorInner.svelte to handle PostgreSQL 14 limitations in UI.
    • Modify RelationPicker.svelte to disable column-specific tracking for PostgreSQL 14.
    • Adjust PublicationPicker.svelte and SlotPicker.svelte for better user feedback and loading states.
    • Enhance GcpTriggerEditorConfigSection.svelte with loading indicators and error handling.

This description was created by Ellipsis for 75c6b37. You can customize this summary. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed everything up to da0a50a in 2 minutes and 0 seconds. Click for details.
  • Reviewed 2658 lines of code in 27 files
  • Skipped 0 files when reviewing.
  • Skipped posting 12 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/triggers.rs:35
  • Draft comment:
    The query for primary_schedule uses the same parameter ($1) for both 'path' and 'script_path'. Verify if this is intended, as it forces these two values to be identical. If they should be separate, adjust the query parameters accordingly.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/src/lib/components/triggers/CaptureWrapper.svelte:188
  • Draft comment:
    The reactive effect $effect(() => { args && (captureActive = false) }) resets the captureActive flag whenever 'args' is truthy. Ensure that this behavior is intended—as it may inadvertently disable capture when arguments are provided.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/src/lib/components/triggers/postgres/RelationPicker.svelte:23
  • Draft comment:
    The reactive block mutates the 'relations' prop directly when pg14 is true. Mutating props can lead to unexpected side effects. Consider cloning or deriving a local copy before making modifications.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% In Svelte, mutating props directly is actually a valid pattern, unlike React. Svelte's reactivity system handles prop mutations gracefully. The code is using Svelte's reactive statements ($:) which is the idiomatic way to handle prop changes. Creating defensive copies would add complexity without clear benefits in this case. The comment comes from a React-centric view where prop mutation is an anti-pattern. But this is Svelte code where different patterns apply. While prop mutation would be problematic in React, Svelte's reactivity system is designed to handle prop mutations properly. The current implementation is idiomatic Svelte code. The comment should be deleted as it suggests applying React patterns to Svelte code where they don't apply. The current implementation is correct for Svelte.
4. frontend/src/lib/components/triggers/postgres/utils.ts:151
  • Draft comment:
    The 'getDefaultTableToTrack' function returns a default object based on the 'pg14' flag. Consider adding inline documentation explaining why for pg14 an array with a default empty table is returned versus an empty array otherwise.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
5. backend/.vscode/settings.json:13
  • Draft comment:
    Typo detected: "restructring" appears to be a misspelling of "restructuring". Please update the text to correct the typo.
  • Reason this comment was not posted:
    Comment was on unchanged code.
6. backend/windmill-api/src/postgres_triggers/handler.rs:837
  • Draft comment:
    Typographical error in the comment: consider rephrasing "if all table is none it means that the publication do not exist in the database" to "if all_table is None, it means that the publication does not exist in the database".
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment correctly identifies a grammatical error, comments about purely cosmetic issues like grammar in code comments are not substantive enough to warrant a PR comment. The meaning is still clear despite the grammatical errors. This doesn't affect functionality or code quality in any meaningful way. The grammar errors could make the code slightly harder to understand for non-native English speakers. Clear documentation is important for maintainability. While clear documentation is valuable, this particular grammar issue doesn't significantly impact comprehension. The meaning remains clear enough, and this kind of minor polish can be handled during regular code maintenance rather than PR review. Delete this comment as it addresses a minor grammatical issue that doesn't materially impact code quality or functionality.
7. backend/windmill-api/src/postgres_triggers/handler.rs:869
  • Draft comment:
    Typo: It looks like the assignment to first is missing a semicolon. Please add a semicolon at the end of line 869.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
8. backend/windmill-api/src/postgres_triggers/handler.rs:1089
  • Draft comment:
    Typo: The variable 'occuped' appears to be misspelled. Please update it to 'occupied' for clarity.
  • Reason this comment was not posted:
    Comment was on unchanged code.
9. backend/windmill-api/src/postgres_triggers/handler.rs:1180
  • Draft comment:
    Typo in debug message: "does not exists" should be "does not exist".
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the grammar, debug messages are internal logging statements not exposed to users. The rules state not to make comments that are obvious or unimportant. A minor grammatical fix in a debug message seems too trivial to warrant a comment. The grammar error could indicate a lack of attention to detail that might manifest in more serious ways elsewhere in the code. Debug messages are still read by developers. While code quality is important, a minor grammatical error in an internal debug message has no functional impact and fixing it provides minimal value. The rules specifically say not to make unimportant comments. This comment should be deleted as it addresses a trivial grammatical issue in a debug message, which falls under the category of unimportant changes per the review rules.
10. frontend/src/lib/components/triggers/gcp/GcpTriggerEditorConfigSection.svelte:71
  • Draft comment:
    Typo found: The function name 'listAllTgoogleTopicSubscriptions' appears to include an extra 'T' at the beginning of 'Google'. Please verify if the correct function name should be 'listAllGoogleTopicSubscriptions'.
  • Reason this comment was not posted:
    Comment was on unchanged code.
11. frontend/src/lib/components/triggers/postgres/PostgresTriggerEditorInner.svelte:90
  • Draft comment:
    Typo: Consider using 'TypeScript' instead of 'Typescript' for the language name to match conventional capitalization.
  • Reason this comment was not posted:
    Comment was on unchanged code.
12. frontend/src/lib/components/triggers/postgres/PostgresTriggerEditorInner.svelte:785
  • Draft comment:
    There's a potential typographical issue with the closing tag on this line. The string "</div" seems to be missing a closing angle bracket. Please consider merging this line with the following line (or modifying the lines) to properly form a complete closing tag, for example: "".
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_lEwzY0sUKkaEBrdX

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

query.push_str(" TABLE ONLY ");
if pg_14 && first {
query.push_str(" TABLE ONLY ");
first = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typographical error: Missing semicolon at the end of the line first = false. Please add a semicolon.

Suggested change
first = false
first = false;

Copy link

cloudflare-workers-and-pages bot commented Jun 1, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 75c6b37
Status: ✅  Deploy successful!
Preview URL: https://1ad0e8d2.windmill.pages.dev
Branch Preview URL: https://dieri-fix-backward-compatibi-z1gr.windmill.pages.dev

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 75c6b37 in 34 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/postgres_triggers/mod.rs:180
  • Draft comment:
    Added missing semicolon for proper statement termination.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_jNWOaQI3JWIDJ8xF

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@rubenfiszel rubenfiszel merged commit 4cbcbdb into main Jun 1, 2025
14 checks passed
@rubenfiszel rubenfiszel deleted the dieri/fix-backward-compatibility-pg-14-for-postgres-trigger branch June 1, 2025 17:11
@github-actions github-actions bot locked and limited conversation to collaborators Jun 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants