From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Amit Kapila' <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Robert Haas <robertmhaas(at)gmail(dot)com>, Jeff Davis <pgsql(at)j-davis(dot)com>, Alexander Lakhin <exclusion(at)gmail(dot)com>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | RE: Is this a problem in GenericXLogFinish()? |
Date: | 2023-11-13 05:47:26 |
Message-ID: | TYAPR01MB5866CE98DC1339C42E45C2ACF5B3A@TYAPR01MB5866.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
fix_hash_squeeze_wal_5.patch | application/octet-stream | 4.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2023-11-13 05:48:13 | Re: Adding facility for injection points (or probe points?) for more advanced tests |
Previous Message | Ajin Cherian | 2023-11-13 05:32:32 | Re: Synchronizing slots from primary to standby |