Error with DEFAULT VALUE in temp table

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\

Hi everyone,
I made patch for this problem, I changed event_trigger, for
definitions of temporality
DEFAULT VALUE

Cheers

Antoine Violin

=?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

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/

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/

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

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/