Re: [PATCH] Support Int64 GUCs - Mailing list pgsql-hackers

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