Re: Is this a problem in GenericXLogFinish()?

On Fri, Nov 10, 2023 at 9:17 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)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.