Re: Possible fails in pg_stat_statements test

From: Julien Rouhaud <rjuju123(at)gmail(dot)com>
To: "Anton A(dot) Melnikov" <aamelnikov(at)inbox(dot)ru>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Possible fails in pg_stat_statements test
Date: 2022-03-31 16:00:36
Message-ID: YkXQJJClWNXawrOb@jrouhaud
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2022-03-31 16:08:20 Re: pgsql: Add 'basebackup_to_shell' contrib module.
Previous Message Nathan Bossart 2022-03-31 15:45:56 Re: Use "WAL segment" instead of "log segment" consistently in user-facing messages