Обсуждение: [PATCH] Fix trigger argument propagation to child partitions

The following patch fixes propagation of arguments to the trigger
function to child partitions both when initially creating the trigger
and when adding new partitions to a partitioned table.

The included regression test should demonstrate the problem, for clarity
repeated in slightly more readable form here:

bb=> create table parted_trig (a int) partition by list (a);
CREATE TABLE
bb=> create table parted_trig1 partition of parted_trig for values in (1);
CREATE TABLE
bb=> create or replace function trigger_notice() returns trigger as $$
bb$>   declare
bb$>     arg1 text = TG_ARGV[0];
bb$>     arg2 integer = TG_ARGV[1];
bb$>   begin
bb$>     raise notice 'trigger % on % % % for % args % %', TG_NAME, TG_TABLE_NAME, TG_WHEN, TG_OP, TG_LEVEL, arg1,
arg2;
bb$>     return null;
bb$>   end;
bb$>   $$ language plpgsql;
CREATE FUNCTION
bb=> create trigger aaa after insert on parted_trig for each row execute procedure trigger_notice('text', 1);
CREATE TRIGGER
bb=> \d parted_trig
                 Tabelle »public.parted_trig«
 Spalte |   Typ   | Sortierfolge | NULL erlaubt? | Vorgabewert 
--------+---------+--------------+---------------+-------------
 a      | integer |              |               | 
Partitionsschlüssel: LIST (a)
Trigger:
    aaa AFTER INSERT ON parted_trig FOR EACH ROW EXECUTE PROCEDURE trigger_notice('text', '1')
Anzahl Partitionen: 1 (Mit \d+ alle anzeigen.)

bb=> \d parted_trig1
                 Tabelle »public.parted_trig1«
 Spalte |   Typ   | Sortierfolge | NULL erlaubt? | Vorgabewert 
--------+---------+--------------+---------------+-------------
 a      | integer |              |               | 
Partition von: parted_trig FOR VALUES IN (1)
Trigger:
    aaa AFTER INSERT ON parted_trig1 FOR EACH ROW EXECUTE PROCEDURE trigger_notice()

Fixed:

bb=> \d parted_trig1
                 Tabelle »public.parted_trig1«
 Spalte |   Typ   | Sortierfolge | NULL erlaubt? | Vorgabewert 
--------+---------+--------------+---------------+-------------
 a      | integer |              |               | 
Partition von: parted_trig FOR VALUES IN (1)
Trigger:
    aaa AFTER INSERT ON parted_trig1 FOR EACH ROW EXECUTE PROCEDURE trigger_notice('text', '1')

Patch is against 11.4, but applies to master with minor offset.

All regression test pass.

On Tue, Jul 09, 2019 at 03:00:27PM +0200, Patrick McHardy wrote:
>The following patch fixes propagation of arguments to the trigger
>function to child partitions both when initially creating the trigger
>and when adding new partitions to a partitioned table.
>

Thanks for the report and bugfix. It seeem the parameters in row triggers
on partitioned tables never worked :-( For a moment I was wondering why it
shows on 11 and not 10 (based on the assumption you'd send a patch against
10 if it was affected), but 10 actually did not support row triggers on
partitioned tables.

The fix seems OK to me, although I see we're parsing tgargs in ruleutils.c
and that version (around line ~1050) uses fastgetattr instead of
heap_getattr, and checks the isnull parameter after the call. I guess we
should do the same thing here.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Jul-09, Tomas Vondra wrote:

> On Tue, Jul 09, 2019 at 03:00:27PM +0200, Patrick McHardy wrote:
> > The following patch fixes propagation of arguments to the trigger
> > function to child partitions both when initially creating the trigger
> > and when adding new partitions to a partitioned table.
> 
> Thanks for the report and bugfix. It seeem the parameters in row triggers
> on partitioned tables never worked :-( For a moment I was wondering why it
> shows on 11 and not 10 (based on the assumption you'd send a patch against
> 10 if it was affected), but 10 actually did not support row triggers on
> partitioned tables.

Right ...

> The fix seems OK to me, although I see we're parsing tgargs in ruleutils.c
> and that version (around line ~1050) uses fastgetattr instead of
> heap_getattr, and checks the isnull parameter after the call. I guess we
> should do the same thing here.

Yeah, absolutely.  The attached v2 is basically Patrick's patch with
very minor style changes.  I'll get this pushed as soon as the tests
finish running.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services