On Wed, Apr 10, 2024 at 1:27 PM Michael Paquier <mich...@paquier.xyz> wrote: > > On Tue, Apr 09, 2024 at 10:25:36AM +0000, Hayato Kuroda (Fujitsu) wrote: > >> On Fri, Apr 05, 2024 at 06:22:58AM +0000, Hayato Kuroda (Fujitsu) wrote: > >> I'm slightly annoyed by the fact that there is no check on > >> is_prim_bucket_same_wrt for buffer 1 in the BLK_NEEDS_REDO case to > >> show the symmetry between the insert and replay paths. Shouldn't > >> there be at least an assert for that in the branch when there are no > >> tuples (imagine an else to cover xldata->ntups == 0)? I mean between > >> just before updating the hash page's opaque area when > >> is_prev_bucket_same_wrt. > > > > Indeed, added an Assert() in else part. Was it same as your expectation? > > Yep, close enough. Thanks to the insert path we know that there will > be no tuples if (is_prim_bucket_same_wrt || is_prev_bucket_same_wrt), > and the replay path where the assertion is added. >
It is fine to have an assertion for this path. + else + { + /* + * See _hash_freeovflpage() which has a similar assertion when + * there are no tuples. + */ + Assert(xldata->is_prim_bucket_same_wrt || + xldata->is_prev_bucket_same_wrt); I can understand this comment as I am aware of this code but not sure it would be equally easy for the people first time looking at this code. One may try to find the equivalent assertion in _hash_freeovflpage(). The alternative could be: "Ensure that the required flags are set when there are no tuples. See _hash_freeovflpage().". I am also fine if you prefer to go with your proposed comment. Otherwise, the patch looks good to me. -- With Regards, Amit Kapila.