Horton

Thread

Commits

  1. Fix pg_dump/pg_restore to restore event triggers later.

  1. Bug in pg_restore with EventTrigger in parallel mode

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2020-02-12T16:59:05Z

    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,10000) 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

  2. Re: Bug in pg_restore with EventTrigger in parallel mode

    Michael Paquier <michael@paquier.xyz> — 2020-02-13T03:52:31Z

    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

  3. Re: Bug in pg_restore with EventTrigger in parallel mode

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2020-02-13T14:27:50Z

    On Thu, Feb 13, 2020 at 12:52 AM Michael Paquier <michael@paquier.xyz>
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

  4. Re: Bug in pg_restore with EventTrigger in parallel mode

    Michael Paquier <michael@paquier.xyz> — 2020-02-20T07:52:32Z

    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

  5. Re: Bug in pg_restore with EventTrigger in parallel mode

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2020-02-20T18:36:01Z

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

  6. Re: Bug in pg_restore with EventTrigger in parallel mode

    vignesh C <vignesh21@gmail.com> — 2020-03-04T16:25:54Z

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

  7. Re: Bug in pg_restore with EventTrigger in parallel mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-07T23:42:16Z

    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

  8. Re: Bug in pg_restore with EventTrigger in parallel mode

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2020-03-09T12:36:15Z

    On Sat, Mar 7, 2020 at 8:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> 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.
>

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

  9. Re: Bug in pg_restore with EventTrigger in parallel mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-09T15:27:05Z

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

  10. Re: Bug in pg_restore with EventTrigger in parallel mode

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2020-03-09T16:56:31Z

    On Mon, Mar 9, 2020 at 12:27 PM Tom Lane <tgl@sss.pgh.pa.us> 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

  11. Re: Bug in pg_restore with EventTrigger in parallel mode

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-09T18:59:35Z

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

  12. Re: Bug in pg_restore with EventTrigger in parallel mode

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2020-03-09T19:44:34Z

    On Mon, Mar 9, 2020 at 3:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> =?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.
>

Great, thanks!

--
   Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento