Re: CREATE TABLE LIKE INCLUDING TRIGGERS - Mailing list pgsql-hackers
| From | Henson Choi |
|---|---|
| Subject | Re: CREATE TABLE LIKE INCLUDING TRIGGERS |
| Date | |
| Msg-id | CAAAe_zA=yA7Odc3vrfp5ywmAuWU6-t7tUS0re-zsDAvgj0Zazg@mail.gmail.com Whole thread |
| In response to | Re: CREATE TABLE LIKE INCLUDING TRIGGERS (jian he <jian.universality@gmail.com>) |
| List | pgsql-hackers |
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
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
pgsql-hackers by date: