Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Hi,

On 2019-06-11 10:49:28 -0400, Tom Lane wrote:
> It doesn't reliably work to do so, and we have a project policy against
> trying, and this code should never have been committed in this state.

I'll also note that I complained about this specific instance being
introduced all the way back in 2013 and then again 2016:

https://www.postgresql.org/message-id/20131118172748.GG20305%40awork2.anarazel.de

On 2013-11-18 18:27:48 +0100, Andres Freund wrote:
> * Why the PG_TRY/PG_CATCH in check_recovery_target_time? Besides being
> really strangely formatted (multiline :? inside a function?) it
> doesn't a) seem to be correct to ignore potential memory allocation
> errors by just switching back into the context that just errored out,
> and continue to work there b) forgo cleanup by just continuing as if
> nothing happened. That's unlikely to be acceptable.
> * You access recovery_target_name[0] unconditionally, although it's

https://www.postgresql.org/message-id/20140123133424.GD29782%40awork2.anarazel.de

On 2016-11-12 08:09:49 -0800, Andres Freund wrote:
> > +static bool
> > +check_recovery_target_time(char **newval, void **extra, GucSource source)
> > +{
> > + TimestampTz time;
> > + TimestampTz *myextra;
> > + MemoryContext oldcontext = CurrentMemoryContext;
> > +
> > + PG_TRY();
> > + {
> > + time = (strcmp(*newval, "") == 0) ?
> > + 0 :
> > + DatumGetTimestampTz(DirectFunctionCall3(timestamptz_in,
> > + CStringGetDatum(*newval),
> > + ObjectIdGetDatum(InvalidOid),
> > + Int32GetDatum(-1)));
> > + }
> > + PG_CATCH();
> > + {
> > + ErrorData *edata;
> > +
> > + /* Save error info */
> > + MemoryContextSwitchTo(oldcontext);
> > + edata = CopyErrorData();
> > + FlushErrorState();
> > +
> > + /* Pass the error message */
> > + GUC_check_errdetail("%s", edata->message);
> > + FreeErrorData(edata);
> > + return false;
> > + }
> > + PG_END_TRY();
>
> Hm, I'm not happy to do that kind of thing. What if there's ever any
> database interaction added to timestamptz_in?
>
> It's also problematic because the parsing of timestamps depends on the
> timezone GUC - which might not yet have taken effect...

I don't have particularly polite words about this.

Greetings,

Andres Freund