On Wed, Oct 26, 2022 at 12:48 PM Michael Paquier <mich...@paquier.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
