On Mon, Mar 18, 2024 at 1:48 PM Cary Huang <cary.hu...@highgo.ca> wrote: > Attached is the v10 patch with the above changes. Thanks again for the review.
Awesome, looks good. On my final review pass I saw one last thing that bothered me (sorry for not seeing it before). The backend version of ASN1_TIME_to_timestamptz returns the following: > + return ((int64)days * 24 * 60 * 60) + (int64)seconds; ...but I think Timestamp[Tz]s are stored as microseconds, so we're off by a factor of a million. This still works because later we cast to double and pass it back through float8_timestamptz, which converts it: > + if (beentry->st_sslstatus->ssl_not_before != 0) > + values[25] = DirectFunctionCall1(float8_timestamptz, > + Float8GetDatum((double) > beentry->st_sslstatus->ssl_not_before)); But anyone who ends up inspecting the value of st_sslstatus->ssl_not_before directly is going to find an incorrect timestamp. I think it'd be clearer to store microseconds to begin with, and then just use TimestampTzGetDatum rather than the DirectFunctionCall1 we have now. (I looked for an existing implementation to reuse and didn't find one. Maybe we should use the overflow-aware multiplication/addition routines -- i.e. pg_mul_s64_overflow et al -- to multiply `days` and `seconds` by USECS_PER_DAY/USECS_PER_SEC and combine them.) And I think sslinfo can remain as-is, because that way overflow is caught by float8_timestamptz. WDYT? --Jacob
