Re: CREATE TABLE LIKE INCLUDING TRIGGERS

Hi Jian,

I didn't read all of v10 line by line, but a few things stood out on a pass;
I've grouped them by how you might want to act on each.

1. Docs / typos (all cosmetic)

- "not be copied" grammar in the source-view sentence of both
create_table.sgml and create_foreign_table.sgml:
now: triggers from a source view are not be copied.
-> triggers from a source view are not copied.

- Typo "cosntraint" in a create_table_like test comment:
now: ... should not copy source table cosntraint trigger
-> ... should not copy source table constraint trigger

- In the create_table_like test, trigger trigtest_after_stmt passes
'trigtest_before_stmt' as its argument, which doesn't match the trigger
name (looks like a copy-paste):
now: CREATE TRIGGER trigtest_after_stmt ...
('trigtest_before_stmt')
-> CREATE TRIGGER trigtest_after_stmt ...
('trigtest_after_stmt')

- 0002: create_table.sgml's INCLUDING TRIGGERS entry doesn't mention
that a
trigger's enabled/disabled (tgenabled) state is preserved. Suggested,
after the source-view sentence ("... are not copied."):
"The enabled state of each trigger (as set by ALTER TABLE ...
ENABLE/DISABLE TRIGGER) is preserved on the new table."

2. Code -- two fixes to consider

(a) Concurrent trigger add can fail CREATE TABLE via a duplicate
(reproduced)

expandTableLikeClause() walks the source trigger array (trigdesc) by
index,
calling generateClonedTriggerStmt(), whose catalog access can process an
invalidation and rebuild the source trigdesc. The array is sorted by
tgname,
so if another session adds a lower-sorting trigger mid-walk, the indexes
shift
and an already-processed trigger gets cloned again.

I reproduced it by putting a sleep in the loop: with t100/t101/t102 on
the
source, adding t000 from another session right after t100 was processed
(CREATE TRIGGER is compatible with AccessShareLock, so it isn't blocked)
made
the next iteration see the rebuilt array [t000, t100, t101, t102], with
nt=1
pointing at t100 again -- so it tried to create t100 twice on the new
table
and the whole CREATE TABLE failed with "trigger ... already exists".

The removal side (DROP/ALTER/RENAME TRIGGER) is blocked by
AccessExclusiveLock
against the source's AccessShareLock, so the array can't shrink and
nothing is
missed; the only thing that can run concurrently is CREATE TRIGGER (a
compatible lock), leaving just this duplicate. It's probably a rare
path, but
it seems worth addressing.

(b) tgenabled channel unification (CreateTriggerFiringOn)

0002 adds a tgenabled field to CreateTrigStmt and sets it in several
places,
but the deferred-unique trigger in index.c and the FK triggers in
tablecmds.c still call CreateTrigger(). CreateTrigger() hardcodes
tgenabled
to TRIGGER_FIRES_ON_ORIGIN internally, so the trigger->tgenabled just
assigned is a dead store. It's harmless today since the value is always
ORIGIN, but it's a field set and then ignored. Unifying those sites on
CreateTriggerFiringOn(..., trigger->tgenabled) (or dropping the
assignment)
would consolidate the two channels.

Best regards,
Henson