Thread: check_recovery_target_lsn() does a PG_CATCH without a throw
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] CALDaNm1KXK9gbZfY-p_peRFm_XrBh1OwQO1Kk6Gig0c0fVZ2uw@mail.gmail.com
Hello > That's not OK. hmm. Did you mean catching only needed errors by errcode? Something like attached? regards, Sergei
Attachment
Sergei Kornilov <sk@zsrv.org> 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
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
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
Attachment
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
Attachment
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
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
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
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
Attachment
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
Attachment
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
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
Attachment
This has been committed. On 2019-06-24 06:06, Michael Paquier wrote: > 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 ended up rewriting this by extracting the parsing code into pg_lsn_in_internal() and having both pg_lsn_in() and check_recovery_target_lsn() calling it. This mirrors similar arrangements in float.c -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Jun 30, 2019 at 11:06:58AM +0200, Peter Eisentraut wrote: > I ended up rewriting this by extracting the parsing code into > pg_lsn_in_internal() and having both pg_lsn_in() and > check_recovery_target_lsn() calling it. This mirrors similar > arrangements in float.c The refactoring looks good to me (including what you have just fixed with PG_RETURN_LSN). Thanks for considering it. -- Michael
Attachment
On Sun, Jun 30, 2019 at 09:35:52PM +0900, Michael Paquier wrote: > The refactoring looks good to me (including what you have just fixed > with PG_RETURN_LSN). Thanks for considering it. This issue was still listed as an open item for v12, so I have removed it. -- Michael