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
Description: PGP signature
