Thread: 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@timescale.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
Hi, > PFA the updated patch. It is worth mentioning that v2 should not be merged as is. Particularly although it changes the following GUCs: > autovacuum_vacuum_threshold > autovacuum_vacuum_insert_threshold > autovacuum_analyze_threshold ... it doesn't affect the code that uses these GUCs which results in casting int64s to ints. I would appreciate a bit more feedback on v2. If the community is fine with modifying these GUCs I will correct the patch in this respect. -- Best regards, Aleksander Alekseev
Hi, Aleksander Alekseev
Thanks for updating the patch.
> On Sep 24, 2024, at 17:27, Aleksander Alekseev <aleksander@timescale.com> wrote:
>
> Hi, Alexander!
>
>> Thank you for your work on this subject.
>
> Thanks for your feedback.
>
>> 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.
>
> Sounds good to me. Fixed.
I found the autovacuum_vacuum_threshold, autovacuum_vacuum_insert_threshold
and autovacuum_analyze_threshold is change to int64 for relation option,
however the GUCs are still integers.
```
postgres=# select * from pg_settings where name = 'autovacuum_vacuum_threshold' \gx
-[ RECORD 1 ]---+------------------------------------------------------------
name | autovacuum_vacuum_threshold
setting | 50
unit |
category | Autovacuum
short_desc | Minimum number of tuple updates or deletes prior to vacuum.
extra_desc |
context | sighup
vartype | integer
source | default
min_val | 0
max_val | 2147483647
enumvals |
boot_val | 50
reset_val | 50
sourcefile |
sourceline |
pending_restart | f
```
Is there something I missed?
>
>>> 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.
>
> I found a great candidate in src/test/modules/delay_execution:
>
> ```
> DefineCustomIntVariable("delay_execution.post_planning_lock_id",
> "Sets the advisory lock ID to be
> locked/unlocked after planning.",
> ```
>
> Advisory lock IDs are bigints [1]. I modified the module to use Int64's.
I check the delay_execution.post_planning_lock_id parameter, and it’s varitype
is int64, maybe bigint is better, see [1].
```
postgres=# select * from pg_settings where name = 'delay_execution.post_planning_lock_id' \gx
-[ RECORD 1 ]---+----------------------------------------------------------------
name | delay_execution.post_planning_lock_id
setting | 0
unit |
category | Customized Options
short_desc | Sets the advisory lock ID to be locked/unlocked after planning.
extra_desc | Zero disables the delay.
context | user
vartype | int64
source | default
min_val | 0
max_val | 9223372036854775807
enumvals |
boot_val | 0
reset_val | 0
sourcefile |
sourceline |
pending_restart | f
```
[1] https://www.postgresql.org/docs/current/datatype-numeric.html
--
Regrads,
Japin Li
On Tue, Sep 24, 2024 at 12:27:20PM +0300, Aleksander Alekseev wrote:
>> 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.
>
> [...]
>
> I found a great candidate in src/test/modules/delay_execution:
>
> ```
> DefineCustomIntVariable("delay_execution.post_planning_lock_id",
> "Sets the advisory lock ID to be
> locked/unlocked after planning.",
> ```
>
> Advisory lock IDs are bigints [1]. I modified the module to use Int64's.
>
> I guess it may also answer Nathan's question.
Hm. I'm not sure I find any of these to be particularly convincing
examples of why we need int64 GUCs. Yes, the GUCs in question could
potentially be set to higher values, but I've yet to hear of this being a
problem in practice. We might not want to encourage such high values,
either.
--
nathan