pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Avoid creation of the free space map for small heap relations.
29+ messages / 6 participants
[nested] [flat]
* pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-01-28 02:47 Amit Kapila <akapila@postgresql.org>
0 siblings, 1 reply; 29+ messages in thread
From: Amit Kapila @ 2019-01-28 02:47 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Avoid creation of the free space map for small heap relations.
Previously, all heaps had FSMs. For very small tables, this means that the
FSM took up more space than the heap did. This is wasteful, so now we
refrain from creating the FSM for heaps with 4 pages or fewer. If the last
known target block has insufficient space, we still try to insert into some
other page before giving up and extending the relation, since doing
otherwise leads to table bloat. Testing showed that trying every page
penalized performance slightly, so we compromise and try every other page.
This way, we visit at most two pages. Any pages with wasted free space
become visible at next relation extension, so we still control table bloat.
As a bonus, directly attempting one or two pages can even be faster than
consulting the FSM would have been.
Once the FSM is created for a heap we don't remove it even if somebody
deletes all the rows from the corresponding relation. We don't think it is
a useful optimization as it is quite likely that relation will again grow
to the same size.
Author: John Naylor with design inputs and some code contribution by Amit Kapila
Reviewed-by: Amit Kapila
Tested-by: Mithun C Y
Discussion: https://www.postgresql.org/message-id/CAJVSVGWvB13PzpbLEecFuGFc5V2fsO736BsdTakPiPAcdMM5tQ@mail.gmail...
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/ac88d2962a96a9c7e83d5acfc28fe49a72812086
Modified Files
--------------
contrib/pageinspect/expected/page.out | 77 ++++-----
contrib/pageinspect/sql/page.sql | 33 ++--
doc/src/sgml/storage.sgml | 13 +-
src/backend/access/brin/brin.c | 2 +-
src/backend/access/brin/brin_pageops.c | 10 +-
src/backend/access/heap/hio.c | 47 +++--
src/backend/access/heap/vacuumlazy.c | 17 +-
src/backend/access/transam/xact.c | 14 ++
src/backend/storage/freespace/README | 38 ++++-
src/backend/storage/freespace/freespace.c | 275 +++++++++++++++++++++++++++++-
src/backend/storage/freespace/indexfsm.c | 6 +-
src/include/storage/freespace.h | 9 +-
src/test/regress/expected/fsm.out | 75 ++++++++
src/test/regress/parallel_schedule | 6 +
src/test/regress/serial_schedule | 1 +
src/test/regress/sql/fsm.sql | 55 ++++++
16 files changed, 576 insertions(+), 102 deletions(-)
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-01-28 03:19 Amit Kapila <amit.kapila16@gmail.com>
parent: Amit Kapila <akapila@postgresql.org>
0 siblings, 1 reply; 29+ messages in thread
From: Amit Kapila @ 2019-01-28 03:19 UTC (permalink / raw)
To: Amit Kapila <akapila@postgresql.org>; +Cc: pgsql-committers@lists.postgresql.org
On Mon, Jan 28, 2019 at 8:17 AM Amit Kapila <akapila@postgresql.org> wrote:
>
> Avoid creation of the free space map for small heap relations.
>
It seems there is some failure due to this on build farm machines. I
will investigate!
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-01-28 04:17 Amit Kapila <amit.kapila16@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: Amit Kapila @ 2019-01-28 04:17 UTC (permalink / raw)
To: Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; John Naylor <john.naylor@2ndquadrant.com>; +Cc: pgsql-committers@lists.postgresql.org
On Mon, Jan 28, 2019 at 8:49 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jan 28, 2019 at 8:17 AM Amit Kapila <akapila@postgresql.org> wrote:
> >
> > Avoid creation of the free space map for small heap relations.
> >
>
> It seems there is some failure due to this on build farm machines. I
> will investigate!
>
The failure is as below:
@@ -26,7 +26,7 @@
pg_relation_size('fsm_check_size', 'fsm') AS fsm_size;
heap_size | fsm_size
-----------+----------
- 24576 | 0
+ 32768 | 0
(1 row)
-- Extend table with enough blocks to exceed the FSM threshold
@@ -56,7 +56,7 @@
SELECT pg_relation_size('fsm_check_size', 'fsm') AS fsm_size;
fsm_size
----------
- 16384
+ 24576
(1 row)
So here in the above tests, we are deleting some rows, performing
vacuum and then checking the size of heap and or vacuum. It seems to
me sometimes the vacuum is *not* able to remove the rows and truncate
the relation/FSM as per tests expectation. One possible theory is
that there is some parallel transaction running which prevents a
vacuum from doing so. We have tried to avoid that by not allowing to
run the FSM test in parallel with other tests, but I think still
something seems to have run in parallel to the FSM test. One
possibility is that autovacuum has triggered to perform truncation of
some other relation (remove pages at the end) which doesn't allow the
FSM test to remove the rows/perform truncation and thus let to the
failure. Can there be anything else which can start a transaction
when a regression test is running? Still thinking, but inputs are
welcome.
If my theory is correct, then in the newly added tests by this patch,
we can't rely on the vacuum to truncate the relation pages at the end
and hence can't rely on heap/FSM size.
Thoughts?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-01-28 04:55 Andrew Gierth <andrew@tao11.riddles.org.uk>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: Andrew Gierth @ 2019-01-28 04:55 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; John Naylor <john.naylor@2ndquadrant.com>; pgsql-committers@lists.postgresql.org
>>>>> "Amit" == Amit Kapila <amit.kapila16@gmail.com> writes:
Amit> One possibility is that autovacuum has triggered to perform
Amit> truncation of some other relation (remove pages at the end) which
Amit> doesn't allow the FSM test to remove the rows/perform truncation
Amit> and thus let to the failure. Can there be anything else which can
Amit> start a transaction when a regression test is running? Still
Amit> thinking, but inputs are welcome.
I've bumped into issues (cf. commit 64ae420b2) with regression tests of
this kind caused by concurrent (auto-)ANALYZE (not VACUUM); VACUUM's
snapshot is ignored for testing for whether row versions can be removed,
but ANALYZE's is not.
--
Andrew (irc:RhodiumToad)
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-01-28 05:37 Amit Kapila <amit.kapila16@gmail.com>
parent: Andrew Gierth <andrew@tao11.riddles.org.uk>
0 siblings, 2 replies; 29+ messages in thread
From: Amit Kapila @ 2019-01-28 05:37 UTC (permalink / raw)
To: Andrew Gierth <andrew@tao11.riddles.org.uk>; +Cc: Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; John Naylor <john.naylor@2ndquadrant.com>; pgsql-committers@lists.postgresql.org
On Mon, Jan 28, 2019 at 10:25 AM Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
>
> >>>>> "Amit" == Amit Kapila <amit.kapila16@gmail.com> writes:
>
> Amit> One possibility is that autovacuum has triggered to perform
> Amit> truncation of some other relation (remove pages at the end) which
> Amit> doesn't allow the FSM test to remove the rows/perform truncation
> Amit> and thus let to the failure. Can there be anything else which can
> Amit> start a transaction when a regression test is running? Still
> Amit> thinking, but inputs are welcome.
>
> I've bumped into issues (cf. commit 64ae420b2) with regression tests of
> this kind caused by concurrent (auto-)ANALYZE (not VACUUM);
>
Yes, so this could be the cause of the problem. I think we need to
change the tests added by the patch such that they don't rely on
vacuum to remove dead-row versions? Do you or anybody else see any
better way to fix this?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-01-28 05:40 Andrew Gierth <andrew@tao11.riddles.org.uk>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 29+ messages in thread
From: Andrew Gierth @ 2019-01-28 05:40 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; John Naylor <john.naylor@2ndquadrant.com>; pgsql-committers@lists.postgresql.org
>>>>> "Amit" == Amit Kapila <amit.kapila16@gmail.com> writes:
Amit> Yes, so this could be the cause of the problem. I think we need
Amit> to change the tests added by the patch such that they don't rely
Amit> on vacuum to remove dead-row versions? Do you or anybody else see
Amit> any better way to fix this?
Do you just need to create free space on a page that didn't have enough
before? It might be worth tweaking the fillfactor rather than trying to
delete anything.
--
Andrew (irc:RhodiumToad)
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-01-28 05:48 Tom Lane <tgl@sss.pgh.pa.us>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 29+ messages in thread
From: Tom Lane @ 2019-01-28 05:48 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Andrew Gierth <andrew@tao11.riddles.org.uk>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; John Naylor <john.naylor@2ndquadrant.com>; pgsql-committers@lists.postgresql.org
Amit Kapila <amit.kapila16@gmail.com> writes:
> Yes, so this could be the cause of the problem. I think we need to
> change the tests added by the patch such that they don't rely on
> vacuum to remove dead-row versions? Do you or anybody else see any
> better way to fix this?
To be blunt, this patch needs to be reverted immediately. The failures
that are showing up are not just "the fsm test is not portable" problems.
See for example
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mantid&dt=2019-01-28%2005%3A07%3A06
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2019-01-28%2003%3A07%3A39
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2019-01-28%2003%3A20%3A02
I don't know what the common thread is here, but you don't get to leave
the buildfarm broken this badly while you figure it out.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-01-28 05:56 Amit Kapila <amit.kapila16@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 29+ messages in thread
From: Amit Kapila @ 2019-01-28 05:56 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andrew Gierth <andrew@tao11.riddles.org.uk>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; John Naylor <john.naylor@2ndquadrant.com>; pgsql-committers@lists.postgresql.org
On Mon, Jan 28, 2019 at 11:18 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > Yes, so this could be the cause of the problem. I think we need to
> > change the tests added by the patch such that they don't rely on
> > vacuum to remove dead-row versions? Do you or anybody else see any
> > better way to fix this?
>
> To be blunt, this patch needs to be reverted immediately.
Okay, I will do it.
> The failures
> that are showing up are not just "the fsm test is not portable" problems.
> See for example
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mantid&dt=2019-01-28%2005%3A07%3A06
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2019-01-28%2003%3A07%3A39
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2019-01-28%2003%3A20%3A02
>
> I don't know what the common thread is here, but you don't get to leave
> the buildfarm broken this badly while you figure it out.
>
Sure, but I am wondering why none of this ever shown in local tests,
as we have done quite some testing related to pgbench as well.
Anyway, I think we need figure that out seprately.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-01-28 05:57 Amit Kapila <amit.kapila16@gmail.com>
parent: Andrew Gierth <andrew@tao11.riddles.org.uk>
0 siblings, 1 reply; 29+ messages in thread
From: Amit Kapila @ 2019-01-28 05:57 UTC (permalink / raw)
To: Andrew Gierth <andrew@tao11.riddles.org.uk>; +Cc: Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; John Naylor <john.naylor@2ndquadrant.com>; pgsql-committers@lists.postgresql.org
On Mon, Jan 28, 2019 at 11:10 AM Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
>
> >>>>> "Amit" == Amit Kapila <amit.kapila16@gmail.com> writes:
>
> Amit> Yes, so this could be the cause of the problem. I think we need
> Amit> to change the tests added by the patch such that they don't rely
> Amit> on vacuum to remove dead-row versions? Do you or anybody else see
> Amit> any better way to fix this?
>
> Do you just need to create free space on a page that didn't have enough
> before? It might be worth tweaking the fillfactor rather than trying to
> delete anything.
>
No, it also depends on vacuum to remove rows and then truncate the
relation accordingly.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-01-28 06:01 Tom Lane <tgl@sss.pgh.pa.us>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 0 replies; 29+ messages in thread
From: Tom Lane @ 2019-01-28 06:01 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Andrew Gierth <andrew@tao11.riddles.org.uk>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; John Naylor <john.naylor@2ndquadrant.com>; pgsql-committers@lists.postgresql.org
Amit Kapila <amit.kapila16@gmail.com> writes:
>> I don't know what the common thread is here, but you don't get to leave
>> the buildfarm broken this badly while you figure it out.
> Sure, but I am wondering why none of this ever shown in local tests,
> as we have done quite some testing related to pgbench as well.
Not sure, but I notice that two of the three critters I just pointed to
use force_parallel_mode. Dromedary does not, but it's old and slow.
Kind of smells like a timing problem ...
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-01-28 06:20 John Naylor <john.naylor@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: John Naylor @ 2019-01-28 06:20 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Andrew Gierth <andrew@tao11.riddles.org.uk>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; pgsql-committers@lists.postgresql.org
On Mon, Jan 28, 2019 at 6:58 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jan 28, 2019 at 11:10 AM Andrew Gierth
> you just need to create free space on a page that didn't have enough
> > before? It might be worth tweaking the fillfactor rather than trying to
> > delete anything.
> >
>
> No, it also depends on vacuum to remove rows and then truncate the
> relation accordingly.
That particular test could be removed -- it's just verifying behavior
that's already been there for years and is a direct consquence of
normal truncation combined with the addressing scheme of the FSM
logic.
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations.
@ 2019-02-04 02:17 Michael Paquier <michael@paquier.xyz>
parent: John Naylor <john.naylor@2ndquadrant.com>
0 siblings, 0 replies; 29+ messages in thread
From: Michael Paquier @ 2019-02-04 02:17 UTC (permalink / raw)
To: John Naylor <john.naylor@2ndquadrant.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Andrew Gierth <andrew@tao11.riddles.org.uk>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; pgsql-committers@lists.postgresql.org
On Mon, Jan 28, 2019 at 07:20:29AM +0100, John Naylor wrote:
> That particular test could be removed -- it's just verifying behavior
> that's already been there for years and is a direct consquence of
> normal truncation combined with the addressing scheme of the FSM
> logic.
Moved to next CF, please feel free to continue this work, this is
interesting stuff.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190204021749.GV1881@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 29+ messages in thread
* pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-04 02:22 Amit Kapila <akapila@postgresql.org>
0 siblings, 1 reply; 29+ messages in thread
From: Amit Kapila @ 2019-02-04 02:22 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Avoid creation of the free space map for small heap relations, take 2.
Previously, all heaps had FSMs. For very small tables, this means that the
FSM took up more space than the heap did. This is wasteful, so now we
refrain from creating the FSM for heaps with 4 pages or fewer. If the last
known target block has insufficient space, we still try to insert into some
other page before giving up and extending the relation, since doing
otherwise leads to table bloat. Testing showed that trying every page
penalized performance slightly, so we compromise and try every other page.
This way, we visit at most two pages. Any pages with wasted free space
become visible at next relation extension, so we still control table bloat.
As a bonus, directly attempting one or two pages can even be faster than
consulting the FSM would have been.
Once the FSM is created for a heap we don't remove it even if somebody
deletes all the rows from the corresponding relation. We don't think it is
a useful optimization as it is quite likely that relation will again grow
to the same size.
Author: John Naylor, Amit Kapila
Reviewed-by: Amit Kapila
Tested-by: Mithun C Y
Discussion: https://www.postgresql.org/message-id/CAJVSVGWvB13PzpbLEecFuGFc5V2fsO736BsdTakPiPAcdMM5tQ@mail.gmail...
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/b0eaa4c51bbff3e3c600b11e5d104d6feb9ca77f
Modified Files
--------------
contrib/pageinspect/expected/page.out | 77 ++++----
contrib/pageinspect/sql/page.sql | 33 ++--
doc/src/sgml/storage.sgml | 13 +-
src/backend/access/brin/brin.c | 2 +-
src/backend/access/brin/brin_pageops.c | 10 +-
src/backend/access/heap/hio.c | 45 +++--
src/backend/access/heap/vacuumlazy.c | 17 +-
src/backend/access/transam/xact.c | 14 ++
src/backend/storage/freespace/README | 38 +++-
src/backend/storage/freespace/freespace.c | 285 +++++++++++++++++++++++++++++-
src/backend/storage/freespace/indexfsm.c | 6 +-
src/include/storage/freespace.h | 9 +-
src/test/regress/expected/fsm.out | 48 +++++
src/test/regress/parallel_schedule | 6 +
src/test/regress/serial_schedule | 1 +
src/test/regress/sql/fsm.sql | 41 +++++
16 files changed, 543 insertions(+), 102 deletions(-)
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-25 17:02 Tom Lane <tgl@sss.pgh.pa.us>
parent: Amit Kapila <akapila@postgresql.org>
0 siblings, 3 replies; 29+ messages in thread
From: Tom Lane @ 2019-02-25 17:02 UTC (permalink / raw)
To: Amit Kapila <akapila@postgresql.org>; +Cc: pgsql-hackers@lists.postgresql.org
Amit Kapila <akapila@postgresql.org> writes:
> Avoid creation of the free space map for small heap relations, take 2.
I think this patch still has some issues. Note the following two
recent buildfarm failures:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=petalura&dt=2019-02-20%2004%3A20%3A01
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dragonet&dt=2019-02-24%2022%3A48%3A02
Both of them failed in an assertion added by this patch:
TRAP: FailedAssertion("!((rel->rd_rel->relkind == 'r' || rel->rd_rel->relkind == 't') && fsm_local_map.map[oldPage] == 0x01)", File: "/home/bf/build/buildfarm-petalura/HEAD/pgsql.build/../pgsql/src/backend/storage/freespace/freespace.c", Line: 229)
...
2019-02-20 06:23:26.660 CET [5c6ce440.3bd2:4] LOG: server process (PID 17981) was terminated by signal 6: Aborted
2019-02-20 06:23:26.660 CET [5c6ce440.3bd2:5] DETAIL: Failed process was running: INSERT INTO T VALUES ( $1 )
I can even offer you a theory as to why we're managing to get through all
the core regression tests and then failing in ecpg: the failing test is
ecpg/test/thread/prep.pgc, and I think that may be the only place in our
tests where we stress concurrent insertions into a single small table.
So I think you've got race-condition issues here.
It might or might not be significant that these two animals are part
of Andres' JIT-enabled menagerie. I could see that affecting test
timing to the point of showing a symptom that's otherwise hard to hit.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-25 17:44 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 29+ messages in thread
From: Tom Lane @ 2019-02-25 17:44 UTC (permalink / raw)
To: Amit Kapila <akapila@postgresql.org>; +Cc: pgsql-hackers@lists.postgresql.org
I wrote:
> Amit Kapila <akapila@postgresql.org> writes:
>> Avoid creation of the free space map for small heap relations, take 2.
> I think this patch still has some issues.
Just out of curiosity ... how can it possibly be even a little bit sane
that fsm_local_map is a single static data structure, without even any
indication of which table it is for?
If, somehow, there's a rational argument for that design, why is it
not explained in freespace.c? The most charitable interpretation of
what I see there is that it's fatally undercommented.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-26 01:59 Amit Kapila <amit.kapila16@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 29+ messages in thread
From: Amit Kapila @ 2019-02-26 01:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; John Naylor <john.naylor@2ndquadrant.com>
On Mon, Feb 25, 2019 at 10:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <akapila@postgresql.org> writes:
> > Avoid creation of the free space map for small heap relations, take 2.
>
> I think this patch still has some issues.
>
I will look into it today and respond back with my findings. John,
see if you also get time to investigate this.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-26 03:08 John Naylor <john.naylor@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: John Naylor @ 2019-02-26 03:08 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Feb 26, 2019 at 8:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> I will look into it today and respond back with my findings. John,
> see if you also get time to investigate this.
Will do, but it will likely be tomorrow
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-26 03:47 Amit Kapila <amit.kapila16@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 29+ messages in thread
From: Amit Kapila @ 2019-02-26 03:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Feb 25, 2019 at 11:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Amit Kapila <akapila@postgresql.org> writes:
> >> Avoid creation of the free space map for small heap relations, take 2.
>
> > I think this patch still has some issues.
>
> Just out of curiosity ... how can it possibly be even a little bit sane
> that fsm_local_map is a single static data structure, without even any
> indication of which table it is for?
>
It is just for finding a free block among a few blocks of relation.
We clear this when we have found a block with enough free space, when
we extend the relation, or on transaction abort. So, I think we don't
need any per table information.
> If, somehow, there's a rational argument for that design, why is it
> not explained in freespace.c? The most charitable interpretation of
> what I see there is that it's fatally undercommented.
>
There is some explanation in freespace.c and in README
(src/backend/storage/freespace/README). I think we should add some
more information where this data structure (FSMLocalMap) is declared.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-26 03:47 Amit Kapila <amit.kapila16@gmail.com>
parent: John Naylor <john.naylor@2ndquadrant.com>
0 siblings, 0 replies; 29+ messages in thread
From: Amit Kapila @ 2019-02-26 03:47 UTC (permalink / raw)
To: John Naylor <john.naylor@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Feb 26, 2019 at 8:38 AM John Naylor <john.naylor@2ndquadrant.com> wrote:
>
> On Tue, Feb 26, 2019 at 8:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > I will look into it today and respond back with my findings. John,
> > see if you also get time to investigate this.
>
> Will do, but it will likely be tomorrow
>
No problem, thanks!
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-26 09:28 Amit Kapila <amit.kapila16@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 2 replies; 29+ messages in thread
From: Amit Kapila @ 2019-02-26 09:28 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; John Naylor <john.naylor@2ndquadrant.com>; +Cc: Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Feb 25, 2019 at 10:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <akapila@postgresql.org> writes:
> > Avoid creation of the free space map for small heap relations, take 2.
>
> I think this patch still has some issues. Note the following two
> recent buildfarm failures:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=petalura&dt=2019-02-20%2004%3A20%3A01
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dragonet&dt=2019-02-24%2022%3A48%3A02
>
> Both of them failed in an assertion added by this patch:
>
> TRAP: FailedAssertion("!((rel->rd_rel->relkind == 'r' || rel->rd_rel->relkind == 't') && fsm_local_map.map[oldPage] == 0x01)", File: "/home/bf/build/buildfarm-petalura/HEAD/pgsql.build/../pgsql/src/backend/storage/freespace/freespace.c", Line: 229)
> ...
> 2019-02-20 06:23:26.660 CET [5c6ce440.3bd2:4] LOG: server process (PID 17981) was terminated by signal 6: Aborted
> 2019-02-20 06:23:26.660 CET [5c6ce440.3bd2:5] DETAIL: Failed process was running: INSERT INTO T VALUES ( $1 )
>
> I can even offer you a theory as to why we're managing to get through all
> the core regression tests and then failing in ecpg: the failing test is
> ecpg/test/thread/prep.pgc, and I think that may be the only place in our
> tests where we stress concurrent insertions into a single small table.
> So I think you've got race-condition issues here.
>
Right, I think I have some theory what is going on here. There seems
to be a narrow window during which this problem can occur (with
concurrent insertions) where we will try to use the local map for
block which doesn't exist in that map.
During insertion (say by session-1) once we have exhausted all the
pages in local map and didn't find available space, we try to extend
the relation. Note, by this time we haven't cleared the local map.
Now, say we didn't got the conditional relation extension lock (in
function RelationGetBufferForTuple, line 561) and before we get the
relation extension lock, the other backend (say session-2) acquired it
and extended the relation in bulk (via RelationAddExtraBlocks) and
updated the FSM (as the total number of blocks exceeds
HEAP_FSM_CREATION_THRESHOLD). After that, session-1 got the lock and
found the page from FSM which has some freespace. Then before it tries
to validate whether the page has enough space to insert a new tuple,
session-2 again inserted few more rows such that the page which
session-1 has got from FSM doesn't have free space. After that,
session-1 will try to record the freespace for this page and find a
new page from FSM, however as our local map is still not cleared, it
will hit the assertion shown above.
Based on above theory, I have tried to reproduce this problem via
debugger and here are the steps, I have followed.
Session-1
-----------------
1. Create table fsm_1(c1 int, c2 char(1500));
2. Attach debugger, set breakpoint on line no. 535 in hio.c ( aka
targetBlock = RecordAndGetPageWithFreeSpace(relation,)
3. insert into fsm_1 values(generate_series(1,10),'aaaaa');
4. Now, when you debug the code at line 535, you must get
InvalidBlockNumber and you can check that local map will still exist
(value of fsm_local_map.nblocks must be greater than 0).
5. Debug till line 561 (else if
(!ConditionalLockRelationForExtension(relation, ExclusiveLock))).
Session-2
------------------
1. Attach debugger, set breakpoint on line no. 599 in hio.c (aka
buffer = ReadBufferBI(relation, P_NEW, RBM_ZERO_AND_LOCK, bistate);)
2. insert into fsm_1 values(generate_series(11,37),'aaaaa');
3. Now leave the debugger of this session at this stage, this ensures
that Session-2 has RelationExtensionLock
Session-1
----------------
6. Step over line 561.
7. Before acquiring RelationExtension Lock in in line 564
(LockRelationForExtension(relation, ExclusiveLock);), allow Session-2
to complete.
Session-2
---------------
4. continue
5. vacuum fsm_1; --This will allow to create FSM for this table.
Session-1
----------------
8. Step through debugger, you will get the block with freespace from
FSM via GetPageWithFreeSpace.
9. Now, it will allow to try that block (via goto loop).
10. Let the debugger in the same state and allow Session-2 to execute
some statements so that the space in this block is used up.
Session-2
----------------
6. insert into fsm_1 values(generate_series(41,50),'aaaaa');
Session-1
----------------
11. Step through debugger and it should hit Assertion in
RecordAndGetPageWithFreeSpace (line 227, freespace.c)
I am looking at code as of commit -
2ab23445bc6af517bddb40d8429146d8ff8d7ff4. I have also tried to
reproduce it via the ecpg test and my colleague Kuntal Ghosh has
helped me to run that ecpg test by varying THREADS and REPEATS in
ecpg/test/thread/prep.pgc on different machines, but no luck till now.
To fix this symptom, we can ensure that once we didn't get any block
from local map, we must clear it. See the attached patch. I will try
to evaluate this code path to see if there is any similar race
condition and will also try to generate a reproducer. I don't have
any great idea to write a reproducer for this issue apart from trying
some thing similar to ecpg/test/thread/prep.pgc, if you can think of
any, I am all ears.
John, others, can you review my findings and patch?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] fix_loc_map_clear_1.patch (537B, ../../CAA4eK1JtJp-DE6QWMHDdZoU5OJEJssheJ0ZDZSim0MzQv6tYpg@mail.gmail.com/2-fix_loc_map_clear_1.patch)
download | inline diff:
diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index d3f207b854..285936fd87 100644
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -1175,5 +1175,11 @@ fsm_local_search(void)
return target_block;
} while (target_block > 0);
+ /*
+ * If we didn't find any block in the local map, then clear it. This
+ * prevents us to use the map again without setting it first.
+ */
+ FSMClearLocalMap();
+
return InvalidBlockNumber;
}
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-27 04:05 Amit Kapila <amit.kapila16@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 29+ messages in thread
From: Amit Kapila @ 2019-02-27 04:05 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; John Naylor <john.naylor@2ndquadrant.com>; +Cc: Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Feb 26, 2019 at 2:58 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Feb 25, 2019 at 10:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
>
> To fix this symptom, we can ensure that once we didn't get any block
> from local map, we must clear it. See the attached patch. I will try
> to evaluate this code path to see if there is any similar race
> condition and will also try to generate a reproducer. I don't have
> any great idea to write a reproducer for this issue apart from trying
> some thing similar to ecpg/test/thread/prep.pgc, if you can think of
> any, I am all ears.
>
I have tried this test many times (more than 1000 times) by varying
thread count, but couldn't reproduce it. My colleague, Kuntal has
tried a similar test overnight, but the issue didn't reproduce which
is not surprising to me seeing the nature of the problem. As I could
reproduce it via the debugger, I think we can go ahead with the fix.
I have improved the comments in the attached patch and I have also
tried to address Tom's concern w.r.t comments by adding additional
comments atop of data-structure used to maintain the local map.
Let me know what you think?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] fix_loc_map_clear_2.patch (1.5K, ../../CAA4eK1LH3AeHc+DvrYUUq6jMokaSg5MrHZ8o54qT3jUgZZc15g@mail.gmail.com/2-fix_loc_map_clear_2.patch)
download | inline diff:
diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index d3f207b..567745b 100644
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -97,7 +97,18 @@ typedef struct
/* Address of the root page. */
static const FSMAddress FSM_ROOT_ADDRESS = {FSM_ROOT_LEVEL, 0};
-/* Local map of block numbers for small heaps with no FSM. */
+/*
+ * For small relations, we don't create FSM to save space, instead we use
+ * local in-memory map of pages to try. To locate free space, we simply try
+ * pages directly without knowing ahead of time how much free space they have.
+ * To maintain good performance, we only mark every other page as available
+ * in this map.
+ *
+ * Note that this map is used to the find the block with required free space
+ * for any given relation. We clear this map when we have found a block with
+ * enough free space, when we extend the relation, or on transaction abort.
+ * See src/backend/storage/freespace/README for further details.
+ */
typedef struct
{
BlockNumber nblocks;
@@ -1175,5 +1186,13 @@ fsm_local_search(void)
return target_block;
} while (target_block > 0);
+ /*
+ * If we didn't find any available block to try in the local map, then
+ * clear it. This prevents us to use the map again without setting it
+ * first which would otherwise lead to the same conclusion again and
+ * again.
+ */
+ FSMClearLocalMap();
+
return InvalidBlockNumber;
}
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-27 05:19 John Naylor <john.naylor@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 0 replies; 29+ messages in thread
From: John Naylor @ 2019-02-27 05:19 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Feb 26, 2019 at 10:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> John, others, can you review my findings and patch?
I can confirm your findings with your debugging steps. Nice work!
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-27 05:37 John Naylor <john.naylor@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: John Naylor @ 2019-02-27 05:37 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Feb 27, 2019 at 5:06 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> I have tried this test many times (more than 1000 times) by varying
> thread count, but couldn't reproduce it. My colleague, Kuntal has
> tried a similar test overnight, but the issue didn't reproduce which
> is not surprising to me seeing the nature of the problem. As I could
> reproduce it via the debugger, I think we can go ahead with the fix.
> I have improved the comments in the attached patch and I have also
> tried to address Tom's concern w.r.t comments by adding additional
> comments atop of data-structure used to maintain the local map.
The flaw in my thinking was treating extension too similarly too
finding an existing block. With your patch clearing the local map in
the correct place, it seems the call at hio.c:682 is now superfluous?
It wasn't sufficient before, so should this call be removed so as not
to mislead? Or maybe keep it to be safe, with a comment like "This
should already be cleared by now, but make sure it is."
+ * To maintain good performance, we only mark every other page as available
+ * in this map.
I think this implementation detail is not needed to comment on the
struct. I think the pointer to the README is sufficient.
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-28 00:24 Amit Kapila <amit.kapila16@gmail.com>
parent: John Naylor <john.naylor@2ndquadrant.com>
0 siblings, 1 reply; 29+ messages in thread
From: Amit Kapila @ 2019-02-28 00:24 UTC (permalink / raw)
To: John Naylor <john.naylor@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Feb 27, 2019 at 11:07 AM John Naylor
<john.naylor@2ndquadrant.com> wrote:
>
> On Wed, Feb 27, 2019 at 5:06 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > I have tried this test many times (more than 1000 times) by varying
> > thread count, but couldn't reproduce it. My colleague, Kuntal has
> > tried a similar test overnight, but the issue didn't reproduce which
> > is not surprising to me seeing the nature of the problem. As I could
> > reproduce it via the debugger, I think we can go ahead with the fix.
> > I have improved the comments in the attached patch and I have also
> > tried to address Tom's concern w.r.t comments by adding additional
> > comments atop of data-structure used to maintain the local map.
>
> The flaw in my thinking was treating extension too similarly too
> finding an existing block. With your patch clearing the local map in
> the correct place, it seems the call at hio.c:682 is now superfluous?
>
What if get some valid block from the first call to
GetPageWithFreeSpace via local map which has required space? I think
in that case we will need the call at hio.c:682. Am I missing
something?
> It wasn't sufficient before, so should this call be removed so as not
> to mislead? Or maybe keep it to be safe, with a comment like "This
> should already be cleared by now, but make sure it is."
>
> + * To maintain good performance, we only mark every other page as available
> + * in this map.
>
> I think this implementation detail is not needed to comment on the
> struct. I think the pointer to the README is sufficient.
>
Fair enough, I will remove that part of a comment once we agree on the
above point.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-28 02:15 John Naylor <john.naylor@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: John Naylor @ 2019-02-28 02:15 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Feb 28, 2019 at 7:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > The flaw in my thinking was treating extension too similarly too
> > finding an existing block. With your patch clearing the local map in
> > the correct place, it seems the call at hio.c:682 is now superfluous?
>
> What if get some valid block from the first call to
> GetPageWithFreeSpace via local map which has required space? I think
> in that case we will need the call at hio.c:682. Am I missing
> something?
Are you referring to the call at line 393? Then the map will be
cleared on line 507 before we return that buffer.
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-28 02:40 Amit Kapila <amit.kapila16@gmail.com>
parent: John Naylor <john.naylor@2ndquadrant.com>
0 siblings, 1 reply; 29+ messages in thread
From: Amit Kapila @ 2019-02-28 02:40 UTC (permalink / raw)
To: John Naylor <john.naylor@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Feb 28, 2019 at 7:45 AM John Naylor <john.naylor@2ndquadrant.com> wrote:
>
> On Thu, Feb 28, 2019 at 7:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > The flaw in my thinking was treating extension too similarly too
> > > finding an existing block. With your patch clearing the local map in
> > > the correct place, it seems the call at hio.c:682 is now superfluous?
> >
> > What if get some valid block from the first call to
> > GetPageWithFreeSpace via local map which has required space? I think
> > in that case we will need the call at hio.c:682. Am I missing
> > something?
>
> Are you referring to the call at line 393? Then the map will be
> cleared on line 507 before we return that buffer.
>
That's correct, I haven't looked at line number very carefully and
assumed that you are saying to remove the call at 507. I also think
that the call at line 682 doesn't seem to be required, but I would
prefer to keep it for the sake of consistency in this function and
safety purpose. However, I think we should add a comment on the lines
suggested by you. I will send the patch after making these
modifications.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-28 03:25 Amit Kapila <amit.kapila16@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: Amit Kapila @ 2019-02-28 03:25 UTC (permalink / raw)
To: John Naylor <john.naylor@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Feb 28, 2019 at 8:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Feb 28, 2019 at 7:45 AM John Naylor <john.naylor@2ndquadrant.com> wrote:
> >
> > On Thu, Feb 28, 2019 at 7:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > The flaw in my thinking was treating extension too similarly too
> > > > finding an existing block. With your patch clearing the local map in
> > > > the correct place, it seems the call at hio.c:682 is now superfluous?
> > >
> > > What if get some valid block from the first call to
> > > GetPageWithFreeSpace via local map which has required space? I think
> > > in that case we will need the call at hio.c:682. Am I missing
> > > something?
> >
> > Are you referring to the call at line 393? Then the map will be
> > cleared on line 507 before we return that buffer.
> >
>
> That's correct, I haven't looked at line number very carefully and
> assumed that you are saying to remove the call at 507. I also think
> that the call at line 682 doesn't seem to be required, but I would
> prefer to keep it for the sake of consistency in this function and
> safety purpose. However, I think we should add a comment on the lines
> suggested by you. I will send the patch after making these
> modifications.
>
Here's an updated patch based on comments by you. I will proceed with
this unless you have any more comments.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] fix_loc_map_clear_3.patch (2.9K, ../../CAA4eK1KtE_6LaQSz+cq8Yp4TC_BySdVn9UmG0Wi_1EOpdobcWg@mail.gmail.com/2-fix_loc_map_clear_3.patch)
download | inline diff:
From 18514905040a37ca35ad2d96b3e3c54c9a771e59 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Thu, 28 Feb 2019 08:46:20 +0530
Subject: [PATCH] Clear the local map when not used.
After commit b0eaa4c51b, we use a local map of pages to find the required
space for small relations. We do clear this map when we have found a block
with enough free space, when we extend the relation, or on transaction
abort so that it can be used next time. However, we miss to clear it when
we didn't find any pages to try from the map which leads to an assertion
failure when we later tried to use it after relation extension.
In the passing, I have improved some comments in this area.
Reported-by: Tom Lane based on buildfarm results
Author: Amit Kapila
Reviewed-by: John Naylor
Tested-by: Kuntal Ghosh
Discussion: https://postgr.es/m/32368.1551114120@sss.pgh.pa.us
---
src/backend/access/heap/hio.c | 5 +----
src/backend/storage/freespace/freespace.c | 19 ++++++++++++++++++-
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index a9c8ec4..69a7a23 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -675,10 +675,7 @@ loop:
*/
RelationSetTargetBlock(relation, BufferGetBlockNumber(buffer));
- /*
- * In case we used an in-memory map of available blocks, reset it for next
- * use.
- */
+ /* This should already be cleared by now, but make sure it is. */
FSMClearLocalMap();
return buffer;
diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index d3f207b..7f80504 100644
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -97,7 +97,16 @@ typedef struct
/* Address of the root page. */
static const FSMAddress FSM_ROOT_ADDRESS = {FSM_ROOT_LEVEL, 0};
-/* Local map of block numbers for small heaps with no FSM. */
+/*
+ * For small relations, we don't create FSM to save space, instead we use
+ * local in-memory map of pages to try. To locate free space, we simply try
+ * pages directly without knowing ahead of time how much free space they have.
+ *
+ * Note that this map is used to the find the block with required free space
+ * for any given relation. We clear this map when we have found a block with
+ * enough free space, when we extend the relation, or on transaction abort.
+ * See src/backend/storage/freespace/README for further details.
+ */
typedef struct
{
BlockNumber nblocks;
@@ -1175,5 +1184,13 @@ fsm_local_search(void)
return target_block;
} while (target_block > 0);
+ /*
+ * If we didn't find any available block to try in the local map, then
+ * clear it. This prevents us to use the map again without setting it
+ * first which would otherwise lead to the same conclusion again and
+ * again.
+ */
+ FSMClearLocalMap();
+
return InvalidBlockNumber;
}
--
1.8.3.1
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-02-28 04:29 John Naylor <john.naylor@2ndquadrant.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: John Naylor @ 2019-02-28 04:29 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Feb 28, 2019 at 10:25 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> Here's an updated patch based on comments by you. I will proceed with
> this unless you have any more comments.
Looks good to me. I would just adjust the grammar in the comment, from
"This prevents us to use the map", to "This prevents us from using the
map". Perhaps also a comma after "first".
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: pgsql: Avoid creation of the free space map for small heap relations, t
@ 2019-03-01 02:19 Amit Kapila <amit.kapila16@gmail.com>
parent: John Naylor <john.naylor@2ndquadrant.com>
0 siblings, 0 replies; 29+ messages in thread
From: Amit Kapila @ 2019-03-01 02:19 UTC (permalink / raw)
To: John Naylor <john.naylor@2ndquadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Amit Kapila <akapila@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Feb 28, 2019 at 9:59 AM John Naylor <john.naylor@2ndquadrant.com> wrote:
>
> On Thu, Feb 28, 2019 at 10:25 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Here's an updated patch based on comments by you. I will proceed with
> > this unless you have any more comments.
>
> Looks good to me. I would just adjust the grammar in the comment, from
> "This prevents us to use the map", to "This prevents us from using the
> map". Perhaps also a comma after "first".
>
Okay, pushed after making changes suggested by you.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 29+ messages in thread
end of thread, other threads:[~2019-03-01 02:19 UTC | newest]
Thread overview: 29+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-01-28 02:47 pgsql: Avoid creation of the free space map for small heap relations. Amit Kapila <akapila@postgresql.org>
2019-01-28 03:19 ` Amit Kapila <amit.kapila16@gmail.com>
2019-01-28 04:17 ` Amit Kapila <amit.kapila16@gmail.com>
2019-01-28 04:55 ` Andrew Gierth <andrew@tao11.riddles.org.uk>
2019-01-28 05:37 ` Amit Kapila <amit.kapila16@gmail.com>
2019-01-28 05:40 ` Andrew Gierth <andrew@tao11.riddles.org.uk>
2019-01-28 05:57 ` Amit Kapila <amit.kapila16@gmail.com>
2019-01-28 06:20 ` John Naylor <john.naylor@2ndquadrant.com>
2019-02-04 02:17 ` Michael Paquier <michael@paquier.xyz>
2019-01-28 05:48 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-01-28 05:56 ` Amit Kapila <amit.kapila16@gmail.com>
2019-01-28 06:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-02-04 02:22 pgsql: Avoid creation of the free space map for small heap relations, t Amit Kapila <akapila@postgresql.org>
2019-02-25 17:02 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t Tom Lane <tgl@sss.pgh.pa.us>
2019-02-25 17:44 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t Tom Lane <tgl@sss.pgh.pa.us>
2019-02-26 03:47 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t Amit Kapila <amit.kapila16@gmail.com>
2019-02-26 01:59 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t Amit Kapila <amit.kapila16@gmail.com>
2019-02-26 03:08 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t John Naylor <john.naylor@2ndquadrant.com>
2019-02-26 03:47 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t Amit Kapila <amit.kapila16@gmail.com>
2019-02-26 09:28 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t Amit Kapila <amit.kapila16@gmail.com>
2019-02-27 04:05 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t Amit Kapila <amit.kapila16@gmail.com>
2019-02-27 05:37 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t John Naylor <john.naylor@2ndquadrant.com>
2019-02-28 00:24 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t Amit Kapila <amit.kapila16@gmail.com>
2019-02-28 02:15 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t John Naylor <john.naylor@2ndquadrant.com>
2019-02-28 02:40 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t Amit Kapila <amit.kapila16@gmail.com>
2019-02-28 03:25 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t Amit Kapila <amit.kapila16@gmail.com>
2019-02-28 04:29 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t John Naylor <john.naylor@2ndquadrant.com>
2019-03-01 02:19 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t Amit Kapila <amit.kapila16@gmail.com>
2019-02-27 05:19 ` Re: pgsql: Avoid creation of the free space map for small heap relations, t John Naylor <john.naylor@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox