Re: check_recovery_target_lsn() does a PG_CATCH without a throw

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