ITADN

[destination-redshift] 4.0.4: NULL-safe PK predicate is not equi-joinable — dedup DELETE/INSERT degrade to a nested loop

#83304Closedcernadasjuan 创建于 20 天前
type/bugarea/connectorscommunityneeds-triageconnectors/destination/redshiftteam/extensibilityautoteamteam/destinations
C
cernadasjuancommented
### Connector Name destination-redshift ### Connector Version 4.0.4 ### What step the error happened? During the sync ### Relevant information ### Summary #83245 replaced plain PK equality with a NULL-safe disjunction at the CDC DELETE and the dedup INSERT sites: ``` -- 4.0.3 target.pk = dedup.pk -- 4.0.4 (target.pk = dedup.pk OR (target.pk IS NULL AND dedup.pk IS NULL)) ``` This is semantically correct but not an equality predicate. Redshift can only choose a hash join or a merge join when the join condition is a pure equality; with the OR the planner falls back to a nested loop (typically DS_BCAST_INNER), taking both statements from roughly O(n+m) to O(n×m) against the final table. The cost scales with final table size × staged batch size, so it degrades as the destination table grows rather than showing up immediately. There is no error — syncs keep succeeding, just slower, until they overrun the schedule interval and start stacking up. #83245 itself already surfaced this property: commit 1842c2c reverted updateExistingRows to plain equality because Redshift rejected the NULL-safe form in UPDATE ... FROM with Target table must be part of an equijoin predicate. That error is the planner reporting that the predicate is not an equi-join. In DELETE ... USING and in INSERT ... WHERE NOT EXISTS, Redshift accepts the same shape but plans it as a nested loop instead of refusing it. ### Impact observed Postgres CDC source → Redshift (provisioned), append_dedup, single-column varchar primary key (id), cursor _ab_cdc_lsn. Same connection, same stream, no other changes. End-to-end sync duration on the day of the upgrade: Connector | Records read | Duration -- | -- | -- 4.0.3 | 218,868 | 2m 15s 4.0.4 | 299,277 | 17m 14s 4.0.4 | 513,812 | 36m 24s 4.0.4 | 755,926 | 1h 43m 59s 4.0.3 (pinned back) | 354,820 | 4m 54s The emitted SQL was read directly out of the destination logs at both ends of that window, so the first and last rows are version-confirmed; the two middle rows fall inside the same upgrade window. These durations confound the predicate change with a growing backlog, so the cleaner apples-to-apples measurement is the T+D block alone: 4.0.4 — BEGIN TRANSACTION at 15:25:49 with ≥2.03M rows in the staging table. Still running 13m 30s later; the DELETE had to be killed manually on the cluster, which rolled the transaction back and failed the sync. 4.0.3 — BEGIN TRANSACTION at 15:49:56 with ≥2.39M rows in the staging table (more rows, since it also absorbed the previous sync's leftovers). Committed at 15:51:57: 2m 01s. The `CREATE TEMP TABLE ... ROW_NUMBER() OVER (PARTITION BY ...)` dedup step is byte-identical between the two versions. The only difference in the emitted SQL is the two PK match sites. ### Why #83261 does not address this #83261 rewrites the dedup INSERT from a correlated NOT EXISTS to a LEFT JOIN anti-join, which fixes the This type of correlated subquery pattern is not supported failure from oncall 13202. But: It carries buildNullSafePkMatch verbatim into the ON clause. A LEFT JOIN whose ON contains a disjunction is still not hash-joinable, so the nested loop remains. It does not touch the CDC DELETE, which is where our sync hung. The AI review report on that PR marks the performance gate PASS on the reasoning that "Redshift generally plans hash anti-joins at least as well as correlated subqueries." That holds for an equality ON clause, not for a disjunctive one. The enclosing statement form is not what defeats the hash join — the OR is. ### Suggested fix Express NULL-safe matching as an equality so the planner can still hash-join: ``` -- instead of target.pk = dedup.pk OR (target.pk IS NULL AND dedup.pk IS NULL) -- emit NVL(target.pk, <sentinel>) = NVL(dedup.pk, <sentinel>) ``` with a per-type sentinel value. This composes for compound keys as an AND of per-column equalities, keeps the whole predicate equi-joinable, and — unlike the current form — is also valid in UPDATE ... FROM. That would let updateExistingRows become NULL-safe too, closing the half-fix that #83261 explicitly flags as unresolved. The sentinel needs a value that cannot occur in the data. The alternative that avoids the question entirely is the NOT NULL PK-signature column already floated in #83261's description: derive a hash of the PK values (with an explicit NULL marker) into a NOT NULL column on both the dedup temp table and the final table, and join on that. Separately, and independently useful: gate the NULL-safe form on declared nullability. buildNullSafePkMatch on master today is unconditional — it maps over primaryKeyColumns and emits the OR form for every one of them, with no nullability check: ```kotlin private fun buildNullSafePkMatch( primaryKeyColumns: List<String>, targetPrefix: String, sourcePrefix: String, ): String { return primaryKeyColumns.joinToString(" AND ") { pk -> "($targetPrefix.$pk = $sourcePrefix.$pk OR ($targetPrefix.$pk IS NULL AND $sourcePrefix.$pk IS NULL))" } } ``` Where the catalog declares a PK column non-nullable, plain equality is exactly equivalent, and emitting it keeps that case off the slow path regardless of which formulation is chosen above. This is a partial mitigation only: CDC sources frequently mark every column nullable in the discovered catalog regardless of the source's NOT NULL constraints, which is the case in this report — id is NOT NULL in Postgres but nullable=true in the Airbyte catalog. Note that IS NOT DISTINCT FROM is not a viable substitute — even where Redshift accepts it, the planner does not treat it as an equality condition. ### Workaround Pin destination-redshift to 4.0.3. Nothing at the connection or schema level avoids the slow path, since the predicate is emitted unconditionally. Pinning re-introduces the NULL-PK duplicate behaviour from oncall 13186, which for streams with a genuinely non-nullable business key is a no-op. ### Process note #83245 was force-merged (/force-merge reason="acceptance tests are passing") and destination-redshift does not have progressive rollout enabled, so 4.0.4 shipped broadly. Two separate production regressions have surfaced from it since: the correlated-subquery rejection tracked in #83261 , and this one. Enabling progressive rollout for this connector would have bounded the blast radius of both. Happy to run any diagnostic query on our cluster and attach the output. ### Relevant log output ```shell ``` ### Contribute - [x] Yes, I want to contribute --- **Internal Tracking:** https://github.com/airbytehq/oncall/issues/13211
关闭于 16 天前 3 条评论