Thread: 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

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

On Fri, Feb 21, 2020 at 12:06 AM Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>
>
>
> On Thu, Feb 20, 2020 at 4:52 AM Michael Paquier <michael@paquier.xyz> 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
afterthat 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

vignesh C <vignesh21@gmail.com> 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 = 0,        /* Main pass (most TOC item types) */
     RESTORE_PASS_ACL,            /* ACL item types */
-    RESTORE_PASS_REFRESH        /* Matview REFRESH items */
+    RESTORE_PASS_POST_ACL        /* Event trigger and matview refresh items */

-#define RESTORE_PASS_LAST RESTORE_PASS_REFRESH
+#define RESTORE_PASS_LAST RESTORE_PASS_POST_ACL
 } RestorePass;

 typedef enum
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 1432dd7..654e2ec 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -24,8 +24,15 @@
  * Sort priority for database object types.
  * Objects are sorted by type, and within a type by name.
  *
- * Because materialized views can potentially reference system views,
- * DO_REFRESH_MATVIEW should always be the last thing on the list.
+ * Triggers, event triggers, and materialized views are intentionally sorted
+ * late.  Triggers must be restored after all data modifications, so that
+ * they don't interfere with loading data.  Event triggers are restored
+ * next-to-last so that they don't interfere with object creations of any
+ * kind.  Matview refreshes are last because they should execute in the
+ * database's normal state (e.g., they must come after all ACLs are restored;
+ * also, if they choose to look at system catalogs, they should see the final
+ * restore state).  If you think to change this, see also the RestorePass
+ * mechanism in pg_backup_archiver.c.
  *
  * NOTE: object-type priorities must match the section assignments made in
  * pg_dump.c; that is, PRE_DATA objects must sort before DO_PRE_DATA_BOUNDARY,
@@ -66,18 +73,18 @@ static const int dbObjectTypePriority[] =
     15,                            /* DO_TSCONFIG */
     16,                            /* DO_FDW */
     17,                            /* DO_FOREIGN_SERVER */
-    33,                            /* DO_DEFAULT_ACL */
+    38,                            /* DO_DEFAULT_ACL --- done in ACL pass */
     3,                            /* DO_TRANSFORM */
     21,                            /* DO_BLOB */
     25,                            /* DO_BLOB_DATA */
     22,                            /* DO_PRE_DATA_BOUNDARY */
     26,                            /* DO_POST_DATA_BOUNDARY */
-    34,                            /* DO_EVENT_TRIGGER */
-    39,                            /* DO_REFRESH_MATVIEW */
-    35,                            /* DO_POLICY */
-    36,                            /* DO_PUBLICATION */
-    37,                            /* DO_PUBLICATION_REL */
-    38                            /* DO_SUBSCRIPTION */
+    39,                            /* DO_EVENT_TRIGGER --- next to last! */
+    40,                            /* DO_REFRESH_MATVIEW --- last! */
+    34,                            /* DO_POLICY */
+    35,                            /* DO_PUBLICATION */
+    36,                            /* DO_PUBLICATION_REL */
+    37                            /* DO_SUBSCRIPTION */
 };

 StaticAssertDecl(lengthof(dbObjectTypePriority) == (DO_SUBSCRIPTION + 1),

=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= <fabriziomello@gmail.com> writes:
> On Sat, Mar 7, 2020 at 8:42 PM Tom Lane <tgl@sss.pgh.pa.us> 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

=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= <fabriziomello@gmail.com> writes:
> On Mon, Mar 9, 2020 at 12:27 PM Tom Lane <tgl@sss.pgh.pa.us> 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