From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
To: Ibrar Ahmed <ibrar.ahmad@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Daniel Gustafsson <daniel@yesql.se>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Subject: Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits
Date: Thu, 27 Aug 2020 00:14:34 +0300
Message-ID: <0f9b217d-3d05-fb0e-b228-7f57c3720a58@postgrespro.ru> (raw)
In-Reply-To: <CALtqXTeEpfYwwAAtbW4RkZLNQoCJKfvhCF6syKGTmc5VEpuh8w@mail.gmail.com>
References: <20200817235413.GA11940@alvherre.pgsql>
<d8450220-00d1-0c88-b3cc-eede5976ebb9@postgrespro.ru>
<CALtqXTeEpfYwwAAtbW4RkZLNQoCJKfvhCF6syKGTmc5VEpuh8w@mail.gmail.com>
On 21.08.2020 19:43, Ibrar Ahmed wrote:
>
>
> On Wed, Aug 19, 2020 at 6:15 PM Anastasia Lubennikova
> <a.lubennikova@postgrespro.ru <mailto:a.lubennikova@postgrespro.ru>>
> wrote:
>
> On 18.08.2020 02:54, Alvaro Herrera wrote:
> > On 2020-Aug-14, Ibrar Ahmed wrote:
> >
> >> The table used for the test contains three columns (integer, text,
> >> varchar).
> >> The total number of rows is 10000000 in total.
> >>
> >> Unpatched (Master: 92c12e46d5f1e25fc85608a6d6a19b8f5ea02600)
> >> COPY: 9069.432 ms vacuum; 2567.961ms
> >> COPY: 9004.533 ms vacuum: 2553.075ms
> >> COPY: 8832.422 ms vacuum: 2540.742ms
> >>
> >> Patched (Master: 92c12e46d5f1e25fc85608a6d6a19b8f5ea02600)
> >> COPY: 10031.723 ms vacuum: 127.524 ms
> >> COPY: 9985.109 ms vacuum: 39.953 ms
> >> COPY: 9283.373 ms vacuum: 37.137 ms
> >>
> >> Time to take the copy slightly increased but the vacuum time
> significantly
> >> decrease.
> > "Slightly"? It seems quite a large performance drop to me --
> more than
> > 10%. Where is that time being spent? Andres said in [1] that he
> > thought the performance shouldn't be affected noticeably, but this
> > doesn't seem to hold true. As I understand, the idea was that there
> > would be little or no additional WAL records .. only flags in the
> > existing record. So what is happening?
> >
> > [1]
> https://postgr.es/m/20190408010427.4l63qr7h2fjcyp77@alap3.anarazel.de
>
> I agree that 10% performance drop is not what we expect with this
> patch.
> Ibrar, can you share more info about your tests? I'd like to
> reproduce
> this slowdown and fix it, if necessary.
>
>
> Here is my test;
>
> postgres=# BEGIN;
>
> BEGIN
>
>
> postgres=*# TRUNCATE foo;
>
> TRUNCATE TABLE
>
>
> postgres=*# COPY foo(id, name, address) FROM '/home/ibrar/bar.csv'
> DELIMITER ',' FREEZE;
>
> COPY 10000000
>
>
>
> --
> Ibrar Ahmed
I've repeated the test and didn't notice any slowdown for COPY FREEZE.
Test data is here [1].
The numbers do fluctuate a bit, but there is no dramatic difference
between master and patched version. So I assume that the performance
drop in your test has something to do with the measurement error.
Unless, you have some non-default configuration that could affect it.
patched:
COPY: 12327,090 ms vacuum: 37,555 ms
COPY: 12939,540 ms vacuum: 35,703 ms
COPY: 12245,819 ms vacuum: 36,273 ms
master:
COPY
COPY: 13253,605 ms vacuum: 3592,849 ms
COPY: 12619,428 ms vacuum: 4253,836 ms
COPY: 12512,940 ms vacuum: 4009,847 ms
I also slightly cleaned up comments, so the new version of the patch is
attached. As this is just a performance optimization documentation is
not needed. It would be great, if other reviewers could run some
independent performance tests, as I believe that this patch is ready for
committer.
[1] https://drive.google.com/file/d/11r19NX6yyPjvxdDub8Ce-kmApRurp4Nx/view
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companyt
Attachments:copy-freeze-vm_freeze_v3.patch (text/x-patch, 11.5K) downloaddiff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.outindex ca4b6e186b..0017e3415c 100644--- a/contrib/pg_visibility/expected/pg_visibility.out+++ b/contrib/pg_visibility/expected/pg_visibility.out@@ -179,6 +179,69 @@ select pg_truncate_visibility_map('test_partition');
(1 row)
+-- test copy freeze+create table copyfreeze (a int, b char(1500));+-- load all rows via COPY FREEZE and ensure that all pages are set all-visible+-- and all-frozen.+begin;+truncate copyfreeze;+copy copyfreeze from stdin freeze;+commit;+select * from pg_visibility_map('copyfreeze');+ blkno | all_visible | all_frozen +-------+-------------+------------+ 0 | t | t+ 1 | t | t+ 2 | t | t+(3 rows)++select * from pg_check_frozen('copyfreeze');+ t_ctid +--------+(0 rows)++-- load half the rows via regular COPY and rest via COPY FREEZE. The pages+-- which are touched by regular COPY must not be set all-visible/all-frozen. On+-- the other hand, pages allocated by COPY FREEZE should be marked+-- all-frozen/all-visible.+begin;+truncate copyfreeze;+copy copyfreeze from stdin;+copy copyfreeze from stdin freeze;+commit;+select * from pg_visibility_map('copyfreeze');+ blkno | all_visible | all_frozen +-------+-------------+------------+ 0 | f | f+ 1 | f | f+ 2 | t | t+(3 rows)++select * from pg_check_frozen('copyfreeze');+ t_ctid +--------+(0 rows)++-- Try a mix of regular COPY and COPY FREEZE.+begin;+truncate copyfreeze;+copy copyfreeze from stdin freeze;+copy copyfreeze from stdin;+copy copyfreeze from stdin freeze;+commit;+select * from pg_visibility_map('copyfreeze');+ blkno | all_visible | all_frozen +-------+-------------+------------+ 0 | t | t+ 1 | f | f+ 2 | t | t+(3 rows)++select * from pg_check_frozen('copyfreeze');+ t_ctid +--------+(0 rows)+
-- cleanup
drop table test_partitioned;
drop view test_view;
@@ -188,3 +251,4 @@ drop server dummy_server;
drop foreign data wrapper dummy;
drop materialized view matview_visibility_test;
drop table regular_table;
+drop table copyfreeze;diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sqlindex f79b54480b..ec1afd4906 100644--- a/contrib/pg_visibility/sql/pg_visibility.sql+++ b/contrib/pg_visibility/sql/pg_visibility.sql@@ -94,6 +94,82 @@ select count(*) > 0 from pg_visibility_map_summary('test_partition');
select * from pg_check_frozen('test_partition'); -- hopefully none
select pg_truncate_visibility_map('test_partition');
+-- test copy freeze+create table copyfreeze (a int, b char(1500));++-- load all rows via COPY FREEZE and ensure that all pages are set all-visible+-- and all-frozen.+begin;+truncate copyfreeze;+copy copyfreeze from stdin freeze;+1 '1'+2 '2'+3 '3'+4 '4'+5 '5'+6 '6'+7 '7'+8 '8'+9 '9'+10 '10'+11 '11'+12 '12'+\.+commit;+select * from pg_visibility_map('copyfreeze');+select * from pg_check_frozen('copyfreeze');++-- load half the rows via regular COPY and rest via COPY FREEZE. The pages+-- which are touched by regular COPY must not be set all-visible/all-frozen. On+-- the other hand, pages allocated by COPY FREEZE should be marked+-- all-frozen/all-visible.+begin;+truncate copyfreeze;+copy copyfreeze from stdin;+1 '1'+2 '2'+3 '3'+4 '4'+5 '5'+6 '6'+\.+copy copyfreeze from stdin freeze;+7 '7'+8 '8'+9 '9'+10 '10'+11 '11'+12 '12'+\.+commit;+select * from pg_visibility_map('copyfreeze');+select * from pg_check_frozen('copyfreeze');++-- Try a mix of regular COPY and COPY FREEZE.+begin;+truncate copyfreeze;+copy copyfreeze from stdin freeze;+1 '1'+2 '2'+3 '3'+4 '4'+5 '5'+\.+copy copyfreeze from stdin;+6 '6'+\.+copy copyfreeze from stdin freeze;+7 '7'+8 '8'+9 '9'+10 '10'+11 '11'+12 '12'+\.+commit;+select * from pg_visibility_map('copyfreeze');+select * from pg_check_frozen('copyfreeze');+
-- cleanup
drop table test_partitioned;
drop view test_view;
@@ -103,3 +179,4 @@ drop server dummy_server;
drop foreign data wrapper dummy;
drop materialized view matview_visibility_test;
drop table regular_table;
+drop table copyfreeze;diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.cindex 9b5f417eac..338854d4ae 100644--- a/src/backend/access/heap/heapam.c+++ b/src/backend/access/heap/heapam.c@@ -2105,6 +2105,7 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
int ndone;
PGAlignedBlock scratch;
Page page;
+ Buffer vmbuffer = InvalidBuffer;
bool needwal;
Size saveFreeSpace;
bool need_tuple_data = RelationIsLogicallyLogged(relation);
@@ -2159,8 +2160,9 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
while (ndone < ntuples)
{
Buffer buffer;
- Buffer vmbuffer = InvalidBuffer;+ bool starting_with_empty_page;
bool all_visible_cleared = false;
+ bool all_frozen_set = false;
int nthispage;
CHECK_FOR_INTERRUPTS();
@@ -2168,12 +2170,20 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
/*
* Find buffer where at least the next tuple will fit. If the page is
* all-visible, this will also pin the requisite visibility map page.
+ *+ * Also pin visibility map page if COPY FREEZE inserts tuples into an+ * empty page. See all_frozen_set below.
*/
buffer = RelationGetBufferForTuple(relation, heaptuples[ndone]->t_len,
InvalidBuffer, options, bistate,
&vmbuffer, NULL);
page = BufferGetPage(buffer);
+ starting_with_empty_page = PageGetMaxOffsetNumber(page) == 0;++ if (starting_with_empty_page && (options & HEAP_INSERT_FROZEN))+ all_frozen_set = true;+
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();
@@ -2207,7 +2217,11 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
log_heap_new_cid(relation, heaptup);
}
- if (PageIsAllVisible(page))+ /*+ * If the page is all visible, need to clear that, unless we're only+ * going to add further frozen rows to it.+ */+ if (PageIsAllVisible(page) && !(options & HEAP_INSERT_FROZEN))
{
all_visible_cleared = true;
PageClearAllVisible(page);
@@ -2215,6 +2229,12 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
BufferGetBlockNumber(buffer),
vmbuffer, VISIBILITYMAP_VALID_BITS);
}
+ /*+ * If we're only adding already frozen rows, and the page was+ * previously empty, mark it as all-visible.+ */+ else if (all_frozen_set)+ PageSetAllVisible(page);
/*
* XXX Should we set PageSetPrunable on this page ? See heap_insert()
@@ -2238,8 +2258,7 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
* If the page was previously empty, we can reinit the page
* instead of restoring the whole thing.
*/
- init = (ItemPointerGetOffsetNumber(&(heaptuples[ndone]->t_self)) == FirstOffsetNumber &&- PageGetMaxOffsetNumber(page) == FirstOffsetNumber + nthispage - 1);+ init = starting_with_empty_page;
/* allocate xl_heap_multi_insert struct from the scratch area */
xlrec = (xl_heap_multi_insert *) scratchptr;
@@ -2257,7 +2276,13 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
/* the rest of the scratch space is used for tuple data */
tupledata = scratchptr;
- xlrec->flags = all_visible_cleared ? XLH_INSERT_ALL_VISIBLE_CLEARED : 0;+ xlrec->flags = 0;+ Assert (!(all_visible_cleared && all_frozen_set));+ if (all_visible_cleared)+ xlrec->flags |= XLH_INSERT_ALL_VISIBLE_CLEARED;+ if (all_frozen_set)+ xlrec->flags |= XLH_INSERT_ALL_FROZEN_SET;+
xlrec->ntuples = nthispage;
/*
@@ -2331,13 +2356,39 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
END_CRIT_SECTION();
- UnlockReleaseBuffer(buffer);- if (vmbuffer != InvalidBuffer)- ReleaseBuffer(vmbuffer);+ if (all_frozen_set)+ {+ Assert(PageIsAllVisible(page));+ /*+ * vmbuffer should be already pinned by RelationGetBufferForTuple,+ * Though, it's fine if is not. all_frozen is just an optimization.+ */+ if (visibilitymap_pin_ok(BufferGetBlockNumber(buffer), vmbuffer))+ /*+ * It's fine to use InvalidTransactionId here - this is only used+ * when HEAP_INSERT_FROZEN is specified, which intentionally+ * violates visibility rules.+ */+ visibilitymap_set(relation, BufferGetBlockNumber(buffer), buffer,+ InvalidXLogRecPtr, vmbuffer,+ InvalidTransactionId,+ VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);+ }++ UnlockReleaseBuffer(buffer);
ndone += nthispage;
++ /*+ * NB: Only release vmbuffer after inserting all tuples - it's fairly+ * likely that we'll insert into subsequent heap pages that are likely+ * to use the same vm page.+ */
}
+ if (vmbuffer != InvalidBuffer)+ ReleaseBuffer(vmbuffer);+
/*
* We're done with the actual inserts. Check for conflicts again, to
* ensure that all rw-conflicts in to these inserts are detected. Without
@@ -8246,7 +8297,8 @@ heap_xlog_multi_insert(XLogReaderState *record)
* The visibility map may need to be fixed even if the heap page is
* already up-to-date.
*/
- if (xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED)+ if ((xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED) &&+ !(xlrec->flags & XLH_INSERT_ALL_FROZEN_SET))
{
Relation reln = CreateFakeRelcacheEntry(rnode);
Buffer vmbuffer = InvalidBuffer;
@@ -8330,6 +8382,8 @@ heap_xlog_multi_insert(XLogReaderState *record)
if (xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED)
PageClearAllVisible(page);
+ if (xlrec->flags & XLH_INSERT_ALL_FROZEN_SET)+ PageSetAllVisible(page);
MarkBufferDirty(buffer);
}
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.cindex aa3f14c019..b264ff53d7 100644--- a/src/backend/access/heap/hio.c+++ b/src/backend/access/heap/hio.c@@ -422,6 +422,14 @@ loop:
buffer = ReadBufferBI(relation, targetBlock, RBM_NORMAL, bistate);
if (PageIsAllVisible(BufferGetPage(buffer)))
visibilitymap_pin(relation, targetBlock, vmbuffer);
+ /*+ * This is for COPY FREEZE needs. If page is empty,+ * pin vmbuffer to set all_frozen bit+ */+ if ((options & HEAP_INSERT_FROZEN) &&+ (PageGetMaxOffsetNumber(BufferGetPage(buffer)) == 0))+ visibilitymap_pin(relation, BufferGetBlockNumber(buffer), vmbuffer);+
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
}
else if (otherBlock == targetBlock)
@@ -608,6 +616,16 @@ loop:
PageInit(page, BufferGetPageSize(buffer), 0);
MarkBufferDirty(buffer);
+ /*+ * This is for COPY FREEZE needs. If page is empty,+ * pin vmbuffer to set all_frozen bit+ */+ if ((options & HEAP_INSERT_FROZEN))+ {+ Assert(PageGetMaxOffsetNumber(BufferGetPage(buffer)) == 0);+ visibilitymap_pin(relation, BufferGetBlockNumber(buffer), vmbuffer);+ }+
/*
* Release the file-extension lock; it's now OK for someone else to extend
* the relation some more.
diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.hindex aa17f7df84..16c1fd5673 100644--- a/src/include/access/heapam_xlog.h+++ b/src/include/access/heapam_xlog.h@@ -69,6 +69,9 @@
#define XLH_INSERT_CONTAINS_NEW_TUPLE (1<<3)
#define XLH_INSERT_ON_TOAST_RELATION (1<<4)
+/* all_frozen_set always implies all_visible_set */+#define XLH_INSERT_ALL_FROZEN_SET (1<<5)+
/*
* xl_heap_update flag values, 8 bits are available.
*/
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.lubennikova@postgrespro.ru, ibrar.ahmad@gmail.com, alvherre@2ndquadrant.com, robertmhaas@gmail.com, daniel@yesql.se, pryzby@telsasoft.com, andres@anarazel.de
Subject: Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits
In-Reply-To: <0f9b217d-3d05-fb0e-b228-7f57c3720a58@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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