Re: check_recovery_target_lsn() does a PG_CATCH without a throw

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