Found while working #168. An AFTER INSERT ... FOR EACH ROW trigger on a pgcolumnar table makes every ordinary INSERT fail.
Reproduce
CREATE TABLE t (id int) USING pgcolumnar;
CREATE TABLE tlog (id int);
CREATE FUNCTION f() RETURNS trigger LANGUAGE plpgsql AS $$
BEGIN INSERT INTO tlog VALUES (NEW.id); RETURN NULL; END $$;
CREATE TRIGGER t_t AFTER INSERT ON t FOR EACH ROW EXECUTE FUNCTION f();
INSERT INTO t VALUES (1);
-- ERROR: failed to fetch tuple1 for AFTER trigger
The same table with the trigger dropped inserts fine, and a heap table with the identical trigger fires it 50 times out of 50.
What it is
An after-row trigger event stores the row's TID when it is queued and re-fetches the row when it fires at end of statement. pgcolumnar buffers rows and flushes a stripe when it fills or at pre-commit, so at end of statement the row is not yet reachable by its synthetic TID and the re-fetch fails.
That is consistent with the timing, but I have not proved it — the flush boundary is the obvious candidate rather than a confirmed cause, and it would be worth confirming before anyone designs around it.
Scope
BEFORE INSERT ... FOR EACH ROW is unaffected: it runs before the row is written and never re-fetches.
- Statement-level triggers are unaffected.
- The importers never fired row triggers at all, so they were not affected — silently, which is its own problem: rows arrived and the user's trigger did not run.
- Tested on 18.4,
-O2 assert build, at 05f8b35.
Why it matters now
#168's fix routes the import path through the executor's index maintenance, which is also what queues after-row trigger events. So a table with both an index and a user after-row trigger now fails the import where it previously succeeded and skipped the trigger.
That is a narrow case and the table is already unusable for ordinary INSERT, so I do not think it should block #168 — a loud failure that matches what INSERT already does seems better than the import quietly not running a user's trigger. But it is a real behaviour change caused by this defect rather than by that fix, which is why it is filed separately.
Before, on main:
import into indexed table with AFTER ROW trigger -> 50 rows, 0 triggers fired
After #168's fix:
-> ERROR: failed to fetch tuple1 for AFTER trigger, 0 rows
If you would rather have no regression at all while this is open, gating the ExecARInsertTriggers call on a non-empty recheck list confines the change to constraints and leaves user triggers exactly as unfired as they are today. It is one condition, and I am happy to add it — say which you prefer.
Found while working #168. An
AFTER INSERT ... FOR EACH ROWtrigger on a pgcolumnar table makes every ordinaryINSERTfail.Reproduce
The same table with the trigger dropped inserts fine, and a heap table with the identical trigger fires it 50 times out of 50.
What it is
An after-row trigger event stores the row's TID when it is queued and re-fetches the row when it fires at end of statement. pgcolumnar buffers rows and flushes a stripe when it fills or at pre-commit, so at end of statement the row is not yet reachable by its synthetic TID and the re-fetch fails.
That is consistent with the timing, but I have not proved it — the flush boundary is the obvious candidate rather than a confirmed cause, and it would be worth confirming before anyone designs around it.
Scope
BEFORE INSERT ... FOR EACH ROWis unaffected: it runs before the row is written and never re-fetches.-O2assert build, at05f8b35.Why it matters now
#168's fix routes the import path through the executor's index maintenance, which is also what queues after-row trigger events. So a table with both an index and a user after-row trigger now fails the import where it previously succeeded and skipped the trigger.
That is a narrow case and the table is already unusable for ordinary
INSERT, so I do not think it should block #168 — a loud failure that matches whatINSERTalready does seems better than the import quietly not running a user's trigger. But it is a real behaviour change caused by this defect rather than by that fix, which is why it is filed separately.Before, on
main:After #168's fix:
If you would rather have no regression at all while this is open, gating the
ExecARInsertTriggerscall on a non-empty recheck list confines the change to constraints and leaves user triggers exactly as unfired as they are today. It is one condition, and I am happy to add it — say which you prefer.