DanielLeens commented on PR #12561: URL: https://github.com/apache/seatunnel/pull/12561#issuecomment-6282851784
Thanks @nzw921rx for the careful read, and thanks @goutamadwant for the patience. Sorry for missing this in my last round: I wrote "no source-level blocker" on `0d9c5e5e7e`, and after re-tracing your three points I think P0 is a real gap that I should have caught. This is a static re-read of head `0d9c5e5e7e`; I did not run PostgreSQL. **P0 (PG 15 restart with an existing publication) - I agree.** - Path: `PostgresIncrementalSourceFactory.java:129-135` calls `retainPublishedColumns` for pgoutput jobs. In `PostgresPublishedColumns.java:88-93`, `publishedColumnsByTable` is only queried when the publication already exists (`publishGeneratedColumns != null`) and `serverVersion >= 150000`. The query at `:248` reads `pg_publication_tables.attnames`, and `:254-258` treats every non-null array as an explicit column list. In `excludedColumns` (`:144-149`) a non-null set short-circuits the `attgenerated` rule. - As I understand the PG 15 catalog view, `attnames` is built from all non-dropped attributes when `prattrs IS NULL`, without filtering generated columns (the `a.attnum = ANY(GPT.attrs) OR GPT.attrs IS NULL` predicate you quoted). So after the first run auto-creates `dbz_publication`, a restart would see a set that still contains the generated column, keep it in the schema, and streamed rows would carry NULL (`SeaTunnelRowDebeziumDeserializationConverters.java:90-92` sets a field that is absent from the record to null), which is exactly the symptom this PR is meant to remove. I am reading the view definition, not reproducing it, so @goutamadwant please confirm with a PG 15 container (table with a `GENERATED ALWAYS AS ... STORED` column, run the job once so the publication exists, then restart and log the excluded set). - Test gap: `PostgresCDCGeneratedColumnIT` uses `postgres:14-alpine` at `:68` and does not pre-create the publication, and the PG 15 container at `:78` only covers an explicit column list (`CREATE PUBLICATION seatunnel_column_list` at `:116`). So neither the first-run-on-15 nor the restart-on-15 path is covered. - Suggested direction (same as @nzw921rx): distinguish "no column list" from "explicit list" with `pg_publication_rel.prattrs IS NOT NULL`, use `attnames` only in the explicit-list case, treat `FOR ALL TABLES` and `FOR TABLES IN SCHEMA` as no list, and add a PG 15 test that pre-creates the publication or runs the job twice. **P1 (primary key kept even if not published) - agree it is a real edge.** `retainColumns` removes PK columns from the excluded set at `PostgresPublishedColumns.java:164-167`. PostgreSQL requires replica-identity columns in the list only for publications that publish UPDATE/DELETE, so an INSERT-only publication may omit the key. In that case the schema keeps the PK but the stream does not carry it, and the converter writes NULL (`SeaTunnelRowDebeziumDeserializationConverters.java:91-92`). Failing fast with a clear message when a PK column is not published seems the safer behaviour for a CDC source; if you prefer to keep it lenient, please document and test it. **P2 (docs) - agree.** The FAQ at `docs/en/connectors/source/PostgreSQL-CDC.md:382` (and the ZH page) says a column list "can leave out any column"; please qualify it for replica-identity columns and mention that PK columns are always retained. I am withdrawing my earlier "ready from code review" for the current head until P0 is addressed. Everything else I traced in the previous rounds (version boundaries, filtered mode, fallback logging, constraint rebuilding) is unchanged. Thanks again both. -- 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]
