Re: CREATE TABLE LIKE INCLUDING TRIGGERS - Mailing list pgsql-hackers

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.