(Adding Tom in CC, in case.) On Tue, Oct 18, 2022 at 04:35:33PM -0400, Corey Huinker wrote: > I agree that we shouldn't spend too much effort on a good error message > here, but perhaps we should have the message mention that it is > date/time-related? A person git-grepping for this error message will get 4 > hits in .c files (date.c, timestamp.c, varbit.c, varchar.c) so even a > slight variation in the message could save them some time.
The message is the same between HEAD and the patch and these have been around for a long time, except that we would see it at parsing time on HEAD, and at executor time with the patch. I would not mind changing if there are better ideas than what's used now, of course ;) > This is an extreme nitpick, but the patchset seems like it should have been > 1 file or 3 (remove name functions, remove time functions, remove > SQLValueFunction infrastructure), but that will only matter in the unlikely > case that we find a need for SQLValueFunction but we want to leave the > timestamp function as COERCE_SQL_SYNTAX. Once the timestamp functions are removed, SQLValueFunction is just dead code so including its removal in 0002 does not change much in my opinion. An other thing I had on my list for this patch was to check its performance impact. So I have spent some time having a look at the perf profiles produced on HEAD and with the patch using queries like SELECT current_role FROM generate_series(1,N) where N > 10M and I have not noticed any major differences in runtime or in the profiles, at the difference that we don't have anymore SQLValueFunction() and its internal functions called, hence they are missing from the stacks, but that's the whole point of the patch. With this in mind, would somebody complain if I commit that? That's a nice reduction in code, while completing the work done in 40c24bf: 25 files changed, 338 insertions(+), 477 deletions(-) Thanks, -- Michael
signature.asc
Description: PGP signature
