Обсуждение: BUG #15310: pg_upgrade dissociates event triggers from extensions

The following bug has been logged on the website:

Bug reference:      15310
Logged by:          Nick Barnes
Email address:      nickbarnes01@gmail.com
PostgreSQL version: 10.4
Operating system:   CentOS 7
Description:

Hi,

I have an extension which contains an event trigger. As expected, CREATE
EXTENSION adds a pg_depend entry between the trigger and the extension. But
after running pg_upgrade, this pg_depend entry is gone (and the extension's
CREATE EVENT TRIGGER statement now shows up in the pg_dump output, causing
the restore to fail with an "event trigger already exists" error).

Reproduced for 9.5->9.6 and 9.6->10 upgrades with the attached script.

Did I miss something, or is this a bug? If so, is an ALTER EXTENSION ... ADD
EVENT TRIGGER command on the upgraded database enough to work around it? It
solves my issue, but I'm not sure if there are other symptoms.

Thanks,
Nick Barnes


export PGDATAOLD=~/9.5/data
export PGDATANEW=~/10/data
export PGBINOLD=/usr/pgsql-9.5/bin
export PGBINNEW=/usr/pgsql-10/bin

# Create extension files
cat >$PGBINOLD/../share/extension/event_trigger_test.control <<EOF
default_version = '1.0'
EOF
cat >$PGBINOLD/../share/extension/event_trigger_test--1.0.sql <<EOF
CREATE FUNCTION t() RETURNS event_trigger LANGUAGE plpgsql AS 'BEGIN END';
CREATE EVENT TRIGGER t ON ddl_command_end EXECUTE PROCEDURE t();
EOF
cp $PGBINOLD/../share/extension/event_trigger_test*
$PGBINNEW/../share/extension/

# Set up old server
$PGBINOLD/initdb -D $PGDATAOLD
$PGBINOLD/pg_ctl start -w -D $PGDATAOLD
$PGBINOLD/createdb test
$PGBINOLD/psql test -c "CREATE EXTENSION event_trigger_test"

# Upgrade
$PGBINOLD/pg_ctl stop -D $PGDATAOLD
$PGBINNEW/initdb -D $PGDATANEW
$PGBINNEW/pg_upgrade
$PGBINNEW/pg_ctl start -w -D $PGDATANEW

# Dump/restore
$PGBINNEW/createdb test2
$PGBINNEW/pg_dump test | $PGBINNEW/psql test2 # ERROR: event trigger "t"
already exists

On Tue, Aug 07, 2018 at 07:40:24PM +1000, Haribabu Kommi wrote:
> Yes, I feel it is a bug. During the dump of the event trigger during
> upgrade, it lost
> the dependency on extension.

That sounds like a bug to me.  Could you add more tests to
src/test/modules/test_pg_dump with an event trigger part of an extension
with a binary upgrade test?  It would be good to check as well
if the other object types you have mentioned correctly work or not.
--
Michael

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> On Tue, Aug 7, 2018 at 10:14 AM PG Bug reporting form <
> noreply@postgresql.org> wrote:
>> I have an extension which contains an event trigger. As expected, CREATE
>> EXTENSION adds a pg_depend entry between the trigger and the extension. But
>> after running pg_upgrade, this pg_depend entry is gone (and the extension's
>> CREATE EVENT TRIGGER statement now shows up in the pg_dump output, causing
>> the restore to fail with an "event trigger already exists" error).
>> Did I miss something, or is this a bug?

> Yes, I feel it is a bug.

Undoubtedly.  I haven't checked your fix in detail yet but it looks
plausible.

I poked around to see if there were any other missing moving parts in
extension membership support.  Mostly we seem to be OK ... but I found out
that CreateUserMapping contains a recordDependencyOnCurrentExtension
call, even though there is no support in the grammar for
ALTER EXTENSION ADD/DROP USER MAPPING, nor does pg_dump check for
extension membership when dumping a user mapping.  (So if someone did
do a CREATE USER MAPPING in an extension, the membership would be recorded
but then silently lost during pg_upgrade, just as with this bug.)

I'm inclined to think that not supporting that is the correct thing:
if we don't allow roles to be extension members, then user mappings
for roles shouldn't be either.  Unless someone can come up with a
convincing use-case for that, I think we should remove the
recordDependencyOnCurrentExtension call from CreateUserMapping.

> Do we need to support the same for trigger also?

No.  Whole tables can be extension members, not properties of tables.

            regards, tom lane

Michael Paquier <michael@paquier.xyz> writes:
> That sounds like a bug to me.  Could you add more tests to
> src/test/modules/test_pg_dump with an event trigger part of an extension
> with a binary upgrade test?

It doesn't seem especially useful to me to lock this particular barn
door behind the horse.  What would be great is a way of cross-checking
that the grammar, pg_dump, and the various callers of
recordDependencyOnCurrentExtension are all on the same page about
which objects can be extension members.  I see no way to automate that
unfortunately :-(

            regards, tom lane