Hi, Alexander! Thank you for working on this! On Thu, 12 Sept 2024 at 15:08, Aleksander Alekseev <aleksan...@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
