Thread: pgsql: Fix more holes with SLRU code in need of int64 for segment numbe
Fix more holes with SLRU code in need of int64 for segment numbers This is a continuation of 3937cadfd438, taking care of more areas I have managed to miss previously. Reported-by: Noah Misch Reviewed-by: Noah Misch Discussion: https://postgr.es/m/20240724130059.1f.nmisch@google.com Backpatch-through: 17 Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/c9e24573905bef7fc3e4efb02bdb4d0cc8e43c51 Modified Files -------------- src/backend/access/transam/multixact.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)
Re: pgsql: Fix more holes with SLRU code in need of int64 for segment numbe
From
Peter Eisentraut
Date:
On 27.07.24 00:24, Michael Paquier wrote: > Fix more holes with SLRU code in need of int64 for segment numbers > > This is a continuation of 3937cadfd438, taking care of more areas I have > managed to miss previously. > > Reported-by: Noah Misch > Reviewed-by: Noah Misch > Discussion: https://postgr.es/m/20240724130059.1f.nmisch@google.com > Backpatch-through: 17 > > Branch > ------ > master > > Details > ------- > https://git.postgresql.org/pg/commitdiff/c9e24573905bef7fc3e4efb02bdb4d0cc8e43c51 I don't understand this patch. The previous patches that this references changed various variables to int64 and made adjustments following from that. But this patch takes variables and function results that are of type int and casts them to unsigned long long before printing. I don't see what that accomplishes, and it's not clear based on just the explanation that this is a continuation of a previous patch that doesn't do that. Is there a plan to change these things to int64 as well at some point?
Re: pgsql: Fix more holes with SLRU code in need of int64 for segment numbe
From
Alexander Korotkov
Date:
On Wed, Aug 7, 2024 at 12:07 PM Peter Eisentraut <peter@eisentraut.org> wrote: > On 27.07.24 00:24, Michael Paquier wrote: > > Fix more holes with SLRU code in need of int64 for segment numbers > > > > This is a continuation of 3937cadfd438, taking care of more areas I have > > managed to miss previously. > > > > Reported-by: Noah Misch > > Reviewed-by: Noah Misch > > Discussion: https://postgr.es/m/20240724130059.1f.nmisch@google.com > > Backpatch-through: 17 > > > > Branch > > ------ > > master > > > > Details > > ------- > > https://git.postgresql.org/pg/commitdiff/c9e24573905bef7fc3e4efb02bdb4d0cc8e43c51 > > I don't understand this patch. The previous patches that this > references changed various variables to int64 and made adjustments > following from that. But this patch takes variables and function > results that are of type int and casts them to unsigned long long before > printing. I don't see what that accomplishes, and it's not clear based > on just the explanation that this is a continuation of a previous patch > that doesn't do that. Is there a plan to change these things to int64 > as well at some point? There is a plan indeed. The patchset [1] should include conversion multixacts to 64-bit (It surely included that in earlier versions, I didn't look the last versions though). I doubt this will be ready for v18. So this commit might be quite preliminary. But I would prefer to leave it there as soon as it has already landed. Opinions? Links. 1. https://www.postgresql.org/message-id/CAJ7c6TND0bCnwU1SmxTsFewK4XJGBep343vf%2BT%2BGQ-a5S5hC0w%40mail.gmail.com ------ Regards, Alexander Korotkov Supabase
Re: pgsql: Fix more holes with SLRU code in need of int64 for segment numbe
From
Peter Eisentraut
Date:
On 07.08.24 17:53, Alexander Korotkov wrote: > On Wed, Aug 7, 2024 at 12:07 PM Peter Eisentraut <peter@eisentraut.org> wrote: >> On 27.07.24 00:24, Michael Paquier wrote: >>> Fix more holes with SLRU code in need of int64 for segment numbers >>> >>> This is a continuation of 3937cadfd438, taking care of more areas I have >>> managed to miss previously. >>> >>> Reported-by: Noah Misch >>> Reviewed-by: Noah Misch >>> Discussion: https://postgr.es/m/20240724130059.1f.nmisch@google.com >>> Backpatch-through: 17 >>> >>> Branch >>> ------ >>> master >>> >>> Details >>> ------- >>> https://git.postgresql.org/pg/commitdiff/c9e24573905bef7fc3e4efb02bdb4d0cc8e43c51 >> >> I don't understand this patch. The previous patches that this >> references changed various variables to int64 and made adjustments >> following from that. But this patch takes variables and function >> results that are of type int and casts them to unsigned long long before >> printing. I don't see what that accomplishes, and it's not clear based >> on just the explanation that this is a continuation of a previous patch >> that doesn't do that. Is there a plan to change these things to int64 >> as well at some point? > > There is a plan indeed. The patchset [1] should include conversion > multixacts to 64-bit (It surely included that in earlier versions, I > didn't look the last versions though). I doubt this will be ready for > v18. So this commit might be quite preliminary. But I would prefer > to leave it there as soon as it has already landed. Opinions? I think you should change the output formats at the same time as you change the variable types. That way the compiler can cross-check this. Otherwise, if you later forget to change a variable, these casts will hide it. Or if the future patches turn out differently, then we have this useless code. > Links. > 1. https://www.postgresql.org/message-id/CAJ7c6TND0bCnwU1SmxTsFewK4XJGBep343vf%2BT%2BGQ-a5S5hC0w%40mail.gmail.com It looks like the commit I'm talking about here is a subset of v55-0001 from that thread? So why is some of this being committed now into v17? But as I wrote above, I think this approach is a bad idea.
Re: pgsql: Fix more holes with SLRU code in need of int64 for segment numbe
From
Alexander Korotkov
Date:
On Wed, Aug 7, 2024 at 10:52 PM Peter Eisentraut <peter@eisentraut.org> wrote: > On 07.08.24 17:53, Alexander Korotkov wrote: > > On Wed, Aug 7, 2024 at 12:07 PM Peter Eisentraut <peter@eisentraut.org> wrote: > >> On 27.07.24 00:24, Michael Paquier wrote: > >>> Fix more holes with SLRU code in need of int64 for segment numbers > >>> > >>> This is a continuation of 3937cadfd438, taking care of more areas I have > >>> managed to miss previously. > >>> > >>> Reported-by: Noah Misch > >>> Reviewed-by: Noah Misch > >>> Discussion: https://postgr.es/m/20240724130059.1f.nmisch@google.com > >>> Backpatch-through: 17 > >>> > >>> Branch > >>> ------ > >>> master > >>> > >>> Details > >>> ------- > >>> https://git.postgresql.org/pg/commitdiff/c9e24573905bef7fc3e4efb02bdb4d0cc8e43c51 > >> > >> I don't understand this patch. The previous patches that this > >> references changed various variables to int64 and made adjustments > >> following from that. But this patch takes variables and function > >> results that are of type int and casts them to unsigned long long before > >> printing. I don't see what that accomplishes, and it's not clear based > >> on just the explanation that this is a continuation of a previous patch > >> that doesn't do that. Is there a plan to change these things to int64 > >> as well at some point? > > > > There is a plan indeed. The patchset [1] should include conversion > > multixacts to 64-bit (It surely included that in earlier versions, I > > didn't look the last versions though). I doubt this will be ready for > > v18. So this commit might be quite preliminary. But I would prefer > > to leave it there as soon as it has already landed. Opinions? > > I think you should change the output formats at the same time as you > change the variable types. That way the compiler can cross-check this. > Otherwise, if you later forget to change a variable, these casts will > hide it. Or if the future patches turn out differently, then we have > this useless code. > > > Links. > > 1. https://www.postgresql.org/message-id/CAJ7c6TND0bCnwU1SmxTsFewK4XJGBep343vf%2BT%2BGQ-a5S5hC0w%40mail.gmail.com > > It looks like the commit I'm talking about here is a subset of v55-0001 > from that thread? Yes, looks like this. > So why is some of this being committed now into v17? > But as I wrote above, I think this approach is a bad idea. OK, I agree that might look annoying. So, it's better to revert now. Michael, what do you think? ------ Regards, Alexander Korotkov Supabase