Thread

Commits

Fix another issue with ENABLE/DISABLE TRIGGER on partitioned tables.
- 4766eef3174b 16.0 landed
- b02bf1e46048 13.11 landed
- 2624de79efe8 15.3 landed
- 0a6aaf01166c 14.8 landed
Fix ENABLE/DISABLE TRIGGER to handle recursion correctly
- ec0925c22a3d 16.0 cited
Allow FOR EACH ROW triggers on partitioned tables
- 86f575948c77 11.0 cited

BUG #17886: Error disabling user triggers on a partitioned table

The Post Office <noreply@postgresql.org> — 2023-04-04T16:47:33Z

The following bug has been logged on the website:

Bug reference:      17886
Logged by:          DzmitryH
Email address:      jazz001319@gmail.com
PostgreSQL version: 14.7
Operating system:   Linux 883a37b156f7 5.15.0-52-generic #58-Ubuntu SM
Description:        

sequence of steps:
1. clean install PostgreSQL 14.7
2. Create test database
3. Create partiton table and partitions (for example only default
partiton)
4. Create trigger 
5. Disable user trigger on partitions

verbose step:
psql (14.7 (Debian 14.7-1.pgdg110+1))
Type "help" for help.

postgres=# \set VERBOSITY verbose
postgres=# create database testdb;
CREATE DATABASE
postgres=# CREATE TABLE IF NOT EXISTS public.test
(
    id bigserial,
    user_id bigint,
    type text NOT NULL,
    status text NOT NULL,
    details jsonb,
    created_timestamp timestamp with time zone NOT NULL DEFAULT 'now()',
        modified_timestamp timestamp with time zone NOT NULL DEFAULT
'now()',
    shard_id integer NOT NULL DEFAULT '1',
    demo boolean NOT NULL,
    CONSTRAINT test_pkey PRIMARY KEY (id, created_timestamp)
) PARTITION BY RANGE (created_timestamp);
CREATE TABLE
postgres=# CREATE TABLE public.test_def PARTITION OF public.test
    DEFAULT;
CREATE TABLE
postgres=# CREATE OR REPLACE FUNCTION
public.update_last_modified_timestamp()
    RETURNS trigger
    LANGUAGE 'plpgsql'
    COST 100
    VOLATILE NOT LEAKPROOF
AS $BODY$
BEGIN
  IF NEW != OLD
  THEN
    NEW.modified_timestamp := CURRENT_TIMESTAMP;
  END IF;
  RETURN NEW;
END;
$BODY$;
CREATE FUNCTION
postgres=# CREATE TRIGGER trigger_test
    BEFORE UPDATE
    ON public.test
    FOR EACH ROW
    EXECUTE FUNCTION public.update_last_modified_timestamp();
CREATE TRIGGER
postgres=# alter table public.test DISABLE TRIGGER USER;
ERROR:  42704: trigger "trigger_test" for table "test_def" does not exist
LOCATION:  EnableDisableTriggerNew, trigger.c:1658

result:
postgres=# alter table public.test DISABLE TRIGGER USER;
ERROR:  42704: trigger "trigger_test" for table "test_def" does not exist
LOCATION:  EnableDisableTriggerNew, trigger.c:1658

Expected Result (Postgresql 15.2 and 14.4 - fine):
testdb=> alter table public.test DISABLE TRIGGER USER;
ALTER TABLE

Re: BUG #17886: Error disabling user triggers on a partitioned table

jazzl 0013 <jazz001319@gmail.com> — 2023-04-04T16:49:20Z

The following bug has been logged on the website:
>
> Bug reference:      17886
> Logged by:          DzmitryH
> Email address:      jazz001319@gmail.com
> PostgreSQL version: 14.7
> Operating system:   Linux 883a37b156f7 5.15.0-52-generic #58-Ubuntu SM
> Description:
>
> sequence of steps:
> 1. clean install PostgreSQL 14.7
> 2. Create test database
> 3. Create partiton table and partitions (for example only default
> partiton)
> 4. Create trigger
> 5. Disable user trigger on partitions
>
> verbose step:
> psql (14.7 (Debian 14.7-1.pgdg110+1))
> Type "help" for help.
>
> postgres=# \set VERBOSITY verbose
> postgres=# create database testdb;
> CREATE DATABASE
> postgres=# CREATE TABLE IF NOT EXISTS public.test
> (
>     id bigserial,
>     user_id bigint,
>     type text NOT NULL,
>     status text NOT NULL,
>     details jsonb,
>     created_timestamp timestamp with time zone NOT NULL DEFAULT 'now()',
>         modified_timestamp timestamp with time zone NOT NULL DEFAULT
> 'now()',
>     shard_id integer NOT NULL DEFAULT '1',
>     demo boolean NOT NULL,
>     CONSTRAINT test_pkey PRIMARY KEY (id, created_timestamp)
> ) PARTITION BY RANGE (created_timestamp);
> CREATE TABLE
> postgres=# CREATE TABLE public.test_def PARTITION OF public.test
>     DEFAULT;
> CREATE TABLE
> postgres=# CREATE OR REPLACE FUNCTION
> public.update_last_modified_timestamp()
>     RETURNS trigger
>     LANGUAGE 'plpgsql'
>     COST 100
>     VOLATILE NOT LEAKPROOF
> AS $BODY$
> BEGIN
>   IF NEW != OLD
>   THEN
>     NEW.modified_timestamp := CURRENT_TIMESTAMP;
>   END IF;
>   RETURN NEW;
> END;
> $BODY$;
> CREATE FUNCTION
> postgres=# CREATE TRIGGER trigger_test
>     BEFORE UPDATE
>     ON public.test
>     FOR EACH ROW
>     EXECUTE FUNCTION public.update_last_modified_timestamp();
> CREATE TRIGGER
> postgres=# alter table public.test DISABLE TRIGGER USER;
> ERROR:  42704: trigger "trigger_test" for table "test_def" does not exist
> LOCATION:  EnableDisableTriggerNew, trigger.c:1658
>
> result:
> postgres=# alter table public.test DISABLE TRIGGER USER;
> ERROR:  42704: trigger "trigger_test" for table "test_def" does not exist
> LOCATION:  EnableDisableTriggerNew, trigger.c:1658
>
> Expected Result (Postgresql 15.2 and 14.4 - fine):
> testdb=> alter table public.test DISABLE TRIGGER USER;
> ALTER TABLE
>
>

Re: BUG #17886: Error disabling user triggers on a partitioned table

Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-04T18:24:32Z

PG Bug reporting form <noreply@postgresql.org> writes:
> result:
> postgres=# alter table public.test DISABLE TRIGGER USER;
> ERROR:  42704: trigger "trigger_test" for table "test_def" does not exist
> LOCATION:  EnableDisableTriggerNew, trigger.c:1658

> Expected Result (Postgresql 15.2 and 14.4 - fine):
> testdb=> alter table public.test DISABLE TRIGGER USER;
> ALTER TABLE

Commit ec0925c22 seems to have been quite snakebit.  I already fixed
a deficiency in it in v15/HEAD, but here we have a different symptom
in the older branches.  What's happening is that EnableDisableTrigger
is ignoring the child trigger because it has tgisinternal set to true
and the command passes skip_system = true.

I'm inclined to think that in the older branches (pre f4566345c)
we need to do

-       if (oldtrig->tgisinternal)
+       if (oldtrig->tgisinternal && !OidIsValid(oldtrig->tgparentid))
        {
            /* system trigger ... ok to process? */

but I've not tested that.  Alvaro, what do you think?

			regards, tom lane

Re: BUG #17886: Error disabling user triggers on a partitioned table

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-04-05T07:33:56Z

On 2023-Apr-04, Tom Lane wrote:

> PG Bug reporting form <noreply@postgresql.org> writes:
> > result:
> > postgres=# alter table public.test DISABLE TRIGGER USER;
> > ERROR:  42704: trigger "trigger_test" for table "test_def" does not exist
> > LOCATION:  EnableDisableTriggerNew, trigger.c:1658
> 
> > Expected Result (Postgresql 15.2 and 14.4 - fine):
> > testdb=> alter table public.test DISABLE TRIGGER USER;
> > ALTER TABLE
> 
> Commit ec0925c22 seems to have been quite snakebit.

No kidding :-(  Clearly, commit 86f575948c77 ("Allow FOR EACH ROW
triggers on partitioned tables") should have included more tests.

> I already fixed
> a deficiency in it in v15/HEAD, but here we have a different symptom
> in the older branches.  What's happening is that EnableDisableTrigger
> is ignoring the child trigger because it has tgisinternal set to true
> and the command passes skip_system = true.

Hmm, right.

> I'm inclined to think that in the older branches (pre f4566345c)
> we need to do
> 
> -       if (oldtrig->tgisinternal)
> +       if (oldtrig->tgisinternal && !OidIsValid(oldtrig->tgparentid))
>         {
>             /* system trigger ... ok to process? */
> 
> but I've not tested that.  Alvaro, what do you think?

Oh, that's a nice and clean solution.  I tested it (on 13 and 14) and it
does solve the problem, and no regression tests fail, but I didn't try
to break it further.  12 and back are unaffected, for lack of
86f575948c77.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Nadie está tan esclavizado como el que se cree libre no siéndolo" (Goethe)

Re: BUG #17886: Error disabling user triggers on a partitioned table

Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-05T16:57:29Z

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Apr-04, Tom Lane wrote:
>> I'm inclined to think that in the older branches (pre f4566345c)
>> we need to do
>> -       if (oldtrig->tgisinternal)
>> +       if (oldtrig->tgisinternal && !OidIsValid(oldtrig->tgparentid))
>> but I've not tested that.  Alvaro, what do you think?

> Oh, that's a nice and clean solution.  I tested it (on 13 and 14) and it
> does solve the problem, and no regression tests fail, but I didn't try
> to break it further.  12 and back are unaffected, for lack of
> 86f575948c77.

Pushed, thanks for looking at it.

			regards, tom lane