[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 be non-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't remember 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 02:08:15PM +0300, Aleksander Alekseev wrote:

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 don't understand why we would want to make these GUCs 64-bit. All of the
allowed values fit in an int32, so AFAICT this would only serve to mislead
users into thinking they could set these much higher than they can/should.

TBH I'm quite skeptical that this would even be particularly useful for
extension authors. In what cases would a floating point value not suffice?
I'm not totally opposed to the idea of 64-bit GUCs, but I'd like more
information about the motivation.

--
nathan

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 be non-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

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, 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.

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]https://www.postgresql.org/docs/current/functions-admin.html#FUNCTIONS-ADVISORY-LOCKS. I modified the module to use Int64's.

I guess it may also answer Nathan's question.

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.

Fair enough.

PFA the updated patch.

[1]: https://www.postgresql.org/docs/current/functions-admin.html#FUNCTIONS-ADVISORY-LOCKS

--
Best regards,
Aleksander Alekseev

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]https://www.postgresql.org/docs/current/datatype-numeric.html.

```
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

Hi,

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?

No, you found a bug. The patch didn't change ConfigureNamesInt64[]
thus these GUCs were still treated as int32s.

Here is the corrected patch v3. Thanks!

=# select * from pg_settings where name = 'autovacuum_vacuum_threshold';
-[ RECORD 1 ]---+------------------------------------------------------------
name | autovacuum_vacuum_threshold
setting | 1234605616436508552
unit |
category | Autovacuum
short_desc | Minimum number of tuple updates or deletes prior to vacuum.
extra_desc |
context | sighup
vartype | int64
source | configuration file
min_val | 0
max_val | 9223372036854775807
enumvals |
boot_val | 50
reset_val | 1234605616436508552
sourcefile | /Users/eax/pginstall/data-master/postgresql.conf
sourceline | 664
pending_restart | f

--
Best regards,
Aleksander Alekseev

On Sep 25, 2024, at 19:03, Aleksander Alekseev <aleksander@timescale.com> wrote:

Hi,

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?

No, you found a bug. The patch didn't change ConfigureNamesInt64[]
thus these GUCs were still treated as int32s.

Here is the corrected patch v3. Thanks!

=# select * from pg_settings where name = 'autovacuum_vacuum_threshold';
-[ RECORD 1 ]---+------------------------------------------------------------
name | autovacuum_vacuum_threshold
setting | 1234605616436508552
unit |
category | Autovacuum
short_desc | Minimum number of tuple updates or deletes prior to vacuum.
extra_desc |
context | sighup
vartype | int64
source | configuration file
min_val | 0
max_val | 9223372036854775807
enumvals |
boot_val | 50
reset_val | 1234605616436508552
sourcefile | /Users/eax/pginstall/data-master/postgresql.conf
sourceline | 664
pending_restart | f

Thanks for updating the patch.

After testing the v3 patch, I found it cannot correctly handle the number with underscore.

See:

```
postgres=# alter system set autovacuum_vacuum_threshold to 2_147_483_648;
ERROR: invalid value for parameter "autovacuum_vacuum_threshold": "2_147_483_648"
postgres=# alter system set autovacuum_vacuum_threshold to 2_147_483_647;
ALTER SYSTEM
```

IIRC, the lexer only supports integers but not int64.

--
Best regards,
Japin Li

Hi,

```
postgres=# alter system set autovacuum_vacuum_threshold to 2_147_483_648;
ERROR: invalid value for parameter "autovacuum_vacuum_threshold": "2_147_483_648"
postgres=# alter system set autovacuum_vacuum_threshold to 2_147_483_647;
ALTER SYSTEM
```

IIRC, the lexer only supports integers but not int64.

Right. Supporting underscores for GUCs was discussed above but not
implemented in the patch. As Alexander rightly pointed out this is not
a priority and can be discussed separately.

--
Best regards,
Aleksander Alekseev

FWIW, I agree with the upthread opinions that we shouldn't do this
(invent int64 GUCs). I don't think we need the added code bloat
and risk of breaking user code that isn't expecting this new GUC
type. We invented the notion of GUC units in part to ensure that
int32 GUCs could be adapted to handle potentially-large numbers.
And there's always the fallback position of using a float8 GUC
if you really feel you need a wider range.

regards, tom lane

Hi, Tom!

On Wed, Sep 25, 2024 at 6:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

FWIW, I agree with the upthread opinions that we shouldn't do this
(invent int64 GUCs). I don't think we need the added code bloat
and risk of breaking user code that isn't expecting this new GUC
type. We invented the notion of GUC units in part to ensure that
int32 GUCs could be adapted to handle potentially-large numbers.
And there's always the fallback position of using a float8 GUC
if you really feel you need a wider range.

Thank you for your feedback.
Do you think we don't need int64 GUCs just now, when 64-bit
transaction ids are far from committable shape? Or do you think we
don't need int64 GUCs even if we have 64-bit transaction ids? If yes,
what do you think we should use for *_age variables with 64-bit
transaction ids?

------
Regards,
Alexander Korotkov
Supabase

Hi Alexander
I think we need int64 GUCs, due to these parameters（
autovacuum_freeze_table_age, autovacuum_freeze_max_age,When a table age is
greater than any of these parameters an aggressive vacuum will be
performed, When we implementing xid64, is it still necessary to be in the
int range? btw, I have a suggestion to record a warning in the log when the
table age exceeds the int maximum. These default values we can set a
reasonable values ,for example autovacuum_freeze_max_age=4294967295 or
8589934592.

Thanks

Alexander Korotkov <aekorotkov@gmail.com> 于2024年9月26日周四 02:05写道：

On Thu, Sep 26, 2024 at 12:30 PM wenhui qiu <qiuwenhuifx@gmail.com> wrote:

I think we need int64 GUCs, due to these parameters（ autovacuum_freeze_table_age, autovacuum_freeze_max_age,When a table age is greater than any of these parameters an aggressive vacuum will be performed, When we implementing xid64, is it still necessary to be in the int range? btw, I have a suggestion to record a warning in the log when the table age exceeds the int maximum. These default values we can set a reasonable values ,for example autovacuum_freeze_max_age=4294967295 or 8589934592.

In principle, even with 64-bit transaction ids we could specify *_age
GUCs as int32 with bigger units or as float8. That feels a bit
awkward for me. This is why I queried more about Tom's opinion in
more details: did he propose to wait with int64 GUCs before we have
64-bit transaction ids, or give up about them completely?

Links.
1. /messages/by-id/3649727.1727276882@sss.pgh.pa.us

------
Regards,
Alexander Korotkov
Supabase

Alexander Korotkov <aekorotkov@gmail.com> writes:

Do you think we don't need int64 GUCs just now, when 64-bit
transaction ids are far from committable shape? Or do you think we
don't need int64 GUCs even if we have 64-bit transaction ids? If yes,
what do you think we should use for *_age variables with 64-bit
transaction ids?

I seriously doubt that _age values exceeding INT32_MAX would be
useful, even in the still-extremely-doubtful situation that we
get to true 64-bit XIDs. But if you think we must have that,
we could still use float8 GUCs for them. float8 is exact up
to 2^53 (given IEEE math), and you certainly aren't going to
convince me that anyone needs _age values exceeding that.
For that matter, an imprecise representation of such an age
limit would still be all right wouldn't it?

regards, tom lane

Hi,

I seriously doubt that _age values exceeding INT32_MAX would be
useful, even in the still-extremely-doubtful situation that we
get to true 64-bit XIDs. But if you think we must have that,
we could still use float8 GUCs for them. float8 is exact up
to 2^53 (given IEEE math), and you certainly aren't going to
convince me that anyone needs _age values exceeding that.
For that matter, an imprecise representation of such an age
limit would still be all right wouldn't it?

Considering the recent feedback. I'm marking the corresponding CF
entry as "Rejected".

Thanks to everyone involved!

--
Best regards,
Aleksander Alekseev

Hi hackers!

Upgraded the "Int64 GUC" patch in order to conform to f3f06b13308e3
updates. Rebased and tested upon the current master (3f9b9621766). The
patch is still needed to be up to date as a part of the xid64 solution.

Best regards,
Evgeny Voropaev,
TantorLabs, LLC.
<evorop@gmail.com>
<evgeny.voropaev@tantorlabs.ru>