Re: Some regression tests for the pg_control_*() functions

On Wed, Oct 26, 2022 at 12:48 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Oct 26, 2022 at 10:13:29AM +0530, Bharath Rupireddy wrote:
> > +1 for improving the test coverage. Is there a strong reason to
> > validate individual output columns rather than select count(*) > 0
> > from pg_control_XXXX(); sort of tests? If the intention is to validate
> > the pg_controlfile contents, we have pg_controldata to look at and
> > pg_control_XXXX() functions doing crc checks.
>
> And it could be possible that the control file finishes by writing
> some incorrect data due to a bug in the backend.

We will have bigger problems when a backend corrupts the pg_control
file, no? The bigger problems could be that the server won't come up
or it behaves abnormally or some other.

> Adding a count(*) or
> similar to get the number of fields of the function is basically the
> same as checking its execution, still I'd like to think that having a
> minimum set of checks would be kind of nice on top of that. Among all
> the ones I wrote in the patch upthread, the following ones would be in
> my minimalistic list:
> - timeline_id > 0
> - timeline_id >= prev_timeline_id
> - checkpoint_lsn >= redo_lsn
> - data_page_checksum_version >= 0
> - Perhaps the various fields of pg_control_init() using their
> lower-bound values.
> - Perhaps pg_control_version and/or catalog_version_no > NN

Can't the CRC check detect any of the above corruptions? Do we have
any evidence of backend corrupting the pg_control file or any of the
above variables while running regression tests?

If the concern is backend corrupting the pg_control file and CRC check
can't detect it, then the extra checks (as proposed in the patch) must
be placed within the core (perhaps before writing/after reading the
pg_control file), not in regression tests for sure.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com