Re: FOR PORTION OF code review

Hi,

On 2026-09-10 10:07:02 -0400, Andres Freund wrote:
> Ran out of energy & time at this point. There's plenty more to look at.

Haven't yet found time to do that, except one thing I was wondering about when
re-reading my email:

> - /*
> * The standard says that each temporal leftover should execute its
> * own INSERT statement, firing all statement and row triggers, but
> * skipping insert permission checks. Therefore we give each insert
> * its own transition table. If we just push & pop a new trigger level
> * for each insert, we get exactly what we need.
> *
> * We have to make sure that the inserts don't add to the ROW_COUNT
> * diagnostic or the command tag, so we pass false for canSetTag.
> */
> AfterTriggerBeginQuery();
> ExecSetupTransitionCaptureState(mtstate, estate);
> fireBSTriggers(mtstate);
> ExecInsert(context, resultRelInfo, leftoverSlot, false, NULL, NULL);
> fireASTriggers(mtstate);
> AfterTriggerEndQuery(estate);
>
>
> Not your fault, but this seems kinda terrible. This basically seems like
> it's making statement level triggers not really work as they're intended
> anymore :(.

ExecSetupTransitionCaptureState() is called once per leftover row, with an
update/delete affecting many rows, that can be a lot of times. Each time it
allocates memory in query context and then overwrites the existing
mtstate->mt_transition_capture (which is later retored, as I earlier
complained about). That's obviously a query-level memory leak?

> I'll also trigger some AI review.

I ran this with the following fixes added:

- Support concrete-typed range opclasses in FOR PORTION OF
- Fire leftover INSERT triggers on the table the leftovers go into
- Reject FOR PORTION OF on views with unqualified INSTEAD rules
- Fix memory leak in FOR PORTION OF domain lookup

This found some things:

- Crash due to wholerow references:

DROP TABLE IF EXISTS fpo_star CASCADE;
CREATE TABLE fpo_star (r int4range);
DELETE FROM fpo_star FOR PORTION OF r ((ROW(fpo_star.*)).f1);

table.* doesn't go through transformColumnRef() when called via
transformExpressionList(), and thus isn't prohibited. I suspect this may be
a wider issue and should be fixed by improving the general infrastructure,
even if it's not a problem today for other places, it seems likely to become
one in the future.

- PL/pgSQL variables and named parameters cannot be bounds

DROP TABLE IF EXISTS plv CASCADE;
DROP FUNCTION IF EXISTS plv_upd(int, int);
CREATE TABLE plv (r int4range, name text);
INSERT INTO plv VALUES ('[1,100)', 'x');
CREATE FUNCTION plv_upd(lo int, hi int) RETURNS void LANGUAGE plpgsql AS $$
BEGIN UPDATE plv FOR PORTION OF r FROM lo TO hi SET name = 'upd'; END $$;
SELECT plv_upd(10, 20);

ERROR: 0A000: cannot use column reference in FOR PORTION OF expression

There are no column references here though...

This, I guess, again might be a more general problem, I haven't looked into
it.

- `pg_get_functiondef()` output does not replay in edge case

Related to the prior one:
DROP FUNCTION IF EXISTS fpo_named_delete(int, int);
DROP TABLE IF EXISTS fpo_named CASCADE;
CREATE TABLE fpo_named (r int4range);
CREATE FUNCTION fpo_named_delete(lo integer, hi integer) RETURNS void LANGUAGE SQL
BEGIN ATOMIC
DELETE FROM fpo_named FOR PORTION OF r FROM $1 TO $2;
END;
SELECT pg_get_functiondef('fpo_named_delete(int,int)'::regprocedure) \gexec

ERROR: 0A000: cannot use column reference in FOR PORTION OF expression
LINE 5: DELETE FROM fpo_named FOR PORTION OF r FROM fpo_named_delet...

- Query level memory leaks

There's at least two:

- the ExecSetupTransitionCaptureState() leak described above

The fix here is to avoid allocating the capture state over and over or at
least to free it.

- With a BEFORE INSERT row trigger returning NEW unmodified,
ExecBRInsertTriggers() copies the tuple out of a virtual slot with
ExecFetchSlotHeapTuple( &should_free) and frees it only when the trigger returns
NULL or a different tuple. plpgsql returns tg_trigtuple itself, so the
copy leaks.

I think this might be a problem in some corner cases before, but is more
easily reached with FPO.

The fix here is to free the tuple in ExecBRInsertTriggers() if it's
allocated.

- DO ALSO rules aren't rejected

They can cause very similar issues to DO INSTEAD.

- DO ALSO doesn't deparse correctly:

DROP TABLE IF EXISTS ivl_t CASCADE;
DROP TYPE IF EXISTS intervalrange CASCADE;
CREATE TYPE intervalrange AS RANGE (subtype = interval);
CREATE TABLE ivl_t (r intervalrange);
CREATE RULE ivl_r AS ON INSERT TO ivl_t DO ALSO
DELETE FROM ivl_t FOR PORTION OF r FROM (INTERVAL '1' HOUR) TO INTERVAL '2' HOUR;
SELECT 'DROP RULE ivl_r ON ivl_t' UNION ALL SELECT pg_get_ruledef(oid) FROM pg_rewrite WHERE rulename = 'ivl_r' \gexec

ERROR: syntax error at or near "'02:00:00'"

- AFTER triggers for leftover tuples fire while outer statement is still
running

Normally AFTER triggers should be able to modify rows. But with the current
nesting of when leftover rows fire triggers that is problematic:

DROP TABLE IF EXISTS armod CASCADE;
DROP FUNCTION IF EXISTS armod_trg() CASCADE;
CREATE TABLE armod (id int, valid_at daterange, name text);
INSERT INTO armod VALUES (1, '[2020-01-01,2021-01-01)', 'a'), (2, '[2020-01-01,2021-01-01)', 'b'), (3, '[2020-01-01,2021-01-01)', 'c');
CREATE FUNCTION armod_trg() RETURNS trigger LANGUAGE plpgsql AS $$
BEGIN
IF pg_trigger_depth() = 1 THEN UPDATE armod SET name = name || '!' WHERE id <> NEW.id; END IF;
RETURN NULL;
END $$;
CREATE TRIGGER armod_ai AFTER INSERT ON armod FOR EACH ROW EXECUTE FUNCTION armod_trg();
UPDATE armod FOR PORTION OF valid_at FROM '2020-03-01' TO '2020-06-01' SET name = name || '*';

ERROR: tuple to be updated was already modified by an operation triggered by the current command
HINT: Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.

Note this is suggesting the use of an AFTER trigger despite already using
one.

Several of these seem more like general infrastructure faults than this
patch's...

Greetings,

Andres Freund