Thread
-
Re: CREATE TABLE LIKE INCLUDING TRIGGERS
Henson Choi <assam258@gmail.com> — 2026-05-05T11:17:31Z
Hi Jian, Thank you for the patch. I applied v9 to a clean checkout, built it, and ran the regression suite — all tests pass. Nice work. I have attached a quality assessment report below. A line-by-line code review will follow in due course. Detailed gcov coverage in HTML format is included in coverage.tgz. CREATE TABLE LIKE INCLUDING TRIGGERS — Coverage Review ====================================================== Patch: CREATE TABLE LIKE INCLUDING TRIGGERS Commitfest: https://commitfest.postgresql.org/patch/6087 Review Methodology: Quality assessment: build verification, regression testing, and code coverage analysis (gcov, modified lines only). This is not a full code review. Limitations: - Not a security audit or formal verification TABLE OF CONTENTS ----------------- 1. Executive Summary 2. Issues Found 2.1 Critical / Major 2.2 Minor (Review Needed) 3. Test Coverage 3.1 Covered Areas 3.2 Untested Items 4. Code Coverage Analysis 4.1 Overall Coverage 4.2 Uncovered Code Risk Assessment 5. Commit Analysis 6. Recommendations 7. Conclusion 1. EXECUTIVE SUMMARY -------------------- Overall assessment: GOOD The patch adds INCLUDING TRIGGERS to CREATE TABLE LIKE (and CREATE FOREIGN TABLE LIKE). Build succeeds and all regression tests pass. The following behaviors are verified by the test suite: - Internal triggers (FK-associated) are not copied - INSTEAD OF triggers on views produce an error - Whole-row references in WHEN clauses produce an error - Trigger comments are copied only when INCLUDING COMMENTS is given - The trigger's enabled state is preserved on the new table Rating by Area: Test Coverage: Good (broad scenario coverage) Documentation: Thin (functional but sparse) Build/Regress: Pass 2. ISSUES FOUND --------------- 2.1 Critical / Major None. 2.2 Minor (Review Needed) #1 [Documentation] INCLUDING TRIGGERS entry is too brief create_table.sgml currently says only: "All non-internal triggers on the original table will be created on the new table." The following behaviors are not mentioned: (a) The trigger's enabled state is preserved on the new table. (b) A WHEN clause with a whole-row reference causes an error. (c) INSTEAD OF triggers cannot be copied to a plain table. Suggested addition after the existing sentence: <para> The enabled state of each trigger (as set by ALTER TABLE ... ENABLE/DISABLE TRIGGER) is preserved on the new table. An error is raised if any trigger's WHEN clause contains a whole-row table reference. INSTEAD OF triggers on views cannot be copied to a plain table. </para> The same applies to the entry in create_foreign_table.sgml. 3. TEST COVERAGE ---------------- 3.1 Covered Areas triggers.sql additions exercise: - Whole-row WHEN clause (OLD IS NOT NULL) → error - Whole-row WHEN clause (NEW IS NOT NULL) → error - Basic LIKE INCLUDING TRIGGERS + INCLUDING COMMENTS - Trigger enabled state: ENABLE REPLICA, DISABLE, ENABLE ALWAYS - INSTEAD OF triggers on a view → error - Statement-level view triggers with transition tables - Partitioned table: table-level trigger only, not partition triggers - Transition table triggers on partitioned table copy - Cross-schema trigger function - Constraint trigger with FROM clause create_table_like.sql additions exercise: - Triggers with WHEN clause and column-specific UPDATE (via LIKE ctl_table INCLUDING ALL on a foreign table) 3.2 Untested Items The following lines were uncovered per gcov analysis (see section 4). Line Code path ------------------------------------------------------------------------------- trigger.c:6959 elog: trigger OID not found in pg_trigger trigger.c:6966 elog: trigger function OID not in syscache trigger.c:7002 elog: tgargs is null when tgnargs > 0 parse_utilcmd.c:1640 continue: skip internal trigger Lines 6959, 6966, 7002 are catalog-inconsistency guards. All three require catalog corruption to reach; untestable under normal operation. No functional risk. Line 1640 (the internal-trigger skip) is reachable but requires a source table that has FK constraints (which create internal triggers). The patch description explicitly states that FK-associated internal triggers are excluded, but no test verifies this behavior. Suggested test (add near the tgenabled block in triggers.sql): -- Internal triggers (e.g. FK) must not be copied CREATE TABLE fk_parent (id int PRIMARY KEY); CREATE TABLE fk_child (id int REFERENCES fk_parent(id)); CREATE TABLE fk_child_copy (LIKE fk_child INCLUDING TRIGGERS); SELECT count(*) FROM pg_trigger WHERE tgrelid = 'fk_child_copy'::regclass; -- expect 0 DROP TABLE fk_child_copy, fk_child, fk_parent; 4. CODE COVERAGE ANALYSIS ------------------------- 4.1 Overall Coverage (modified lines only) trigger.c 96.8% (91 / 94 lines) parse_utilcmd.c 92.3% (12 / 13 lines) ----------------------------------------------- Combined 96.8% (120 / 124 lines) 4.2 Uncovered Code Risk Assessment trigger.c:6959, 6966, 7002 — catalog-inconsistency guards. All three require catalog corruption to reach; untestable under normal operation. No functional risk. parse_utilcmd.c:1640 — internal trigger skip. Reachable with a table that has FK constraints. A test is straightforward; see section 3.2 for sample SQL. 5. COMMIT ANALYSIS ------------------ 2 commits: Commit Area Files +/- Key Content ----------------------------------------------------------------------- e73f551 Core feature 14 +531/-19 generateClonedTriggerStmt, expandTableLikeClause, grammar, docs, tests 39ae762 tgenabled copy 8 +27/-6 trigger enabled state preserved on new table The two-commit structure is sensible given that the enabled state copy was a post-review addition. Squashing is straightforward if preferred. 6. RECOMMENDATIONS ------------------ 1. Add a test verifying that FK internal triggers are not copied (sample SQL in section 3.2). 2. Expand the INCLUDING TRIGGERS documentation entry (section 2.2 #1). 7. CONCLUSION ------------- From a quality standpoint, the patch is in good shape: regression tests pass, coverage is at 96.8% on modified lines, and the key scenarios (enabled state, whole-row errors, partitioned tables, cross-schema functions) are all exercised by the test suite. The missing test for internal trigger exclusion (#1) and the documentation gaps (#2) are worth addressing before commit. The remaining items are minor.