Thread: Add pg_freespacemap extension sql test

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

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

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

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.

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

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

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-2717: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

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

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