RE: Is this a problem in GenericXLogFinish()?

Dear Amit,

Thanks for reviewing! PSA new version patch.

> > > 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?

CHECKPOINT command is needed for writing a hash pages to disk. IIUC if the command
is omitted, none of tuples are recorded to hash index *file*, so squeeze would not
be occurred.

You can test by 1) restoring changes for hashovfl.c then 2) removing CHECKPOINT
command. Server crash would not be occurred.

> > 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)?

I meant to say that the primary bucket page still can have more tuples. Maybe I
should say "will not be full". Reworded.

> Few other minor comments:
> =======================
> 1. Can we use ROLLBACK instead of ABORT in the tests?

Changed. IIRC they have same meaning, but it seems that most of sql files have
"ROLLBACK".

> 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.

Reverted. I thought that the variable i would be used only when nitups>0 so that
it could be removed, but it was not my business.

> 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?

Moved.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED