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

Hi Jian,

This is the line-by-line code review I promised earlier.  The overall
conclusion is that the patch is in good shape.  I have some minor
items below, none of which are blockers.

WHAT LOOKS GOOD
---------------

The keyword placement is correct.  TRIGGERS is classified as
UNRESERVED_KEYWORD and also listed in bare_label_keyword, so existing
SQL that uses "triggers" as an identifier continues to work without
quoting.  No compatibility concern here.

generateClonedTriggerStmt() is well-structured.  The WHEN-clause
remapping via map_variable_attnos() and the transformed flag to prevent
re-transformation in CreateTriggerFiringOn() are correct.

pg_dump needs no changes.  LIKE-copied triggers are stored as
independent pg_trigger rows, so the existing dump logic picks them up
automatically.

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).

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);

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."

   Two problems with the latter:
   a) "on the original table" is redundant; no other INCLUDING option
      includes this phrase (compare INCLUDING STATISTICS: "Extended
      statistics are copied to the new table.").
   b) "will be created" misrepresents the action; the correct verb
      is "are copied", matching the pattern used elsewhere.

   Recommended wording for create_table.sgml:

     "All non-internal triggers are copied to the new table."

4. Code comment: capitalisation in generateClonedTriggerStmt()
   (trigger.c)

     /* Reconstruct trigger function String list */

   "String" should be lowercase "string".

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 */

6. Commit message grammar (commit e73f551)

   Several sentences contain grammatical errors:

   a) "This will copy all source table's trigger to the new table."
      -> "...triggers..."

   b) "Internal trigger (such as foreign key associated trigger) won't
       being copied to new table."
      -> "Internal triggers (such as foreign-key-associated triggers)
         won't be copied to the new table."

   c) "However this command will fail if the source table's trigger
       contain whole-row reference."
      -> "However, this command will fail if the source table's triggers
         contain a whole-row reference."

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.
      */

SUMMARY OF ITEMS TO ADDRESS
----------------------------

  [1] create_table_like.sql: define and drop trigger_func locally so
      the file can be run in isolation.

  [2] Add a test for EXCLUDING TRIGGERS.

  [3] create_table.sgml: align the INCLUDING TRIGGERS description with
      create_foreign_table.sgml, which already reads correctly:
      "All non-internal triggers are copied to the new table."

  [4] trigger.c generateClonedTriggerStmt(): lowercase "string" in
      comment "/* Reconstruct trigger function String list */".

  [5] parse_utilcmd.c expandTableLikeClause(): simplify or remove the
      comment "/* We make use of CreateTrigStmt's trigcomment option */".

  [6] Commit message: fix grammar in items (a), (b), (c) listed above.

Items [3]-[6] are purely cosmetic.  [1] and [2] are worth considering
before commit, but none of these are blockers.

Thanks for the patch.

Best regards,
Henson Choi