Horton

Thread

Commits

  1. Fix oversights in pg_event_trigger_dropped_objects() fixes.

  2. Report the correct is_temporary flag for column defaults.

  1. Error with DEFAULT VALUE in temp table

    Антуан Виолин <violin.antuan@gmail.com> — 2024-12-07T11:38:02Z

    Hi everyone,
I found one bug, when you delete temporary table with DEFAULT VALUE. The
row about this VALUE in the pg_event_trigger_dropped_objects() contains
“False” in the column “is_temporary”. But if you look at column “name_obj”,
you see “for pg_temp.table_name”. So PostgreSQL know, that it is temporary.

Cheers

Antoine Violin

  2. Re: Error with DEFAULT VALUE in temp table

    Антуан Виолин <violin.antuan@gmail.com> — 2025-06-30T07:36:09Z

    Hi everyone,
I made patch for this problem

Cheers

Antoine Violin

сб, 7 дек. 2024 г. в 18:38, Антуан Виолин <violin.antuan@gmail.com>:

> Hi everyone,
> I found one bug, when you delete temporary table with DEFAULT VALUE. The
> row about this VALUE in the pg_event_trigger_dropped_objects() contains
> “False” in the column “is_temporary”. But if you look at column “name_obj”,
> you see “for pg_temp.table_name”. So PostgreSQL know, that it is temporary.
>
> Cheers
>
> Antoine Violin
>

  3. Re: Error with DEFAULT VALUE in temp table

    Антуан Виолин <violin.antuan@gmail.com> — 2025-06-30T07:46:27Z

    > Hi everyone,
> I found one bug, when you delete temporary table with DEFAULT VALUE. The
> row about this VALUE in the pg_event_trigger_dropped_objects() contains
> “False” in the column “is_temporary”. But if you look at column “name_obj”,
> you see “for pg_temp.table_name”. So PostgreSQL know, that it is temporary.
>
> Cheers
>
> Antoine Violin\
>
Hi everyone,
I made patch for this problem, I changed event_trigger, for
definitions of temporality
DEFAULT VALUE

Cheers

Antoine Violin

  4. Re: Error with DEFAULT VALUE in temp table

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-23T16:14:54Z

    =?UTF-8?B?0JDQvdGC0YPQsNC9INCS0LjQvtC70LjQvQ==?= <violin.antuan@gmail.com> writes:
> I made patch for this problem, I changed event_trigger, for
> definitions of temporality
> DEFAULT VALUE

I looked over this patch.  I understand what you want to fix,
and I agree with the general plan of pushing the namespace-lookup
code into a subroutine so we can more easily point it at a different
object for the attrdef case.  However, you've done no favors for
future readers of the code:

* "process_catalog_object" is about as generic and uninformative a
name as one could easily think of.  I'd suggest something more like
"identify_object_namespace" --- I'm not wedded to that exact choice,
but the name should indicate what the function is trying to do.

* The new function also lacks a header comment, which isn't OK
except maybe for extremely trivial functions with obvious APIs.
Here I think you need to explain what values it outputs, and
you certainly need to explain what the return value means.

* The comment block at lines 1297ff is now kind of dangling,
because you moved half of what it's talking about to somewhere else.
Perhaps some of that text belongs in the new function's header
comment.

* Zero comments in the new code block for "object->classId ==
AttrDefaultRelationId" are not OK either.  I'd expect to see
something like "We treat a column default as temp if its table
is temp".

* I wonder if the check for is_objectclass_supported shouldn't
move into the new function too.  It's not really a concern
of the outer function where the new function is getting its
information from.

* If I'm reading it correctly, the patch depends on the
assumption that attrdefs aren't supported by the
is_objectclass_supported() infrastructure.  I'm not sure
that's right even today, and it sure seems like something
that could get broken by well-intentioned future patches.


Something that isn't the fault of your patch, but could be
improved while we're here:

* It seems rather messy and poorly-thought-out that schemas
themselves are handled in two separate places in the function,
at lines 1283ff and 1367ff.  Seems like that could be unified
and also made to look more like the equivalent code for
objects-contained-in-schemas.


Taking my last three comments together, maybe what we want for
the overall structure in EventTriggerSQLDropAddObject is

    if (object->classId == NamespaceRelationId)
    {
        code for the schema case;
    }
    else if (object->classId == AttrDefaultRelationId)
    {
        code for the attrdef case;
    }
    else
    {
        generic case;
    }

where the second and third blocks use this new function.

			regards, tom lane

  5. Re: Error with DEFAULT VALUE in temp table

    Sergey Shinderuk <s.shinderuk@postgrespro.ru> — 2025-09-12T11:01:42Z

    Hi, Tom!

Thank you for working on this. I see you've fixed the patch and 
committed it as a0b99fc1220. I tested it a bit and see some side effects 
which may be unintentional.

1. SCHEMA lost object_name.

Before:

     postgres=# create schema foo;
     CREATE SCHEMA
     postgres=# drop schema foo;
     DROP SCHEMA
     postgres=# select * from dropped_objects \gx
     -[ RECORD 1 ]---+-------
     n               | 1
     classid         | 2615
     objid           | 16404
     objsubid        | 0
     original        | t
     normal          | f
     is_temporary    | f
     object_type     | schema
     schema_name     |
     object_name     | foo
     object_identity | foo
     address_names   | {foo}
     address_args    | {}

After:

     postgres=# select * from dropped_objects \gx
     -[ RECORD 1 ]---+-------
     n               | 1
     classid         | 2615
     objid           | 16394
     objsubid        | 0
     original        | t
     normal          | f
     is_temporary    | f
     object_type     | schema
     schema_name     |
     object_name     |
     object_identity | foo
     address_names   | {foo}
     address_args    | {}


2. DEFAULT VALUE now has schema_name and object_name.

Before:

     postgres=# create temp table bar (a int default 0);
     CREATE TABLE
     postgres=# drop table bar;
     DROP TABLE
     postgres=# select * from dropped_objects where object_type = 
'default value' \gx
     -[ RECORD 1 ]---+------------------
     n               | 4
     classid         | 2604
     objid           | 16422
     objsubid        | 0
     original        | f
     normal          | f
     is_temporary    | f
     object_type     | default value
     schema_name     |
     object_name     |
     object_identity | for pg_temp.bar.a
     address_names   | {pg_temp,bar,a}
     address_args    | {}

After:

     postgres=# select * from dropped_objects where object_type = 
'default value' \gx
     -[ RECORD 1 ]---+------------------
     n               | 4
     classid         | 2604
     objid           | 16430
     objsubid        | 0
     original        | f
     normal          | f
     is_temporary    | t
     object_type     | default value
     schema_name     | pg_temp
     object_name     | bar
     object_identity | for pg_temp.bar.a
     address_names   | {pg_temp,bar,a}
     address_args    | {}

This may be intentional, but doesn't quite match the description for 
object_name in the docs:

     Name of the object, if the combination of schema and name can be 
used as a unique identifier for the object; otherwise NULL.

Also it doesn't match with the record for the column itself:

     postgres=# create temp table bar (a int default 0);
     CREATE TABLE
     postgres=# alter table bar drop column a;
     ALTER TABLE
     postgres=# select * from dropped_objects \gx
     -[ RECORD 1 ]---+------------------
     n               | 1
     classid         | 1259
     objid           | 16435
     objsubid        | 1
     original        | t
     normal          | f
     is_temporary    | t
     object_type     | table column
     schema_name     | pg_temp
     object_name     |
     object_identity | pg_temp.bar.a
     address_names   | {pg_temp,bar,a}
     address_args    | {}
     -[ RECORD 2 ]---+------------------
     n               | 2
     classid         | 2604
     objid           | 16438
     objsubid        | 0
     original        | f
     normal          | f
     is_temporary    | t
     object_type     | default value
     schema_name     | pg_temp
     object_name     | bar
     object_identity | for pg_temp.bar.a
     address_names   | {pg_temp,bar,a}
     address_args    | {}

object_name is null for the table column, but not null for its default 
value.

As for schema_name, I'm not sure whether it should be null or not. 
Currently schema_name is null for triggers and policy objects, but that 
may be accidental.

Best regards,

-- 
Sergey Shinderuk		https://postgrespro.com/

  6. Re: Error with DEFAULT VALUE in temp table

    Sergey Shinderuk <s.shinderuk@postgrespro.ru> — 2025-09-12T12:43:09Z

    On 12.09.2025 14:01, Sergey Shinderuk wrote:
> object_name is null for the table column, but not null for its default 
> value.
> 
> As for schema_name, I'm not sure whether it should be null or not. 
> Currently schema_name is null for triggers and policy objects, but that 
> may be accidental.


Perhaps "default value" should be like "table constraint", which have 
schema_name and null object_name.


     postgres=# create temp table bar (a int not null default 0);
     CREATE TABLE
     postgres=# alter table bar drop column a;
     ALTER TABLE
     postgres=# select * from dropped_objects \gx
     -[ RECORD 1 ]---+------------------------------
     n               | 1
     classid         | 1259
     objid           | 16445
     objsubid        | 1
     original        | t
     normal          | f
     is_temporary    | t
     object_type     | table column
     schema_name     | pg_temp
     object_name     |
     object_identity | pg_temp.bar.a
     address_names   | {pg_temp,bar,a}
     address_args    | {}
     -[ RECORD 2 ]---+------------------------------
     n               | 2
     classid         | 2604
     objid           | 16448
     objsubid        | 0
     original        | f
     normal          | f
     is_temporary    | t
     object_type     | default value
     schema_name     | pg_temp
     object_name     | bar
     object_identity | for pg_temp.bar.a
     address_names   | {pg_temp,bar,a}
     address_args    | {}
     -[ RECORD 3 ]---+------------------------------
     n               | 3
     classid         | 2606
     objid           | 16449
     objsubid        | 0
     original        | f
     normal          | f
     is_temporary    | t
     object_type     | table constraint
     schema_name     | pg_temp
     object_name     |
     object_identity | bar_a_not_null on pg_temp.bar
     address_names   | {pg_temp,bar,bar_a_not_null}
     address_args    | {}


Best regards,

-- 
Sergey Shinderuk		https://postgrespro.com/

  7. Re: Error with DEFAULT VALUE in temp table

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-12T21:19:28Z

    Sergey Shinderuk <s.shinderuk@postgrespro.ru> writes:
> Thank you for working on this. I see you've fixed the patch and 
> committed it as a0b99fc1220. I tested it a bit and see some side effects 
> which may be unintentional.

Many thanks for double-checking!

> 1. SCHEMA lost object_name.

Ugh.  I was misled first by assuming that EventTriggerSQLDropAddObject
didn't have *other* pre-existing bugs, and second by overestimating
the test coverage for this function.  In particular I thought that
this coding pattern:

    if (is_objectclass_supported(object->classId))
    {
        ...
    }
    else
    {
        if (object->classId == NamespaceRelationId &&
            isTempNamespace(object->objectId))
            obj->istemp = true;
    }

meant that is_objectclass_supported() doesn't return true for
NamespaceRelationId --- a conclusion I should have realized was silly,
I guess.  So that "else" action was unreachable, and the code failed
to set "istemp" true for its own temp schema.  But I took it on faith
and supposed that we weren't filling objname for schemas.

I would have spotted the behavior change if event_trigger.sql
made any attempt to verify more than a few of the outputs of
pg_event_trigger_dropped_objects(), but it didn't.  So the attached
patch fixes that test script to print all the expected-to-be-stable
outputs.

> 2. DEFAULT VALUE now has schema_name and object_name.

Setting schema_name is expected I think: you can hardly opine that
an object is temp unless it's associated with a temp schema.
You're right that setting object_name to the table name is the
wrong thing, and again I blame that on poor test coverage.

> Currently schema_name is null for triggers and policy objects, but that 
> may be accidental.

Double ugh.  Triggers and policy objects have this exact same bug.
Fixed (and tested) in the attached.

I'm tempted to wonder if the objectaddress.c ObjectProperty
infrastructure should grow some support for cases like these,
but right now I think it'd be about a wash in terms of the
amount of code added.

			regards, tom lane

  8. Re: Error with DEFAULT VALUE in temp table

    Sergey Shinderuk <s.shinderuk@postgrespro.ru> — 2025-09-13T06:08:21Z

    On 13.09.2025 00:19, Tom Lane wrote:
> Fixed (and tested) in the attached.

Great! Thank you.


 > So that "else" action was unreachable, and the code failed
 > to set "istemp" true for its own temp schema.

As for dropping my own temp schema, it's still a bit inconsistent (as it 
was before):

     postgres=# select pg_my_temp_schema()::regnamespace;
      pg_my_temp_schema
     -------------------
      pg_temp_0
     (1 row)
     postgres=# drop schema pg_temp_0;
     DROP SCHEMA

     postgres=# select * from dropped_objects where object_type = 
'schema' and is_temporary \gx
     -[ RECORD 1 ]---+----------
     n               | 7
     classid         | 2615
     objid           | 16398
     objsubid        | 0
     original        | t
     normal          | f
     is_temporary    | t
     object_type     | schema
     schema_name     |
     object_name     | pg_temp_0
     object_identity | pg_temp
     address_names   | {pg_temp}
     address_args    | {}

object_identity is pg_temp, but object_name is pg_temp_0. But maybe 
that's okay. Anyway, I don't think that dropping my own temp schema 
makes sense.


Also I noticed that schema_name for temp functions doesn't match with 
object_identity (pg_temp vs pg_temp_1):

     postgres=# create function pg_temp.bar(int) returns int as 'select 
$1' language sql;
     CREATE FUNCTION
     postgres=# drop function pg_temp.bar(int);
     DROP FUNCTION

     postgres=# select * from dropped_objects where object_type = 
'function' and is_temporary \gx
     -[ RECORD 1 ]---+-----------------------
     n               | 8
     classid         | 1255
     objid           | 16412
     objsubid        | 0
     original        | t
     normal          | f
     is_temporary    | t
     object_type     | function
     schema_name     | pg_temp
     object_name     |
     object_identity | pg_temp_1.bar(integer)
     address_names   | {pg_temp,bar}
     address_args    | {integer}

There should be a call to get_namespace_name_or_temp somewhere, I guess.

If you say this should be fixed, I can come up with a patch later. But 
maybe it's trivial.


Thanks again!

-- 
Sergey Shinderuk		https://postgrespro.com/