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: /messages/by-id/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(-)
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: /messages/by-id/20240724130059.1f.nmisch@google.com
Backpatch-through: 17Branch
------
masterDetails
-------
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?
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: /messages/by-id/20240724130059.1f.nmisch@google.com
Backpatch-through: 17Branch
------
masterDetails
-------
https://git.postgresql.org/pg/commitdiff/c9e24573905bef7fc3e4efb02bdb4d0cc8e43c51I 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. /messages/by-id/CAJ7c6TND0bCnwU1SmxTsFewK4XJGBep343vf+T+GQ-a5S5hC0w@mail.gmail.com
------
Regards,
Alexander Korotkov
Supabase
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: /messages/by-id/20240724130059.1f.nmisch@google.com
Backpatch-through: 17Branch
------
masterDetails
-------
https://git.postgresql.org/pg/commitdiff/c9e24573905bef7fc3e4efb02bdb4d0cc8e43c51I 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. /messages/by-id/CAJ7c6TND0bCnwU1SmxTsFewK4XJGBep343vf+T+GQ-a5S5hC0w@mail.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.
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: /messages/by-id/20240724130059.1f.nmisch@google.com
Backpatch-through: 17Branch
------
masterDetails
-------
https://git.postgresql.org/pg/commitdiff/c9e24573905bef7fc3e4efb02bdb4d0cc8e43c51I 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. /messages/by-id/CAJ7c6TND0bCnwU1SmxTsFewK4XJGBep343vf+T+GQ-a5S5hC0w@mail.gmail.comIt 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
On Aug 8, 2024, at 5:05, Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Wed, Aug 7, 2024 at 10:52 PM Peter Eisentraut <peter@eisentraut.org> wrote: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?
The argument is two-fold here. The point of this change is that we were forcibly doing a cast to int with int64 values returned, so this commit limits the risks of missing paths in the future, while being consistent with all the SLRU code marking segment numbers with int64 for short *and* long segment file names.
So at the end, I’d rather let the code as it is now, and keep a line of marking all the segment numbers with int64 and be consistent with what all the SLRU internals think what segment numbers should be. This is also the argument of Noah’s upthread as far as I understand (Noah, feel free to correct me if you think differently).
(I’m without laptop access for quite a few days)
--
Michael
Import Notes
Resolved by subject fallback
On 08.08.24 01:15, Michael Paquier wrote:
On Aug 8, 2024, at 5:05, Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Wed, Aug 7, 2024 at 10:52 PM Peter Eisentraut <peter@eisentraut.org> wrote: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?The argument is two-fold here. The point of this change is that we were forcibly doing a cast to int with int64 values returned, so this commit limits the risks of missing paths in the future, while being consistent with all the SLRU code marking segment numbers with int64 for short *and* long segment file names.
No, this is not what *this* patch does. (I suppose some of the related
patches might be doing that.) This patch just casts a few things that
are int to unsigned long long int before printing them.
On Fri, Aug 9, 2024 at 4:09 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 08.08.24 01:15, Michael Paquier wrote:
On Aug 8, 2024, at 5:05, Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Wed, Aug 7, 2024 at 10:52 PM Peter Eisentraut <peter@eisentraut.org> wrote: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?The argument is two-fold here. The point of this change is that we were forcibly doing a cast to int with int64 values returned, so this commit limits the risks of missing paths in the future, while being consistent with all the SLRU code marking segment numbers with int64 for short *and* long segment file names.
No, this is not what *this* patch does. (I suppose some of the related
patches might be doing that.) This patch just casts a few things that
are int to unsigned long long int before printing them.
As pointed by Noah Misch [1], unlike the commit the patch [2] also
changed segment-returning functions to return int64. Thus, in the
patch output formats make much more sense, because they match the
input data types. Michael, are you intended to push the remaining
part of the patch [2]?
Links
1. /messages/by-id/20240810175055.cd.nmisch@google.com
2. /messages/by-id/ZqGvzSbW5TGKqZcE@paquier.xyz
------
Regards,
Alexander Korotkov
Supabase
On Aug 13, 2024, at 6:35, Alexander Korotkov <aekorotkov@gmail.com> wrote:.
As pointed by Noah Misch [1], unlike the commit the patch [2] also
changed segment-returning functions to return int64. Thus, in the
patch output formats make much more sense, because they match the
input data types. Michael, are you intended to push the remaining
part of the patch [2]?
Guess so. I could look at that next week, not before.
--
Michael
On 12.08.24 23:35, Alexander Korotkov wrote:
On Fri, Aug 9, 2024 at 4:09 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 08.08.24 01:15, Michael Paquier wrote:
On Aug 8, 2024, at 5:05, Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Wed, Aug 7, 2024 at 10:52 PM Peter Eisentraut <peter@eisentraut.org> wrote: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?The argument is two-fold here. The point of this change is that we were forcibly doing a cast to int with int64 values returned, so this commit limits the risks of missing paths in the future, while being consistent with all the SLRU code marking segment numbers with int64 for short *and* long segment file names.
No, this is not what *this* patch does. (I suppose some of the related
patches might be doing that.) This patch just casts a few things that
are int to unsigned long long int before printing them.As pointed by Noah Misch [1], unlike the commit the patch [2] also
changed segment-returning functions to return int64. Thus, in the
patch output formats make much more sense, because they match the
input data types. Michael, are you intended to push the remaining
part of the patch [2]?Links
1. /messages/by-id/20240810175055.cd.nmisch@google.com
2. /messages/by-id/ZqGvzSbW5TGKqZcE@paquier.xyz
Ah yes, it makes sense together with [2].
On Tue, Aug 13, 2024 at 1:30 AM Michael Paquier <michael@paquier.xyz> wrote:
On Aug 13, 2024, at 6:35, Alexander Korotkov <aekorotkov@gmail.com> wrote:.
As pointed by Noah Misch [1], unlike the commit the patch [2] also
changed segment-returning functions to return int64. Thus, in the
patch output formats make much more sense, because they match the
input data types. Michael, are you intended to push the remaining
part of the patch [2]?Guess so. I could look at that next week, not before.
OK. I made an attached patch from the missing parts.
Do you mind if I push that? If yes, feel free to use it.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
v1-0001-Fix-even-more-holes-with-SLRU-code-in-need-of-int.patchapplication/octet-stream; name=v1-0001-Fix-even-more-holes-with-SLRU-code-in-need-of-int.patchDownload+7-8
On Wed, Aug 14, 2024 at 5:13 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Tue, Aug 13, 2024 at 1:30 AM Michael Paquier <michael@paquier.xyz> wrote:
On Aug 13, 2024, at 6:35, Alexander Korotkov <aekorotkov@gmail.com> wrote:.
As pointed by Noah Misch [1], unlike the commit the patch [2] also
changed segment-returning functions to return int64. Thus, in the
patch output formats make much more sense, because they match the
input data types. Michael, are you intended to push the remaining
part of the patch [2]?Guess so. I could look at that next week, not before.
OK. I made an attached patch from the missing parts.
Do you mind if I push that? If yes, feel free to use it.
I mean, I'd like to push it if you don't object.
If you object and like to push it yourself, feel free to use this patch.
------
Regards,
Alexander Korotkov
Supabase
On Aug 15, 2024, at 6:26, Alexander Korotkov <aekorotkov@gmail.com> wrote:
I mean, I'd like to push it if you don't object.
If you object and like to push it yourself, feel free to use this patch.
I’d like to take a look at what you have here to get an opinion. Could you wait for a few days?
--
Michael
On Thu, Aug 15, 2024 at 2:07 AM Michael Paquier <michael@paquier.xyz> wrote:
On Aug 15, 2024, at 6:26, Alexander Korotkov <aekorotkov@gmail.com> wrote:
I mean, I'd like to push it if you don't object.
If you object and like to push it yourself, feel free to use this patch.I’d like to take a look at what you have here to get an opinion. Could you wait for a few days?
Sure, NP.
------
Regards,
Alexander Korotkov
Supabase
On Thu, Aug 15, 2024 at 03:40:12AM +0300, Alexander Korotkov wrote:
On Thu, Aug 15, 2024 at 2:07 AM Michael Paquier <michael@paquier.xyz> wrote:
I’d like to take a look at what you have here to get an opinion. Could you wait for a few days?
Sure, NP.
My apologies for the delay. I have double-checked my reflog history,
and I've messed up two `git add` while rebasing.
I've spent some time rechecking the whole area, and finished with the
same patch as what you have attached at [1]/messages/by-id/CAPpHfdui0-GmLd6V-H8RhcaytyLZfR2p6QG+E_LTsD_i=bQ6NA@mail.gmail.com -- Michael. As I'm responsible for
the mistake, I have handled that myself. Thanks for the pokes!
For the archives, Noah has mentioned the same here:
/messages/by-id/20240810175055.cd.nmisch@google.com
[1]: /messages/by-id/CAPpHfdui0-GmLd6V-H8RhcaytyLZfR2p6QG+E_LTsD_i=bQ6NA@mail.gmail.com -- Michael
--
Michael