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