Feat: Added a Statistics node #2018#9748
Open
mzabuawala wants to merge 2 commits into
Open
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:
WalkthroughAdds a new Statistics schema child: backend Flask view handlers, SQL templates for default/PG16/PG17, frontend node/UI schema and webpack wiring, documentation, and regression tests with helpers. ChangesStatistics schema child feature
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (UI)
participant Frontend as statistics.js (AMD)
participant Server as StatisticsView (pgAdmin REST)
participant Templates as SQLTemplates (Jinja)
participant DB as PostgreSQL
Browser->>Frontend: user action (create/list/update/delete)
Frontend->>Server: REST call (create/list/update/delete)
Server->>Templates: render SQL (version-specific)
Templates-->>Server: rendered SQL
Server->>DB: execute SQL
DB-->>Server: result / rows / OID
Server-->>Frontend: JSON response
Frontend-->>Browser: update UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
mzabuawala
force-pushed
the
Feat_STATISTICS
branch
8 times, most recently
from
0c50973 to
ab59af0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (9)
web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sql (1)
3-7: Redundant conditional produces identical output.The
{% if %}and{% else %}branches on line 4 emit the same SQL:{{ conn|qtIdent(o_data.schema, o_data.name) }}. The conditional is dead code and can be simplified.♻️ Proposed simplification
{### Rename statistics ###} {% if data.name and data.name != o_data.name %} -ALTER STATISTICS {% if data.schema and data.schema != o_data.schema %}{{ conn|qtIdent(o_data.schema, o_data.name) }}{% else %}{{ conn|qtIdent(o_data.schema, o_data.name) }}{% endif %} +ALTER STATISTICS {{ conn|qtIdent(o_data.schema, o_data.name) }} RENAME TO {{ conn|qtIdent(data.name) }};🤖 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/statistics/templates/statistics/sql/17_plus/update.sql` around lines 3 - 7, The conditional around emitting the identifier in the ALTER STATISTICS statement is redundant because both branches output {{ conn|qtIdent(o_data.schema, o_data.name) }}; remove the entire {% if data.schema and data.schema != o_data.schema %} ... {% else %} ... {% endif %} block and replace it with a single occurrence of {{ conn|qtIdent(o_data.schema, o_data.name) }} so ALTER STATISTICS always uses that identifier (refer to the ALTER STATISTICS line and the conn|qtIdent(o_data.schema, o_data.name) expression).web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/backend_support.sql (1)
1-5: LGTM with a minor optimization suggestion.The logic correctly detects PostgreSQL 10+ extended statistics support. Using
EXISTSwould be slightly more efficient as it short-circuits on the first match:♻️ Optional: Use EXISTS for efficiency
{### Check if extended statistics are supported (PostgreSQL 10+) ###} SELECT - CASE WHEN COUNT(*) > 0 THEN TRUE ELSE FALSE END AS has_statistics -FROM pg_catalog.pg_class -WHERE relname='pg_statistic_ext' + EXISTS ( + SELECT 1 FROM pg_catalog.pg_class WHERE relname = 'pg_statistic_ext' + ) AS has_statistics🤖 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/statistics/templates/statistics/sql/default/backend_support.sql` around lines 1 - 5, Replace the COUNT-based detection of extended statistics with an EXISTS-based check to short-circuit on the first match: update the query that produces has_statistics (currently scanning pg_catalog.pg_class with WHERE relname='pg_statistic_ext') to use an EXISTS subquery so it returns TRUE as soon as a matching pg_statistic_ext row is found, keeping the same column name has_statistics and still querying pg_catalog.pg_class with relname='pg_statistic_ext'.web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sql (1)
3-7: Redundant conditional produces identical output.Same issue as in the 17_plus template: both branches output
{{ conn|qtIdent(o_data.schema, o_data.name) }}.♻️ Proposed simplification
{### Rename statistics ###} {% if data.name and data.name != o_data.name %} -ALTER STATISTICS {% if data.schema and data.schema != o_data.schema %}{{ conn|qtIdent(o_data.schema, o_data.name) }}{% else %}{{ conn|qtIdent(o_data.schema, o_data.name) }}{% endif %} +ALTER STATISTICS {{ conn|qtIdent(o_data.schema, o_data.name) }} RENAME TO {{ conn|qtIdent(data.name) }};🤖 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/statistics/templates/statistics/sql/16_plus/update.sql` around lines 3 - 7, The inner conditional in the update.sql template is redundant because both branches render {{ conn|qtIdent(o_data.schema, o_data.name) }}; simplify by removing the conditional and directly use {{ conn|qtIdent(o_data.schema, o_data.name) }} where the ALTER STATISTICS target is built (keep the outer check {% if data.name and data.name != o_data.name %} ... RENAME TO {{ conn|qtIdent(data.name) }}; intact); no other logic changes required.web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/stats.sql (1)
1-1: Misleading version comment for a default template.The comment states "PostgreSQL 10-11" but this file is in the
defaultfolder, which typically serves as the fallback for versions without specific templates. Consider either moving this to a version-specific folder (e.g.,10_plus) or updating the comment to reflect its actual usage scope.🤖 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/statistics/templates/statistics/sql/default/stats.sql` at line 1, The comment in the stats.sql default template incorrectly labels the template as "PostgreSQL 10-11"; either remove or update that version-specific wording in the stats.sql file (the default template for statistics) to indicate it is a generic/fallback template, or move this template into a version-specific directory (e.g., the 10_plus template set) so the comment accurately reflects its scope; update the header comment in stats.sql (or relocate the file) and ensure the comment matches the template's actual usage.web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.json (1)
232-249: Limited test coverage for update operations.The
statistics_putsuite only tests renaming. Consider adding test cases for:
- Schema change
- Owner change
- Comment update
- Non-existing object (negative test, similar to delete/get suites)
🤖 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/statistics/tests/statistics_test_data.json` around lines 232 - 249, Add broader update test cases to the statistics_put suite: keep the existing "Update statistics: Change name." case and add tests that exercise schema change (set a new "schema" in test_data), owner change (set an "owner" value), comment update (set a "comment" field), and a negative test where the object does not exist (use the same "url": "/browser/statistics/obj/" but set an invalid identifier in inventory_data or test_data and assert a non-200 status and error_msg). Ensure each new case follows the same shape as the existing entry (fields: name, url, is_positive_test, inventory_data, test_data, mocking_required, mock_data, expected_data) and uses unique descriptive names so they run alongside the current "Update statistics: Change name." case.web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/create.sql (1)
1-14: Remove duplicate template; 17_plus/create.sql differs from 16_plus/create.sql only in the version comment.The two templates have identical SQL logic. If PostgreSQL 17 introduced no syntax changes for
CREATE STATISTICS, remove the 17_plus template and let template resolution fall back to 16_plus.🤖 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/statistics/templates/statistics/sql/17_plus/create.sql` around lines 1 - 14, The 17_plus/create.sql template duplicates the CREATE STATISTICS logic from 16_plus/create.sql (the CREATE STATISTICS block and COMMENT ON STATISTICS handling), so delete the redundant 17_plus/create.sql file and rely on template resolution to fall back to 16_plus/create.sql; ensure no unique content from 17_plus (e.g., version-only comment) is required elsewhere and update any references or tests that explicitly point to the 17_plus template to use the fallback or 16_plus instead.web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/properties.sql (1)
1-1: Minor: Inaccurate comment for the version-specific template.The comment states "PostgreSQL 14+" but this template is in the
16_plusfolder. Consider updating the comment to reflect this is for PostgreSQL 16+.-{### Query extended statistics properties from pg_statistic_ext (PostgreSQL 14+) ###} +{### Query extended statistics properties from pg_statistic_ext (PostgreSQL 16+) ###}🤖 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/statistics/templates/statistics/sql/16_plus/properties.sql` at line 1, The header comment in the template is inaccurate — update the comment string "{### Query extended statistics properties from pg_statistic_ext (PostgreSQL 14+) ###}" to reference PostgreSQL 16+ (e.g. change "PostgreSQL 14+" to "PostgreSQL 16+") so it correctly reflects the file's 16_plus template; ensure the modified comment remains the same format and location.web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_add.py (1)
75-75: Unused variabledb_user.The variable
db_useris assigned but never used in the method.def runTest(self): """This function will add statistics under schema node.""" - db_user = self.server["username"] self.data["schema"] = self.schema_name🤖 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/statistics/tests/test_statistics_add.py` at line 75, The assignment to db_user in the test (db_user = self.server["username"]) is unused; remove this unused variable assignment from the test in tests/test_statistics_add.py, or if the intent was to reference the server username later, replace usages to use db_user consistently (e.g., use db_user where self.server["username"] is currently repeated) so there are no unused variables.web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/properties.sql (1)
1-1: Minor: Update version comment to match template location.The comment indicates "PostgreSQL 14+" but this template resides in the
17_plusfolder.-{### Query extended statistics properties from pg_statistic_ext (PostgreSQL 14+) ###} +{### Query extended statistics properties from pg_statistic_ext (PostgreSQL 17+) ###}🤖 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/statistics/templates/statistics/sql/17_plus/properties.sql` at line 1, Update the version comment at the top of the template: replace "PostgreSQL 14+" with "PostgreSQL 17+" so the header in the statistics SQL template in the 17_plus folder accurately reflects the template location; ensure the comment string in the file that currently reads "{### Query extended statistics properties from pg_statistic_ext (PostgreSQL 14+) ###}" is edited to say "(PostgreSQL 17+)" to keep versioning consistent.
🤖 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/statistics/__init__.py`:
- Around line 422-435: The current parsing uses split(',') which breaks SQL
expressions with commas; update the parsing in the block that checks
has_expression_list to stop naive comma-splitting: if data['expression_list'] is
provided, first try to json.loads it as an array of expressions and map each
element to {'expression': <elem>}; if json.loads fails, treat the entire
expression_list string as a single expression (wrap it as [{'expression':
expression_list}]) instead of splitting; keep the existing assignment to
data['expressions'] but remove the list comprehension that splits on ',' and add
validation/logging for malformed JSON so callers (UI) are encouraged to send a
structured list.
- Around line 667-677: The function msql() currently assigns the second value
from self.get_SQL(...) to the name "_" which shadows the module-level gettext
alias and causes an UnboundLocalError when _("...") is called during validation;
change the assignment "sql, _ = self.get_SQL(gid, sid, did, data, scid, stid)"
to use a different local name (e.g., "sql, sql_params" or "sql, dummy") and
update any subsequent uses of that second value accordingly so "_" remains the
gettext function and is not shadowed.
- Around line 565-570: The render_template calls that build the DELETE SQL
(using self.template_path and self._DELETE_SQL) do not pass the DB connection
into the template context, but the delete.sql uses {{ conn|qtIdent(schema, name)
}}; update both render_template invocations (the one around the block
referencing res['rows'][0]['name']/['schema'] and the other similar call later)
to include conn=self.conn in their keyword args so the qtIdent filter can access
the connection.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.ui.js`:
- Around line 80-127: Add a dependency-driven reset for schema changes: in the
field definition with id 'schema' add a depChange handler that clears 'table'
and 'columns' (e.g., return {table: null, columns: []}) so stale selections
cannot persist when schema changes; also update the 'table' field's options
loader usage (the options or optionsLoaded logic that currently uses
this.fieldOptions.tables / obj.allTablesOptions) so it takes the selected schema
from state (use the same state.schema value or call
obj.getTableOid/state.schema) when fetching tables, and update the table-loader
in statistics.js to request tables scoped to the selected schema rather than a
global list. Ensure references: id 'schema', id 'table', id 'columns',
obj.getTableOid, and obj.fieldOptions.getColumns are used to locate and change
the code.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sql`:
- Line 1: Update the misleading header comment "{### SQL to update extended
statistics object (PostgreSQL 13+) ###}" to reflect the directory name by
changing "PostgreSQL 13+" to "PostgreSQL 16+" so the comment matches the folder
`16_plus`; locate the comment in the template file's top-level SQL header (the
exact string above) and replace the version number accordingly.
- Around line 22-25: Validate and cast data.stattarget to an integer before
rendering this template: ensure wherever request.form/request.json populates
data.stattarget you apply explicit int() casting and/or numeric validation and
reject non-integer input, so the template variable data.stattarget used in the
ALTER STATISTICS ... SET STATISTICS statement is guaranteed to be an integer;
keep the existing use of qtIdent(...) for schema/name but enforce integer
semantics for data.stattarget and return a validation error if casting fails.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/stats.sql`:
- Around line 14-18: The stxkind CASE in the stats.sql template is missing the
mapping for PostgreSQL 15's 'e' kind (expression statistics); update the CASE
that checks "kind" (the stxkind case in the statistics/sql/default/stats.sql
template) to include WHEN 'e' THEN 'expressions' so expression statistics are
returned by the default template fallback used for versions that don't override
this file.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.json`:
- Around line 74-77: Update the inaccurate skip message for the inventory_data
test fixture: locate the JSON object named "inventory_data" (which currently
sets "server_min_version": 140000) and change its "skip_msg" to either reflect
that MCV extended statistics were introduced in PostgreSQL 12 (e.g., "MCV
statistics introduced in PG 12") or remove the message entirely if the
server_min_version is intended to indicate a different PG14+ requirement; ensure
the skip_msg matches the actual gating condition tied to server_min_version.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_add.py`:
- Around line 103-106: The test uses eval(self.mock_data["return_value"]) inside
the patch side_effect which is unsafe; replace eval with a safe parser or
factory – e.g., use ast.literal_eval or json.loads on
self.mock_data["return_value"] (or create a centralized factory function that
maps known mock keys to concrete return objects) and pass that parsed value as
the side_effect in the patch for self.mock_data["function_name"], leaving
statistics_utils.api_create(self) call unchanged; ensure tests that set
mock_data return_value produce JSON/text or keys compatible with the chosen
parser/factory and update any test fixtures accordingly.
- Around line 31-33: The setUp method in StatisticsAddTestCase is missing a call
to its parent initializer; update StatisticsAddTestCase.setUp to call
super().setUp() at the start (so BaseTestGenerator and other test setup logic
runs) before assigning self.data = self.test_data, ensuring proper test
initialization.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete_multiple.py`:
- Around line 117-120: The test is using an invalid MIME type 'html/json' for
the JSON POST payload; locate the call that passes content_type='html/json' (in
the test in test_statistics_delete_multiple.py where the client request is made
with follow_redirects=True, data=data, content_type=...) and change the
content_type value to 'application/json' so the request uses the correct JSON
Content-Type header.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete.py`:
- Around line 129-134: The test currently only verifies deletion of
self.statistics_name but misses verifying self.statistics_name_2 when
self.is_list is true; update the teardown/backend verification in
test_statistics_delete (after calling statistics_utils.verify_statistics) to
also call statistics_utils.verify_statistics(self.server, self.db_name,
self.statistics_name_2) and assert it is None when self.is_list is True (i.e.,
only perform the second check when setUp() created self.statistics_name_2);
ensure both assertions use self.assertIsNone with descriptive messages so a
partial batch-delete (first removed, second present) will fail the test.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_get.py`:
- Around line 134-138: The test uses eval(self.mock_data["return_value"]) which
is unsafe; replace it with a safe parser such as
ast.literal_eval(self.mock_data["return_value"]) after importing ast, or better
yet return a concrete object via a small factory/mapping keyed by
self.mock_data["return_value"] and referenced in the patch call; update the
patch call in test_statistics_get.py (the block using
self.mock_data["function_name"] and statistics_utils.api_get(self)) to use the
safe parsed/constructed object instead of eval and ensure the corresponding
import or factory mapping is added to the test file.
- Around line 58-64: The code reads server_con["data"]["version"] before
verifying the connection succeeded, which can raise a KeyError if connect_server
failed; update the logic in the block that calls
server_utils.connect_server(self, self.server_id) so you first check
server_con.get("info") == "Server connected." (or existence of "data") and only
then read ver = server_con["data"]["version"]; if the check fails, raise the
exception immediately, otherwise compare ver to self.data["server_max_version"]
and call self.skipTest(self.data["skip_msg"]) as before.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_put.py`:
- Around line 112-116: The test uses eval(self.mock_data["return_value"]) inside
the patch side_effect (in test_statistics_put.py) which is a security risk;
replace eval with a safe factory/parser: add a helper like
create_side_effect(config) (or reuse ast.literal_eval if values are simple
literals) that inspects self.mock_data (keys "type"/"value"/"class"/"message" or
"value") and returns either an exception instance or a plain value, then change
the patch to use side_effect=[create_side_effect(self.mock_data)] referencing
the same function name used in the diff instead of eval; ensure the helper
handles exception creation (using getattr on builtins for class names) and
simple value returns to avoid executing arbitrary code.
- Around line 110-120: The test currently only sets response when
self.mocking_required is True, so if self.is_positive_test is False and
self.mocking_required is False response remains undefined; ensure response is
always assigned by calling statistics_utils.api_put(self) in the missing branch
(i.e., add an else branch to the if self.mocking_required block or initialize
response before the conditional) so response is defined before
utils.assert_status_code and utils.assert_error_message; reference: response,
self.mocking_required, self.is_positive_test, and statistics_utils.api_put.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.py`:
- Around line 153-189: The create_statistics() helper opens a DB connection via
test_utils.get_db_connection and currently only commits and returns the
statistics OID on the happy path, leaking connections on errors; update
create_statistics() to ensure connection cleanup by moving
test_utils.set_isolation_level(connection, old_isolation_level),
connection.commit() and connection.close() into a finally block (or closing the
cursor/connection in finally) so the connection is always closed even on
exceptions, and apply the same finally-based cleanup pattern to other helpers
that use test_utils.get_db_connection, pg_cursor, connection, and
test_utils.set_isolation_level.
---
Nitpick comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/properties.sql`:
- Line 1: The header comment in the template is inaccurate — update the comment
string "{### Query extended statistics properties from pg_statistic_ext
(PostgreSQL 14+) ###}" to reference PostgreSQL 16+ (e.g. change "PostgreSQL 14+"
to "PostgreSQL 16+") so it correctly reflects the file's 16_plus template;
ensure the modified comment remains the same format and location.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sql`:
- Around line 3-7: The inner conditional in the update.sql template is redundant
because both branches render {{ conn|qtIdent(o_data.schema, o_data.name) }};
simplify by removing the conditional and directly use {{
conn|qtIdent(o_data.schema, o_data.name) }} where the ALTER STATISTICS target is
built (keep the outer check {% if data.name and data.name != o_data.name %} ...
RENAME TO {{ conn|qtIdent(data.name) }}; intact); no other logic changes
required.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/create.sql`:
- Around line 1-14: The 17_plus/create.sql template duplicates the CREATE
STATISTICS logic from 16_plus/create.sql (the CREATE STATISTICS block and
COMMENT ON STATISTICS handling), so delete the redundant 17_plus/create.sql file
and rely on template resolution to fall back to 16_plus/create.sql; ensure no
unique content from 17_plus (e.g., version-only comment) is required elsewhere
and update any references or tests that explicitly point to the 17_plus template
to use the fallback or 16_plus instead.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/properties.sql`:
- Line 1: Update the version comment at the top of the template: replace
"PostgreSQL 14+" with "PostgreSQL 17+" so the header in the statistics SQL
template in the 17_plus folder accurately reflects the template location; ensure
the comment string in the file that currently reads "{### Query extended
statistics properties from pg_statistic_ext (PostgreSQL 14+) ###}" is edited to
say "(PostgreSQL 17+)" to keep versioning consistent.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sql`:
- Around line 3-7: The conditional around emitting the identifier in the ALTER
STATISTICS statement is redundant because both branches output {{
conn|qtIdent(o_data.schema, o_data.name) }}; remove the entire {% if data.schema
and data.schema != o_data.schema %} ... {% else %} ... {% endif %} block and
replace it with a single occurrence of {{ conn|qtIdent(o_data.schema,
o_data.name) }} so ALTER STATISTICS always uses that identifier (refer to the
ALTER STATISTICS line and the conn|qtIdent(o_data.schema, o_data.name)
expression).
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/backend_support.sql`:
- Around line 1-5: Replace the COUNT-based detection of extended statistics with
an EXISTS-based check to short-circuit on the first match: update the query that
produces has_statistics (currently scanning pg_catalog.pg_class with WHERE
relname='pg_statistic_ext') to use an EXISTS subquery so it returns TRUE as soon
as a matching pg_statistic_ext row is found, keeping the same column name
has_statistics and still querying pg_catalog.pg_class with
relname='pg_statistic_ext'.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/stats.sql`:
- Line 1: The comment in the stats.sql default template incorrectly labels the
template as "PostgreSQL 10-11"; either remove or update that version-specific
wording in the stats.sql file (the default template for statistics) to indicate
it is a generic/fallback template, or move this template into a version-specific
directory (e.g., the 10_plus template set) so the comment accurately reflects
its scope; update the header comment in stats.sql (or relocate the file) and
ensure the comment matches the template's actual usage.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.json`:
- Around line 232-249: Add broader update test cases to the statistics_put
suite: keep the existing "Update statistics: Change name." case and add tests
that exercise schema change (set a new "schema" in test_data), owner change (set
an "owner" value), comment update (set a "comment" field), and a negative test
where the object does not exist (use the same "url": "/browser/statistics/obj/"
but set an invalid identifier in inventory_data or test_data and assert a
non-200 status and error_msg). Ensure each new case follows the same shape as
the existing entry (fields: name, url, is_positive_test, inventory_data,
test_data, mocking_required, mock_data, expected_data) and uses unique
descriptive names so they run alongside the current "Update statistics: Change
name." case.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_add.py`:
- Line 75: The assignment to db_user in the test (db_user =
self.server["username"]) is unused; remove this unused variable assignment from
the test in tests/test_statistics_add.py, or if the intent was to reference the
server username later, replace usages to use db_user consistently (e.g., use
db_user where self.server["username"] is currently repeated) so there are no
unused variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9674af98-86d0-4fcb-8398-4e95fbb92be2
⛔ Files ignored due to path filters (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/img/coll-statistics.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/img/statistics.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
web/pgadmin/browser/server_groups/servers/databases/schemas/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.jsweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.ui.jsweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/backend_support.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/coll_stats.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/count.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/delete.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/get_oid.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/nodes.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/stats.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/pg/14_plus/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/pg/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.jsonweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_add.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete_multiple.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_get.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_put.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.pyweb/webpack.config.jsweb/webpack.shim.js
mzabuawala
force-pushed
the
Feat_STATISTICS
branch
3 times, most recently
from
fe79b94 to
25bb575
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_add.py (1)
31-33:⚠️ Potential issue | 🟠 MajorCall
super().setUp()before usingBaseTestGeneratorstate.This is the only new statistics test that skips the base initializer, so later accesses to scenario/base attributes can run against partially initialized state.
Minimal fix
def setUp(self): + super().setUp() # Load test data self.data = self.test_data🤖 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/statistics/tests/test_statistics_add.py` around lines 31 - 33, The setUp method in the test (function setUp) uses BaseTestGenerator state before initializing it; modify setUp to call super().setUp() as the first action so the base class initializer runs before accessing self.test_data or other scenario/base attributes (i.e., add a call to super().setUp() at the top of setUp in the TestStatisticsAdd test file).web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py (2)
890-895:⚠️ Potential issue | 🟠 MajorPass
conninto the schema-diff DROP render.The
target_onlydiff path still rendersdelete.sqlwithoutconn=self.conn, soqtIdentcannot quote the qualified statistics name there.Minimal fix
sql = render_template( "/".join([self.template_path, self._DELETE_SQL]), name=target_params['name'], schema=target_params['schema'], - cascade=False + cascade=False, + conn=self.conn )🤖 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/statistics/__init__.py` around lines 890 - 895, The target-only diff branch renders the delete.sql template without the DB connection, so qtIdent cannot properly quote the qualified statistics name; update the render_template call that builds sql (using self.template_path and self._DELETE_SQL) to pass conn=self.conn (i.e., render_template(..., name=..., schema=..., cascade=False, conn=self.conn)) so the template can use the connection for quoting.
422-435:⚠️ Potential issue | 🟠 MajorStop parsing expressions with
split(','), and reuse the same normalizer in preview SQL.
coalesce(a, b)becomes two broken entries here, andmsql()renders the same CREATE template without even buildingdata['expressions']. Extract one normalization step and use it for both create and preview paths instead of comma-splitting free text.Also applies to: 644-679, 728-733
🤖 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/statistics/__init__.py` around lines 422 - 435, Replace the ad-hoc comma split logic that builds data['expressions'] from data['expression_list'] with a shared normalization helper so complex expressions like "coalesce(a, b)" are preserved; create a function (e.g. normalize_expressions) that takes the raw expression_list string, parses/normalizes it into a list of {'expression': ...} entries (preserving parentheses and function calls, trimming whitespace, and ignoring empty items) and call this helper wherever the preview/build path currently handles expression_list (the code that assigns data['expressions'] from data['expression_list'] and the preview path that uses msql), removing any split(',') usage and ensuring both creation and preview use the same normalized output.web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.py (1)
93-133:⚠️ Potential issue | 🟠 MajorClose these DB helpers in
finally.These helpers only close the connection on the success path. Any exception before
connection.close()leaks a backend session, and the helpers that switch isolation level can also skip restoring it. Move cleanup intofinallyforcreate_table_for_statistics(),create_statistics(),verify_statistics(),get_statistics_id(),delete_statistics(), andget_statistics_columns().Also applies to: 153-190, 205-224, 239-259, 275-296, 311-332
🤖 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/statistics/tests/utils.py` around lines 93 - 133, The helpers (create_table_for_statistics, create_statistics, verify_statistics, get_statistics_id, delete_statistics, get_statistics_columns) currently only close connections on the success path; move the cleanup into a finally block: ensure any created cursor is closed, the connection isolation level is restored to old_isolation_level if set, and the connection is closed in finally even if exceptions occur; also ensure you only call set_isolation_level and commit/rollback when connection is not None and handle exceptions during commit by rolling back before closing to avoid leaking backend sessions.web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete.py (1)
129-134:⚠️ Potential issue | 🟠 MajorVerify the second statistics object in list-delete mode.
When
self.is_listis true, the positive path only checksself.statistics_name. A batch delete that leavesself.statistics_name_2behind will still pass.Minimal fix
statistics_response = statistics_utils.verify_statistics( self.server, self.db_name, self.statistics_name ) self.assertIsNone(statistics_response, "Deleted statistics still present") + if self.is_list: + statistics_response = statistics_utils.verify_statistics( + self.server, self.db_name, self.statistics_name_2 + ) + self.assertIsNone( + statistics_response, + "Second deleted statistics still present" + )🤖 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/statistics/tests/test_statistics_delete.py` around lines 129 - 134, The test only verifies deletion of self.statistics_name when self.is_list is True, allowing self.statistics_name_2 to remain; update the teardown/assertion to also verify that self.statistics_name_2 is deleted in list-delete mode by calling statistics_utils.verify_statistics for self.statistics_name_2 (in addition to self.statistics_name) and asserting the response is None; modify the block that currently calls statistics_utils.verify_statistics(self.server, self.db_name, self.statistics_name) to perform the same verification for self.statistics_name_2 when self.is_list is True.
🤖 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/statistics/tests/test_statistics_delete_multiple.py`:
- Around line 111-121: The test currently only asserts a 200 response after
calling self.tester.delete with data=json.dumps({'ids': self.statistics_ids})
but doesn't verify backend deletion; after the delete request (the call to
self.tester.delete in test_statistics_delete_multiple.py), re-query the two
statistics by their generated names/IDs (referencing self.statistics_ids and any
helper used to fetch statistics in the test class) and assert that neither
exists (e.g., returns None or 404) to ensure both records were removed rather
than relying solely on response.status_code.
---
Duplicate comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py`:
- Around line 890-895: The target-only diff branch renders the delete.sql
template without the DB connection, so qtIdent cannot properly quote the
qualified statistics name; update the render_template call that builds sql
(using self.template_path and self._DELETE_SQL) to pass conn=self.conn (i.e.,
render_template(..., name=..., schema=..., cascade=False, conn=self.conn)) so
the template can use the connection for quoting.
- Around line 422-435: Replace the ad-hoc comma split logic that builds
data['expressions'] from data['expression_list'] with a shared normalization
helper so complex expressions like "coalesce(a, b)" are preserved; create a
function (e.g. normalize_expressions) that takes the raw expression_list string,
parses/normalizes it into a list of {'expression': ...} entries (preserving
parentheses and function calls, trimming whitespace, and ignoring empty items)
and call this helper wherever the preview/build path currently handles
expression_list (the code that assigns data['expressions'] from
data['expression_list'] and the preview path that uses msql), removing any
split(',') usage and ensuring both creation and preview use the same normalized
output.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_add.py`:
- Around line 31-33: The setUp method in the test (function setUp) uses
BaseTestGenerator state before initializing it; modify setUp to call
super().setUp() as the first action so the base class initializer runs before
accessing self.test_data or other scenario/base attributes (i.e., add a call to
super().setUp() at the top of setUp in the TestStatisticsAdd test file).
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete.py`:
- Around line 129-134: The test only verifies deletion of self.statistics_name
when self.is_list is True, allowing self.statistics_name_2 to remain; update the
teardown/assertion to also verify that self.statistics_name_2 is deleted in
list-delete mode by calling statistics_utils.verify_statistics for
self.statistics_name_2 (in addition to self.statistics_name) and asserting the
response is None; modify the block that currently calls
statistics_utils.verify_statistics(self.server, self.db_name,
self.statistics_name) to perform the same verification for
self.statistics_name_2 when self.is_list is True.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.py`:
- Around line 93-133: The helpers (create_table_for_statistics,
create_statistics, verify_statistics, get_statistics_id, delete_statistics,
get_statistics_columns) currently only close connections on the success path;
move the cleanup into a finally block: ensure any created cursor is closed, the
connection isolation level is restored to old_isolation_level if set, and the
connection is closed in finally even if exceptions occur; also ensure you only
call set_isolation_level and commit/rollback when connection is not None and
handle exceptions during commit by rolling back before closing to avoid leaking
backend sessions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 559c1b33-8f6b-404a-82fc-f369275fd60b
⛔ Files ignored due to path filters (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/img/coll-statistics.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/img/statistics.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
web/pgadmin/browser/server_groups/servers/databases/schemas/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.jsweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.ui.jsweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/backend_support.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/coll_stats.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/count.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/delete.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/get_oid.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/nodes.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/stats.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/pg/14_plus/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/pg/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.jsonweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_add.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete_multiple.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_get.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_put.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.pyweb/webpack.config.jsweb/webpack.shim.js
✅ Files skipped from review due to trivial changes (14)
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/count.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/coll_stats.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/backend_support.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/get_oid.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/nodes.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/stats.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/create.sql
- web/webpack.config.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/properties.sql
- web/webpack.shim.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sql
🚧 Files skipped from review as they are similar to previous changes (7)
- web/pgadmin/browser/server_groups/servers/databases/schemas/init.py
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/delete.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/update.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.ui.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.json
Contributor
Author
|
Hi! Just following up on this PR. |
dpage
reviewed
Jun 9, 2026
dpage
left a comment
dpage
left a comment
Contributor
There was a problem hiding this comment.
Review — Extended Statistics node
Thanks for this, Murtuza — it's a thorough, well-structured node and a welcome addition (long-standing #2018). The backend REST surface, version-aware templates, UI schema, and webpack wiring all follow pgAdmin conventions, and you've already addressed most of CodeRabbit's first pass. I've left detailed notes on the individual threads; summarising the items I'd still like to see before merge:
Should fix
-
Expression-based statistics don't round-trip. (see my reply on the
_fetch_properties()thread)stxexprsis discarded after being reduced to a boolean, so the SQL tab and schema-diff emit aCREATE STATISTICS … ON FROM …with an emptyONclause for expression-based objects — that's invalid SQL a user can copy/run, not just a display gap.pg_get_expr(s.stxexprs, s.stxrelid)deparses it cleanly (we already rely onpg_get_exprfor index and partition expressions elsewhere). Please also add an expression-based create/round-trip test, since nothing currently exercises that path. -
Schema/Table desync in the create dialog. (see my reply on the
statistics.ui.jsthread) Changing the Schema dropdown doesn't reset the Table/Columns selection, and the table loader doesn't follow the selected schema — so a user can submit a table that belongs to a different schema. -
Docs + help page missing.
statistics.jswiresdialogHelp: 'statistics_dialog.html'plus create/alter help pages, but this PR adds nodocs/content and no release-notes entry. The dialog Help button will 404, and a new node needs documentation + a release note before it can merge.
Smaller items (details on the individual threads): missing super().setUp() in test_statistics_add.py; the list-delete test only verifies one of the two created objects; stats.sql omits the 'e' (expressions) kind; a defense-in-depth int() cast on stattarget; and the duplicated if/else branches at the top of 16_plus/17_plus update.sql.
The eval() and content_type='html/json' items CodeRabbit raised match patterns already used in 200+ existing test files, so I wouldn't block on those.
Nice work overall — once the expression round-trip and the docs are sorted, this is close.
mzabuawala
force-pushed
the
Feat_STATISTICS
branch
5 times, most recently
from
fcc666e to
039d397
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py (3)
900-907:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
connwhen schema-diff renders DROP SQL.
delete.sqlusesqtIdent, so the target-only diff path still needs the connection in the template context. Without it, DROP generation for statistics that exist only in the target fails during render.Suggested fix
elif comp_status == 'target_only': # Object exists in target only - generate DROP sql = render_template( "/".join([self.template_path, self._DELETE_SQL]), name=target_params['name'], schema=target_params['schema'], - cascade=False + cascade=False, + conn=self.conn )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py` around lines 900 - 907, The target-only branch in the statistics diff uses render_template to create a DROP (render_template call in the comp_status == 'target_only' branch in statistics/__init__.py) but omits the DB connection so templates (delete.sql which calls qtIdent) fail to render; fix by passing the current connection into the template context (add conn=conn to the render_template call that uses self.template_path and self._DELETE_SQL) so qtIdent and other connection-bound helpers are available.
422-435:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't comma-split raw SQL expressions.
split(',')breaks valid expressions likecoalesce(a, b). This needs either a structured list from the UI or a safer fallback that treats the submitted text as one expression instead of tokenizing on commas.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py` around lines 422 - 435, The current code tokenizes expression_list by comma (using expr_list.split(',')) which breaks valid SQL like "coalesce(a, b)"; instead, when has_expression_list is true and no structured expressions are provided, populate data['expressions'] with a single entry containing the entire trimmed expr_list (e.g. data['expressions'] = [{'expression': expr_list}]) and remove the comma-splitting logic; keep respect for existing structured 'expressions' if already present (use the has_expression_list and data['expressions'] checks to decide).
366-371:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStop dropping renderable expressions from fetched properties.
sql()and schema-diff both reuse_fetch_properties(). Once this replacesexpressionswith a boolean, expression-based statistics no longer have anything to feed back intocreate.sql, so reverse-engineered CREATE and source-only diff SQL still collapse to an emptyONclause. Pull a deparsed expression list through here instead of discarding it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py` around lines 366 - 371, The current _fetch_properties() replaces the original pg_node_tree 'expressions' with a boolean and discards the actual expression data; instead, deparse the pg_node_tree bytea into a renderable SQL expression list and keep it on the returned row (e.g. set row['expressions'] to the deparsed SQL string/list and still set row['has_expressions'] = True/False). Update _fetch_properties() to call the existing PG node deparser/utility to convert the raw expressions into SQL, assign that deparsed value back into row (or row['expressions_deparsed'] if you prefer a new key), and retain the has_expressions boolean so sql() and schema-diff can consume the original expression text when generating CREATE/ON clauses.
🧹 Nitpick comments (3)
web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sql (1)
22-24: ⚡ Quick winCast
stattargetbefore emitting it intoSET STATISTICS.The current callers validate this today, but the template still renders
data.stattargetraw. Casting here keeps the SQL generation defensive if another path reuses the template and aligns this file with the existing statistics/columns convention.Suggested fix
ALTER STATISTICS {{ conn|qtIdent(data.schema if data.schema else o_data.schema, data.name if data.name else o_data.name) }} - SET STATISTICS {% if data.stattarget == -1 or data.stattarget == 'DEFAULT' %}DEFAULT{% else %}{{ data.stattarget }}{% endif %}; + SET STATISTICS {% if data.stattarget == -1 or data.stattarget == 'DEFAULT' %}DEFAULT{% else %}{{ data.stattarget|int }}{% endif %};Based on learnings:
SET STATISTICStemplate values in statistics/columns SQL should be rendered as integers rather than emitted raw.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sql` around lines 22 - 24, The template currently emits data.stattarget raw into the ALTER STATISTICS ... SET STATISTICS clause; update the template so the value is cast to an integer before rendering (handle the DEFAULT/-1 case unchanged) to match the statistics/columns convention—modify the conditional that outputs {{ data.stattarget }} (used alongside data.stattarget and o_data.stattarget in the ALTER STATISTICS block) to emit an integer-casted value instead, ensuring the SET STATISTICS expression always renders an integer literal or DEFAULT.Source: Learnings
web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_get.py (1)
123-131: ⚡ Quick winAssert the returned statistics values, not just key presence.
In single-object mode this only checks that
columnsandstat_typesexist, not that they match the statistics created insetUp(). In list mode it does not validate the payload at all. A handler returning the wrong columns/types—or an unrelated list—still passes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_get.py` around lines 123 - 131, The test only checks presence of keys in response_data (and nothing in list mode); update the assertions to validate actual returned values against the statistics created in setUp: when self.is_list is False assert response_data['name'] == self.statistics_name and assert response_data['columns'] equals the expected columns and response_data['stat_types'] equals the expected stat_types from setUp; when self.is_list is True iterate the returned list (response_data) to find the entry with name == self.statistics_name and assert that its 'columns' and 'stat_types' match the expected values. Use the existing variables response_data, self.is_list and self.statistics_name to locate and compare the payload.web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_put.py (1)
101-122: ⚡ Quick winVerify the mutation result, not just that PUT returned a
node.A no-op handler that returns
200with a stubnodepayload still passes here. Re-fetch the statistics afterapi_put()and assert the fields driven byself.dataactually changed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_put.py` around lines 101 - 122, The test only checks that PUT returned a 'node' but not that the persisted statistics actually changed; after a successful positive PUT (when self.is_positive_test is True and utils.assert_status_code passes), call the corresponding fetch helper (e.g. statistics_utils.api_get(self)) to re-retrieve the updated statistics, parse the response JSON (response_data = json.loads(...)) and assert the specific fields from self.data are present and equal to the expected values on response_data['node']; update the positive branch in test_statistics_put to perform these re-fetch and field-equality assertions instead of merely asserting 'node' existence, using the existing helpers (statistics_utils.api_get, utils.assert_status_code) and self.data keys to locate expected fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py`:
- Around line 400-402: The code assigns request.form (an ImmutableMultiDict) to
data and later mutates it, causing TypeError; change the assignment to make a
mutable copy (e.g., use request.form.copy() or dict(request.form)) so subsequent
writes like data['expressions'] = ... and data['stattarget'] = int(...) succeed;
update both places where data is set from request.form (the create and update
handlers in the module, around the blocks initializing data from
request.form/request.data) so they always operate on a mutable dict.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete.py`:
- Around line 142-155: The negative-delete branch currently may call the
single-object DELETE URL or leave response unassigned; update the else branch so
it mirrors the positive-path route selection: inside the if
self.mocking_required block keep the patch of self.mock_data["function_name"]
but ensure you call statistics_utils.api_delete(self) inside that context and
assign its return to response; elif 'statistics_id' in self.data still set
self.statistics_id = self.data["statistics_id"] and call
statistics_utils.api_delete(self) assigning response; otherwise explicitly
select list-mode by clearing or not setting self.statistics_id (e.g.,
delattr(self, 'statistics_id') or set to None) and then call
statistics_utils.api_delete(self) so response is always assigned.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_get.py`:
- Around line 132-146: The negative GET branch can leave response undefined and
mis-route list-mode requests to the object URL; update the control flow around
mocking_required / self.data to mirror list-vs-object routing: keep the existing
with-patch path using self.mock_data and statistics_utils.api_get(self), change
the current elif 'statistics_id' in self.data branch to set self.statistics_id =
self.data["statistics_id"] and call statistics_utils.api_get(self) for the
object GET, and add a final else that calls statistics_utils.api_get(self) for
the collection/list GET so response is always defined and list-mode exercises
the collection route; reference symbols: self.mocking_required,
self.mock_data["function_name"], self.mock_data["return_value"], statistics_id /
self.statistics_id, statistics_utils.api_get, and self.data.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.py`:
- Around line 321-329: get_statistics_columns() currently uses
array_agg(a.attname) without a defined order, so column names can come back in
arbitrary order; change the SQL to preserve stxkeys order by unnesting s.stxkeys
WITH ORDINALITY (or using generate_series) to produce an ordinal position, join
that result to pg_catalog.pg_attribute on attnum and stxrelid, and then
aggregate the names ordered by that ordinal (e.g. array_agg(a.attname ORDER BY
ord)). Update the query used in pg_cursor.execute (referencing
pg_catalog.pg_statistic_ext, stxkeys, a.attnum, and array_agg) so columns =
pg_cursor.fetchone() returns names in the same order as the statistics
definition.
---
Duplicate comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py`:
- Around line 900-907: The target-only branch in the statistics diff uses
render_template to create a DROP (render_template call in the comp_status ==
'target_only' branch in statistics/__init__.py) but omits the DB connection so
templates (delete.sql which calls qtIdent) fail to render; fix by passing the
current connection into the template context (add conn=conn to the
render_template call that uses self.template_path and self._DELETE_SQL) so
qtIdent and other connection-bound helpers are available.
- Around line 422-435: The current code tokenizes expression_list by comma
(using expr_list.split(',')) which breaks valid SQL like "coalesce(a, b)";
instead, when has_expression_list is true and no structured expressions are
provided, populate data['expressions'] with a single entry containing the entire
trimmed expr_list (e.g. data['expressions'] = [{'expression': expr_list}]) and
remove the comma-splitting logic; keep respect for existing structured
'expressions' if already present (use the has_expression_list and
data['expressions'] checks to decide).
- Around line 366-371: The current _fetch_properties() replaces the original
pg_node_tree 'expressions' with a boolean and discards the actual expression
data; instead, deparse the pg_node_tree bytea into a renderable SQL expression
list and keep it on the returned row (e.g. set row['expressions'] to the
deparsed SQL string/list and still set row['has_expressions'] = True/False).
Update _fetch_properties() to call the existing PG node deparser/utility to
convert the raw expressions into SQL, assign that deparsed value back into row
(or row['expressions_deparsed'] if you prefer a new key), and retain the
has_expressions boolean so sql() and schema-diff can consume the original
expression text when generating CREATE/ON clauses.
---
Nitpick comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sql`:
- Around line 22-24: The template currently emits data.stattarget raw into the
ALTER STATISTICS ... SET STATISTICS clause; update the template so the value is
cast to an integer before rendering (handle the DEFAULT/-1 case unchanged) to
match the statistics/columns convention—modify the conditional that outputs {{
data.stattarget }} (used alongside data.stattarget and o_data.stattarget in the
ALTER STATISTICS block) to emit an integer-casted value instead, ensuring the
SET STATISTICS expression always renders an integer literal or DEFAULT.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_get.py`:
- Around line 123-131: The test only checks presence of keys in response_data
(and nothing in list mode); update the assertions to validate actual returned
values against the statistics created in setUp: when self.is_list is False
assert response_data['name'] == self.statistics_name and assert
response_data['columns'] equals the expected columns and
response_data['stat_types'] equals the expected stat_types from setUp; when
self.is_list is True iterate the returned list (response_data) to find the entry
with name == self.statistics_name and assert that its 'columns' and 'stat_types'
match the expected values. Use the existing variables response_data,
self.is_list and self.statistics_name to locate and compare the payload.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_put.py`:
- Around line 101-122: The test only checks that PUT returned a 'node' but not
that the persisted statistics actually changed; after a successful positive PUT
(when self.is_positive_test is True and utils.assert_status_code passes), call
the corresponding fetch helper (e.g. statistics_utils.api_get(self)) to
re-retrieve the updated statistics, parse the response JSON (response_data =
json.loads(...)) and assert the specific fields from self.data are present and
equal to the expected values on response_data['node']; update the positive
branch in test_statistics_put to perform these re-fetch and field-equality
assertions instead of merely asserting 'node' existence, using the existing
helpers (statistics_utils.api_get, utils.assert_status_code) and self.data keys
to locate expected fields.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b0823f3-018f-4124-bdd0-4eaa2da93dc7
⛔ Files ignored due to path filters (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/img/coll-statistics.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/img/statistics.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
web/pgadmin/browser/server_groups/servers/databases/schemas/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.jsweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.ui.jsweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/backend_support.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/coll_stats.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/count.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/delete.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/get_oid.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/nodes.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/stats.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/pg/14_plus/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/pg/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.jsonweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_add.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete_multiple.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_get.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_put.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.pyweb/webpack.config.jsweb/webpack.shim.js
✅ Files skipped from review due to trivial changes (3)
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/backend_support.sql
- web/webpack.shim.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/count.sql
🚧 Files skipped from review as they are similar to previous changes (17)
- web/webpack.config.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/nodes.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/stats.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/delete.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/coll_stats.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.json
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.ui.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/update.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/init.py
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/properties.sql
mzabuawala
force-pushed
the
Feat_STATISTICS
branch
from
4c461e1 to
60c41ad
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py (3)
899-904:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
conninto the schema-diff DROP render as well.This is the remaining
delete.sqlrender path that still omitsconn, so thetarget_onlyschema-diff branch cannot reliably render the DROP statement. It should mirror the fixed runtime delete path and passconn=self.conn.🩹 Minimal fix
elif comp_status == 'target_only': # Object exists in target only - generate DROP sql = render_template( "/".join([self.template_path, self._DELETE_SQL]), name=target_params['name'], schema=target_params['schema'], - cascade=False + cascade=False, + conn=self.conn )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py` around lines 899 - 904, The render_template call that builds the DROP SQL for schema-diff currently omits the DB connection context; update the call that uses self.template_path and self._DELETE_SQL (where name=target_params['name'], schema=target_params['schema'], cascade=False) to include conn=self.conn so it mirrors the runtime delete path and allows the target_only schema-diff branch to render DROP correctly.
424-432:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
expression_listis still parsed with an ambiguous comma split.This backend contract cannot distinguish “multiple expressions” from “one expression containing commas”, so valid inputs like
coalesce(a, b), upper(c)are mis-tokenized into invalid fragments. Because the UI accepts arbitrary SQL text here, this needs a structured payload (for example, an array of expressions) or SQL-aware parsing rather thansplit(',').🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py` around lines 424 - 432, The current naive comma-split of expression_list produces wrong tokens for SQL expressions containing commas; instead, update the contract and code to accept a structured "expressions" array (each item e.g. {'expression': '...'} ) and make the parsing block prefer data.get('expressions') if present; remove or deprecate the expression_list->split(',') logic in the section that sets data['expressions']; if you must support legacy callers, replace the simple split with an SQL-aware tokenizer (e.g., use a SQL parsing library such as sqlparse or a proper delimiter-aware routine) to correctly split top-level expressions while preserving commas inside parenthesis/quotes.
397-399:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCopy
request.formbefore handing it to mutable code paths.Both handlers keep the original
ImmutableMultiDict, but later writes at Line 428 and Line 711 mutatedata. Form-encoded create/update requests will raiseTypeErrorinstead of producing SQL. Userequest.form.copy()in both places so the downstream mutations stay on a mutableMultiDict.🩹 Minimal fix
- data = request.form if request.form else json.loads( + data = request.form.copy() if request.form else json.loads( request.data )- data = request.form if request.form else json.loads( + data = request.form.copy() if request.form else json.loads( request.data )Also applies to: 600-602
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py` around lines 397 - 399, The code assigns data = request.form (falling back to json.loads(request.data)) but later mutates data, which fails when request.form is an ImmutableMultiDict; change both occurrences that set data from request.form to use request.form.copy() so downstream writes operate on a mutable MultiDict (keep the json.loads fallback unchanged); search for uses of request.form -> data in the create/update handlers (the blocks that later write to data) and update them to request.form.copy().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.py`:
- Around line 899-904: The render_template call that builds the DROP SQL for
schema-diff currently omits the DB connection context; update the call that uses
self.template_path and self._DELETE_SQL (where name=target_params['name'],
schema=target_params['schema'], cascade=False) to include conn=self.conn so it
mirrors the runtime delete path and allows the target_only schema-diff branch to
render DROP correctly.
- Around line 424-432: The current naive comma-split of expression_list produces
wrong tokens for SQL expressions containing commas; instead, update the contract
and code to accept a structured "expressions" array (each item e.g.
{'expression': '...'} ) and make the parsing block prefer
data.get('expressions') if present; remove or deprecate the
expression_list->split(',') logic in the section that sets data['expressions'];
if you must support legacy callers, replace the simple split with an SQL-aware
tokenizer (e.g., use a SQL parsing library such as sqlparse or a proper
delimiter-aware routine) to correctly split top-level expressions while
preserving commas inside parenthesis/quotes.
- Around line 397-399: The code assigns data = request.form (falling back to
json.loads(request.data)) but later mutates data, which fails when request.form
is an ImmutableMultiDict; change both occurrences that set data from
request.form to use request.form.copy() so downstream writes operate on a
mutable MultiDict (keep the json.loads fallback unchanged); search for uses of
request.form -> data in the create/update handlers (the blocks that later write
to data) and update them to request.form.copy().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22e3f03f-917f-4169-b96a-9361e4e84ad7
⛔ Files ignored due to path filters (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/img/coll-statistics.svgis excluded by!**/*.svgweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/img/statistics.svgis excluded by!**/*.svg
📒 Files selected for processing (27)
docs/en_US/managing_database_objects.rstdocs/en_US/release_notes_9_16.rstdocs/en_US/statistics_dialog.rstweb/pgadmin/browser/server_groups/servers/databases/schemas/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.jsweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.ui.jsweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/backend_support.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/coll_stats.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/count.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/delete.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/get_oid.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/nodes.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/stats.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/pg/14_plus/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/pg/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.json
✅ Files skipped from review due to trivial changes (3)
- docs/en_US/managing_database_objects.rst
- docs/en_US/statistics_dialog.rst
- docs/en_US/release_notes_9_16.rst
🚧 Files skipped from review as they are similar to previous changes (19)
- web/pgadmin/browser/server_groups/servers/databases/schemas/init.py
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/coll_stats.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/nodes.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/backend_support.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/count.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/stats.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/update.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/delete.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.json
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/static/js/statistics.ui.js
mzabuawala
force-pushed
the
Feat_STATISTICS
branch
from
60c41ad to
daceeb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.py (1)
380-385:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSQL alias collision:
sused twice will cause a runtime error.The
pg_statistic_exttable is aliased ass, and thenunnest(...)is also aliased ass. PostgreSQL will reject this with "table name 's' specified more than once". Additionally,s.stxrelidands.oidnow reference the wrong alias.🐛 Proposed fix - use distinct alias for unnest
pg_cursor.execute( - f"SELECT array_agg(a.attname ORDER BY s.stxkeys_pos) " - f"FROM pg_catalog.pg_statistic_ext s, " - f"unnest(s.stxkeys) WITH ORDINALITY AS s(attnum, stxkeys_pos) " + f"SELECT array_agg(a.attname ORDER BY k.ord) " + f"FROM pg_catalog.pg_statistic_ext s " + f"JOIN unnest(s.stxkeys) WITH ORDINALITY AS k(attnum, ord) " + f" ON TRUE " f"JOIN pg_catalog.pg_attribute a " - f" ON a.attrelid = s.stxrelid AND a.attnum = s.attnum " + f" ON a.attrelid = s.stxrelid AND a.attnum = k.attnum " f"WHERE s.oid = {statistics_oid}" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.py` around lines 380 - 385, The SQL uses the alias "s" for both pg_statistic_ext and the unnest(...) which causes a naming collision; update the unnest alias to a distinct name (e.g., u) and change any references that should come from the unnest to use that alias (for example, in the JOIN use a.attnum = u.attnum) while keeping pg_statistic_ext as s (so s.stxrelid and s.oid remain unchanged); locate the SQL string in utils.py where statistics_oid is interpolated and apply these alias fixes to the JOIN and WHERE references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete.py`:
- Around line 143-160: The test currently invokes statistics_utils.api_delete
and stores it in delete_call before applying the mock, causing side-effectful
DELETE to run prior to patching; update the logic in the test so that you only
call statistics_utils.api_delete inside the branches after checking
self.mocking_required (i.e., compute response by calling
statistics_utils.api_delete within the with patch(...) block when
self.mocking_required is True, and only set self.statistics_id and call
statistics_utils.api_delete (assign to response) in the non-mocking branches);
reference and change the use of delete_call, self.mocking_required, the with
patch(self.mock_data["function_name"], ...) block, and assignment to
self.statistics_id so DELETE is not executed until after the mock is applied.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.py`:
- Around line 211-248: The try/except block around
get_db_connection()/pg_cursor.execute() can leak the DB connection if an
exception occurs; update the function to use a try/finally (or
try/except/finally) so that connection cleanup always runs: ensure pg_cursor is
closed, reset the isolation level via test_utils.set_isolation_level(connection,
old_isolation_level) if it was changed, and call connection.close() in the
finally block (while still re-raising the exception or returning statistics_oid
as appropriate). Reference the existing symbols get_db_connection,
set_isolation_level, connection, pg_cursor, and statistics_oid when making the
change.
---
Duplicate comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.py`:
- Around line 380-385: The SQL uses the alias "s" for both pg_statistic_ext and
the unnest(...) which causes a naming collision; update the unnest alias to a
distinct name (e.g., u) and change any references that should come from the
unnest to use that alias (for example, in the JOIN use a.attnum = u.attnum)
while keeping pg_statistic_ext as s (so s.stxrelid and s.oid remain unchanged);
locate the SQL string in utils.py where statistics_oid is interpolated and apply
these alias fixes to the JOIN and WHERE references.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3234aac9-c193-49ea-ae25-e8955be2365d
📒 Files selected for processing (16)
docs/en_US/managing_database_objects.rstdocs/en_US/release_notes_9_16.rstdocs/en_US/statistics_dialog.rstweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/update.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/properties.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.jsonweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_delete.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/test_statistics_get.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/utils.py
✅ Files skipped from review due to trivial changes (2)
- docs/en_US/release_notes_9_16.rst
- docs/en_US/managing_database_objects.rst
🚧 Files skipped from review as they are similar to previous changes (10)
- docs/en_US/statistics_dialog.rst
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/update.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/default/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/17_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/tests/statistics_test_data.json
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/templates/statistics/sql/16_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/statistics/init.py
mzabuawala
force-pushed
the
Feat_STATISTICS
branch
2 times, most recently
from
9ce9f6d to
cffafff
Compare
mzabuawala
force-pushed
the
Feat_STATISTICS
branch
from
cffafff to
c52c15c
Compare
Contributor
Author
Done |
mzabuawala
force-pushed
the
Feat_STATISTICS
branch
from
c52c15c to
382cffe
Compare
Contributor
Author
|
Hi @dpage PR is updated with the fixes, please review again. |
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.
Fixes - #2018
Summary by CodeRabbit
New Features
Tests
Documentation