On Thu, Jan 12, 2023 at 11:24 AM Nathan Bossart
<nathandbossart@gmail.com> wrote:
>
> On Sun, Dec 18, 2022 at 12:53:31PM -0800, Andrey Borodin wrote:
> > I've rewritten this part to correctly report all timeouts that did
> > happen. However there's now a tricky comma-formatting code which was
> > tested only manually.
>
> I suspect this will make translation difficult.
I use special functions for this like _()
char* lock_reason = lock_timeout_occurred ? _("lock timeout") : "";
and then
ereport(ERROR, (errcode(err_code),
errmsg("canceling statement due to %s%s%s%s%s", lock_reason, comma1,
stmt_reason, comma2, tx_reason)));
I hope it will be translatable...
> >> > > > + ahprintf(AH, "SET transaction_timeout = 0;\n");
> >> > >
> >> > > Hm - why is that the right thing to do?
> >> > Because transaction_timeout has effects of statement_timeout.
> >>
> >> I guess it's just following precedent - but it seems a bit presumptuous
> >> to just disable safety settings a DBA might have set up. That makes some
> >> sense for e.g. idle_in_transaction_session_timeout, because I think
> >> e.g. parallel backup can lead to a connection being idle for a bit.
> >
> > I do not know. My reasoning - everywhere we turn off
> > statement_timeout, we should turn off transaction_timeout too.
> > But I have no strong opinion here. I left this code as is in the patch
> > so far. For the same reason I did not change anything in
> > pg_backup_archiver.c.
>
> From 8383486's commit message:
>
> We disable statement_timeout and lock_timeout during dump and restore,
> to prevent any global settings that might exist from breaking routine
> backups.
>
> I imagine changing this could disrupt existing servers that depend on these
> overrides during backups, although I think Andres has a good point about
> disabling safety settings. This might be a good topic for another thread.
>
+1.
Thanks for the review!
Best regards, Andrey Borodin.