Re: CREATE TABLE LIKE INCLUDING TRIGGERS

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