Re: [PATCH] Support Int64 GUCs

Hi, Aleksander!

Thank you for your work on this subject.

On Thu, Sep 12, 2024 at 2:08 PM Aleksander Alekseev
<aleksander(at)timescale(dot)com> wrote:
> Attached is a self-sufficient patch extracted from a larger patchset
> [1]. The entire patchset probably will not proceed further in the
> nearest future. Since there was interest in this particular patch it
> deserves being discussed in a separate thread.
>
> Currently we support 32-bit integer values in GUCs, but don't support
> 64-bit ones. The proposed patch adds this support.
>
> Firstly, it adds DefineCustomInt64Variable() which can be used by the
> extension authors.
>
> Secondly, the following core GUCs are made 64-bit:
>
> ```
> autovacuum_freeze_min_age
> autovacuum_freeze_max_age
> autovacuum_freeze_table_age
> autovacuum_multixact_freeze_min_age
> autovacuum_multixact_freeze_max_age
> autovacuum_multixact_freeze_table_age
> ```
>
> I see several open questions with the patch in its current state.
>
> Firstly, I'm not sure if it is beneficial to affect the named GUCs out
> of the context of the larger patchset. Perhaps we have better GUCs
> that could benefit from being 64-bit? Or should we just leave alone
> the core GUCs and focus on providing DefineCustomInt64Variable() ?

It doesn't look like these *_age GUCs could benefit from being 64-bit,
before 64-bit transaction ids get in. However, I think there are some
better candidates.

autovacuum_vacuum_threshold
autovacuum_vacuum_insert_threshold
autovacuum_analyze_threshold

This GUCs specify number of tuples before vacuum/analyze. That could
be more than 2^31. With large tables of small tuples, I can't even
say this is always impractical to have values greater than 2^31.

> Secondly, DefineCustomInt64Variable() is not test-covered. Turned out
> it was not even defined (although declared) in the original patch.
> This was fixed in the attached version. Maybe one of the test modules
> could use it even if it makes little sense for this particular module?
> For instance, test/modules/worker_spi/ could use it for
> worker_spi.naptime.

I don't think there are good candidates among existing extension GUCs.
I think we could add something for pure testing purposes somewhere in
src/test/modules.

> Last but not least, large values like 12345678912345 could be
> difficult to read. Perhaps we should also support 12_345_678_912_345
> syntax? This is not implemented in the attached patch and arguably
> could be discussed separately when and if we merge it.

I also think we're good with 12345678912345 so far.

------
Regards,
Alexander Korotkov
Supabase