pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feed[BUG v13] Crash with event trigger in extension
21+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2020-10-27 cc623ed2f4 93f726c04f 66c036b4f1 7978ad0254 41c742a432 Fix use-after-free bug with event triggers and ALTER TABLE.
2020-09-16 ced138e8cb 873cb8fca9 Fix use-after-free bug with event triggers in an extension script
* [BUG v13] Crash with event trigger in extension
@ 2020-09-02 17:37 Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
0 siblings, 1 reply; 21+ messages in thread
From: Jehan-Guillaume de Rorthais @ 2020-09-02 17:37 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hi,
Philippe Beaudoin reported to me a crash he was able to reproduce with some
extension update script. Please, find in attachment a script that triggers it.
Just edit the three first vars to point to a temporary installation and
instance.
Some git bisect shows the bug was introduced by b5810de3f4, where
multi-statement query strings are using different short memory contexts.
To make it short, any ALTER TABLE command in an extension script is
currently calling lappend to add its command to the
currentEventTriggerState->commandList list from its own short context instead
of using the context of currentEventTriggerState.
Because of this, when we call EventTriggerCollectSimpleCommand() for the top
level "ALTER EXTENSION" query, the list has long been removed and a new call of
lappend on currentEventTriggerState->commandList raise
Assert(IsPointerList(list)).
Please, find in attachement a bug fix proposal. I did a very quick look at
other ->commandList manipulations in event_trigger.c. They all appear to
be in the correct context in a first look.
Regards,
Attachments:
bug-ext.bash (application/octet-stream, 1.1K) download
v1-0001-Fix-crash-with-alter-table-event-triggers-in-extensi.patch (text/x-patch, 0B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-02 17:53 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
0 siblings, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2020-09-02 17:53 UTC (permalink / raw)
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; +Cc: pgsql-bugs@lists.postgresql.org
On 2020-Sep-02, Jehan-Guillaume de Rorthais wrote:
> Hi,
>
> Philippe Beaudoin reported to me a crash he was able to reproduce with some
> extension update script. Please, find in attachment a script that triggers it.
> Just edit the three first vars to point to a temporary installation and
> instance.
>
> Some git bisect shows the bug was introduced by b5810de3f4, where
> multi-statement query strings are using different short memory contexts.
Good find.
I think we should turn your case into a proper TAP test so that we can
incorporate it, for coverage -- the lines you patch do appear covered,
but obviously not in these specific conditions. I'm just not very sure
*where* to add it; maybe src/test/modules/test_extensions is the place
to have it, though that one doesn't have TAP tests currently. An easy
addition, though.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-03 17:41 Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 21+ messages in thread
From: Jehan-Guillaume de Rorthais @ 2020-09-03 17:41 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Wed, 2 Sep 2020 13:53:28 -0400
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2020-Sep-02, Jehan-Guillaume de Rorthais wrote:
>
> > Hi,
> >
> > Philippe Beaudoin reported to me a crash he was able to reproduce with some
> > extension update script. Please, find in attachment a script that triggers
> > it. Just edit the three first vars to point to a temporary installation and
> > instance.
> >
> > Some git bisect shows the bug was introduced by b5810de3f4, where
> > multi-statement query strings are using different short memory contexts.
>
> Good find.
>
> I think we should turn your case into a proper TAP test so that we can
> incorporate it, for coverage -- the lines you patch do appear covered,
> but obviously not in these specific conditions. I'm just not very sure
> *where* to add it; maybe src/test/modules/test_extensions is the place
> to have it, though that one doesn't have TAP tests currently. An easy
> addition, though.
Sure, please find in attachment patch v2 adding a regression test.
Regards,
Attachments:
v2-0001-Fix-crash-with-alter-table-event-triggers-in-extensi.patch (text/x-patch, 0B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-04 01:20 Michael Paquier <michael@paquier.xyz>
parent: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
0 siblings, 1 reply; 21+ messages in thread
From: Michael Paquier @ 2020-09-04 01:20 UTC (permalink / raw)
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-bugs@lists.postgresql.org
On Thu, Sep 03, 2020 at 07:41:14PM +0200, Jehan-Guillaume de Rorthais wrote:
> +$node->safe_psql('postgres',
> + q{CREATE EXTENSION test_event_trigger VERSION '1.0'});
> +$node->safe_psql('postgres',
> + q{ALTER EXTENSION test_event_trigger UPDATE TO '2.0'});
> +
> +my $ans = $node->safe_psql('postgres', q{
> + SELECT extversion
> + FROM pg_catalog.pg_extension
> + WHERE extname='test_event_trigger'
> +});
> +
> +is($ans, '2.0', 'Extension test_event_trigger upgraded');
I may be missing something of course, but if this just tests a
sequence of SQL commands, why don't you just include a SQL test in
text_extensions/sql/? That would be much cheaper than having to
create a full scenario with a TAP script.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-04 09:37 Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 21+ messages in thread
From: Jehan-Guillaume de Rorthais @ 2020-09-04 09:37 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-bugs@lists.postgresql.org
On Fri, 4 Sep 2020 10:20:13 +0900
Michael Paquier <michael@paquier.xyz> wrote:
> On Thu, Sep 03, 2020 at 07:41:14PM +0200, Jehan-Guillaume de Rorthais wrote:
> > +$node->safe_psql('postgres',
> > + q{CREATE EXTENSION test_event_trigger VERSION '1.0'});
> > +$node->safe_psql('postgres',
> > + q{ALTER EXTENSION test_event_trigger UPDATE TO '2.0'});
> > +
> > +my $ans = $node->safe_psql('postgres', q{
> > + SELECT extversion
> > + FROM pg_catalog.pg_extension
> > + WHERE extname='test_event_trigger'
> > +});
> > +
> > +is($ans, '2.0', 'Extension test_event_trigger upgraded');
>
> I may be missing something of course, but if this just tests a
> sequence of SQL commands, why don't you just include a SQL test in
> text_extensions/sql/? That would be much cheaper than having to
> create a full scenario with a TAP script.
Indeed, it's much more simple and cheaper. However, the result is a bit
confusing as it's not a wrong SQL result, but the backend that actually crash.
The crash message itself appears in results/test_extensions.out, but no other
tests are run. Neither from the same SQL file or from other ones. That Means
the diff is quite large, but the error appears close from the top.
I don't know if some other crash regression tests has been included that way in
the past. But if this is OK, I can append the test to sql/test_extensions.sql
and remove the TAP. Let me know what you think.
Regards,
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-04 13:27 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
0 siblings, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2020-09-04 13:27 UTC (permalink / raw)
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org
On 2020-Sep-04, Jehan-Guillaume de Rorthais wrote:
> Indeed, it's much more simple and cheaper. However, the result is a bit
> confusing as it's not a wrong SQL result, but the backend that actually crash.
> The crash message itself appears in results/test_extensions.out, but no other
> tests are run. Neither from the same SQL file or from other ones. That Means
> the diff is quite large, but the error appears close from the top.
Oh! If it crashes without the extension, then we don't *need* the
exension and surely we don't need the test to be in src/test/modules/;
my inclination would be to put it somewhere in
src/test/regress/sql/event_triggers.sql, maybe at the bottom, with a
comment that references this thread.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-04 14:56 Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 21+ messages in thread
From: Jehan-Guillaume de Rorthais @ 2020-09-04 14:56 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org
On Fri, 4 Sep 2020 09:27:15 -0400
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2020-Sep-04, Jehan-Guillaume de Rorthais wrote:
>
> > Indeed, it's much more simple and cheaper. However, the result is a bit
> > confusing as it's not a wrong SQL result, but the backend that actually
> > crash. The crash message itself appears in results/test_extensions.out, but
> > no other tests are run. Neither from the same SQL file or from other ones.
> > That Means the diff is quite large, but the error appears close from the
> > top.
>
> Oh! If it crashes without the extension, then we don't *need* the
> exension and surely we don't need the test to be in src/test/modules/;
Oh wait. It's not what I meant. We *need* an extension to trigger the
backend crash.
> my inclination would be to put it somewhere in
> src/test/regress/sql/event_triggers.sql, maybe at the bottom, with a
> comment that references this thread.
As the regression need an extension to be installed, it still makes sens to
test from src/test/modules/test_extension where various extensions are
installed to run the checks.
Please find in attachment patch v3.
Attachments:
v3-0001-Fix-crash-with-alter-table-event-triggers-in-extensi.patch (text/x-patch, 0B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-07 01:35 Michael Paquier <michael@paquier.xyz>
parent: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
0 siblings, 1 reply; 21+ messages in thread
From: Michael Paquier @ 2020-09-07 01:35 UTC (permalink / raw)
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-bugs@lists.postgresql.org
On Fri, Sep 04, 2020 at 04:56:58PM +0200, Jehan-Guillaume de Rorthais wrote:
> As the regression need an extension to be installed, it still makes sens to
> test from src/test/modules/test_extension where various extensions are
> installed to run the checks.
>
> Please find in attachment patch v3.
Yeah, the manipulation of the memory context makes the situation
tricky to adapt to, and putting a test in src/test/regress/ would
force us to add a special rule like what we do for contrib/spi/ in
GNUMakefile. So let's keep the test as cheap as we can, and the
structure you have here looks good enough to me.
> +-- This a regression test to make sure an extension upgrade does not crash when
> +-- an event trigger is triggered from the extension script.
> +-- See: https://postgr.es/m/20200902193715.6e0269d4%40firost
> +
> +CREATE EXTENSION test_event_trigger VERSION '1.0';
> +ALTER EXTENSION test_event_trigger UPDATE TO '2.0';
Here I would advise to use the correct message ID, aka
20200902193715.6e0269d4@firost. I cannot say for others, but
sometimes I do searches with the message ID directly in my inbox, and
this would not match. So it is better to keep a correct reference
without having to apply the extra math than %40 is @.
The two new SQL files to install and upgrade test_event_trigger are
missing a couple of things at their top, like:
/* src/test/modules/test_extensions/test_event_trigger--1.0.sql */
-- complain if script is sourced in psql, rather than via CREATE
EXTENSION
\echo Use "CREATE EXTENSION test_event_trigger" to load this file. \quit
> +SELECT extversion
> +FROM pg_catalog.pg_extension
> +WHERE extname='test_event_trigger';
> + extversion
> +------------
> + 2.0
> +(1 row)
What's the point of having this query? The previous command
succeeded, so we would be fine with the upgraded version.
More comments in each script would be helpful to document to the
reader why those objects are defined as such and their purpose in
life.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-08 05:27 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 21+ messages in thread
From: Michael Paquier @ 2020-09-08 05:27 UTC (permalink / raw)
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-bugs@lists.postgresql.org
On Mon, Sep 07, 2020 at 10:35:08AM +0900, Michael Paquier wrote:
> More comments in each script would be helpful to document to the
> reader why those objects are defined as such and their purpose in
> life.
I have looked at that, and tweaked a couple of things as per the
attached. Alvaro, Jehan-Guillaume, is that fine to you?
--
Michael
Attachments:
v4-0001-Fix-crash-with-alter-table-event-triggers-in-exte.patch (text/x-diff, 6.4K) download
From a6abdb2f2efa36a7c7457d92351227dece30f26e Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 8 Sep 2020 14:23:48 +0900
Subject: [PATCH v4] Fix crash with alter table event triggers in extension
ALTER commands in an extension script have been adding their commands to
the event trigger command list using their own memory context.
This is broken since b5810de3f4 where each statement uses a short-living
memory context, meaning that the command list could get freed before
getting called afterwards.
Reported-by: Philippe Beaudoin
Author: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
---
src/backend/commands/event_trigger.c | 5 +++++
src/test/modules/test_extensions/Makefile | 6 ++++--
.../test_extensions/expected/test_extensions.out | 6 ++++++
.../test_extensions/sql/test_extensions.sql | 7 +++++++
.../test_event_trigger--1.0--2.0.sql | 7 +++++++
.../test_extensions/test_event_trigger--1.0.sql | 16 ++++++++++++++++
.../test_extensions/test_event_trigger.control | 3 +++
7 files changed, 48 insertions(+), 2 deletions(-)
create mode 100644 src/test/modules/test_extensions/test_event_trigger--1.0--2.0.sql
create mode 100644 src/test/modules/test_extensions/test_event_trigger--1.0.sql
create mode 100644 src/test/modules/test_extensions/test_event_trigger.control
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 7844880170..f11d8061b8 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1646,9 +1646,14 @@ EventTriggerAlterTableEnd(void)
/* If no subcommands, don't collect */
if (list_length(currentEventTriggerState->currentCommand->d.alterTable.subcmds) != 0)
{
+ MemoryContext oldcxt =
+ MemoryContextSwitchTo(currentEventTriggerState->cxt);
+
currentEventTriggerState->commandList =
lappend(currentEventTriggerState->commandList,
currentEventTriggerState->currentCommand);
+
+ MemoryContextSwitchTo(oldcxt);
}
else
pfree(currentEventTriggerState->currentCommand);
diff --git a/src/test/modules/test_extensions/Makefile b/src/test/modules/test_extensions/Makefile
index d18108e4e5..04a9b42f68 100644
--- a/src/test/modules/test_extensions/Makefile
+++ b/src/test/modules/test_extensions/Makefile
@@ -4,11 +4,13 @@ MODULE = test_extensions
PGFILEDESC = "test_extensions - regression testing for EXTENSION support"
EXTENSION = test_ext1 test_ext2 test_ext3 test_ext4 test_ext5 test_ext6 \
- test_ext7 test_ext8 test_ext_cyclic1 test_ext_cyclic2
+ test_ext7 test_ext8 test_ext_cyclic1 test_ext_cyclic2 \
+ test_event_trigger
DATA = test_ext1--1.0.sql test_ext2--1.0.sql test_ext3--1.0.sql \
test_ext4--1.0.sql test_ext5--1.0.sql test_ext6--1.0.sql \
test_ext7--1.0.sql test_ext7--1.0--2.0.sql test_ext8--1.0.sql \
- test_ext_cyclic1--1.0.sql test_ext_cyclic2--1.0.sql
+ test_ext_cyclic1--1.0.sql test_ext_cyclic2--1.0.sql \
+ test_event_trigger--1.0.sql test_event_trigger--1.0--2.0.sql
REGRESS = test_extensions test_extdepend
diff --git a/src/test/modules/test_extensions/expected/test_extensions.out b/src/test/modules/test_extensions/expected/test_extensions.out
index b5cbdfcad4..31b9ad8e06 100644
--- a/src/test/modules/test_extensions/expected/test_extensions.out
+++ b/src/test/modules/test_extensions/expected/test_extensions.out
@@ -154,3 +154,9 @@ DROP TABLE test_ext4_tab;
DROP FUNCTION create_extension_with_temp_schema();
RESET client_min_messages;
\unset SHOW_CONTEXT
+-- Test case of an event trigger run from an extension script
+-- with an extension upgrade.
+-- See: https://postgr.es/m/20200902193715.6e0269d4@firost
+CREATE EXTENSION test_event_trigger VERSION '1.0';
+ALTER EXTENSION test_event_trigger UPDATE TO '2.0';
+DROP EXTENSION test_event_trigger;
diff --git a/src/test/modules/test_extensions/sql/test_extensions.sql b/src/test/modules/test_extensions/sql/test_extensions.sql
index f505466ab4..93a4b5bdc9 100644
--- a/src/test/modules/test_extensions/sql/test_extensions.sql
+++ b/src/test/modules/test_extensions/sql/test_extensions.sql
@@ -93,3 +93,10 @@ DROP TABLE test_ext4_tab;
DROP FUNCTION create_extension_with_temp_schema();
RESET client_min_messages;
\unset SHOW_CONTEXT
+
+-- Test case of an event trigger run from an extension script
+-- with an extension upgrade.
+-- See: https://postgr.es/m/20200902193715.6e0269d4@firost
+CREATE EXTENSION test_event_trigger VERSION '1.0';
+ALTER EXTENSION test_event_trigger UPDATE TO '2.0';
+DROP EXTENSION test_event_trigger;
diff --git a/src/test/modules/test_extensions/test_event_trigger--1.0--2.0.sql b/src/test/modules/test_extensions/test_event_trigger--1.0--2.0.sql
new file mode 100644
index 0000000000..fdd2f3542e
--- /dev/null
+++ b/src/test/modules/test_extensions/test_event_trigger--1.0--2.0.sql
@@ -0,0 +1,7 @@
+/* src/test/modules/test_extensions/test_event_trigger--1.0--2.0.sql */
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION test_event_trigger UPDATE TO '2.0'" to load this file. \quit
+
+-- Test extension upgrade with event trigger.
+ALTER EVENT TRIGGER table_rewrite_trg DISABLE;
+ALTER TABLE t DROP COLUMN id;
diff --git a/src/test/modules/test_extensions/test_event_trigger--1.0.sql b/src/test/modules/test_extensions/test_event_trigger--1.0.sql
new file mode 100644
index 0000000000..0071712cb8
--- /dev/null
+++ b/src/test/modules/test_extensions/test_event_trigger--1.0.sql
@@ -0,0 +1,16 @@
+/* src/test/modules/test_extensions/test_event_trigger--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_event_trigger" to load this file. \quit
+
+-- Base table with event trigger, used in a regression test involving
+-- extension upgrades.
+CREATE TABLE t (id text);
+CREATE OR REPLACE FUNCTION _evt_table_rewrite_fnct()
+RETURNS EVENT_TRIGGER LANGUAGE plpgsql AS
+$$
+ BEGIN
+ END;
+$$;
+CREATE EVENT TRIGGER table_rewrite_trg
+ ON table_rewrite
+ EXECUTE PROCEDURE _evt_table_rewrite_fnct();
diff --git a/src/test/modules/test_extensions/test_event_trigger.control b/src/test/modules/test_extensions/test_event_trigger.control
new file mode 100644
index 0000000000..840c729620
--- /dev/null
+++ b/src/test/modules/test_extensions/test_event_trigger.control
@@ -0,0 +1,3 @@
+comment = 'Test extension - event trigger'
+default_version = '2.0'
+relocatable = true
--
2.28.0
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-08 14:15 Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 21+ messages in thread
From: Jehan-Guillaume de Rorthais @ 2020-09-08 14:15 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-bugs@lists.postgresql.org
On Tue, 8 Sep 2020 14:27:03 +0900
Michael Paquier <michael@paquier.xyz> wrote:
> On Mon, Sep 07, 2020 at 10:35:08AM +0900, Michael Paquier wrote:
> > More comments in each script would be helpful to document to the
> > reader why those objects are defined as such and their purpose in
> > life.
>
> I have looked at that, and tweaked a couple of things as per the
> attached. Alvaro, Jehan-Guillaume, is that fine to you?
I was actually working on an answer with a new version of the patch, but you've
been faster. Thanks.
I would probably set the error message in VERSION '1.0' to
CREATE EXTENSION test_event_trigger VERSION '1.0'
But it's really nitpicking at this point.
More importantly, I wonder if this bug might be the visible tip of another
bigger one. I was wondering why this crash do not triggers without disabling
the event trigger first.
It appears that when keeping the event trigger enabled,
currentEventTriggerState might be initiated multiple times: one for the top
level "alter extension ... update", then once per DDL query triggering en event
in the extension update script. I suppose there's no crash because the very
first currentEventTriggerState initialized at top level is overrode by next
ones, each being destroyed in their own context. So at the end of the
extension script, currentEventTriggerState is just destroyed and empty.
I did not had time to investigate further more and produce a demo case though.
I might misunderstand something here.
Comment?
++
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-08 14:33 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
0 siblings, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2020-09-08 14:33 UTC (permalink / raw)
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org
On 2020-Sep-08, Jehan-Guillaume de Rorthais wrote:
> It appears that when keeping the event trigger enabled,
> currentEventTriggerState might be initiated multiple times: one for the top
> level "alter extension ... update", then once per DDL query triggering en event
> in the extension update script. I suppose there's no crash because the very
> first currentEventTriggerState initialized at top level is overrode by next
> ones, each being destroyed in their own context. So at the end of the
> extension script, currentEventTriggerState is just destroyed and empty.
Hmm. I spent a lot of time making sure the event trigger thing worked
correctly in reentrant cases, but I don't think the case of execution of
extension scripts was ever considered. So you're right that it might be
broken, since it appears not to be thoroughly tested (if at all).
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-08 17:07 Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 21+ messages in thread
From: Jehan-Guillaume de Rorthais @ 2020-09-08 17:07 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org
On Tue, 8 Sep 2020 11:33:24 -0300
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2020-Sep-08, Jehan-Guillaume de Rorthais wrote:
>
> > It appears that when keeping the event trigger enabled,
> > currentEventTriggerState might be initiated multiple times: one for the top
> > level "alter extension ... update", then once per DDL query triggering en
> > event in the extension update script. I suppose there's no crash because
> > the very first currentEventTriggerState initialized at top level is
> > overrode by next ones, each being destroyed in their own context. So at the
> > end of the extension script, currentEventTriggerState is just destroyed and
> > empty.
>
> Hmm. I spent a lot of time making sure the event trigger thing worked
> correctly in reentrant cases, but I don't think the case of execution of
> extension scripts was ever considered. So you're right that it might be
> broken, since it appears not to be thoroughly tested (if at all).
Argh. After some more debugging, I realized I missed the fact that
EventTriggerBeginCompleteQuery/EventTriggerEndCompleteQuery were actually
stacking allocations using currentEventTriggerState->previous. My bad, sorry for
your time.
I still have a last question in mind though. While running the bellow
scenario, the DDL in the extension do not trigger evt_ext_ddl_fnct. Is it
normal/expected?
BEGIN;
CREATE OR REPLACE FUNCTION _evt_ext_ddl_fnct()
RETURNS EVENT_TRIGGER LANGUAGE plpgsql AS
$$
DECLARE r record;
BEGIN
FOR r in SELECT * FROM pg_event_trigger_ddl_commands() LOOP
raise notice 'called %: %.%',
quote_ident(r.command_tag),
quote_ident(r.schema_name),
quote_ident(r.object_identity);
end loop;
END;
$$;
CREATE EVENT TRIGGER evt_ext_ddl_fnct
ON ddl_command_end
EXECUTE PROCEDURE _evt_ext_ddl_fnct();
CREATE EXTENSION test_event_trigger VERSION '1.0';
ALTER EXTENSION test_event_trigger UPDATE TO '2.0';
CREATE TABLE test( i int);
ROLLBACK;
The only output is:
NOTICE: called "CREATE EXTENSION": <NULL>.test_event_trigger
NOTICE: called "ALTER EXTENSION": <NULL>.test_event_trigger
NOTICE: called "CREATE TABLE": public."public.test"
Regards,
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-08 17:32 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
0 siblings, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2020-09-08 17:32 UTC (permalink / raw)
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org
On 2020-Sep-08, Jehan-Guillaume de Rorthais wrote:
> I still have a last question in mind though. While running the bellow
> scenario, the DDL in the extension do not trigger evt_ext_ddl_fnct. Is it
> normal/expected?
Hmm, I think this is intentional; see standard_ProcessUtility and
ProcessUtilitySlow, noting the value of isCompleteQuery for the
execution of the extension script.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-09 10:58 Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 21+ messages in thread
From: Jehan-Guillaume de Rorthais @ 2020-09-09 10:58 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org
On Tue, 8 Sep 2020 14:32:22 -0300
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2020-Sep-08, Jehan-Guillaume de Rorthais wrote:
>
> > I still have a last question in mind though. While running the bellow
> > scenario, the DDL in the extension do not trigger evt_ext_ddl_fnct. Is it
> > normal/expected?
>
> Hmm, I think this is intentional; see standard_ProcessUtility and
> ProcessUtilitySlow, noting the value of isCompleteQuery for the
> execution of the extension script.
According to extension.c:execute_sql_string(), queries from
extension script are executed as PROCESS_UTILITY_QUERY context. So
isCompleteQuery is indeed always true in ProcessUtilitySlow. A breakpoint here
while running my test case confirm this.
Maybe you were talking about isTopLevel? But this one doesn't seem considered
while defining if event triggers should trigger or not.
Anyway, if event trigger should not trigger during create/alter extension, I
suppose the original memory context bug that starts this discussion shouldn't
happen in the first place (but need to be fixed anyway), isn't it?
Regards,
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-11 06:14 Michael Paquier <michael@paquier.xyz>
parent: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
0 siblings, 1 reply; 21+ messages in thread
From: Michael Paquier @ 2020-09-11 06:14 UTC (permalink / raw)
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-bugs@lists.postgresql.org
On Wed, Sep 09, 2020 at 12:58:40PM +0200, Jehan-Guillaume de Rorthais wrote:
> According to extension.c:execute_sql_string(), queries from
> extension script are executed as PROCESS_UTILITY_QUERY context. So
> isCompleteQuery is indeed always true in ProcessUtilitySlow. A breakpoint here
> while running my test case confirm this.
>
> Maybe you were talking about isTopLevel? But this one doesn't seem considered
> while defining if event triggers should trigger or not.
>
> Anyway, if event trigger should not trigger during create/alter extension, I
> suppose the original memory context bug that starts this discussion shouldn't
> happen in the first place (but need to be fixed anyway), isn't it?
If I read correctly the code of event_trigger.c, command collection
and execution are and should be two different things, meaning that we
should still collect the commands and then at execution time we decide
if the event trigger associated to the commands should be fired or
not.
Anyway, based on your example in [1], I can see that the event trigger
for ddl_command_end is correctly triggered for the ALTER TABLE command
included in the extension upgrade script if the event trigger is
enabled at the time the extension script triggers, which is the
behavior I would expect. What may be a problem though is that the
NOTICE you are trying to print does not show up, but I think that this
is caused by the particular context where the SQL queries from an
extension script are triggered within the backend in this case.
Also, if you want to make sure of the event trigger execution, you can
just change the notice to an exception in _evt_ext_ddl_fnct() and you
would see ALTER EXTENSION fail, pg_event_trigger_ddl_commands
capturing correctly the information associated to ALTER TABLE for
table "t":
ERROR: P0001: called "ALTER TABLE": public."public.t"
CONTEXT: PL/pgSQL function _evt_ext_ddl_fnct() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3878
FWIW, I think that the fix proposed is fine as-is, and that we had
better apply it. Alvaro, perhaps you would prefer taking care of it?
[1]: https://www.postgresql.org/message-id/20200908190759.12405fe5@firost
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-11 08:09 Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 21+ messages in thread
From: Jehan-Guillaume de Rorthais @ 2020-09-11 08:09 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-bugs@lists.postgresql.org
On Fri, 11 Sep 2020 15:14:41 +0900
Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, Sep 09, 2020 at 12:58:40PM +0200, Jehan-Guillaume de Rorthais wrote:
> > According to extension.c:execute_sql_string(), queries from
> > extension script are executed as PROCESS_UTILITY_QUERY context. So
> > isCompleteQuery is indeed always true in ProcessUtilitySlow. A breakpoint
> > here while running my test case confirm this.
> >
> > Maybe you were talking about isTopLevel? But this one doesn't seem
> > considered while defining if event triggers should trigger or not.
> >
> > Anyway, if event trigger should not trigger during create/alter extension, I
> > suppose the original memory context bug that starts this discussion
> > shouldn't happen in the first place (but need to be fixed anyway), isn't
> > it?
>
> If I read correctly the code of event_trigger.c, command collection
> and execution are and should be two different things, meaning that we
> should still collect the commands and then at execution time we decide
> if the event trigger associated to the commands should be fired or
> not.
>
> Anyway, based on your example in [1], I can see that the event trigger
> for ddl_command_end is correctly triggered for the ALTER TABLE command
> included in the extension upgrade script if the event trigger is
> enabled at the time the extension script triggers, which is the
> behavior I would expect. What may be a problem though is that the
> NOTICE you are trying to print does not show up, but I think that this
> is caused by the particular context where the SQL queries from an
> extension script are triggered within the backend in this case.
> Also, if you want to make sure of the event trigger execution, you can
> just change the notice to an exception in _evt_ext_ddl_fnct() and you
> would see ALTER EXTENSION fail, pg_event_trigger_ddl_commands
> capturing correctly the information associated to ALTER TABLE for
> table "t":
> ERROR: P0001: called "ALTER TABLE": public."public.t"
> CONTEXT: PL/pgSQL function _evt_ext_ddl_fnct() line 5 at RAISE
> LOCATION: exec_stmt_raise, pl_exec.c:3878
Thank for your investigation, explanation and time.
> FWIW, I think that the fix proposed is fine as-is, and that we had
> better apply it.
+1
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-12 00:21 Michael Paquier <michael@paquier.xyz>
parent: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
0 siblings, 2 replies; 21+ messages in thread
From: Michael Paquier @ 2020-09-12 00:21 UTC (permalink / raw)
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-bugs@lists.postgresql.org
On Fri, Sep 11, 2020 at 10:09:00AM +0200, Jehan-Guillaume de Rorthais wrote:
> On Fri, 11 Sep 2020 15:14:41 +0900 Michael Paquier <michael@paquier.xyz> wrote:
>> FWIW, I think that the fix proposed is fine as-is, and that we had
>> better apply it.
>
> +1
Alvaro, would you prefer doing the final checks on this patch by
yourself and potentially commit it?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-12 00:22 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 21+ messages in thread
From: Alvaro Herrera @ 2020-09-12 00:22 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; pgsql-bugs@lists.postgresql.org
On 2020-Sep-12, Michael Paquier wrote:
> On Fri, Sep 11, 2020 at 10:09:00AM +0200, Jehan-Guillaume de Rorthais wrote:
> > On Fri, 11 Sep 2020 15:14:41 +0900 Michael Paquier <michael@paquier.xyz> wrote:
> >> FWIW, I think that the fix proposed is fine as-is, and that we had
> >> better apply it.
> >
> > +1
>
> Alvaro, would you prefer doing the final checks on this patch by
> yourself and potentially commit it?
Yes, thanks.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-16 00:26 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 21+ messages in thread
From: Alvaro Herrera @ 2020-09-16 00:26 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; pgsql-bugs@lists.postgresql.org
On 2020-Sep-12, Michael Paquier wrote:
> On Fri, Sep 11, 2020 at 10:09:00AM +0200, Jehan-Guillaume de Rorthais wrote:
> > On Fri, 11 Sep 2020 15:14:41 +0900 Michael Paquier <michael@paquier.xyz> wrote:
> >> FWIW, I think that the fix proposed is fine as-is, and that we had
> >> better apply it.
> >
> > +1
>
> Alvaro, would you prefer doing the final checks on this patch by
> yourself and potentially commit it?
I gave this a very long look and was convinced that it was going to
break with multi-statement query strings, but it turns out that it
wasn't. So I pushed it without any other changes.
I forgot to add reviewer credit to the commit message :-( My apologies
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-16 06:36 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 21+ messages in thread
From: Michael Paquier @ 2020-09-16 06:36 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>; pgsql-bugs@lists.postgresql.org
On Tue, Sep 15, 2020 at 09:26:39PM -0300, Alvaro Herrera wrote:
> I gave this a very long look and was convinced that it was going to
> break with multi-statement query strings, but it turns out that it
> wasn't. So I pushed it without any other changes.
>
> I forgot to add reviewer credit to the commit message :-( My apologies
No worries. Thanks for committing.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: [BUG v13] Crash with event trigger in extension
@ 2020-09-16 08:53 Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 21+ messages in thread
From: Jehan-Guillaume de Rorthais @ 2020-09-16 08:53 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-bugs@lists.postgresql.org
On Wed, 16 Sep 2020 15:36:53 +0900
Michael Paquier <michael@paquier.xyz> wrote:
> On Tue, Sep 15, 2020 at 09:26:39PM -0300, Alvaro Herrera wrote:
> > I gave this a very long look and was convinced that it was going to
> > break with multi-statement query strings, but it turns out that it
> > wasn't. So I pushed it without any other changes.
> >
> > I forgot to add reviewer credit to the commit message :-( My apologies
>
> No worries. Thanks for committing.
Thank you both.
^ permalink raw reply [nested|flat] 21+ messages in thread
end of thread, other threads:[~2020-09-16 08:53 UTC | newest]
Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-02 17:37 [BUG v13] Crash with event trigger in extension Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
2020-09-02 17:53 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-03 17:41 ` Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
2020-09-04 01:20 ` Michael Paquier <michael@paquier.xyz>
2020-09-04 09:37 ` Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
2020-09-04 13:27 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-04 14:56 ` Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
2020-09-07 01:35 ` Michael Paquier <michael@paquier.xyz>
2020-09-08 05:27 ` Michael Paquier <michael@paquier.xyz>
2020-09-08 14:15 ` Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
2020-09-08 14:33 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-08 17:07 ` Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
2020-09-08 17:32 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-09 10:58 ` Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
2020-09-11 06:14 ` Michael Paquier <michael@paquier.xyz>
2020-09-11 08:09 ` Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
2020-09-12 00:21 ` Michael Paquier <michael@paquier.xyz>
2020-09-12 00:22 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-16 00:26 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-16 06:36 ` Michael Paquier <michael@paquier.xyz>
2020-09-16 08:53 ` Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox