On Thu, May 7, 2026 at 9:38 AM Henson Choi <[email protected]> wrote: > > MINOR ISSUES > ------------ > > 1. Test: cross-file dependency on trigger_func (create_table_like.sql) > > create_table_like.sql references trigger_func without creating it, > relying on triggers.sql having run first. The dependency is noted > in a comment, and parallel_schedule guarantees the correct order for > a full "make check" run. However, "make check TESTS=create_table_like" > will fail because trigger_func does not exist. > > PostgreSQL convention is that each test file creates and drops the > objects it needs. Please add a local definition of trigger_func in > create_table_like.sql (and drop it at the end of the new block). > OK.
> 2. Test: EXCLUDING TRIGGERS not exercised > > The grammar now accepts EXCLUDING TRIGGERS, but no test uses it. > The default (no option) is equivalent, but a one-line smoke test > would confirm the keyword is accepted and has the expected effect: > > CREATE TABLE t_excl (LIKE source_table EXCLUDING TRIGGERS); > OK. > 3. Documentation: create_table.sgml wording inconsistent with > create_foreign_table.sgml > > create_foreign_table.sgml already reads: > > "All non-internal triggers are copied to the new table." > > create_table.sgml reads: > > "All non-internal triggers on the original table will be created > on the new table." > > Recommended wording for create_table.sgml: > > "All non-internal triggers are copied to the new table." > OK. > 4. Code comment: capitalisation in generateClonedTriggerStmt() > (trigger.c) > > /* Reconstruct trigger function String list */ > > "String" should be lowercase "string". > Other places also have "String list", it refers to the T_String node type. > 5. Code comment: overly verbose in expandTableLikeClause() > (parse_utilcmd.c) > > /* We make use of CreateTrigStmt's trigcomment option */ > > The code is self-explanatory. I would recommend removing it or > replacing it with something like: > > /* pass comment through to CreateTrigger */ > Other places in parse_utilcmd.c have: /* * We make use of IndexStmt's idxcomment option, so as not to * need to know now what name the index will have. */ /* * We make use of CreateStatsStmt's stxcomment option, so as * not to need to know now what name the statistics will have. */ We can change it to: /* * We make use of CreateTrigStmt's trigcomment option, so as * not to need to know now what name the triggers will have. */ > > 7. Whole-row reference restriction: implementation gap or deliberate? > > Triggers whose WHEN clause contains a whole-row reference (OLD.*, > NEW.*) are rejected. Is this a deliberate decision, or a known gap > left for a follow-up? If the latter, a XXX comment at the rejection > site would help future contributors: > > /* > * XXX: whole-row Vars could in principle be handled by passing the > * target table's composite type OID as to_rowtype, but > * generateClonedTriggerStmt() currently has no access to it. > */ > Other places already reject it (search found_whole_row in parse_utilcmd.c). It cannot be supported because the source table's whole-row type differs from the target table's whole-row type. The v10 in https://www.postgresql.org/message-id/CACJufxEcKTa5DaDJS%3DZ25xezCEyuLbSzORDSmT4%3DjyZymsAK8A%40mail.gmail.com has addressed most of the issues mentioned above. Only issue remaining is changing one of the comments to /* * We make use of CreateTrigStmt's trigcomment option, so as * not to need to know now what name the triggers will have. */
