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

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

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,Whena 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
awarning in the log when the table age exceeds the int maximum. These default values we can set a reasonable values
,forexample 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. https://www.postgresql.org/message-id/3649727.1727276882%40sss.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>

Pavel Borisov <pashkin.elfe@gmail.com> writes:
> I see Aleksander worked on this patch (with many other hackers) and marked
> it as rejected. And now Evgeniy, a new developer wants to continue and
> rebase this patch and also the patches in another 64-xid thread
> disregarding the fact that the patch is rejected. It's not a crime. It's
> rejected, true.

No, but it's a waste of time.  Just rebasing the patch does nothing to
counter the arguments that made us reject it before.

            regards, tom lane