On Thu, Jan 12, 2023 at 11:24 AM Nathan Bossart <nathandboss...@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.