check_recovery_target_lsn() does a PG_CATCH without a throw

classic Classic list List threaded Threaded
13 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


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 Peter Eisentraut-6
On 2019-06-12 13:16, Peter Eisentraut wrote:
> 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().

I think that won't work either.  What we need to do is postpone the
interpretation of the timestamp string until after all the GUC
processing is done.  So check_recovery_target_time() would just do some
basic parsing checks, but stores the string.  Then when we need the
recovery_target_time_value we do the final parsing.  Then we can be sure
that the time zone is all set.

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


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-20 15:42:14 +0200, Peter Eisentraut wrote:

> On 2019-06-12 13:16, Peter Eisentraut wrote:
> > 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().
>
> I think that won't work either.  What we need to do is postpone the
> interpretation of the timestamp string until after all the GUC
> processing is done.  So check_recovery_target_time() would just do some
> basic parsing checks, but stores the string.  Then when we need the
> recovery_target_time_value we do the final parsing.  Then we can be sure
> that the time zone is all set.

That sounds right to me.

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
On 2019-06-20 18:05, Andres Freund wrote:

> Hi,
>
> On 2019-06-20 15:42:14 +0200, Peter Eisentraut wrote:
>> On 2019-06-12 13:16, Peter Eisentraut wrote:
>>> 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().
>>
>> I think that won't work either.  What we need to do is postpone the
>> interpretation of the timestamp string until after all the GUC
>> processing is done.  So check_recovery_target_time() would just do some
>> basic parsing checks, but stores the string.  Then when we need the
>> recovery_target_time_value we do the final parsing.  Then we can be sure
>> that the time zone is all set.
>
> That sounds right to me.
Updated patch for that.

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

v2-0001-Remove-explicit-error-handling-for-obsolete-date-.patch (20K) Download Attachment
v2-0002-Don-t-call-data-type-input-functions-in-GUC-check.patch (11K) 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 Sun, Jun 23, 2019 at 07:21:02PM +0200, Peter Eisentraut wrote:
> Updated patch for that.

I have been looking at this patch set.  Patch 0001 looks good to me.
You are removing all traces of a set of timestamp keywords not
supported anymore, and no objections from my side for this cleanup.

+#define MAXPG_LSNCOMPONENT 8
+
 static bool
 check_recovery_target_lsn(char **newval, void **extra, GucSource source)
Let's avoid the duplication for the declarations.  I would suggest to
move the definitions of MAXPG_LSNLEN and MAXPG_LSNCOMPONENT to
pg_lsn.h.  Funny part, I was actually in need of this definition a
couple of days ago for a LSN string in a frontend tool.  I would
suggest renames at the same time:
- PG_LSN_LEN
- PG_LSN_COMPONENT

I think that should have a third definition for
"0123456789abcdefABCDEF", say PG_LSN_CHARACTERS, and we could have one
more for the separator '/'.

Avoiding the duplication between pg_lsn.c and guc.c is proving to be
rather ugly and reduces the readability within pg_lsn.c, so please let
me withdraw my previous objection.  (Looked at that part.)

-       if (strcmp(*newval, "epoch") == 0 ||
-           strcmp(*newval, "infinity") == 0 ||
-           strcmp(*newval, "-infinity") == 0 ||
Why do you remove these?  They should still be rejected because they
make no sense as recovery targets, no?

It may be worth mentioning that AdjustTimestampForTypmod() is not
duplicated because we don't care about the typmod in this case.
--
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-24 06:06, Michael Paquier wrote:
> -       if (strcmp(*newval, "epoch") == 0 ||
> -           strcmp(*newval, "infinity") == 0 ||
> -           strcmp(*newval, "-infinity") == 0 ||
> Why do you remove these?  They should still be rejected because they
> make no sense as recovery targets, no?

Yeah but the new code already rejects those anyway.  Note how
timestamptz_in() has explicit switch cases to accept those, and we
didn't carry those over into check_recovery_time().

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


Reply | Threaded
Open this post in threaded view
|

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Michael Paquier-2
On Mon, Jun 24, 2019 at 11:27:26PM +0200, Peter Eisentraut wrote:
> Yeah but the new code already rejects those anyway.  Note how
> timestamptz_in() has explicit switch cases to accept those, and we
> didn't carry those over into check_recovery_time().

Ditto.  I was not paying much attention to the code.  Your patch
indeed rejects anything else than DTK_DATE.  So we are good here,
sorry for the noise.
--
Michael

signature.asc (849 bytes) Download Attachment