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