Re: [PATCH] Support Int64 GUCs

Hi, Aleksander Alekseev

Thanks for updating the patch.

> On Sep 24, 2024, at 17:27, Aleksander Alekseev <aleksander(at)timescale(dot)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