Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Avoid instabilities with the regression tests of pg_freespacemap

  2. Add some basic regression tests for pg_freespacemap

  1. Add pg_freespacemap extension sql test

    Dong Wook Lee <sh95119@gmail.com> — 2022-03-08T14:39:08Z

    Hi,
I just added some tests for the pg_freespacemap extension because the test
coverage was 0 percent.
But I don't know if I did it correctly.

---
Regards
Lee Dong Wook

  2. Re: Add pg_freespacemap extension sql test

    Dong Wook Lee <sh95119@gmail.com> — 2022-03-08T14:43:47Z

    I'm sorry for attaching the wrong patch file.

2022년 3월 8일 (화) 오후 11:39, Dong Wook Lee <sh95119@gmail.com>님이 작성:

> Hi,
> I just added some tests for the pg_freespacemap extension because the test
> coverage was 0 percent.
> But I don't know if I did it correctly.
>
> ---
> Regards
> Lee Dong Wook
>

  3. Re: Add pg_freespacemap extension sql test

    Justin Pryzby <pryzby@telsasoft.com> — 2022-03-08T14:45:13Z

    On Tue, Mar 08, 2022 at 11:39:08PM +0900, Dong Wook Lee wrote:
> Hi,
> I just added some tests for the pg_freespacemap extension because the test
> coverage was 0 percent.
> But I don't know if I did it correctly.

The patch only touches doc/*.sgml.
I suppose you forgot to use "git add".

-- 
Justin

  4. Re: Add pg_freespacemap extension sql test

    Dong Wook Lee <sh95119@gmail.com> — 2022-03-08T14:54:12Z

    That's right, so I attached the correct file again.


2022년 3월 8일 (화) 오후 11:45, Justin Pryzby <pryzby@telsasoft.com>님이 작성:

> On Tue, Mar 08, 2022 at 11:39:08PM +0900, Dong Wook Lee wrote:
> > Hi,
> > I just added some tests for the pg_freespacemap extension because the
> test
> > coverage was 0 percent.
> > But I don't know if I did it correctly.
>
> The patch only touches doc/*.sgml.
> I suppose you forgot to use "git add".
>
> --
> Justin
>

  5. Re: Add pg_freespacemap extension sql test

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-08T16:19:18Z

    Dong Wook Lee <sh95119@gmail.com> writes:
> [ 0001_add_test_pg_fsm.patch ]

I think having some coverage here would be great, but I'm concerned that
this patch doesn't look very portable.  Aren't the numbers liable to
change on 32-bit machines, in particular?

			regards, tom lane

  6. Re: Add pg_freespacemap extension sql test

    Dong Wook Lee <sh95119@gmail.com> — 2022-03-09T11:13:15Z

    2022년 3월 9일 (수) 오전 1:19, Tom Lane <tgl@sss.pgh.pa.us>님이 작성:

> Dong Wook Lee <sh95119@gmail.com> writes:
> > [ 0001_add_test_pg_fsm.patch ]
>
> I think having some coverage here would be great, but I'm concerned that
> this patch doesn't look very portable.  Aren't the numbers liable to
> change on 32-bit machines, in particular?
>
>                         regards, tom lane
>

I agree with you, but I have no good idea how to deal with it.
Can the Perl TAP test be a good way?
Thought?

  7. Re: Add pg_freespacemap extension sql test

    Michael Paquier <michael@paquier.xyz> — 2022-03-11T05:51:35Z

    On Wed, Mar 09, 2022 at 08:13:15PM +0900, Dong Wook Lee wrote:
> I agree with you, but I have no good idea how to deal with it.

Well, my guess is that you basically just care about being able to
detect if there is free space in the map or not, which goes down to
detecting if pg_freespace() returns 0 or a number strictly higher than
0, so wouldn't it be enough to stick some > 0 in your test queries?
Btw, if you want to test 32-bit builds, gcc allows that by passing
down -m32.

> Can the Perl TAP test be a good way?

That does not seem necessary here.
--
Michael

  8. Re: Add pg_freespacemap extension sql test

    Dong Wook Lee <sh95119@gmail.com> — 2022-03-15T13:50:25Z

    2022년 3월 11일 (금) 오후 2:51, Michael Paquier <michael@paquier.xyz>님이 작성:
>
> On Wed, Mar 09, 2022 at 08:13:15PM +0900, Dong Wook Lee wrote:
> > I agree with you, but I have no good idea how to deal with it.
>
> Well, my guess is that you basically just care about being able to
> detect if there is free space in the map or not, which goes down to
> detecting if pg_freespace() returns 0 or a number strictly higher than
> 0, so wouldn't it be enough to stick some > 0 in your test queries?
> Btw, if you want to test 32-bit builds, gcc allows that by passing
> down -m32.
>
> > Can the Perl TAP test be a good way?
>
> That does not seem necessary here.
> --
> Michael

so, you mean it's not necessary to add cases for negative numbers or
beyond the range?
I just wrote down testable cases, and if it doesn't have a big
advantage, I don't mind not adding that case.

  9. Re: Add pg_freespacemap extension sql test

    Dong Wook Lee <sh95119@gmail.com> — 2022-03-19T16:18:26Z

    > Well, my guess is that you basically just care about being able to
> detect if there is free space in the map or not, which goes down to
> detecting if pg_freespace() returns 0 or a number strictly higher than
> 0, so wouldn't it be enough to stick some > 0 in your test queries?

I edited the previous patch file.
Am I correct in understanding that?

  10. Re: Add pg_freespacemap extension sql test

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2022-03-19T18:13:38Z

    On Sat, Mar 19, 2022 at 1:18 PM Dong Wook Lee <sh95119@gmail.com> wrote:
>
> > Well, my guess is that you basically just care about being able to
> > detect if there is free space in the map or not, which goes down to
> > detecting if pg_freespace() returns 0 or a number strictly higher than
> > 0, so wouldn't it be enough to stick some > 0 in your test queries?
>
> I edited the previous patch file.
> Am I correct in understanding that?
>

I think what Michael meant is something like attached.

Regards,

--
Fabrízio de Royes Mello

  11. Re: Add pg_freespacemap extension sql test

    Dong Wook Lee <sh95119@gmail.com> — 2022-03-21T12:12:37Z

    2022년 3월 20일 (일) 03:13, Fabrízio de Royes Mello <fabriziomello@gmail.com>님이
작성:

>
>
> On Sat, Mar 19, 2022 at 1:18 PM Dong Wook Lee <sh95119@gmail.com> wrote:
> >
> > > Well, my guess is that you basically just care about being able to
> > > detect if there is free space in the map or not, which goes down to
> > > detecting if pg_freespace() returns 0 or a number strictly higher than
> > > 0, so wouldn't it be enough to stick some > 0 in your test queries?
> >
> > I edited the previous patch file.
> > Am I correct in understanding that?
> >
>
> I think what Michael meant is something like attached.
>
> Regards,
>
> --
> Fabrízio de Royes Mello
>

I think you’re right, thank you for sending it instead of me.

>

  12. Re: Add pg_freespacemap extension sql test

    Michael Paquier <michael@paquier.xyz> — 2022-03-23T06:05:31Z

    On Mon, Mar 21, 2022 at 09:12:37PM +0900, Dong Wook Lee wrote:
> 2022년 3월 20일 (일) 03:13, Fabrízio de Royes Mello <fabriziomello@gmail.com>님이
> 작성:
>> On Sat, Mar 19, 2022 at 1:18 PM Dong Wook Lee <sh95119@gmail.com> wrote:
>>>> Well, my guess is that you basically just care about being able to
>>>> detect if there is free space in the map or not, which goes down to
>>>> detecting if pg_freespace() returns 0 or a number strictly higher than
>>>> 0, so wouldn't it be enough to stick some > 0 in your test queries?
>>>
>>> I edited the previous patch file.
>>> Am I correct in understanding that?
>>>
>>
>> I think what Michael meant is something like attached.
> 
> I think you’re right, thank you for sending it instead of me.

Yes, something like v3 was what I was referring to as we cannot rely
on exact numbers for this test suite.  At least, we can check if there
is a FSM for a given block, even if that can be limited.

After review, I don't like much the idea of allowing concurrent
autovacuums to run in parallel of the table(s) of this test, so we'd
better disable it explicitely.  "t1" is also a very generic name to
use in a regression test.  Another thing that itched me is that we
could also test more with indexes, particularly with btree, BRIN and
hash (the latter should not have a FSM with 10 pages as per the first
group batch, and each one has a stable an initial state).  Finally,
making the tests stable across 32-bit compilations (say gcc -m32) is
proving to be tricky, but it should be safe enough to check if the FSM
is computed or not with a minimal number of tuples.

Btw, a .gitignore was also forgotten.

I have extended the set of tests as of the attached, running these
across everything I could (CI, all my hosts including Windows, macos,
Linux).  We could do more later, of course, but this looks enough to
me as a first step.  And I think that this will not upset the
buildfarm.
--
Michael

  13. Re: Add pg_freespacemap extension sql test

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2022-03-23T13:45:19Z

    On Wed, Mar 23, 2022 at 3:05 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> After review, I don't like much the idea of allowing concurrent
> autovacuums to run in parallel of the table(s) of this test, so we'd
> better disable it explicitely.

Make sense.

>  "t1" is also a very generic name to use in a regression test.

Agreed!

>  Another thing that itched me is that we
> could also test more with indexes, particularly with btree, BRIN and
> hash (the latter should not have a FSM with 10 pages as per the first
> group batch, and each one has a stable an initial state).

What about GIN/GIST indexes?

> I have extended the set of tests as of the attached, running these
> across everything I could (CI, all my hosts including Windows, macos,
> Linux).  We could do more later, of course, but this looks enough to
> me as a first step.  And I think that this will not upset the
> buildfarm.

Also LGTM.

Regards,

--
Fabrízio de Royes Mello

  14. Re: Add pg_freespacemap extension sql test

    Michael Paquier <michael@paquier.xyz> — 2022-03-24T00:40:23Z

    On Wed, Mar 23, 2022 at 10:45:19AM -0300, Fabrízio de Royes Mello wrote:
> On Wed, Mar 23, 2022 at 3:05 AM Michael Paquier <michael@paquier.xyz> wrote:
>>  Another thing that itched me is that we
>> could also test more with indexes, particularly with btree, BRIN and
>> hash (the latter should not have a FSM with 10 pages as per the first
>> group batch, and each one has a stable an initial state).
> 
> What about GIN/GIST indexes?

Yes, we could extend that more.  For now, I am curious to see what the
buildfarm has to say with the current contents of the patch, and I can
keep an eye on the buildfarm today, so I have applied it.
--
Michael

  15. Re: Add pg_freespacemap extension sql test

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-27T17:18:46Z

    Michael Paquier <michael@paquier.xyz> writes:
> Yes, we could extend that more.  For now, I am curious to see what the
> buildfarm has to say with the current contents of the patch, and I can
> keep an eye on the buildfarm today, so I have applied it.

It seems this is unstable under valgrind [1]:

--- /mnt/resource/bf/build/skink-master/HEAD/pgsql/contrib/pg_freespacemap/expected/pg_freespacemap.out	2022-03-24 09:39:43.974477703 +0000
+++ /mnt/resource/bf/build/skink-master/HEAD/pgsql.build/contrib/pg_freespacemap/results/pg_freespacemap.out	2022-03-27 17:07:23.896287669 +0000
@@ -60,6 +60,7 @@
     ORDER BY 1, 2;
        id        | blkno | is_avail 
 -----------------+-------+----------
+ freespace_tab   |     0 | t
  freespace_brin  |     0 | f
  freespace_brin  |     1 | f
  freespace_brin  |     2 | t
@@ -75,7 +76,7 @@
  freespace_hash  |     7 | f
  freespace_hash  |     8 | f
  freespace_hash  |     9 | f
-(15 rows)
+(16 rows)
 
 -- failures with incorrect block number
 SELECT * FROM pg_freespace('freespace_tab', -1);

skink has passed several runs since the commit went in, so it's
"unstable" not "fails consistently".  I see the test tries to
disable autovacuum on that table, so that doesn't seem to be
the problem ... what is?

			regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2022-03-27%2008%3A26%3A20

  16. Re: Add pg_freespacemap extension sql test

    Michael Paquier <michael@paquier.xyz> — 2022-03-28T03:12:48Z

    On Sun, Mar 27, 2022 at 01:18:46PM -0400, Tom Lane wrote:
> skink has passed several runs since the commit went in, so it's
> "unstable" not "fails consistently".  I see the test tries to
> disable autovacuum on that table, so that doesn't seem to be
> the problem ... what is?

This is a race condition, directly unrelated to valgrind but easier to
trigger under it because things get slower.  It takes me a dozen of
tries to be able to reproduce the failure locally, but I can wiht
valgrind enabled.

So, the output of the test is simply telling us that the FSM of the
main table is not getting truncated.  From what I can see, the
difference is in should_attempt_truncation(), where we finish with
nonempty_pages set to 1 rather than 0 on failure.  And it just takes
one autovacuum to run in parallel of the manual VACUUM after the
DELETE to prevent the removal of those tuples, which is what I can see
from the logs on failure:
LOG:  statement: DELETE FROM freespace_tab;
DEBUG: autovacuum: processing database "contrib_regression"
LOG:  statement: VACUUM freespace_tab;

It seems to me here that the snapshot hold by autovacuum during the
scan of pg_database to find the relations to process is enough to
prevent the FSM truncation, as the tuples cleaned up by the DELETE
query still need to be visible.  One simple way to keep this test
would be a custom configuration file with autovacuum disabled and
NO_INSTALLCHECK.  Any better ideas?
--
Michael

  17. Re: Add pg_freespacemap extension sql test

    Michael Paquier <michael@paquier.xyz> — 2022-03-29T05:05:56Z

    On Mon, Mar 28, 2022 at 12:12:48PM +0900, Michael Paquier wrote:
> It seems to me here that the snapshot hold by autovacuum during the
> scan of pg_database to find the relations to process is enough to
> prevent the FSM truncation, as the tuples cleaned up by the DELETE
> query still need to be visible.  One simple way to keep this test
> would be a custom configuration file with autovacuum disabled and
> NO_INSTALLCHECK.

Well, done this way.  We already do that in other tests that rely on a
FSM truncation to happen, like 008_fsm_truncation.pl.
--
Michael