Copilot commented on code in PR #10912:
URL: https://github.com/apache/gravitino/pull/10912#discussion_r3166690458
##########
scripts/postgresql/upgrade-1.2.0-to-1.3.0-postgresql.sql:
##########
@@ -17,6 +17,28 @@
-- under the License.
--
+ALTER TABLE role_meta ADD COLUMN updated_at BIGINT NOT NULL DEFAULT 0;
+ALTER TABLE user_meta ADD COLUMN updated_at BIGINT NOT NULL DEFAULT 0;
+ALTER TABLE group_meta ADD COLUMN updated_at BIGINT NOT NULL DEFAULT 0;
+ALTER TABLE owner_meta ADD COLUMN updated_at BIGINT NOT NULL DEFAULT 0;
Review Comment:
The new `updated_at` columns are added without `IF NOT EXISTS`, but later in
this same script `view_meta.audit_info` is added with `IF NOT EXISTS`. If this
upgrade script is re-run (or partially applied), these `ALTER TABLE ... ADD
COLUMN` statements will fail; consider using `ADD COLUMN IF NOT EXISTS` for
consistency and safer re-execution.
```suggestion
ALTER TABLE role_meta ADD COLUMN IF NOT EXISTS updated_at BIGINT NOT NULL
DEFAULT 0;
ALTER TABLE user_meta ADD COLUMN IF NOT EXISTS updated_at BIGINT NOT NULL
DEFAULT 0;
ALTER TABLE group_meta ADD COLUMN IF NOT EXISTS updated_at BIGINT NOT NULL
DEFAULT 0;
ALTER TABLE owner_meta ADD COLUMN IF NOT EXISTS updated_at BIGINT NOT NULL
DEFAULT 0;
```
##########
scripts/postgresql/schema-1.3.0-postgresql.sql:
##########
@@ -290,6 +290,7 @@ CREATE TABLE IF NOT EXISTS user_meta (
current_version INT NOT NULL DEFAULT 1,
last_version INT NOT NULL DEFAULT 1,
deleted_at BIGINT NOT NULL DEFAULT 0,
+ updated_at BIGINT NOT NULL DEFAULT 0,
PRIMARY KEY (user_id),
UNIQUE (metalake_id, user_name, deleted_at)
Review Comment:
The upgrade script adds new covering indexes (`idx_user_meta_name_del_upd`,
`idx_group_meta_del_upd`, `idx_role_meta_del_upd`,
`idx_owner_meta_obj_del_upd`, etc.), but the 1.3.0 baseline schema does not
create them. Fresh installs on 1.3.0 will miss these indexes, which can
significantly impact the cache/poller query performance; please add the same
indexes to the schema script.
##########
scripts/postgresql/schema-1.3.0-postgresql.sql:
##########
@@ -483,12 +486,16 @@ CREATE TABLE IF NOT EXISTS owner_meta (
current_version INT NOT NULL DEFAULT 1,
last_version INT NOT NULL DEFAULT 1,
deleted_at BIGINT NOT NULL DEFAULT 0,
+ updated_at BIGINT NOT NULL DEFAULT 0,
PRIMARY KEY (id),
Review Comment:
`updated_at` is added to `owner_meta`, but the schema’s `COMMENT ON COLUMN
owner_meta.*` section does not include `owner_meta.updated_at` (it currently
documents up through `deleted_at`). Please add a column comment for
`updated_at` for consistency.
##########
scripts/postgresql/schema-1.3.0-postgresql.sql:
##########
@@ -290,6 +290,7 @@ CREATE TABLE IF NOT EXISTS user_meta (
current_version INT NOT NULL DEFAULT 1,
last_version INT NOT NULL DEFAULT 1,
deleted_at BIGINT NOT NULL DEFAULT 0,
+ updated_at BIGINT NOT NULL DEFAULT 0,
PRIMARY KEY (user_id),
UNIQUE (metalake_id, user_name, deleted_at)
);
Review Comment:
`updated_at` is added to `user_meta`, but the corresponding `COMMENT ON
COLUMN user_meta.updated_at ...` entry is missing later in the schema file (the
comment block currently stops at `deleted_at`). Please add a column comment for
`updated_at` to keep documentation consistent with the rest of the schema.
##########
scripts/mysql/upgrade-1.2.0-to-1.3.0-mysql.sql:
##########
@@ -17,6 +17,58 @@
-- under the License.
--
+-- Role privilege tracking (strong consistency -- Step 3 version check)
+ALTER TABLE `role_meta`
+ ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0
+ COMMENT 'Set to currentTimeMillis() on any privilege grant/revoke for this
role.
+ JcasbinAuthorizer compares db.updated_at vs cached updated_at per
request
+ to decide whether to reload JCasbin policies for this role.';
+
+-- User role assignment tracking (strong consistency -- Step 1a version check)
+ALTER TABLE `user_meta`
+ ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0
+ COMMENT 'Set to currentTimeMillis() on any role assign/revoke for this
user.
+ JcasbinAuthorizer compares db.updated_at vs cached updated_at per
request
+ to decide whether to reload the user-role mapping.';
+
+-- Group role assignment tracking (strong consistency -- Step 1b version check)
+ALTER TABLE `group_meta`
+ ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0
+ COMMENT 'Set to currentTimeMillis() on any role assign/revoke for this
group.
+ JcasbinAuthorizer compares db.updated_at vs cached updated_at per
request
+ to decide whether to reload the group-role mapping.';
+
+-- Ownership mutation tracking (eventual consistency -- owner change poller)
+ALTER TABLE `owner_meta`
+ ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0
+ COMMENT 'Set to currentTimeMillis() on any ownership transfer.
+ The owner change poller reads updated_at > maxSeen to find
changed rows
+ and invalidates only the specific metadataObjectIds in
ownerRelCache.';
+
+-- Covering indexes for high-frequency read predicates
+CREATE INDEX idx_user_meta_name_del_upd
+ ON user_meta (metalake_id, user_name, deleted_at, updated_at);
+CREATE INDEX idx_group_meta_del_upd
+ ON group_meta (group_id, deleted_at, updated_at);
+CREATE INDEX idx_role_meta_del_upd
+ ON role_meta (role_id, deleted_at, updated_at);
+CREATE INDEX idx_owner_meta_obj_del_upd
+ ON owner_meta (metadata_object_id, deleted_at, updated_at);
+CREATE INDEX idx_owner_meta_del_upd_obj
+ ON owner_meta (deleted_at, updated_at, metadata_object_id);
+
+-- Entity name->id mutation tracking (eventual consistency -- entity change
poller)
+CREATE TABLE IF NOT EXISTS `entity_change_log` (
+ `id` BIGINT UNSIGNED NOT NULL AUTO_INCREMENT,
+ `metalake_name` VARCHAR(128) NOT NULL,
+ `entity_type` VARCHAR(32) NOT NULL COMMENT 'METALAKE | CATALOG |
SCHEMA | TABLE | FILESET | TOPIC | MODEL | VIEW',
+ `full_name` VARCHAR(512) NOT NULL COMMENT 'Dot-separated full name of
the affected entity. For RENAME, stores the OLD name (the stale key to
invalidate). For DROP/ALTER, the entity name.',
Review Comment:
The `full_name` column comment mentions "DROP/ALTER", but the documented
`operate_type` codes only include RENAME/DROP/INSERT. This is
confusing/inaccurate documentation; please either remove the ALTER mention or
document the corresponding operate type/code if ALTER is intended to be logged.
```suggestion
`full_name` VARCHAR(512) NOT NULL COMMENT 'Dot-separated full name
of the affected entity. For RENAME, stores the OLD name (the stale key to
invalidate). For DROP, stores the entity name.',
```
##########
scripts/postgresql/schema-1.3.0-postgresql.sql:
##########
@@ -312,6 +313,7 @@ CREATE TABLE IF NOT EXISTS role_meta (
current_version INT NOT NULL DEFAULT 1,
last_version INT NOT NULL DEFAULT 1,
deleted_at BIGINT NOT NULL DEFAULT 0,
+ updated_at BIGINT NOT NULL DEFAULT 0,
PRIMARY KEY (role_id),
UNIQUE (metalake_id, role_name, deleted_at)
);
Review Comment:
`updated_at` is added to `role_meta`, but there is no matching `COMMENT ON
COLUMN role_meta.updated_at ...` entry in the schema’s comment section for this
table. Please add it for consistency with the other documented columns.
##########
scripts/postgresql/upgrade-1.2.0-to-1.3.0-postgresql.sql:
##########
@@ -17,6 +17,28 @@
-- under the License.
--
+ALTER TABLE role_meta ADD COLUMN updated_at BIGINT NOT NULL DEFAULT 0;
+ALTER TABLE user_meta ADD COLUMN updated_at BIGINT NOT NULL DEFAULT 0;
+ALTER TABLE group_meta ADD COLUMN updated_at BIGINT NOT NULL DEFAULT 0;
+ALTER TABLE owner_meta ADD COLUMN updated_at BIGINT NOT NULL DEFAULT 0;
+
+CREATE INDEX idx_user_meta_name_del_upd ON user_meta (metalake_id, user_name,
deleted_at, updated_at);
+CREATE INDEX idx_group_meta_del_upd ON group_meta (group_id, deleted_at,
updated_at);
+CREATE INDEX idx_role_meta_del_upd ON role_meta (role_id, deleted_at,
updated_at);
+CREATE INDEX idx_owner_meta_obj_del_upd ON owner_meta (metadata_object_id,
deleted_at, updated_at);
+CREATE INDEX idx_owner_meta_del_upd_obj ON owner_meta (deleted_at, updated_at,
metadata_object_id);
Review Comment:
These indexes are created without `IF NOT EXISTS` while this script uses
`CREATE TABLE IF NOT EXISTS` and later uses `CREATE INDEX IF NOT EXISTS` for
`idx_ecl_created_at`. Creating an index without `IF NOT EXISTS` makes the
script non-idempotent and can break reruns; consider switching these to `CREATE
INDEX IF NOT EXISTS` as well.
```suggestion
CREATE INDEX IF NOT EXISTS idx_user_meta_name_del_upd ON user_meta
(metalake_id, user_name, deleted_at, updated_at);
CREATE INDEX IF NOT EXISTS idx_group_meta_del_upd ON group_meta (group_id,
deleted_at, updated_at);
CREATE INDEX IF NOT EXISTS idx_role_meta_del_upd ON role_meta (role_id,
deleted_at, updated_at);
CREATE INDEX IF NOT EXISTS idx_owner_meta_obj_del_upd ON owner_meta
(metadata_object_id, deleted_at, updated_at);
CREATE INDEX IF NOT EXISTS idx_owner_meta_del_upd_obj ON owner_meta
(deleted_at, updated_at, metadata_object_id);
```
##########
scripts/mysql/upgrade-1.2.0-to-1.3.0-mysql.sql:
##########
@@ -17,6 +17,58 @@
-- under the License.
--
+-- Role privilege tracking (strong consistency -- Step 3 version check)
+ALTER TABLE `role_meta`
+ ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0
+ COMMENT 'Set to currentTimeMillis() on any privilege grant/revoke for this
role.
+ JcasbinAuthorizer compares db.updated_at vs cached updated_at per
request
+ to decide whether to reload JCasbin policies for this role.';
+
+-- User role assignment tracking (strong consistency -- Step 1a version check)
+ALTER TABLE `user_meta`
+ ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0
+ COMMENT 'Set to currentTimeMillis() on any role assign/revoke for this
user.
+ JcasbinAuthorizer compares db.updated_at vs cached updated_at per
request
+ to decide whether to reload the user-role mapping.';
+
+-- Group role assignment tracking (strong consistency -- Step 1b version check)
+ALTER TABLE `group_meta`
+ ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0
+ COMMENT 'Set to currentTimeMillis() on any role assign/revoke for this
group.
+ JcasbinAuthorizer compares db.updated_at vs cached updated_at per
request
+ to decide whether to reload the group-role mapping.';
+
+-- Ownership mutation tracking (eventual consistency -- owner change poller)
+ALTER TABLE `owner_meta`
+ ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0
Review Comment:
`updated_at` is being added as a signed `BIGINT`. In the MySQL schemas,
timestamp/epoch-millis fields (e.g., `deleted_at`) are typically `BIGINT(20)
UNSIGNED`; using a signed type here is inconsistent and could allow negative
values. Consider making `updated_at` an unsigned BIGINT (and keep the
schema/upgrade scripts consistent).
```suggestion
ADD COLUMN `updated_at` BIGINT(20) UNSIGNED NOT NULL DEFAULT 0
COMMENT 'Set to currentTimeMillis() on any privilege grant/revoke for
this role.
JcasbinAuthorizer compares db.updated_at vs cached updated_at
per request
to decide whether to reload JCasbin policies for this role.';
-- User role assignment tracking (strong consistency -- Step 1a version
check)
ALTER TABLE `user_meta`
ADD COLUMN `updated_at` BIGINT(20) UNSIGNED NOT NULL DEFAULT 0
COMMENT 'Set to currentTimeMillis() on any role assign/revoke for this
user.
JcasbinAuthorizer compares db.updated_at vs cached updated_at
per request
to decide whether to reload the user-role mapping.';
-- Group role assignment tracking (strong consistency -- Step 1b version
check)
ALTER TABLE `group_meta`
ADD COLUMN `updated_at` BIGINT(20) UNSIGNED NOT NULL DEFAULT 0
COMMENT 'Set to currentTimeMillis() on any role assign/revoke for this
group.
JcasbinAuthorizer compares db.updated_at vs cached updated_at
per request
to decide whether to reload the group-role mapping.';
-- Ownership mutation tracking (eventual consistency -- owner change poller)
ALTER TABLE `owner_meta`
ADD COLUMN `updated_at` BIGINT(20) UNSIGNED NOT NULL DEFAULT 0
```
##########
scripts/mysql/schema-1.3.0-mysql.sql:
##########
@@ -179,6 +180,7 @@ CREATE TABLE IF NOT EXISTS `role_meta` (
`current_version` INT UNSIGNED NOT NULL DEFAULT 1 COMMENT 'role current
version',
`last_version` INT UNSIGNED NOT NULL DEFAULT 1 COMMENT 'role last version',
`deleted_at` BIGINT(20) UNSIGNED NOT NULL DEFAULT 0 COMMENT 'role deleted
at',
+ `updated_at` BIGINT NOT NULL DEFAULT 0,
PRIMARY KEY (`role_id`),
UNIQUE KEY `uk_mid_rn_del` (`metalake_id`, `role_name`, `deleted_at`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin COMMENT 'role
metadata';
Review Comment:
In this schema file, most columns include type details and `COMMENT`
strings, but the new `updated_at` columns are missing comments (and are defined
as signed `BIGINT` rather than the existing `BIGINT(20) UNSIGNED` pattern used
for timestamps like `deleted_at`). Please align `updated_at` with the existing
timestamp column conventions for
`user_meta`/`role_meta`/`group_meta`/`owner_meta`.
##########
scripts/mysql/schema-1.3.0-mysql.sql:
##########
@@ -166,6 +166,7 @@ CREATE TABLE IF NOT EXISTS `user_meta` (
`current_version` INT UNSIGNED NOT NULL DEFAULT 1 COMMENT 'user current
version',
`last_version` INT UNSIGNED NOT NULL DEFAULT 1 COMMENT 'user last version',
`deleted_at` BIGINT(20) UNSIGNED NOT NULL DEFAULT 0 COMMENT 'user deleted
at',
+ `updated_at` BIGINT NOT NULL DEFAULT 0,
PRIMARY KEY (`user_id`),
UNIQUE KEY `uk_mid_us_del` (`metalake_id`, `user_name`, `deleted_at`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin COMMENT 'user
metadata';
Review Comment:
The 1.3.0 MySQL upgrade script creates several new covering indexes for
`updated_at` reads, but the 1.3.0 baseline schema does not include them. This
means fresh installs won’t get the intended indexes; please add the same
indexes to the schema script (for `user_meta`, `group_meta`, `role_meta`, and
`owner_meta`).
##########
scripts/postgresql/schema-1.3.0-postgresql.sql:
##########
@@ -387,6 +389,7 @@ CREATE TABLE IF NOT EXISTS group_meta (
current_version INT NOT NULL DEFAULT 1,
last_version INT NOT NULL DEFAULT 1,
deleted_at BIGINT NOT NULL DEFAULT 0,
+ updated_at BIGINT NOT NULL DEFAULT 0,
PRIMARY KEY (group_id),
UNIQUE (metalake_id, group_name, deleted_at)
);
Review Comment:
`updated_at` is added to `group_meta`, but the schema’s `COMMENT ON COLUMN`
entries for `group_meta` do not document `updated_at`. Please add a column
comment so the table remains fully documented like the others.
##########
scripts/h2/schema-1.3.0-h2.sql:
##########
@@ -175,6 +175,7 @@ CREATE TABLE IF NOT EXISTS `user_meta` (
`current_version` INT UNSIGNED NOT NULL DEFAULT 1 COMMENT 'user current
version',
`last_version` INT UNSIGNED NOT NULL DEFAULT 1 COMMENT 'user last version',
`deleted_at` BIGINT(20) UNSIGNED NOT NULL DEFAULT 0 COMMENT 'user deleted
at',
+ `updated_at` BIGINT NOT NULL DEFAULT 0 COMMENT 'updated_at',
PRIMARY KEY (`user_id`),
CONSTRAINT `uk_mid_us_del` UNIQUE (`metalake_id`, `user_name`,
`deleted_at`)
) ENGINE=InnoDB;
Review Comment:
The 1.3.0 H2 baseline schema adds `updated_at` columns but does not define
the new covering indexes that the H2 upgrade script creates (e.g.,
`idx_user_meta_name_del_upd`, `idx_group_meta_del_upd`,
`idx_role_meta_del_upd`, `idx_owner_meta_obj_del_upd`,
`idx_owner_meta_del_upd_obj`). Fresh installs on H2 will miss these indexes;
please add them to the schema script too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
