Generate ALTER TABLE SET UNLOGGED/LOGGED when persistence is changed (#9677)#9768
Open
pavanmanishd wants to merge 3 commits intopgadmin-org:masterfrom
Open
Generate ALTER TABLE SET UNLOGGED/LOGGED when persistence is changed (#9677)#9768pavanmanishd wants to merge 3 commits intopgadmin-org:masterfrom
pavanmanishd wants to merge 3 commits intopgadmin-org:masterfrom
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
Comment on lines
+54
to
60
| {## Change table persistence (UNLOGGED / LOGGED) ##} | ||
| {#####################################################} | ||
| {% if data.relpersistence is defined and data.relpersistence != o_data.relpersistence %} | ||
| ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.name)}} | ||
| SET {% if data.relpersistence %}UNLOGGED{% else %}LOGGED{% endif %}; | ||
|
|
||
| {% endif %} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
BASE="web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql"
echo "1) List all table update templates by version:"
fd -t f "update.sql" "$BASE" | sort
echo
echo "2) Check relpersistence block presence in each update template:"
while IFS= read -r f; do
if rg -n "relpersistence|SET .*UNLOGGED|SET .*LOGGED|Change table persistence" "$f" >/dev/null; then
echo "HAS $f"
else
echo "MISSING $f"
fi
done < <(fd -t f "update.sql" "$BASE" | sort)
echo
echo "3) Inspect template-selection logic for server-version routing:"
rg -n -C3 "11_plus|12_plus|15_plus|template_path|update.sql" \
web/pgadmin/browser/server_groups/servers/databases/schemas/tablesRepository: pgadmin-org/pgadmin4
Length of output: 678
🏁 Script executed:
#!/bin/bash
set -euo pipefail
BASE="web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql"
echo "=== Checking relpersistence block in each update.sql ==="
echo
for version_dir in default 11_plus 12_plus 15_plus; do
file="$BASE/$version_dir/update.sql"
echo "--- $version_dir/update.sql ---"
if [ -f "$file" ]; then
if rg "relpersistence|UNLOGGED|LOGGED" "$file" >/dev/null 2>&1; then
echo "✓ Has relpersistence logic"
rg -n "relpersistence|Change table persistence" "$file" -B2 -A5
else
echo "✗ MISSING relpersistence logic"
fi
else
echo "✗ File does not exist"
fi
echo
done
echo "=== Checking template routing logic ==="
rg -n "template_path|get_template_path|12_plus|15_plus" \
web/pgadmin/browser/server_groups/servers/databases/schemas/tables \
--type py -A3 -B1 | head -100Repository: pgadmin-org/pgadmin4
Length of output: 12490
🏁 Script executed:
#!/bin/bash
echo "=== 12_plus/update.sql ==="
wc -l web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/12_plus/update.sql
echo
echo "=== First 20 lines of 12_plus/update.sql ==="
head -20 web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/12_plus/update.sql
echo
echo "=== 15_plus/update.sql ==="
wc -l web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/15_plus/update.sql
echo
echo "=== First 20 lines of 15_plus/update.sql ==="
head -20 web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/15_plus/update.sqlRepository: pgadmin-org/pgadmin4
Length of output: 2134
🏁 Script executed:
#!/bin/bash
# Check if there's any fallback or warning mechanism
echo "=== Check for any fallback to default template ==="
grep -r "fallback\|default\|relpersistence" \
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/utils.py \
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py \
2>/dev/null || echo "No fallback mechanism found"Repository: pgadmin-org/pgadmin4
Length of output: 3451
Add relpersistence block to 12_plus and 15_plus templates.
The table persistence toggle (UNLOGGED/LOGGED) works for PostgreSQL 11 and earlier, but is missing from the 12_plus and 15_plus update.sql templates. Users on PostgreSQL 12+ cannot modify table persistence through the UI.
Add the following block to both web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/12_plus/update.sql and 15_plus/update.sql:
Diff for both files
{#####################################################}
{## Change table schema ##}
{#####################################################}
{% if data.schema and data.schema != o_data.schema %}
ALTER TABLE IF EXISTS {{conn|qtIdent(o_data.schema, data.name)}}
SET SCHEMA {{conn|qtIdent(data.schema)}};
{% endif %}
+{#####################################################}
+{## Change table persistence (UNLOGGED / LOGGED) ##}
+{#####################################################}
+{% if data.relpersistence is defined and data.relpersistence != o_data.relpersistence %}
+ALTER TABLE IF EXISTS {{conn|qtIdent(data.schema, data.name)}}
+ SET {% if data.relpersistence %}UNLOGGED{% else %}LOGGED{% endif %};
+
+{% endif %}
{#####################################################}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/11_plus/update.sql`
around lines 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+.
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
1 participant
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.
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