(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
