On Fri, Nov 10, 2023 at 9:17 AM Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.com> wrote: > > > Next we should add some test codes. I will continue considering but please > > post > > anything > > If you have idea. > > And I did, PSA the patch. This patch adds two parts in hash_index.sql. > > In the first part, the primary bucket page is filled by live tuples and some > overflow > pages are by dead ones. This leads removal of overflow pages without moving > tuples. > HEAD will crash if this were executed, which is already reported on hackers. >
+-- And do CHECKPOINT and vacuum. Some overflow pages will be removed. +CHECKPOINT; +VACUUM hash_cleanup_heap; Why do we need CHECKPOINT in the above test? > The second one tests a case (ntups == 0 && is_prim_bucket_same_wrt == false && > is_prev_bucket_same_wrt == true), which is never done before. > +-- Insert few tuples, the primary bucket page will not satisfy +INSERT INTO hash_cleanup_heap SELECT 1 FROM generate_series(1, 50) as i; What do you mean by the second part of the sentence (the primary bucket page will not satisfy)? Few other minor comments: ======================= 1. Can we use ROLLBACK instead of ABORT in the tests? 2. - for (i = 0; i < nitups; i++) + for (int i = 0; i < nitups; i++) I don't think there is a need to make this unrelated change. 3. + /* + * A write buffer needs to be registered even if no tuples are added to + * it to ensure that we can acquire a cleanup lock on it if it is the + * same as primary bucket buffer or update the nextblkno if it is same + * as the previous bucket buffer. + */ + else if (xlrec.is_prim_bucket_same_wrt || xlrec.is_prev_bucket_same_wrt) + { + uint8 wbuf_flags; + + Assert(xlrec.ntups == 0); Can we move this comment inside else if, just before Assert? -- With Regards, Amit Kapila.