From: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
---|---|
To: | "Drouvot, Bertrand" <bdrouvot(at)amazon(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Add Information during standby recovery conflicts |
Date: | 2020-11-30 03:41:27 |
Message-ID: | CAD21AoCjJv07qG6d5nkYXgJVC=TAR1eSC1OJMtvmkJd7aS9pgA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sun, Nov 29, 2020 at 3:47 PM Drouvot, Bertrand <bdrouvot(at)amazon(dot)com> wrote:
>
> Hi Alvaro,
>
> On 11/28/20 6:36 PM, Alvaro Herrera wrote:
> > Hi Bertrand,
> >
> > On 2020-Nov-28, Drouvot, Bertrand wrote:
> >
> >> + if (nprocs > 0)
> >> + {
> >> + ereport(LOG,
> >> + (errmsg("recovery still waiting after %ld.%03d ms: %s",
> >> + msecs, usecs, _(get_recovery_conflict_desc(reason))),
> >> + (errdetail_log_plural("Conflicting process: %s.",
> >> + "Conflicting processes: %s.",
> >> + nprocs, buf.data))));
> >> + }
> >> + else
> >> + {
> >> + ereport(LOG,
> >> + (errmsg("recovery still waiting after %ld.%03d ms: %s",
> >> + msecs, usecs, _(get_recovery_conflict_desc(reason)))));
> >> + }
> >> +
> >> + pfree(buf.data);
> >> + }
> >> + else
> >> + ereport(LOG,
> >> + (errmsg("recovery still waiting after %ld.%03d ms: %s",
> >> + msecs, usecs, _(get_recovery_conflict_desc(reason)))));
> >> +}
> > Another trivial stylistic point is that you can collapse all these
> > ereport calls into one, with something like
> >
> > ereport(LOG,
> > errmsg("recovery still waiting after ...", opts),
> > waitlist != NULL ? errdetail_log_plural("foo bar baz", opts) : 0);
> >
> > where the "waitlist" has been constructed beforehand, or is set to NULL
> > if there's no process list.
>
> Nice!
>
> >
> >> + switch (reason)
> >> + {
> >> + case PROCSIG_RECOVERY_CONFLICT_BUFFERPIN:
> >> + reasonDesc = gettext_noop("for recovery conflict on buffer pin");
> >> + break;
> > Pure bikeshedding after discussing this with my pillow: I think I'd get
> > rid of the initial "for" in these messages.
>
> both comments implemented in the new attached version.
>
Thank you for updating the patch!
+ /* Also, set deadlock timeout for logging purpose if necessary */
+ if (log_recovery_conflict_waits && !need_log)
+ {
+ timeouts[cnt].id = STANDBY_TIMEOUT;
+ timeouts[cnt].type = TMPARAM_AFTER;
+ timeouts[cnt].delay_ms = DeadlockTimeout;
+ cnt++;
+ }
You changed to 'need_log' but this condition seems not correct. I
think we need to set this timeout when both log_recovery_conflict and
need_log is true.
The rest of the patch looks good to me.
Regards,
--
Masahiko Sawada
EnterpriseDB: https://www.enterprisedb.com/
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2020-11-30 03:52:46 | Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions |
Previous Message | David G. Johnston | 2020-11-30 03:24:02 | Re: [patch] [doc] Minor variable related cleanup and rewording of plpgsql docs |