diff --git a/.github/workflows/run-tests-tiered.yml b/.github/workflows/run-tests-tiered.yml index 52497501..c8110bd3 100644 --- a/.github/workflows/run-tests-tiered.yml +++ b/.github/workflows/run-tests-tiered.yml @@ -156,6 +156,7 @@ jobs: - blob-snapshot-release - follow-defer-indexes - fk-not-valid + - defer-validate-fks steps: - name: Checkout repository uses: actions/checkout@v6 diff --git a/docs/include/clone.rst b/docs/include/clone.rst index 61f7563c..8c01f053 100644 --- a/docs/include/clone.rst +++ b/docs/include/clone.rst @@ -47,5 +47,6 @@ --endpos Stop replaying changes when reaching this LSN --defer-indexes Defer index building until after all table data is copied --defer-analyze Defer ANALYZE until after post-data restore + --defer-validate-fks Create FK constraints as NOT VALID, skipping validation scan --use-copy-binary Use the COPY BINARY format for COPY operations diff --git a/src/bin/pgcopydb/cli_clone_follow.c b/src/bin/pgcopydb/cli_clone_follow.c index a8c09447..232c3ace 100644 --- a/src/bin/pgcopydb/cli_clone_follow.c +++ b/src/bin/pgcopydb/cli_clone_follow.c @@ -70,6 +70,7 @@ " --endpos Stop replaying changes when reaching this LSN\n" \ " --defer-indexes Defer index building until after all table data is copied\n" \ " --defer-analyze Defer ANALYZE until after post-data restore\n" \ + " --defer-validate-fks Create FK constraints as NOT VALID, skipping validation scan\n" \ " --use-copy-binary Use the COPY BINARY format for COPY operations\n" \ CommandLine clone_command = diff --git a/src/bin/pgcopydb/cli_common.c b/src/bin/pgcopydb/cli_common.c index 09eea4b5..0e6a79e6 100644 --- a/src/bin/pgcopydb/cli_common.c +++ b/src/bin/pgcopydb/cli_common.c @@ -260,7 +260,9 @@ cli_copydb_getenv(CopyDBOptions *options) { PGCOPYDB_DEFER_INDEXES, ENV_TYPE_BOOL, &(options->deferIndexes) }, { PGCOPYDB_DEFER_ANALYZE, ENV_TYPE_BOOL, - &(options->deferAnalyze) } + &(options->deferAnalyze) }, + { PGCOPYDB_DEFER_VALIDATE_FKS, ENV_TYPE_BOOL, + &(options->deferValidateFKs) } }; int parserCount = sizeof(parsers) / sizeof(parsers[0]); @@ -654,6 +656,7 @@ cli_copy_db_getopts(int argc, char **argv) { "restore-tolerance", required_argument, NULL, 256 }, { "defer-indexes", no_argument, NULL, 257 }, { "defer-analyze", no_argument, NULL, 258 }, + { "defer-validate-fks", no_argument, NULL, 259 }, { "help", no_argument, NULL, 'h' }, { NULL, 0, NULL, 0 } }; @@ -1149,6 +1152,13 @@ cli_copy_db_getopts(int argc, char **argv) break; } + case 259: + { + options.deferValidateFKs = true; + log_trace("--defer-validate-fks"); + break; + } + case '?': default: { diff --git a/src/bin/pgcopydb/cli_common.h b/src/bin/pgcopydb/cli_common.h index b3548d35..fefc3436 100644 --- a/src/bin/pgcopydb/cli_common.h +++ b/src/bin/pgcopydb/cli_common.h @@ -73,6 +73,7 @@ typedef struct CopyDBOptions bool skipXidCheck; bool deferIndexes; bool deferAnalyze; + bool deferValidateFKs; bool noRolesPasswords; bool failFast; bool useCopyBinary; diff --git a/src/bin/pgcopydb/copydb.c b/src/bin/pgcopydb/copydb.c index 200ec9aa..a73c1dbf 100644 --- a/src/bin/pgcopydb/copydb.c +++ b/src/bin/pgcopydb/copydb.c @@ -519,6 +519,7 @@ copydb_init_specs(CopyDataSpec *specs, .skipXidCheck = options->skipXidCheck, .deferIndexes = options->deferIndexes, .deferAnalyze = options->deferAnalyze, + .deferValidateFKs = options->deferValidateFKs, .noRolesPasswords = options->noRolesPasswords, .failFast = options->failFast, .useCopyBinary = options->useCopyBinary, diff --git a/src/bin/pgcopydb/copydb.h b/src/bin/pgcopydb/copydb.h index 796f096b..f0915b33 100644 --- a/src/bin/pgcopydb/copydb.h +++ b/src/bin/pgcopydb/copydb.h @@ -235,6 +235,7 @@ typedef struct CopyDataSpec bool skipXidCheck; bool deferIndexes; bool deferAnalyze; + bool deferValidateFKs; bool noRolesPasswords; bool useCopyBinary; diff --git a/src/bin/pgcopydb/defaults.h b/src/bin/pgcopydb/defaults.h index af2c9f5e..944756a0 100644 --- a/src/bin/pgcopydb/defaults.h +++ b/src/bin/pgcopydb/defaults.h @@ -74,6 +74,7 @@ #define PGCOPYDB_RESTORE_TOLERANCE "PGCOPYDB_RESTORE_TOLERANCE" #define PGCOPYDB_DEFER_INDEXES "PGCOPYDB_DEFER_INDEXES" #define PGCOPYDB_DEFER_ANALYZE "PGCOPYDB_DEFER_ANALYZE" +#define PGCOPYDB_DEFER_VALIDATE_FKS "PGCOPYDB_DEFER_VALIDATE_FKS" /* default values for the command line options */ #define DEFAULT_TABLE_JOBS 4 diff --git a/src/bin/pgcopydb/indexes.c b/src/bin/pgcopydb/indexes.c index 73eb1895..80437ff1 100644 --- a/src/bin/pgcopydb/indexes.c +++ b/src/bin/pgcopydb/indexes.c @@ -1483,6 +1483,7 @@ copydb_create_fk_constraints(CopyDataSpec *specs) bool success = true; int notValidCount = 0; int sourceNotValidCount = 0; + int deferredNotValidCount = 0; for (int i = 0; i < fkArray.count; i++) { @@ -1515,14 +1516,26 @@ copydb_create_fk_constraints(CopyDataSpec *specs) * for such constraints, so the constraintDef handles this, but we * log it explicitly for clarity. */ + bool notValid = false; if (!fk->convalidated) { log_notice("FK constraint \"%s\" on %s is NOT VALID on source, " "creating as NOT VALID on target", fk->conname, fk->tableQname); + notValid = true; sourceNotValidCount++; } + if (specs->deferValidateFKs) + { + log_notice("Defer validate is set for FKs, " + "creating FK constraint \"%s\" on %s " + "as NOT VALID on target", + fk->conname, fk->tableQname); + notValid = true; + deferredNotValidCount++; + } + /* * Build the ALTER TABLE ADD CONSTRAINT command. */ @@ -1544,6 +1557,16 @@ copydb_create_fk_constraints(CopyDataSpec *specs) } } + /* + * Source-NOT-VALID constraints already carry NOT VALID inside + * constraintDef (from pg_get_constraintdef), so we only append it + * here when the source was validated and we're deferring. + */ + if (specs->deferValidateFKs && fk->convalidated) + { + appendPQExpBufferStr(cmd, " NOT VALID"); + } + if (PQExpBufferBroken(cmd)) { log_error("Failed to create query for FK constraint \"%s\": " @@ -1567,16 +1590,15 @@ copydb_create_fk_constraints(CopyDataSpec *specs) log_notice("Creating FK constraint: %s", cmd->data); - bool notValid = false; - if (!pgsql_execute(&dst, cmd->data)) { /* * Check if the failure is due to a foreign key violation - * (SQLSTATE 23503). If so, retry with NOT VALID. + * (SQLSTATE 23503). If so, retry with NOT VALID only if + * the original attempt didn't already include NOT VALID. */ if (strcmp(dst.sqlstate, - STR_ERRCODE_FOREIGN_KEY_VIOLATION) == 0) + STR_ERRCODE_FOREIGN_KEY_VIOLATION) == 0 && !notValid) { log_warn("FK constraint \"%s\" on %s has pre-existing data " "violations, retrying with NOT VALID", @@ -1703,6 +1725,13 @@ copydb_create_fk_constraints(CopyDataSpec *specs) notValidCount); } + if (deferredNotValidCount > 0) + { + log_warn("%d FK constraint(s) created as NOT VALID due to " + "defer-validate flag being used", + deferredNotValidCount); + } + /* cleanup */ for (int i = 0; i < fkArray.count; i++) { @@ -1716,4 +1745,4 @@ copydb_create_fk_constraints(CopyDataSpec *specs) (void) pgsql_finish(&dst); return success; -} +} \ No newline at end of file diff --git a/tests/Makefile b/tests/Makefile index 37bda801..970ea0e3 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -14,7 +14,7 @@ all: pagila pagila-multi-steps blobs unit filtering filtering-standby extensions cdc-wal2json cdc-test-decoding cdc-endpos-between-transaction cdc-low-level \ follow-wal2json follow-standby follow-9.6 follow-data-only \ endpos-in-multi-wal-txn exclude-extension \ - blob-snapshot-release follow-defer-indexes fk-not-valid; + blob-snapshot-release follow-defer-indexes fk-not-valid defer-validate-fks; pagila: build $(MAKE) -C $@ @@ -79,6 +79,9 @@ follow-defer-indexes: build fk-not-valid: build $(MAKE) -C $@ +defer-validate-fks: build + $(MAKE) -C $@ + timescaledb: build $(MAKE) -C $@ @@ -91,4 +94,4 @@ build: .PHONY: cdc-wal2json cdc-test-decoding cdc-low-level .PHONY: follow-wal2json follow-standby follow-9.6 .PHONY: endpos-in-multi-wal-txn exclude-extension -.PHONY: blob-snapshot-release follow-defer-indexes fk-not-valid +.PHONY: blob-snapshot-release follow-defer-indexes fk-not-valid defer-validate-fks diff --git a/tests/defer-validate-fks/Dockerfile b/tests/defer-validate-fks/Dockerfile new file mode 100644 index 00000000..1a4cb78c --- /dev/null +++ b/tests/defer-validate-fks/Dockerfile @@ -0,0 +1,9 @@ +FROM pagila + +COPY --from=pgcopydb /usr/local/bin/pgcopydb /usr/local/bin + +WORKDIR /usr/src/pgcopydb +COPY copydb.sh copydb.sh + +USER docker +CMD ["/usr/src/pgcopydb/copydb.sh"] diff --git a/tests/defer-validate-fks/Makefile b/tests/defer-validate-fks/Makefile new file mode 100644 index 00000000..d405ef0e --- /dev/null +++ b/tests/defer-validate-fks/Makefile @@ -0,0 +1,15 @@ +# Copyright (c) 2021 The PostgreSQL Global Development Group. +# Licensed under the PostgreSQL License. + +test: down run down ; + +run: build + $(DOCKER) compose run test + +down: + $(DOCKER) compose down + +build: + $(DOCKER) compose build + +.PHONY: down build test diff --git a/tests/defer-validate-fks/compose.yaml b/tests/defer-validate-fks/compose.yaml new file mode 100644 index 00000000..cfebc6da --- /dev/null +++ b/tests/defer-validate-fks/compose.yaml @@ -0,0 +1,42 @@ +services: + source: + image: postgres:${PGVERSION:-16} + expose: + - 5432 + environment: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: h4ckm3 + POSTGRES_HOST_AUTH_METHOD: trust + command: > + -c ssl=on + -c ssl_cert_file=/etc/ssl/certs/ssl-cert-snakeoil.pem + -c ssl_key_file=/etc/ssl/private/ssl-cert-snakeoil.key + target: + image: postgres:${PGVERSION:-16} + expose: + - 5432 + environment: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: h4ckm3 + POSTGRES_HOST_AUTH_METHOD: trust + command: > + -c ssl=on + -c ssl_cert_file=/etc/ssl/certs/ssl-cert-snakeoil.pem + -c ssl_key_file=/etc/ssl/private/ssl-cert-snakeoil.key + test: + build: + context: . + dockerfile: Dockerfile + cap_add: + - SYS_ADMIN + - SYS_PTRACE + environment: + PGSSLMODE: "require" + PGCOPYDB_SOURCE_PGURI: postgres://postgres:h4ckm3@source/postgres + PGCOPYDB_TARGET_PGURI: postgres://postgres:h4ckm3@target/postgres + PGCOPYDB_TABLE_JOBS: 4 + PGCOPYDB_INDEX_JOBS: 4 + PGCOPYDB_FAIL_FAST: "true" + depends_on: + - source + - target diff --git a/tests/defer-validate-fks/copydb.sh b/tests/defer-validate-fks/copydb.sh new file mode 100755 index 00000000..c18bffa8 --- /dev/null +++ b/tests/defer-validate-fks/copydb.sh @@ -0,0 +1,164 @@ +#! /bin/bash + +set -x +set -e + +# Disable pager for psql to avoid hanging in non-interactive environments +export PAGER=cat + +# This script expects the following environment variables to be set: +# +# - PGCOPYDB_SOURCE_PGURI +# - PGCOPYDB_TARGET_PGURI +# - PGCOPYDB_TABLE_JOBS +# - PGCOPYDB_INDEX_JOBS + +# make sure source and target databases are ready +pgcopydb ping + +# +# Build a source schema with two FK shapes: +# +# 1. A pair of tables with clean data and a validating FK on source +# (convalidated = true). With --defer-validate-fks we expect this to +# land on the target as NOT VALID. +# +# 2. A pair of tables where the FK was added NOT VALID on source +# (convalidated = false). It should remain NOT VALID on the target +# regardless of the flag. +# +psql -d ${PGCOPYDB_SOURCE_PGURI} <<'SQL' + +CREATE TABLE clean_parent ( + id serial PRIMARY KEY, + name text +); + +CREATE TABLE clean_child ( + id serial PRIMARY KEY, + parent_id integer REFERENCES clean_parent(id) +); + +INSERT INTO clean_parent VALUES (1, 'p1'), (2, 'p2'); +INSERT INTO clean_child VALUES (1, 1), (2, 2); + +-- Source-side NOT VALID FK +CREATE TABLE legacy_parent ( + id serial PRIMARY KEY, + name text +); + +CREATE TABLE legacy_child ( + id serial PRIMARY KEY, + parent_id integer +); + +INSERT INTO legacy_parent VALUES (1, 'lp1'); +INSERT INTO legacy_child VALUES (1, 1), (2, 999); + +ALTER TABLE legacy_child + ADD CONSTRAINT legacy_child_parent_id_fkey + FOREIGN KEY (parent_id) REFERENCES legacy_parent(id) + NOT VALID; + +SQL + +# +# Sanity-check the source state: clean FK is convalidated, legacy FK is not. +# +src_clean_state=$(psql -AtX -d ${PGCOPYDB_SOURCE_PGURI} -c \ + "SELECT convalidated FROM pg_constraint WHERE conname = 'clean_child_parent_id_fkey'") +src_legacy_state=$(psql -AtX -d ${PGCOPYDB_SOURCE_PGURI} -c \ + "SELECT convalidated FROM pg_constraint WHERE conname = 'legacy_child_parent_id_fkey'") + +echo "=== Source clean FK convalidated: ${src_clean_state} (expect t) ===" +echo "=== Source legacy FK convalidated: ${src_legacy_state} (expect f) ===" + +if [ "${src_clean_state}" != "t" ] || [ "${src_legacy_state}" != "f" ]; then + echo "ERROR: source FK setup is wrong" + exit 1 +fi + +# +# Run pgcopydb clone with --defer-validate-fks. Every FK should land on +# the target as NOT VALID; pgcopydb should not run any validating seqscans. +# +pgcopydb clone --notice --defer-validate-fks + +echo "" +echo "=== Clone completed, verifying results ===" +echo "" + +# +# Both FKs should be NOT VALID on the target. +# +tgt_clean_state=$(psql -AtX -d ${PGCOPYDB_TARGET_PGURI} -c \ + "SELECT convalidated FROM pg_constraint WHERE conname = 'clean_child_parent_id_fkey'") +tgt_legacy_state=$(psql -AtX -d ${PGCOPYDB_TARGET_PGURI} -c \ + "SELECT convalidated FROM pg_constraint WHERE conname = 'legacy_child_parent_id_fkey'") + +echo "Target clean FK convalidated: ${tgt_clean_state} (expect f)" +echo "Target legacy FK convalidated: ${tgt_legacy_state} (expect f)" + +if [ "${tgt_clean_state}" != "f" ]; then + echo "ERROR: clean FK should be NOT VALID on target with --defer-validate-fks" + exit 1 +fi + +if [ "${tgt_legacy_state}" != "f" ]; then + echo "ERROR: source-NOT-VALID FK should remain NOT VALID on target" + exit 1 +fi + +# +# Data should still be copied in full. +# +tgt_clean_rows=$(psql -AtX -d ${PGCOPYDB_TARGET_PGURI} -c "SELECT count(*) FROM clean_child") +tgt_legacy_rows=$(psql -AtX -d ${PGCOPYDB_TARGET_PGURI} -c "SELECT count(*) FROM legacy_child") + +if [ "${tgt_clean_rows}" != "2" ] || [ "${tgt_legacy_rows}" != "2" ]; then + echo "ERROR: row count mismatch (clean=${tgt_clean_rows}, legacy=${tgt_legacy_rows})" + exit 1 +fi + +# +# Future writes should still be enforced even though the constraint is +# NOT VALID — NOT VALID only skips checks against pre-existing rows. +# +set +e +result=$(psql -d ${PGCOPYDB_TARGET_PGURI} -c \ + "INSERT INTO clean_child (id, parent_id) VALUES (99, 9999)" 2>&1) +set -e + +if echo "${result}" | grep -q "violates foreign key constraint"; then + echo "Future writes correctly enforced on NOT VALID constraint." +else + echo "ERROR: NOT VALID constraint should still enforce on new writes!" + echo "psql output: ${result}" + exit 1 +fi + +# +# A manual VALIDATE CONSTRAINT on the clean FK should succeed (data is +# clean by construction) and flip convalidated to true. This proves the +# deferred validation path works for callers that want to validate later. +# +psql -d ${PGCOPYDB_TARGET_PGURI} -c \ + "ALTER TABLE clean_child VALIDATE CONSTRAINT clean_child_parent_id_fkey" + +post_validate_state=$(psql -AtX -d ${PGCOPYDB_TARGET_PGURI} -c \ + "SELECT convalidated FROM pg_constraint WHERE conname = 'clean_child_parent_id_fkey'") + +if [ "${post_validate_state}" != "t" ]; then + echo "ERROR: VALIDATE CONSTRAINT should have set convalidated=t" + exit 1 +fi + +echo "" +echo "defer-validate-fks test: PASSED" +echo "" +echo " - Clean FK landed on target as NOT VALID per --defer-validate-fks" +echo " - Source-NOT-VALID FK stayed NOT VALID on target" +echo " - Data was copied fully" +echo " - Future writes still enforced on NOT VALID constraints" +echo " - Manual VALIDATE CONSTRAINT succeeds against clean data"