On Tue, Feb 5, 2019 at 4:04 AM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Mon, Feb 4, 2019 at 2:27 PM John Naylor <john.nay...@2ndquadrant.com>
> wrote:
> >
> > 1. Earlier, I had a test to ensure that free space towards the front
> > of the relation was visible with no FSM. In [1], I rewrote it without
> > using vacuum, so we can consider adding it back now if desired. I can
> > prepare a patch for this.
> >
>
> Yes, this is required. It is generally a good practise to add test (unless
> it takes a lot of time) which covers new code/functionality.
>
> > 2. As a follow-on, since we don't rely on vacuum to remove dead rows,
> > we could try putting the fsm.sql test in some existing group in the
> > parallel schedule, rather than its own group is it is now.
> >
>
> +1.
This is done in 0001.
> > 3. While looking at 0d1fe9f74e, it occurred to me that I ignored this
> > patch's effects on GetRecordedFreeSpace(), which will return zero for
> > tables with no FSM.
> >
>
> Right, but what exactly we want to do for it? Do you want to add a comment
> atop of this function?
Hmm, the comment already says "according to the FSM", so maybe it's
already obvious. I was thinking more about maybe commenting the
callsite where it's helpful, as in 0002.
> > The other callers are in:
> > contrib/pg_freespacemap/pg_freespacemap.c
> > contrib/pgstattuple/pgstatapprox.c
> >
> > For pg_freespacemap, this doesn't matter, since it's just reporting
> > the facts. For pgstattuple_approx(), it might under-estimate the free
> > space and over-estimate the number of live tuples.
> >
>
> Sure, but without patch also, it can do so, if the vacuum hasn't updated
> freespace map.
Okay, then maybe we don't need to do anything else here.
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From ff821d820630313e4dcea65aa1b35d47a0736c64 Mon Sep 17 00:00:00 2001
From: John Naylor <jcnay...@gmail.com>
Date: Tue, 5 Feb 2019 10:32:19 +0100
Subject: [PATCH v1 1/2] Improve FSM regression test
In the interest of caution, in commit b0eaa4c51bb we left out a
test that used vacuum to remove dead rows. This test has been
rewritten to use fillfactor instead to control free space. Since
we no longer need to remove dead rows as part of the test, put
the fsm regression test in a parallel group.
---
src/test/regress/expected/fsm.out | 25 ++++++++++++++++++++++---
src/test/regress/parallel_schedule | 8 +-------
src/test/regress/sql/fsm.sql | 22 ++++++++++++++++++++--
3 files changed, 43 insertions(+), 12 deletions(-)
diff --git a/src/test/regress/expected/fsm.out b/src/test/regress/expected/fsm.out
index b02993188c..d747c1a7dd 100644
--- a/src/test/regress/expected/fsm.out
+++ b/src/test/regress/expected/fsm.out
@@ -2,14 +2,33 @@
-- Free Space Map test
--
CREATE TABLE fsm_check_size (num int, str text);
--- With one block, there should be no FSM
-INSERT INTO fsm_check_size VALUES(1, 'a');
+-- Fill 3 blocks with one record each
+ALTER TABLE fsm_check_size SET (fillfactor=15);
+INSERT INTO fsm_check_size SELECT i, rpad('', 1024, 'a')
+FROM generate_series(1,3) i;
+-- There should be no FSM
VACUUM fsm_check_size;
SELECT pg_relation_size('fsm_check_size', 'main') AS heap_size,
pg_relation_size('fsm_check_size', 'fsm') AS fsm_size;
heap_size | fsm_size
-----------+----------
- 8192 | 0
+ 24576 | 0
+(1 row)
+
+-- Fill most of the last block
+ALTER TABLE fsm_check_size SET (fillfactor=100);
+INSERT INTO fsm_check_size SELECT i, rpad('', 1024, 'a')
+FROM generate_series(101,105) i;
+-- Make sure records can go into any block but the last one
+ALTER TABLE fsm_check_size SET (fillfactor=30);
+-- Insert large record and make sure it does not cause the relation to extend
+INSERT INTO fsm_check_size VALUES (111, rpad('', 1024, 'a'));
+VACUUM fsm_check_size;
+SELECT pg_relation_size('fsm_check_size', 'main') AS heap_size,
+pg_relation_size('fsm_check_size', 'fsm') AS fsm_size;
+ heap_size | fsm_size
+-----------+----------
+ 24576 | 0
(1 row)
-- Extend table with enough blocks to exceed the FSM threshold
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 4051a4ad4e..a956775dd1 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -23,7 +23,7 @@ test: numerology
# ----------
# The second group of parallel tests
# ----------
-test: point lseg line box path polygon circle date time timetz timestamp timestamptz interval inet macaddr macaddr8 tstypes
+test: point lseg line box path polygon circle date time timetz timestamp timestamptz interval inet macaddr macaddr8 tstypes fsm
# ----------
# Another group of parallel tests
@@ -68,12 +68,6 @@ test: create_aggregate create_function_3 create_cast constraints triggers inheri
# ----------
test: sanity_check
-# ----------
-# fsm does a delete followed by vacuum, and running it in parallel can prevent
-# removal of rows.
-# ----------
-test: fsm
-
# ----------
# Believe it or not, select creates a table, subsequent
# tests need.
diff --git a/src/test/regress/sql/fsm.sql b/src/test/regress/sql/fsm.sql
index 332c3e2b2d..2f169c0508 100644
--- a/src/test/regress/sql/fsm.sql
+++ b/src/test/regress/sql/fsm.sql
@@ -4,8 +4,26 @@
CREATE TABLE fsm_check_size (num int, str text);
--- With one block, there should be no FSM
-INSERT INTO fsm_check_size VALUES(1, 'a');
+-- Fill 3 blocks with one record each
+ALTER TABLE fsm_check_size SET (fillfactor=15);
+INSERT INTO fsm_check_size SELECT i, rpad('', 1024, 'a')
+FROM generate_series(1,3) i;
+
+-- There should be no FSM
+VACUUM fsm_check_size;
+SELECT pg_relation_size('fsm_check_size', 'main') AS heap_size,
+pg_relation_size('fsm_check_size', 'fsm') AS fsm_size;
+
+-- Fill most of the last block
+ALTER TABLE fsm_check_size SET (fillfactor=100);
+INSERT INTO fsm_check_size SELECT i, rpad('', 1024, 'a')
+FROM generate_series(101,105) i;
+
+-- Make sure records can go into any block but the last one
+ALTER TABLE fsm_check_size SET (fillfactor=30);
+
+-- Insert large record and make sure it does not cause the relation to extend
+INSERT INTO fsm_check_size VALUES (111, rpad('', 1024, 'a'));
VACUUM fsm_check_size;
SELECT pg_relation_size('fsm_check_size', 'main') AS heap_size,
--
2.17.1
From f6ddb0cdddfb2bb0c3195edcbb60d3ca5f679a90 Mon Sep 17 00:00:00 2001
From: John Naylor <jcnay...@gmail.com>
Date: Tue, 5 Feb 2019 10:42:18 +0100
Subject: [PATCH v1 2/2] Add note that relations with no FSM will report no
free space.
---
contrib/pgstattuple/pgstatapprox.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/contrib/pgstattuple/pgstatapprox.c b/contrib/pgstattuple/pgstatapprox.c
index ff7c255a2d..23ccaaaca6 100644
--- a/contrib/pgstattuple/pgstatapprox.c
+++ b/contrib/pgstattuple/pgstatapprox.c
@@ -89,6 +89,8 @@ statapprox_heap(Relation rel, output_type *stat)
/*
* If the page has only visible tuples, then we can find out the free
* space from the FSM and move on.
+ * Note: If a relation has no FSM, GetRecordedFreeSpace() will report
+ * zero free space. This is fine for the purposes of approximation.
*/
if (VM_ALL_VISIBLE(rel, blkno, &vmbuffer))
{
--
2.17.1
