On Mon, 8 Jul 2024 at 14:50, jian he <jian.universality@gmail.com> wrote:
>
> looks good to me.
Thanks for looking.
> /*
> * Protect against overflows in timestamp_mi. XXX convert to
> * ereturn one day?
> */
> if (!TIMESTAMP_NOT_FINITE(start) && !TIMESTAMP_NOT_FINITE(finish) &&
> !pg_sub_s64_overflow(finish, start, &dummy))
>
> i don't understand the comment "XXX convert to ereturn one day?".
The problem I'm trying to work around there is that timestamp_mi
raises an ERROR if there's an overflow. I don't want the support
function to cause an ERROR so I'm trying to only call timestamp_mi in
cases where it won't error. The ereturn mention is a reference to
ERROR raising infrastructure added by d9f7f5d32 and so far only used
by input functions. It would be possible to use that to save from
having to do the pg_sub_s64_overflow(). Instead, we could check if
any errors were found and only proceed with the remaining part of the
calculation if none were found.
I've tried to improve the comment in the attached version. I removed
the reference to ereturn.
> do we need to add unlikely for "pg_sub_s64_overflow", i saw most of
> pg_sub_s64_overflow have unlikely.
I was hoping the condition would be likely() rather than unlikely().
However, I didn't consider that the code path was hot enough for it to
matter. It's just a function we call once during planning if we find a
call to generate_series_timestamp(). It's not like it's called a
million or a billion times during execution like a function such as
int4pl() could be.
David