Re: Bug in pg_restore with EventTrigger in parallel mode

On Sat, Mar 7, 2020 at 8:42 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> vignesh C <vignesh21(at)gmail(dot)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.
>

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