Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-07-04 Thread Michael Paquier
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 signature.asc Descripti

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-30 Thread Michael Paquier
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 g

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-30 Thread Peter Eisentraut
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_LSN

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-24 Thread Michael Paquier
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 th

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-24 Thread Peter Eisentraut
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,

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-23 Thread Michael Paquier
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. +#def

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-23 Thread Peter Eisentraut
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

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-20 Thread 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(). >

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-20 Thread Peter Eisentraut
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 i

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-13 Thread Peter Eisentraut
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

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-12 Thread Michael Paquier
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 ac

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-12 Thread Peter Eisentraut
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() an

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-11 Thread 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 20

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-11 Thread Tom Lane
Sergei Kornilov 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 t

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

2019-06-11 Thread Sergei Kornilov
Hello > That's not OK. hmm. Did you mean catching only needed errors by errcode? Something like attached? regards, Sergeidiff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 1208eb9a68..2993b8aa08 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/g