Generate ALTER TABLE SET UNLOGGED/LOGGED when persistence is changed (#9677) - #9768
Generate ALTER TABLE SET UNLOGGED/LOGGED when persistence is changed (#9677)#9768pavanmanishd wants to merge 3 commits into
Conversation
Add onMouseDown handler to the tab close button that fires layoutDocker.close() when mouse button 1 (middle click) is pressed. preventDefault() stops the browser's auto-scroll cursor from appearing. This matches the standard UX in Chrome, Firefox, and VS Code where middle-click on a tab closes it.
…istence changes The Unlogged toggle in Table Properties was read-only — toggling it and saving did not generate any DDL. Both the default/ and 11_plus/ update.sql templates were missing the relpersistence alter block. Add the missing section to both templates: ALTER TABLE ... SET UNLOGGED; (when relpersistence toggled on) ALTER TABLE ... SET LOGGED; (when relpersistence toggled off) The relpersistence value is already fetched by properties.sql and tracked in the form; this change wires it through to the DDL generation.
WalkthroughAdded functionality to update table persistence settings (LOGGED/UNLOGGED) in pgAdmin's SQL generation templates for PostgreSQL. Changes include conditional ALTER TABLE statements that execute when table persistence needs modification, with proper Jinja template control flow restructuring in two version-specific template files. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/11_plus/update.sql`:
- Around line 54-60: Copy the relpersistence change-block from the 11_plus
update.sql into the 12_plus and 15_plus update.sql templates: add the
conditional that checks if data.relpersistence is defined and differs from
o_data.relpersistence, then run ALTER TABLE IF EXISTS
{{conn|qtIdent(data.schema, data.name)}} SET {% if data.relpersistence
%}UNLOGGED{% else %}LOGGED{% endif %}; ensuring the same Jinja condition and
qtIdent usage are used so table persistence can be toggled for PostgreSQL 12+
and 15+.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: adf0b540-773f-4ce0-977d-494c32ad1672
📒 Files selected for processing (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/11_plus/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/default/update.sql
|
Hi @pavanmanishd, please address the coderabbitai review comments. |
|
Merged to As part of the merge I completed the fix per @coderabbitai's catch and @khushboovashi's request: the persistence block was missing from the The squash rewrites the SHA so GitHub won't auto-close this — closing manually. Thanks @pavanmanishd! |
|
Tip For best results, initiate chat on the files or code changes.
(ノ◕ヮ◕)ノ*:・゚✧ 🐇 |
Closes #9677
Problem
The Unlogged toggle in Table Properties → Advanced has no effect. Toggling it and saving produces no DDL — the
relpersistencechange was tracked in the form but never written to the update SQL templates.Fix
Add the missing block to both
default/update.sqland11_plus/update.sql:{% if data.relpersistence is defined and data.relpersistence != o_data.relpersistence %} ALTER TABLE IF EXISTS ... SET {% if data.relpersistence %}UNLOGGED{% else %}LOGGED{% endif %}; {% endif %}The
relpersistencefield is already fetched byproperties.sql(relpersistence = 'u'→ true) and tracked in the form; this change wires it through to DDL generation.Summary by CodeRabbit