Re: Bug in pg_restore with EventTrigger in parallel mode
On Mon, Mar 9, 2020 at 3:59 PM Tom Lane wrote: > > =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= writes: > > On Mon, Mar 9, 2020 at 12:27 PM Tom Lane wrote: > >> Which, TBH, makes me wonder about the validity of the original complaint > >> in this thread. I don't mind delaying ET restore as long as we feasibly > >> can; but if you have an ET that is going to misbehave during restore, > >> you are in for pain, and it's hard to consider that that pain isn't > >> self-inflicted. > > > The proposed patch solve the original complain. I was just trying to > > understand completely what you pointed out before and I agree with you. > > Thanks for the clear explanation. > > OK, thanks for confirming that this solves your issue in practice. > > > About the patch LGTM and IMHO we should back-patch it to all supported > > versions. > > Done. > Great, thanks! -- Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Re: Bug in pg_restore with EventTrigger in parallel mode
=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= writes: > On Mon, Mar 9, 2020 at 12:27 PM Tom Lane wrote: >> Which, TBH, makes me wonder about the validity of the original complaint >> in this thread. I don't mind delaying ET restore as long as we feasibly >> can; but if you have an ET that is going to misbehave during restore, >> you are in for pain, and it's hard to consider that that pain isn't >> self-inflicted. > The proposed patch solve the original complain. I was just trying to > understand completely what you pointed out before and I agree with you. > Thanks for the clear explanation. OK, thanks for confirming that this solves your issue in practice. > About the patch LGTM and IMHO we should back-patch it to all supported > versions. Done. regards, tom lane
Re: Bug in pg_restore with EventTrigger in parallel mode
On Mon, Mar 9, 2020 at 12:27 PM Tom Lane wrote: > > In the case of event triggers, the obvious counterexample is that if > you restore ET A and then ET B, ET A might interfere with the attempt > to restore ET B. (And we have no way to know whether restoring B > before A would be better or worse.) > Yeap... you're correct. > So on the whole I find "restore matviews as if they'd been refreshed > after the restore" to be a more trustworthy approach than the other > way. At some level we have to trust that ETs aren't going to totally > bollix the restore. > Ok. > Which, TBH, makes me wonder about the validity of the original complaint > in this thread. I don't mind delaying ET restore as long as we feasibly > can; but if you have an ET that is going to misbehave during restore, > you are in for pain, and it's hard to consider that that pain isn't > self-inflicted. > The proposed patch solve the original complain. I was just trying to understand completely what you pointed out before and I agree with you. Thanks for the clear explanation. About the patch LGTM and IMHO we should back-patch it to all supported versions. Regards, -- Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Re: Bug in pg_restore with EventTrigger in parallel mode
=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= writes: > On Sat, Mar 7, 2020 at 8:42 PM Tom Lane wrote: >> However, I think that the existing code is correct to restore event >> triggers before matview refreshes, not after as this patch would have us >> do. The basic idea for matview refresh is that it should happen in the >> normal running state of the database. If an event trigger interferes with >> that, it would've done so in normal running as well. > I'm not totally sure if it's entirely correct. > For example if I write an EventTrigger to perform some kind of DDL auditing > then during the restore the "refresh maview" operation will be audited and > IMHO it's wrong. The big problem I've got with this line of reasoning is that not everything can be the last restore step. There was already an argument that matviews should be refreshed last so they can see the final state of the catalogs, in case you have a matview over some catalog (and of course that applies to pg_event_trigger as much as any other catalog). Admittedly, that seems like an unlikely use-case, but it demonstrates that there are limits to how much we can guarantee about dump/restore producing just the same state that prevailed before the dump. In the case of event triggers, the obvious counterexample is that if you restore ET A and then ET B, ET A might interfere with the attempt to restore ET B. (And we have no way to know whether restoring B before A would be better or worse.) So on the whole I find "restore matviews as if they'd been refreshed after the restore" to be a more trustworthy approach than the other way. At some level we have to trust that ETs aren't going to totally bollix the restore. Which, TBH, makes me wonder about the validity of the original complaint in this thread. I don't mind delaying ET restore as long as we feasibly can; but if you have an ET that is going to misbehave during restore, you are in for pain, and it's hard to consider that that pain isn't self-inflicted. regards, tom lane
Re: Bug in pg_restore with EventTrigger in parallel mode
On Sat, Mar 7, 2020 at 8:42 PM Tom Lane wrote: > > vignesh C writes: > > I'm not sure if we can add a test for this, can you have a thought > > about this to check if we can add a test. > > Yeah, I'm not quite sure if a test is worth the trouble or not. > > We clearly do need to restore event triggers later than we do now, even > without considering parallel restore: they should not be able to prevent > us from executing other restore actions. This is just like the rule that > we don't restore DML triggers until after we've loaded data. > Ok. > However, I think that the existing code is correct to restore event > triggers before matview refreshes, not after as this patch would have us > do. The basic idea for matview refresh is that it should happen in the > normal running state of the database. If an event trigger interferes with > that, it would've done so in normal running as well. > I'm not totally sure if it's entirely correct. For example if I write an EventTrigger to perform some kind of DDL auditing then during the restore the "refresh maview" operation will be audited and IMHO it's wrong. > I'm also not terribly on board with loading more functionality onto the > RestorePass mechanism. That's a crock that should go away someday, > because it basically duplicates and overrides pg_dump's normal object > sorting mechanism. So we don't want it doing more than it absolutely > has to. But in this case, I don't see any reason why we can't just > restore event triggers and matviews in the same post-ACL restore pass. Totally agree with it. > In a serial restore, that will make the event triggers come first > because of the existing sort rules. In a parallel restore, it's possible > that they'd be intermixed, but that doesn't bother me. Again, if your > event triggers have side-effects on your matview refreshes, you're > going to have some issues anyway. > IMHO EventTriggers can't be fired during pg_restore under any circumstances because can lead us to a different database state than the dump used. > So that leads me to the attached, which renames the "RESTORE_PASS_REFRESH" > symbol for clarity, and updates the pg_dump_sort.c code and comments > to match what's really going on. > Ok. Regards, -- Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Re: Bug in pg_restore with EventTrigger in parallel mode
vignesh C writes:
> I'm not sure if we can add a test for this, can you have a thought
> about this to check if we can add a test.
Yeah, I'm not quite sure if a test is worth the trouble or not.
We clearly do need to restore event triggers later than we do now, even
without considering parallel restore: they should not be able to prevent
us from executing other restore actions. This is just like the rule that
we don't restore DML triggers until after we've loaded data.
However, I think that the existing code is correct to restore event
triggers before matview refreshes, not after as this patch would have us
do. The basic idea for matview refresh is that it should happen in the
normal running state of the database. If an event trigger interferes with
that, it would've done so in normal running as well.
I'm also not terribly on board with loading more functionality onto the
RestorePass mechanism. That's a crock that should go away someday,
because it basically duplicates and overrides pg_dump's normal object
sorting mechanism. So we don't want it doing more than it absolutely
has to. But in this case, I don't see any reason why we can't just
restore event triggers and matviews in the same post-ACL restore pass.
In a serial restore, that will make the event triggers come first
because of the existing sort rules. In a parallel restore, it's possible
that they'd be intermixed, but that doesn't bother me. Again, if your
event triggers have side-effects on your matview refreshes, you're
going to have some issues anyway.
So that leads me to the attached, which renames the "RESTORE_PASS_REFRESH"
symbol for clarity, and updates the pg_dump_sort.c code and comments
to match what's really going on.
regards, tom lane
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 15900ff..d3f85d6 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -670,11 +670,11 @@ RestoreArchive(Archive *AHX)
{
/*
* In serial mode, process everything in three phases: normal items,
- * then ACLs, then matview refresh items. We might be able to skip
- * one or both extra phases in some cases, eg data-only restores.
+ * then ACLs, then post-ACL items. We might be able to skip one or
+ * both extra phases in some cases, eg data-only restores.
*/
bool haveACL = false;
- bool haveRefresh = false;
+ bool havePostACL = false;
for (te = AH->toc->next; te != AH->toc; te = te->next)
{
@@ -689,8 +689,8 @@ RestoreArchive(Archive *AHX)
case RESTORE_PASS_ACL:
haveACL = true;
break;
-case RESTORE_PASS_REFRESH:
- haveRefresh = true;
+case RESTORE_PASS_POST_ACL:
+ havePostACL = true;
break;
}
}
@@ -705,12 +705,12 @@ RestoreArchive(Archive *AHX)
}
}
- if (haveRefresh)
+ if (havePostACL)
{
for (te = AH->toc->next; te != AH->toc; te = te->next)
{
if ((te->reqs & (REQ_SCHEMA | REQ_DATA)) != 0 &&
- _tocEntryRestorePass(te) == RESTORE_PASS_REFRESH)
+ _tocEntryRestorePass(te) == RESTORE_PASS_POST_ACL)
(void) restore_toc_entry(AH, te, false);
}
}
@@ -3082,8 +3082,9 @@ _tocEntryRestorePass(TocEntry *te)
strcmp(te->desc, "ACL LANGUAGE") == 0 ||
strcmp(te->desc, "DEFAULT ACL") == 0)
return RESTORE_PASS_ACL;
- if (strcmp(te->desc, "MATERIALIZED VIEW DATA") == 0)
- return RESTORE_PASS_REFRESH;
+ if (strcmp(te->desc, "EVENT TRIGGER") == 0 ||
+ strcmp(te->desc, "MATERIALIZED VIEW DATA") == 0)
+ return RESTORE_PASS_POST_ACL;
return RESTORE_PASS_MAIN;
}
diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_backup_archiver.h
index 6768f92..67f3474 100644
--- a/src/bin/pg_dump/pg_backup_archiver.h
+++ b/src/bin/pg_dump/pg_backup_archiver.h
@@ -209,10 +209,14 @@ typedef enum
* data restore failures. On the other hand, matview REFRESH commands should
* come out after ACLs, as otherwise non-superuser-owned matviews might not
* be able to execute. (If the permissions at the time of dumping would not
- * allow a REFRESH, too bad; we won't fix that for you.) These considerations
- * force us to make three passes over the TOC, restoring the appropriate
- * subset of items in each pass. We assume that the dependency sort resulted
- * in an appropriate ordering of items within each subset.
+ * allow a REFRESH, too bad; we won't fix that for you.) We also want event
+ * triggers to be restored after ACLs, so that they can't mess those up.
+ *
+ * These considerations force us to make three passes over the TOC,
+ * restoring the appropriate subset of items in each pass. We assume that
+ * the dependency sort resulted in an appropriate ordering of items within
+ * each subset.
+ *
* XXX This mechanism should be superseded by tracking dependencies on ACLs
* properly; but we'll still need it for old dump files even after that.
*/
@@ -220,9 +224,9 @@ typedef enum
{
RESTORE_PASS_MAIN =
Re: Bug in pg_restore with EventTrigger in parallel mode
On Fri, Feb 21, 2020 at 12:06 AM Fabrízio de Royes Mello wrote: > > > > On Thu, Feb 20, 2020 at 4:52 AM Michael Paquier wrote: > > > > That sounds right, as event triggers could interact with GRANT and > > REFRESH of matviews, so they should be logically last. Looking at the > > recent commit history, this would be similar to 3eb9a5e as we don't > > really have a way to treat event triggers as dependency-sortable > > objects. > > > > Indeed... event triggers should be the last thing to be restored. > > > What kind of errors did you see in this customer > > environment? Errors triggered by one or more event triggers blocking > > some commands based on a tag match? > > > > By error I meant the weird behavior I described before that pg_restore create > the event triggers in parallel mode and after that other objects are created > then the event trigger is fired during the restore... > > Have a look at the new attached patch. > The test works fine with the patch. Few comments: There is minor code alignment that need to be fixed: git apply fix_pg_restore_parallel_with_event_trigger_v2.patch fix_pg_restore_parallel_with_event_trigger_v2.patch:11: trailing whitespace. * then ACLs, matview refresh items, then event triggers. We might be warning: 1 line adds whitespace errors. I'm not sure if we can add a test for this, can you have a thought about this to check if we can add a test. Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com
Re: Bug in pg_restore with EventTrigger in parallel mode
On Thu, Feb 20, 2020 at 4:52 AM Michael Paquier wrote:
>
> That sounds right, as event triggers could interact with GRANT and
> REFRESH of matviews, so they should be logically last. Looking at the
> recent commit history, this would be similar to 3eb9a5e as we don't
> really have a way to treat event triggers as dependency-sortable
> objects.
>
Indeed... event triggers should be the last thing to be restored.
> What kind of errors did you see in this customer
> environment? Errors triggered by one or more event triggers blocking
> some commands based on a tag match?
>
By error I meant the weird behavior I described before that pg_restore
create the event triggers in parallel mode and after that other objects are
created then the event trigger is fired during the restore...
Have a look at the new attached patch.
Regards,
--
Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 77bf9edaba..faa93aaf9a 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -670,11 +670,13 @@ RestoreArchive(Archive *AHX)
{
/*
* In serial mode, process everything in three phases: normal items,
- * then ACLs, then matview refresh items. We might be able to skip
- * one or both extra phases in some cases, eg data-only restores.
+ * then ACLs, matview refresh items, then event triggers. We might be
+ * able to skip one or both extra phases in some cases, eg data-only
+ * restores.
*/
bool haveACL = false;
bool haveRefresh = false;
+ bool haveEventTrigger = false;
for (te = AH->toc->next; te != AH->toc; te = te->next)
{
@@ -692,6 +694,9 @@ RestoreArchive(Archive *AHX)
case RESTORE_PASS_REFRESH:
haveRefresh = true;
break;
+case RESTORE_PASS_EVENT_TRIGGER:
+ haveEventTrigger = true;
+ break;
}
}
@@ -714,6 +719,16 @@ RestoreArchive(Archive *AHX)
(void) restore_toc_entry(AH, te, false);
}
}
+
+ if (haveEventTrigger)
+ {
+ for (te = AH->toc->next; te != AH->toc; te = te->next)
+ {
+if ((te->reqs & (REQ_SCHEMA | REQ_DATA)) != 0 &&
+ _tocEntryRestorePass(te) == RESTORE_PASS_EVENT_TRIGGER)
+ (void) restore_toc_entry(AH, te, false);
+ }
+ }
}
if (ropt->single_txn)
@@ -3084,6 +3099,8 @@ _tocEntryRestorePass(TocEntry *te)
return RESTORE_PASS_ACL;
if (strcmp(te->desc, "MATERIALIZED VIEW DATA") == 0)
return RESTORE_PASS_REFRESH;
+ if (strcmp(te->desc, "EVENT TRIGGER") == 0)
+ return RESTORE_PASS_EVENT_TRIGGER;
return RESTORE_PASS_MAIN;
}
diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_backup_archiver.h
index 6768f92976..204019b514 100644
--- a/src/bin/pg_dump/pg_backup_archiver.h
+++ b/src/bin/pg_dump/pg_backup_archiver.h
@@ -220,9 +220,10 @@ typedef enum
{
RESTORE_PASS_MAIN = 0, /* Main pass (most TOC item types) */
RESTORE_PASS_ACL, /* ACL item types */
- RESTORE_PASS_REFRESH /* Matview REFRESH items */
+ RESTORE_PASS_REFRESH, /* Matview REFRESH items */
+ RESTORE_PASS_EVENT_TRIGGER /* Event Trigger items */
-#define RESTORE_PASS_LAST RESTORE_PASS_REFRESH
+#define RESTORE_PASS_LAST RESTORE_PASS_EVENT_TRIGGER
} RestorePass;
typedef enum
Re: Bug in pg_restore with EventTrigger in parallel mode
On Wed, Feb 12, 2020 at 01:59:05PM -0300, Fabrízio de Royes Mello wrote: > In parallel mode it's firing the EventTrigger and it can't be happen. > Poking around it I did some test with attached just to leave EventTriggers > in pending_list to process it in restore_toc_entries_postfork and > everything is ok, but my solution is very ugly, so maybe we need to invent > a new RestorePass to take care of it like RESTORE_PASS_ACL and > RESTORE_PASS_REFRESH. I can provide a more polished patch if it'll be a > good way to do that. That sounds right, as event triggers could interact with GRANT and REFRESH of matviews, so they should be logically last. Looking at the recent commit history, this would be similar to 3eb9a5e as we don't really have a way to treat event triggers as dependency-sortable objects. What kind of errors did you see in this customer environment? Errors triggered by one or more event triggers blocking some commands based on a tag match? -- Michael signature.asc Description: PGP signature
Re: Bug in pg_restore with EventTrigger in parallel mode
On Thu, Feb 13, 2020 at 12:52 AM Michael Paquier wrote: > On Wed, Feb 12, 2020 at 01:59:05PM -0300, Fabrízio de Royes Mello wrote: > > In parallel mode it's firing the EventTrigger and it can't be happen. > > Poking around it I did some test with attached just to leave > EventTriggers > > in pending_list to process it in restore_toc_entries_postfork and > > everything is ok, but my solution is very ugly, so maybe we need to > invent > > a new RestorePass to take care of it like RESTORE_PASS_ACL and > > RESTORE_PASS_REFRESH. I can provide a more polished patch if it'll be a > > good way to do that. > > Could you add that as a bug fix to the next CF [1]? > > [1]: https://commitfest.postgresql.org/27/ > > Done, thanks! https://commitfest.postgresql.org/27/2450/ Regards, -- Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Re: Bug in pg_restore with EventTrigger in parallel mode
On Wed, Feb 12, 2020 at 01:59:05PM -0300, Fabrízio de Royes Mello wrote: > In parallel mode it's firing the EventTrigger and it can't be happen. > Poking around it I did some test with attached just to leave EventTriggers > in pending_list to process it in restore_toc_entries_postfork and > everything is ok, but my solution is very ugly, so maybe we need to invent > a new RestorePass to take care of it like RESTORE_PASS_ACL and > RESTORE_PASS_REFRESH. I can provide a more polished patch if it'll be a > good way to do that. Could you add that as a bug fix to the next CF [1]? [1]: https://commitfest.postgresql.org/27/ -- Michael signature.asc Description: PGP signature
Bug in pg_restore with EventTrigger in parallel mode
Hi all,
Today digging into a customer issue about errors in pg_restore I realized
that pg_restore dispatch a worker to restore EventTrigger
during restore_toc_entries_parallel. IMHO EventTriggers should be restored
during the restore_toc_entries_postfork in serial mode.
For example this simple database schema:
BEGIN;
CREATE TABLE foo(c1 bigserial NOT NULL, c2 varchar(100) NOT NULL, PRIMARY
KEY (c1));
INSERT INTO foo (c2) SELECT 'Foo '||id FROM generate_series(0,10) id;
CREATE INDEX foo_1 ON foo (c2);
CREATE TABLE bar(c1 bigserial NOT NULL, c2 bigint REFERENCES public.foo, c3
varchar(100), PRIMARY KEY (c1));
INSERT INTO bar (c2, c3) SELECT (random()*10)::bigint+1, 'Bar '||id FROM
generate_series(1,1) id;
CREATE INDEX bar_1 ON bar (c2);
CREATE INDEX bar_2 ON bar (c3);
CREATE OR REPLACE FUNCTION f_test_ddl_trigger()
RETURNS event_trigger AS
$$
DECLARE
r RECORD;
BEGIN
FOR r IN
SELECT objid, objsubid, schema_name, objid::regclass::text AS
table_name, command_tag, object_type, object_identity
FROM pg_event_trigger_ddl_commands()
LOOP
RAISE INFO 'RUN EVENT TRIGGER %', r;
END LOOP;
END;
$$
LANGUAGE plpgsql;
CREATE EVENT TRIGGER test_ddl_trigger
ON ddl_command_end
EXECUTE PROCEDURE f_test_ddl_trigger();
COMMIT;
Running the dump:
$ bin/pg_dump -Fc -f /tmp/teste.dump fabrizio
Restoring with one worker everything is ok:
fabrizio@macanudo:~/pgsql
$ bin/pg_restore -Fc -d fabrizio_restore_serial /tmp/teste.dump | grep 'RUN
EVENT TRIGGER'
Running with more the one worker:
fabrizio@macanudo:~/pgsql
$ bin/pg_restore -Fc -j2 -d fabrizio_restore_parallel /tmp/teste.dump |
grep 'RUN EVENT TRIGGER'
pg_restore: INFO: RUN EVENT TRIGGER (16906,0,public,public.bar,"ALTER
TABLE",table,public.bar)
In parallel mode it's firing the EventTrigger and it can't be happen.
Poking around it I did some test with attached just to leave EventTriggers
in pending_list to process it in restore_toc_entries_postfork and
everything is ok, but my solution is very ugly, so maybe we need to invent
a new RestorePass to take care of it like RESTORE_PASS_ACL and
RESTORE_PASS_REFRESH. I can provide a more polished patch if it'll be a
good way to do that.
Regards,
--
Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 77bf9edaba..9762e4c973 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -4357,7 +4357,8 @@ move_to_ready_list(TocEntry *pending_list,
next_te = te->pending_next;
if (te->depCount == 0 &&
- _tocEntryRestorePass(te) == pass)
+ _tocEntryRestorePass(te) == pass &&
+ strcmp(te->desc, "EVENT TRIGGER") != 0) /* leave EVENT TRIGGER in pending_list */
{
/* Remove it from pending_list ... */
pending_list_remove(te);
