Copilot commented on code in PR #6521:
URL: https://github.com/apache/hive/pull/6521#discussion_r3629308744
##########
standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-4.2.0-to-4.3.0.mssql.sql:
##########
@@ -3,8 +3,105 @@ SELECT 'Upgrading MetaStore schema from 4.2.0 to 4.3.0' AS
MESSAGE;
ALTER TABLE HIVE_LOCKS ADD HL_CATALOG nvarchar(128) NOT NULL DEFAULT 'hive';
ALTER TABLE MATERIALIZATION_REBUILD_LOCKS ADD MRL_CAT_NAME nvarchar(128) NOT
NULL DEFAULT 'hive';
+-- Add surrogate primary keys for HA database replication. Adding IDENTITY
columns
+-- and clustered PKs via ALTER TABLE takes schema modification locks and
rebuilds
+-- large tables. Rebuild each populated table via a TMP_* copy/swap instead.
+-- Counter tables contain a single row and keep the two-step ALTER form. Plan a
+-- maintenance window for busy metastores; duration scales with row counts.
+
+CREATE TABLE TMP_TXN_COMPONENTS(
+ TC_ID bigint NOT NULL IDENTITY(1,1),
+ TC_TXNID bigint NOT NULL,
+ TC_DATABASE nvarchar(128) NOT NULL,
+ TC_TABLE nvarchar(256) NULL,
+ TC_PARTITION nvarchar(767) NULL,
+ TC_OPERATION_TYPE char(1) NOT NULL,
+ TC_WRITEID bigint,
+ PRIMARY KEY CLUSTERED (TC_ID ASC)
+);
+INSERT INTO TMP_TXN_COMPONENTS (TC_TXNID, TC_DATABASE, TC_TABLE, TC_PARTITION,
TC_OPERATION_TYPE, TC_WRITEID)
+ SELECT TC_TXNID, TC_DATABASE, TC_TABLE, TC_PARTITION, TC_OPERATION_TYPE,
TC_WRITEID FROM TXN_COMPONENTS;
+DROP TABLE TXN_COMPONENTS;
+EXEC sp_rename 'TMP_TXN_COMPONENTS', 'TXN_COMPONENTS';
+ALTER TABLE TXN_COMPONENTS WITH CHECK ADD FOREIGN KEY(TC_TXNID) REFERENCES
TXNS (TXN_ID);
+CREATE INDEX TC_TXNID_INDEX ON TXN_COMPONENTS (TC_TXNID);
Review Comment:
This has the same failure-mode as the Oracle script: dropping the original
table before the replacement is fully in place is not recoverable if the script
errors mid-way (and SQL Server DDL is commonly run outside an explicit
transaction in schema upgrades). Use a safer rename-based swap (rename original
to *_OLD, rename TMP_* to original, validate, then drop *_OLD), which also
makes manual rollback possible.
##########
standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-4.2.0-to-4.3.0.oracle.sql:
##########
@@ -3,8 +3,123 @@ SELECT 'Upgrading MetaStore schema from 4.2.0 to 4.3.0' AS
Status from dual;
ALTER TABLE HIVE_LOCKS ADD (HL_CATALOG VARCHAR2(128) DEFAULT 'hive' NOT NULL);
ALTER TABLE MATERIALIZATION_REBUILD_LOCKS ADD (MRL_CAT_NAME VARCHAR2(128)
DEFAULT 'hive' NOT NULL);
+-- Add surrogate primary keys for HA database replication. Oracle IDENTITY
columns
+-- do not backfill existing rows when added via ALTER TABLE, so rebuild each
+-- populated table via a TMP_* copy/swap (same pattern as counter tables
below).
+-- Plan a maintenance window: each swap locks the table and scales with row
count.
+
+CREATE TABLE TMP_TXN_COMPONENTS (
+ TC_ID NUMBER(19) GENERATED BY DEFAULT AS IDENTITY NOT NULL,
+ TC_TXNID NUMBER(19) NOT NULL,
+ TC_DATABASE VARCHAR2(128) NOT NULL,
+ TC_TABLE VARCHAR2(256),
+ TC_PARTITION VARCHAR2(767) NULL,
+ TC_OPERATION_TYPE char(1) NOT NULL,
+ TC_WRITEID NUMBER(19),
+ CONSTRAINT TMP_TXN_COMPONENTS_PK PRIMARY KEY (TC_ID)
+) ROWDEPENDENCIES;
+INSERT INTO TMP_TXN_COMPONENTS (TC_TXNID, TC_DATABASE, TC_TABLE, TC_PARTITION,
TC_OPERATION_TYPE, TC_WRITEID)
+ SELECT TC_TXNID, TC_DATABASE, TC_TABLE, TC_PARTITION, TC_OPERATION_TYPE,
TC_WRITEID FROM TXN_COMPONENTS;
+DROP TABLE TXN_COMPONENTS;
+ALTER TABLE TMP_TXN_COMPONENTS RENAME TO TXN_COMPONENTS;
+ALTER TABLE TXN_COMPONENTS ADD CONSTRAINT TXN_COMPONENTS_FK1 FOREIGN KEY
(TC_TXNID) REFERENCES TXNS (TXN_ID);
+CREATE INDEX TC_TXNID_INDEX ON TXN_COMPONENTS (TC_TXNID);
Review Comment:
The upgrade uses a destructive drop-then-rename pattern. Oracle DDL
auto-commits, so if the script fails after `DROP TABLE` (or between multiple
table rebuild steps), the metastore can be left without the original table and
without the replacement. Prefer a safer swap pattern: rename the original table
to a *_OLD name first, rename TMP_* into place, validate (optionally row
counts), and only then drop the *_OLD table at the end.
##########
standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-4.2.0-to-4.3.0.mysql.sql:
##########
@@ -3,8 +3,98 @@ SELECT 'Upgrading MetaStore schema from 4.2.0 to 4.3.0' AS
MESSAGE;
ALTER TABLE HIVE_LOCKS ADD HL_CATALOG varchar(128) NOT NULL DEFAULT 'hive';
ALTER TABLE MATERIALIZATION_REBUILD_LOCKS ADD MRL_CAT_NAME varchar(128) NOT
NULL DEFAULT 'hive';
+-- Add surrogate primary keys for HA database replication. Adding
AUTO_INCREMENT
+-- PRIMARY KEY via ALTER TABLE rebuilds large InnoDB tables and blocks writes
for
+-- the duration. Rebuild each populated table via a TMP_* copy/swap instead.
+-- Counter tables contain a single row and keep the one-step ALTER form. Plan a
+-- maintenance window for busy metastores; duration scales with row counts.
+
+CREATE TABLE TMP_TXN_COMPONENTS (
+ TC_ID bigint NOT NULL AUTO_INCREMENT,
+ TC_TXNID bigint NOT NULL,
+ TC_DATABASE varchar(128) NOT NULL,
+ TC_TABLE varchar(256),
+ TC_PARTITION varchar(767),
+ TC_OPERATION_TYPE char(1) NOT NULL,
+ TC_WRITEID bigint,
+ PRIMARY KEY (TC_ID),
+ FOREIGN KEY (TC_TXNID) REFERENCES TXNS (TXN_ID)
+) ENGINE=InnoDB DEFAULT CHARSET=latin1;
+INSERT INTO TMP_TXN_COMPONENTS (TC_TXNID, TC_DATABASE, TC_TABLE, TC_PARTITION,
TC_OPERATION_TYPE, TC_WRITEID)
+ SELECT TC_TXNID, TC_DATABASE, TC_TABLE, TC_PARTITION, TC_OPERATION_TYPE,
TC_WRITEID FROM TXN_COMPONENTS;
+RENAME TABLE TXN_COMPONENTS TO TXN_COMPONENTS_OLD, TMP_TXN_COMPONENTS TO
TXN_COMPONENTS;
+DROP TABLE TXN_COMPONENTS_OLD;
Review Comment:
The atomic rename step is good for safety, but immediately dropping the
*_OLD table removes the easiest rollback path if a later statement in the
upgrade fails. Consider delaying the `DROP TABLE *_OLD` statements until the
end of the script (or at least until after all dependent indexes/constraints
are recreated) so operators can recover by renaming back if needed.
##########
standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-4.2.0-to-4.3.0.postgres.sql:
##########
@@ -5,6 +5,60 @@ ALTER TABLE "MATERIALIZATION_REBUILD_LOCKS" ADD COLUMN
"MRL_CAT_NAME" varchar(12
CREATE INDEX "MIN_HISTORY_WRITE_ID_IDX" ON "MIN_HISTORY_WRITE_ID"
("MH_DATABASE", "MH_TABLE", "MH_WRITEID");
+-- Add surrogate primary keys for HA database replication. For large
transactional
+-- tables, add the column without a default first, backfill via sequence, then
set
+-- NOT NULL and add the PK. This avoids the long ACCESS EXCLUSIVE table
rewrite that
+-- ADD COLUMN ... bigserial PRIMARY KEY takes on populated tables. Counter
tables
+-- contain a single row and keep the one-step form. Plan a maintenance window
+-- for busy metastores as backfill duration scales with
TXN_COMPONENTS/WRITE_SET row counts.
Review Comment:
The PR description states 'Does this PR introduce any user-facing change?
No', but these schema upgrades have direct operator-facing impact (maintenance
window/downtime considerations, potentially long-running backfills/rebuilds).
Please update the PR description to reflect the operational impact and expected
upgrade behavior; this helps downstream release notes and upgrade planning.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
