Re: Flush pgstats file during checkpoints

On Mon, Jul 29, 2024 at 04:46:17AM +0000, Bertrand Drouvot wrote:
> 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.

I did attach the right set of patches, please ignore 0001 entirely:
the patch series is made of three patches, beginning with 0002 :)

> Looking at 0002:
>
> 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?

Good idea, so added.

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

Hmm. We could, but this order is not buggy either. So I've let it
as-is for now, just adding the WARNINGs.

By the way, I have noticed an extra path where a WARNING would not be
logged while playing with corrupted pgstats files: when the entry type
itself is incorrect. I have added an extra elog() in this case, and
applied 0001. Err.. 0002, sorry ;)

> Looking at 0003: LGTM
> Looking at 0004: LGTM

Thanks. Attached are the two remaining patches, for now. I'm
planning to get back to these in a few days.
--
Michael