From: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, Konstantin Knizhnik <knizhnik(at)garret(dot)ru>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Flush pgstats file during checkpoints |
Date: | 2024-07-29 04:46:17 |
Message-ID: | ZqcemaAFlAx75Ru2@ip-10-97-1-34.eu-west-3.compute.internal |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On Tue, Jul 23, 2024 at 12:52:11PM +0900, Michael Paquier wrote:
> On Mon, Jul 22, 2024 at 07:01:41AM +0000, Bertrand Drouvot wrote:
> > 3 ===
> >
> > + /*
> > + * Read the redo LSN stored in the file.
> > + */
> > + if (!read_chunk_s(fpin, &file_redo) ||
> > + file_redo != redo)
> > + goto error;
> >
> > I wonder if it would make sense to have dedicated error messages for
> > "file_redo != redo" and for "format_id != PGSTAT_FILE_FORMAT_ID". That would
> > ease to diagnose as to why the stat file is discarded.
>
> Yep. This has been itching me quite a bit, and that's a bit more than
> just the format ID or the redo LSN: it relates to all the read_chunk()
> callers. I've taken a shot at this with patch 0001, implemented on
> top of the rest.
Thanks! 0001 attached is v4-0001-Revert-Test-that-vacuum-removes-tuples-older-than.patch
so I guess you did not attached the right one.
> Attaching a new v4 series, with all these comments addressed.
Thanks!
Looking at 0002:
1 ===
if (!read_chunk(fpin, ptr, info->shared_data_len))
+ {
+ elog(WARNING, "could not read data of stats kind %d for entry of type %c",
+ kind, t);
Nit: what about to include the "info->shared_data_len" value in the WARNING?
2 ===
if (!read_chunk_s(fpin, &name))
+ {
+ elog(WARNING, "could not read name of stats kind %d for entry of type %c",
+ kind, t);
goto error;
+ }
if (!pgstat_is_kind_valid(kind))
+ {
+ elog(WARNING, "invalid stats kind %d for entry of type %c",
+ kind, t);
goto error;
+ }
Shouldn't we swap those 2 tests so that we check that the kind is valid right
after this one?
if (!read_chunk_s(fpin, &kind))
+ {
+ elog(WARNING, "could not read stats kind for entry of type %c", t);
goto error;
+ }
Looking at 0003: LGTM
Looking at 0004: LGTM
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From | Date | Subject | |
---|---|---|---|
Next Message | Bertrand Drouvot | 2024-07-29 05:00:55 | Re: Allow logical failover slots to wait on synchronous replication |
Previous Message | Amit Kapila | 2024-07-29 04:01:07 | Re: Conflict detection and logging in logical replication |