check_recovery_target_lsn() does a PG_CATCH without a throw

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

check_recovery_target_lsn() does a PG_CATCH without a throw

Andres Freund
Hi,

While working on fixing [1] I noticed that 2dedf4d9a899 "Integrate
recovery.conf into postgresql.conf" added two non-rethrowing PG_CATCH
uses. That's not OK. See

https://www.postgresql.org/message-id/1676.1548726280%40sss.pgh.pa.us
https://postgr.es/m/20190206160958.GA22304%40alvherre.pgsql
etc.

static bool
check_recovery_target_time(char **newval, void **extra, GucSource source)
{
    if (strcmp(*newval, "") != 0)
    {
        TimestampTz time;
        TimestampTz *myextra;
        MemoryContext oldcontext = CurrentMemoryContext;

        /* reject some special values */
        if (strcmp(*newval, "epoch") == 0 ||
            strcmp(*newval, "infinity") == 0 ||
            strcmp(*newval, "-infinity") == 0 ||
            strcmp(*newval, "now") == 0 ||
            strcmp(*newval, "today") == 0 ||
            strcmp(*newval, "tomorrow") == 0 ||
            strcmp(*newval, "yesterday") == 0)
        {
            return false;
        }

        PG_TRY();
        {
            time = 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();

same in check_recovery_target_lsn.

I'll add an open item.

Greetings,

Andres Freund

[1] [hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Sergei Kornilov
Hello

> That's not OK.

hmm. Did you mean catching only needed errors by errcode? Something like attached?

regards, Sergei

check_recovery_target_re_throw_error.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Tom Lane-2
Sergei Kornilov <[hidden email]> writes:
>> That's not OK.

> hmm. Did you mean catching only needed errors by errcode? Something like attached?

No, he means you can't EVER catch an error and not re-throw it, unless
you do a full (sub)transaction abort and cleanup instead of re-throwing.
We've been around on this repeatedly because people want to believe they
can take shortcuts.  (See e.g. discussions for the jsonpath stuff.)
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.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Andres Freund
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


Reply | Threaded
Open this post in threaded view
|

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Peter Eisentraut-6
In reply to this post by Andres Freund
On 2019-06-11 08:11, Andres Freund wrote:
> While working on fixing [1] I noticed that 2dedf4d9a899 "Integrate
> recovery.conf into postgresql.conf" added two non-rethrowing PG_CATCH
> uses. That's not OK.

Right.  Here is a patch that addresses this by copying the relevant code
from pg_lsn_in() and timestamptz_in() directly into the check hooks.
It's obviously a bit unfortunate not to be able to share that code, but
it's not actually that much.

I haven't figured out the time zone issue yet, but I guess the solution
might involve moving some of the code from check_recovery_target_time()
to assign_recovery_target_time().

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

0001-Remove-explicit-error-handling-for-obsolete-date-tim.patch (20K) Download Attachment
0002-Don-t-call-data-type-input-functions-in-GUC-check-ho.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Michael Paquier-2
On Wed, Jun 12, 2019 at 01:16:54PM +0200, Peter Eisentraut wrote:
> Right.  Here is a patch that addresses this by copying the relevant code
> from pg_lsn_in() and timestamptz_in() directly into the check hooks.
> It's obviously a bit unfortunate not to be able to share that code,
> but it's not actually that much.

+    len1 = strspn(str, "0123456789abcdefABCDEF");
+    if (len1 < 1 || len1 > MAXPG_LSNCOMPONENT || str[len1] != '/')
+        return false;
+
+    len2 = strspn(str + len1 + 1, "0123456789abcdefABCDEF");
+    if (len2 < 1 || len2 > MAXPG_LSNCOMPONENT || str[len1 + 1 + len2] != '\0')
+        return false;
Speaking about pg_lsn.  We have introduced it to reduce the amount of
duplication when mapping an LSN to text, so I am not much a fan of
this patch which adds again a duplication.  You also lose some error
context as you get the same type of error when parsing the first or
the second part of the LSN.  Couldn't you refactor the whole so as an
error string is present as in GUC_check_errdetail()?  I would just put
a wrapper in pg_lsn.c, like pg_lsn_parse() which returns uint64.

The same remark applies to the timestamp_in portion..
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Peter Eisentraut-6
On 2019-06-13 08:55, Michael Paquier wrote:
> Speaking about pg_lsn.  We have introduced it to reduce the amount of
> duplication when mapping an LSN to text, so I am not much a fan of
> this patch which adds again a duplication.  You also lose some error
> context as you get the same type of error when parsing the first or
> the second part of the LSN.  Couldn't you refactor the whole so as an
> error string is present as in GUC_check_errdetail()?

There isn't really much more detail to be had.  pg_lsn_in() just reports
"invalid input syntax for type pg_lsn", and with the current patch the
GUC system would report something like 'invalid value for parameter
"recovery_target_time"'.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services