Thread: Re: [PATCH] Support Int64 GUCs
Hi, Alexander!
Thank you for working on this!
On Thu, 12 Sept 2024 at 15:08, Aleksander Alekseev <aleksander@timescale.com> wrote:
Hi,
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() ?
I think the direction is good and delivering 64-bit GUCs is very much worth committing.
The patch itself looks good, but we could need to add locks against concurrently modifying 64-bit values, which could be non-atomic on older architectures.
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.
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 think 12345678912345 is good enough. Underscore dividers make reading little bit easier but look weird overall. I can't remember other places where we output long numbers with dividers.
Regards,
Pavel Borisov
Supabase
Hi Pavel, > I think the direction is good and delivering 64-bit GUCs is very much worth committing. > The patch itself looks good, but we could need to add locks against concurrently modifying 64-bit values, which could benon-atomic on older architectures. Thanks for the feedback. > I think 12345678912345 is good enough. Underscore dividers make reading little bit easier but look weird overall. I can'tremember other places where we output long numbers with dividers. We already support this in SQL: psql (18devel) =# SELECT 123_456; ?column? ---------- 123456 -- Best regards, Aleksander Alekseev
On Thu, Sep 12, 2024 at 2:29 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > Hi, Alexander! > Thank you for working on this! > > On Thu, 12 Sept 2024 at 15:08, Aleksander Alekseev <aleksander@timescale.com> wrote: >> >> Hi, >> >> 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() ? > > I think the direction is good and delivering 64-bit GUCs is very much worth committing. > The patch itself looks good, but we could need to add locks against concurrently modifying 64-bit values, which could benon-atomic on older architectures. GUCs are located in the local memory. No concurrent read/writes of them are possible. It might happen that SIGHUP comes during read/write of the GUC variable. But, that's protected the other way: SignalHandlerForConfigReload() just sets the ConfigReloadPending flag, which is processed during CHECK_FOR_INTERRUPTS(). So, I don't see a need to locks here. ------ Regards, Alexander Korotkov Supabase