Merge #150952 #151045 #151487

150952: changefeedccl: fix incorrect previous values for dropped columns r=fqazi a=fqazi

Previously, when we adjusted when the backfill was emitted for the drop column under the declarative schema changer when schema_locked was active, we wound up emitting the previous values incorrectly. What this meant was that in the Avro format the old and new values would be the same when a column was being dropped. This was observed by the cdc/schemareg test. To address this, this patch correctly resolves the previous values in the drop column case from the last descriptor version.

Fixes: #150540

Release note: None

151045: sql/catalog: fix policy dependency handling during backup/restore r=spilchen a=spilchen

Previously, when performing table level restores, policies with missing dependencies (UDFs, types, or relations) were not handled correctly. Unlike triggers, which are dropped when their dependencies are missing, policies would remain in an inconsistent state, potentially causing restore failures or leaving dangling references.

This change extends the existing trigger dependency handling logic to also cover RLS policies. When a policy depends on objects that aren't being restored (e.g., a UDF referenced in a USING clause), the policy is now properly dropped. The implementation also ensures that back-references are cleaned up appropriately, preventing dangling references that could interfere with subsequent operations like dropping sequences or types.

Additionally, the schema changer state rewriting logic is enhanced to handle missing dependencies for policy expressions (PolicyUsingExpr, PolicyWithCheckExpr, PolicyDeps) during restore operations.

Fixes #151042.

Release note (bug fix): Fixed an issue where Row Level Security policies with missing dependencies during table-level restores could cause inconsistent state or restore failures.

Epic: None

151487: server: fix potential nil pointer when starting flight recorder r=yuzefovich a=dhartunian

When the flightt recorder constructor returns an error in `NewFlightRecorder`, we should make sure not to call `Start`.

Epic: None
Release note: None

Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Matt Spilchen <[email protected]>
Co-authored-by: David Hartunian <[email protected]>

Author: 4 people

pkg/backup/testdata/backup-restore/policy-with-deps

@@ -0,0 +1,363 @@
+# Test backing up and restoring tables and databases with policies that have
+# dependencies on functions, types, and sequences.
+new-cluster name=s
+----
+
+exec-sql
+CREATE DATABASE testdb;
+----
+
+exec-sql
+USE testdb;
+----
+
+# ==============================================================================
+# Test 1: Cross-table dependencies (types and functions)
+# ==============================================================================
+
+# Create the dependencies: types and functions
+exec-sql
+CREATE TYPE status AS ENUM ('active', 'inactive');
+----
+
+exec-sql
+CREATE TABLE users (id INT, status string);
+----
+
+exec-sql
+CREATE FUNCTION f1() RETURNS INT LANGUAGE PLpgSQL AS $$
+  BEGIN
+    RETURN 1;
+  END
+$$;
+----
+
+# Create policy with both type and function dependencies
+exec-sql
+CREATE POLICY cross_table_policy ON users
+  FOR SELECT
+  USING (status::status = 'active'::status and f1() = 1);
+----
+
+# Backup everything
+exec-sql
+BACKUP DATABASE testdb INTO 'nodelocal://1/backup_directory';
+----
+
+# Test 1a: Restore table to a new database with skip_missing_udfs
+exec-sql
+CREATE DATABASE newdb;
+----
+
+exec-sql
+RESTORE TABLE users
+FROM LATEST IN 'nodelocal://1/backup_directory'
+WITH into_db = 'newdb', skip_missing_udfs;
+----
+
+exec-sql
+USE newdb;
+----
+
+# The table should be restored without the policy since type and function dependencies are missing
+query-sql
+SHOW POLICIES FOR users;
+----
+
+# Test 1b: Drop original dependencies and restore with skip_missing_udfs in same database
+exec-sql
+USE testdb;
+----
+
+exec-sql
+DROP TABLE users;
+----
+
+exec-sql
+DROP TYPE status;
+----
+
+exec-sql
+RESTORE TABLE users
+FROM LATEST IN 'nodelocal://1/backup_directory'
+WITH skip_missing_udfs;
+----
+
+# Verify the table was restored without the policy
+query-sql
+SHOW POLICIES FOR users;
+----
+
+# Verify we can manipulate the table now that policies are gone
+exec-sql
+ALTER TABLE users RENAME TO users_no_policy;
+----
+
+exec-sql
+INSERT INTO users_no_policy (id, status) VALUES (1, 'active');
+----
+
+query-sql
+SELECT * FROM users_no_policy;
+----
+1 active
+
+# Clean up for next test
+exec-sql
+DROP TABLE users_no_policy;
+----
+
+exec-sql
+DROP FUNCTION f1;
+----
+
+exec-sql
+DROP DATABASE newdb;
+----
+
+# ==============================================================================
+# Test 2: Comprehensive dependencies (functions, types, and sequences)
+# ==============================================================================
+
+# Set up the dependencies: functions, types, and sequences
+exec-sql
+CREATE FUNCTION is_admin(role text) RETURNS BOOL LANGUAGE SQL AS $$ SELECT role = 'admin' $$;
+----
+
+exec-sql
+CREATE TYPE user_role AS ENUM ('admin', 'user', 'guest');
+----
+
+exec-sql
+CREATE SEQUENCE user_counter;
+----
+
+exec-sql
+CREATE FUNCTION get_next_id() RETURNS INT LANGUAGE SQL AS $$ SELECT nextval('user_counter') $$;
+----
+
+# Create the table with dependencies
+exec-sql
+CREATE TABLE accounts (id INT PRIMARY KEY DEFAULT get_next_id(), name TEXT, role user_role DEFAULT 'user');
+----
+
+exec-sql
+ALTER TABLE accounts ENABLE ROW LEVEL SECURITY;
+----
+
+# Create the policy with dependencies on functions, types, and sequences
+exec-sql
+CREATE POLICY admin_policy ON accounts 
+FOR ALL 
+USING (is_admin(role::text)) 
+WITH CHECK (role = 'admin'::user_role AND id > 0 AND nextval('user_counter') < 10000);
+----
+
+# Verify the policy was created
+query-sql
+SHOW POLICIES FOR accounts;
+----
+admin_policy ALL permissive {public} public.is_admin("role"::STRING) (("role" = 'admin':::public.user_role) AND (id > 0:::INT8)) AND (nextval('public.user_counter'::REGCLASS) < 10000:::INT8)
+
+# Test data insertion
+exec-sql
+INSERT INTO accounts (name, role) VALUES ('test_admin', 'admin'::user_role);
+----
+
+exec-sql
+INSERT INTO accounts (name, role) VALUES ('test_user', 'user'::user_role);
+----
+
+# Backup the database again
+exec-sql
+BACKUP DATABASE testdb INTO 'nodelocal://1/test/';
+----
+
+# Verify what was backed up
+query-sql
+WITH descs AS (
+  SHOW BACKUP LATEST IN 'nodelocal://1/test/'
+)
+SELECT database_name, parent_schema_name, object_name, object_type, is_full_cluster FROM descs
+WHERE object_name NOT LIKE '%users%' OR object_name IS NULL
+ORDER BY database_name, parent_schema_name, object_name
+----
+<nil> <nil> testdb database false
+testdb <nil> public schema false
+testdb public _status type false
+testdb public _user_role type false
+testdb public accounts table false
+testdb public get_next_id function false
+testdb public is_admin function false
+testdb public status type false
+testdb public user_counter table false
+testdb public user_role type false
+
+# Test 2a: Full database restore with new name
+exec-sql
+RESTORE DATABASE testdb FROM LATEST IN 'nodelocal://1/test/' WITH new_db_name = testdb_full;
+----
+
+exec-sql
+USE testdb_full;
+----
+
+# Verify the policy was restored correctly
+query-sql
+SHOW POLICIES FOR accounts;
+----
+admin_policy ALL permissive {public} public.is_admin("role"::STRING) (("role" = 'admin':::public.user_role) AND (id > 0:::INT8)) AND (nextval('public.user_counter'::REGCLASS) < 10000:::INT8)
+
+# Test that dependencies are correctly maintained - should fail to drop due to policy dependencies
+exec-sql
+DROP FUNCTION is_admin
+----
+pq: cannot drop function "is_admin" because other objects ([testdb_full.public.accounts]) still depend on it
+
+exec-sql
+DROP TYPE user_role
+----
+pq: cannot drop type "user_role" because other objects ([testdb_full.public.accounts]) still depend on it
+
+exec-sql
+DROP SEQUENCE user_counter
+----
+pq: cannot drop sequence user_counter because other objects depend on it
+
+# Test that the policy still works after restore
+exec-sql
+INSERT INTO accounts (name, role) VALUES ('restored_admin', 'admin'::user_role);
+----
+
+exec-sql
+INSERT INTO accounts (name, role) VALUES ('restored_user', 'user'::user_role);
+----
+
+# Verify data was restored and new data can be inserted
+query-sql
+SELECT count(*) FROM accounts;
+----
+4
+
+# Test 2b: Partial restore with missing dependencies
+exec-sql
+USE testdb;
+----
+
+exec-sql
+CREATE DATABASE db2;
+----
+
+# Restore with skip_missing_udfs - the table should be restored without the policy
+exec-sql
+RESTORE TABLE accounts, user_counter FROM LATEST IN 'nodelocal://1/test/' WITH into_db = 'db2', skip_missing_udfs;
+----
+
+exec-sql
+USE db2;
+----
+
+# The table should be restored without the policy since dependencies are missing
+query-sql
+SHOW POLICIES FOR accounts;
+----
+
+
+exec-sql
+DROP TABLE accounts;
+----
+
+
+exec-sql
+DROP SEQUENCE user_counter;
+----
+
+
+# ==============================================================================
+# Test 3: Sequence dependency verification after policy removal
+# ==============================================================================
+
+exec-sql
+USE testdb;
+----
+
+exec-sql
+CREATE SEQUENCE counter;
+----
+
+exec-sql
+CREATE TABLE sequence_test (id INT, status string);
+----
+
+exec-sql
+CREATE FUNCTION f2() RETURNS INT LANGUAGE PLpgSQL AS $$
+  BEGIN
+    RETURN 1;
+  END
+$$;
+----
+
+# Create policy with both sequence and function dependencies
+exec-sql
+CREATE POLICY sequence_policy ON sequence_test
+  FOR SELECT
+  USING (nextval('counter') < 10000 and f2() = 1);
+----
+
+# Backup the database with the new objects
+exec-sql
+BACKUP DATABASE testdb INTO 'nodelocal://1/sequence_test/';
+----
+
+exec-sql
+CREATE DATABASE seqdb;
+----
+
+# Restore table and sequence but not the function - policy should be dropped
+exec-sql
+RESTORE TABLE sequence_test, counter
+FROM LATEST IN 'nodelocal://1/sequence_test/'
+WITH into_db = 'seqdb', skip_missing_udfs;
+----
+
+exec-sql
+USE seqdb;
+----
+
+# Verify the policy was not restored
+query-sql
+SHOW POLICIES FOR sequence_test;
+----
+
+# Verify the sequence exists
+query-sql
+SELECT 'counter'::REGCLASS::OID > 0 as sequence_exists;
+----
+true
+
+# The sequence should be droppable because there are no dependencies from the table
+# since the policy was dropped due to missing function dependency
+exec-sql
+DROP SEQUENCE counter;
+----
+
+exec-sql
+DROP TABLE sequence_test;
+----
+
+# Clean up
+exec-sql
+DROP DATABASE testdb;
+----
+
+exec-sql
+DROP DATABASE testdb_full;
+----
+
+exec-sql
+DROP DATABASE db2;
+----
+
+exec-sql
+DROP DATABASE seqdb;
+----

pkg/ccl/changefeedccl/cdcevent/BUILD.bazel

@@ -30,6 +30,7 @@ go_library(
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/row",
         "//pkg/sql/rowenc",
+        "//pkg/sql/sem/catid",
         "//pkg/sql/sem/eval",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sessiondatapb",