aglinxinyuan commented on code in PR #7055: URL: https://github.com/apache/texera/pull/7055#discussion_r3752819254
########## sql/updates/33.sql: ########## @@ -0,0 +1,254 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +-- Relocate login credentials out of "user" into auth_provider. +-- +-- Moves `password` / `google_id` into an auth_provider row per (user, provider), so a user can +-- hold several external identities instead of exactly one Google account, and renames +-- `google_avatar` to the provider-neutral `avatar`. The rename is in place: the column keeps +-- its width and every stored value, so this migration does not change what any user's avatar +-- resolves to. + +\c texera_db + +SET search_path TO texera_db; + +BEGIN; + +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'provider_type_enum') THEN + CREATE TYPE provider_type_enum AS ENUM ('LOCAL', 'GOOGLE'); + END IF; +END +$$; + +-- provider_id is nullable here and tightened to NOT NULL below, once the backfill has given +-- every row a handle. +CREATE TABLE IF NOT EXISTS auth_provider +( + uid INT NOT NULL, + provider_type provider_type_enum NOT NULL, + provider_id VARCHAR(256), + password VARCHAR(256), -- hashed credential; only for LOCAL + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + PRIMARY KEY (uid, provider_type), + FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE, + CONSTRAINT uq_provider_identity UNIQUE (provider_type, provider_id) Review Comment: `uq_provider_identity` is in force before the backfill and before the handle-minting `UPDATE` runs under it, so the dedup loop at line 129 can never do the job its comment describes — *"The loop is bounded because a suffixed handle can itself collide with a literal one"*. That collision happens **within** the minting `UPDATE`, and Postgres checks a non-deferrable unique constraint as each index tuple is inserted, not at statement end. The statement aborts before any later pass can run. Reproduced against Postgres 16 with this exact script, on a pre-33 schema holding three users — `john` (uid 1), `john` (uid 2), `john-2` (uid 3): ``` ERROR: duplicate key value violates unique constraint "uq_provider_identity" DETAIL: Key (provider_type, provider_id)=(LOCAL, john-2) already exists. CONTEXT: PL/pgSQL function inline_code_block line 11 at FOR over SELECT rows ``` uid 2 is minted `john-2`, which collides with uid 3's literal name inside the same statement. The enclosing `BEGIN … COMMIT` rolls back and Liquibase marks changeset 33 failed — which is the *"deployment that cannot start"* outcome the comment on line 61 is trying to avoid. Suggested fix, matching the precedent in `28.sql` (which deduplicates and only then adds `dataset_owner_uid_name_key`): drop the inline constraint here and add it next to the existing `SET NOT NULL` on line 226: ```sql ALTER TABLE auth_provider ALTER COLUMN provider_id SET NOT NULL; ALTER TABLE auth_provider ADD CONSTRAINT uq_provider_identity UNIQUE (provider_type, provider_id); ``` I verified that patch on the same data: it commits, and the loop finally resolves the three accounts to `john` / `john-2` / `john-2-3`. The unpatched script is fine on ordinary duplicates (`john`, `john`, `mary` → `john`, `john-2`, `mary`) — it is specifically the suffixed-collides-with-literal case that fails. Bonus: once the constraint moves, the backfill on line 107 can insert `provider_id` straight from `"user".name` instead of NULL-then-mint. The *"handle is deliberately left NULL here"* note only exists because the constraint is in force this early. -- 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]
