Horton

Thread

Commits

  1. Don't record FDW user mappings as members of extensions.

  2. Fix pg_upgrade to handle event triggers in extensions correctly.

  1. BUG #15310: pg_upgrade dissociates event triggers from extensions

    The Post Office <noreply@postgresql.org> — 2018-08-07T00:13:58Z

    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

  2. Re: BUG #15310: pg_upgrade dissociates event triggers from extensions

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-08-07T09:40:24Z

    On Tue, Aug 7, 2018 at 10:14 AM PG Bug reporting form <
noreply@postgresql.org> wrote:

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


Yes, I feel it is a bug. During the dump of the event trigger during
upgrade, it lost
the dependency on extension.


> 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.
>

Yes, the above command can solve your problem.

While checking this issue, why it is missed to dump the extension
dependency,
I observed that there is call to the
function binary_upgrade_extension_member()
to add extension dependency for particular types in the pg_dump.

DO_EVENT_TRIGGER

The above type as per the bug is losing the dependency, by adding a
function call in dumpEventTrigger() function the dependency issue is
resolved.
Patch attached.

DO_SHELL_TYPE

I am not able to generate a scenario for the above type where it can loss
the
extension dependency.

The other types that don't create extension dependency are as follows,

DO_INDEX, DO_STATSEXT, DO_RULE, DO_TRIGGER,
DO_POLICY, DO_PUBLICATION, DO_SUBSCRIPTION

As per the above, a trigger is not depends on an extension, but an event
trigger
does. Do we need to support the same for trigger also?

Regards,
Haribabu Kommi
Fujitsu Australia

  3. Re: BUG #15310: pg_upgrade dissociates event triggers from extensions

    Michael Paquier <michael@paquier.xyz> — 2018-08-07T11:29:10Z

    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

  4. Re: BUG #15310: pg_upgrade dissociates event triggers from extensions

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-07T18:40:13Z

    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

  5. Re: BUG #15310: pg_upgrade dissociates event triggers from extensions

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-07T20:38:03Z

    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