Hi, On Thu, Mar 31, 2022 at 06:08:01PM +0300, Anton A. Melnikov wrote: > Hello! > > On 30.03.2022 22:36, Robert Haas wrote: > > I don't think that the idea of "extra" WAL records is very principled. > > It's pretty vague what "extra" means, and your definition seems to be > > basically "whatever would be needed to make this test case pass."
I agree, and even it there was a better definition there probably isn't much to learn from it. > I > > think the problem is basically with the test cases's idea that # of > > WAL records and # of table rows ought to be equal. I think that's just > > false. In general, we'd also have to worry about index insertions, > > which would provoke variable numbers of WAL records depending on > > whether they cause a page split. And we'd have to worry about TOAST > > table insertions, which could produce different numbers of records > > depending on the size of the data, the configured block size and TOAST > > threshold, and whether the TOAST table index incurs a page split. Indeed, we added this test as it was hitting only a few queries with small rows, which we thought would be stable, but that's apparently not the case. I think the reason we never had any problem is that the buildfarm currently doesn't run pg_stat_statement regression test, as it's marked as NO_INSTALLCHECK. Other CI systems like at pgpro evidently have a different approach. > > If it's true that this test case sometimes randomly fails, then we > > ought to fix that somehow, maybe by just removing this particular > > check from the test case, or changing it to >=, or something like > > that. But I don't think adding a new counter is the right idea. > > Indeed. Then there is a very simple solution for this particular case as > wal_records counter may only sometime becomes greater but never less. > The corresponding patch is attached. +1 for this approach, and the patch looks good to me.