Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k8NwU-00063B-G8 for pgsql-hackers@arkaria.postgresql.org; Wed, 19 Aug 2020 13:15:51 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1k8NwT-00048R-8p for pgsql-hackers@arkaria.postgresql.org; Wed, 19 Aug 2020 13:15:49 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k8NwS-00048I-Qw for pgsql-hackers@lists.postgresql.org; Wed, 19 Aug 2020 13:15:49 +0000 Received: from mail.postgrespro.ru ([93.174.131.139]) by magus.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1k8NwN-00006b-KO for pgsql-hackers@postgresql.org; Wed, 19 Aug 2020 13:15:46 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.postgrespro.ru (Postfix) with ESMTP id EEA0B21C2E0B; Wed, 19 Aug 2020 16:15:41 +0300 (MSK) X-Virus-Scanned: Debian amavisd-new at postgrespro.ru X-Spam-Flag: NO X-Spam-Score: 0 X-Spam-Level: X-Spam-Status: No, score=x tagged_above=-99 required=4 WHITELISTED tests=[] autolearn=unavailable Received: from [192.168.27.134] (unknown [93.174.131.141]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mail.postgrespro.ru (Postfix) with ESMTPSA id BCEDA21C2DF7; Wed, 19 Aug 2020 16:15:41 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mail; t=1597842941; bh=cqf6BxqI9Q8RPdSSMi6UbJq9SiiPsxSLWqvu/X0BGI8=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=mEZ+Ld4DMFwuNVwJ8ffNuPtQmnNKYYcJEb0cwxnGt2FB8SqhcOh+4o6nOzyCjQIIi kQLWsVa8HdD9JjO8r3hegctTjNoynCcd9AdjWTG+Bc4dd5WamIZemp6Kxz84e2SNib hcDN1fUyg639Eav03AA1ck1RlbuliwPjpur1UOCU= Subject: Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits To: Alvaro Herrera , Ibrar Ahmed Cc: Robert Haas , Daniel Gustafsson , Justin Pryzby , pgsql-hackers , Andres Freund References: <20200817235413.GA11940@alvherre.pgsql> From: Anastasia Lubennikova Message-ID: Date: Wed, 19 Aug 2020 16:15:40 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200817235413.GA11940@alvherre.pgsql> Content-Type: multipart/mixed; boundary="------------050B3F4A6532E47E6BDA7FDC" Content-Language: en-US List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk This is a multi-part message in MIME format. --------------050B3F4A6532E47E6BDA7FDC Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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. I've run some tests on my laptop and COPY FREEZE shows the same time for both versions, while VACUUM is much faster on the patched version. I've also checked WAL generation and it shows that the patch works correctly as it doesn't add any records for COPY. Not patched: Time: 54883,356 ms (00:54,883) Time: 65373,333 ms (01:05,373) Time: 64684,592 ms (01:04,685) VACUUM Time: 60861,670 ms (01:00,862) COPY      wal_bytes 3765 MB VACUUM wal_bytes 6015 MB table size                 5971 MB Patched: Time: 53142,947 ms (00:53,143) Time: 65420,812 ms (01:05,421) Time: 66600,114 ms (01:06,600) VACUUM Time: 63,401 ms COPY      wal_bytes 3765 MB VACUUM wal_bytes 30 kB table size                 5971 MB The test script is attached. > Also, when Andres posted this patch first, he said this was only for > heap_multi_insert because it was a prototype. But I think we expect > that the table_insert path (CIM_SINGLE mode in copy) should also receive > that treatment. I am afraid that extra checks for COPY FREEZE  in heap_insert() will slow down normal insertions. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company --------------050B3F4A6532E47E6BDA7FDC Content-Type: text/x-patch; charset=UTF-8; name="copy-freeze-vm_freeze_v2.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="copy-freeze-vm_freeze_v2.patch" diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out index 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.sql index 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.c index 8eb276e464..37c182a6dc 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 @@ -8255,7 +8306,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; @@ -8339,6 +8391,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.c index aa3f14c019..ec435869cf 100644 --- a/src/backend/access/heap/hio.c +++ b/src/backend/access/heap/hio.c @@ -142,6 +142,9 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2, /* Figure out which pins we need but don't have. */ need_to_pin_buffer1 = PageIsAllVisible(BufferGetPage(buffer1)) && !visibilitymap_pin_ok(block1, *vmbuffer1); + + // TODO do we need special code for COPY FREEZE here? + need_to_pin_buffer2 = buffer2 != InvalidBuffer && PageIsAllVisible(BufferGetPage(buffer2)) && !visibilitymap_pin_ok(block2, *vmbuffer2); @@ -422,6 +425,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 +619,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.h index 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. */ --------------050B3F4A6532E47E6BDA7FDC Content-Type: application/sql; name="copy_freeze_test.sql" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="copy_freeze_test.sql" Q1JFQVRFIEVYVEVOU0lPTiBwZ19zdGF0X3N0YXRlbWVudHM7CgpcdGltaW5nCgpiZWdpbjsK Ci0tRFJPUCBUQUJMRSBsYXJnZV90ZXN0OwpDUkVBVEUgVEFCTEUgbGFyZ2VfdGVzdCAobnVt MSBiaWdpbnQsIG51bTIgZG91YmxlIHByZWNpc2lvbiwgbnVtMyBkb3VibGUgcHJlY2lzaW9u KTsKCi0tIGdlbmVyYXRlIHRlc3QgZGF0YQotLSBDT1BZIChTRUxFQ1Qgcm91bmQocmFuZG9t KCkqMTAwKSwgcmFuZG9tKCksIHJhbmRvbSgpKjE0MiAgICAgICAgIAotLSAgICAgIEZST00g Z2VuZXJhdGVfc2VyaWVzKDEsIDQwMDAwMDAwKSBzKGkpKSAgICAgICAgICAgICAgICAgICAg ICAgICAKLS0gICAgICAgVE8gJy9ob21lL2FuYXN0YXNpYS9vcmlnaW5hbF9yZXBvL2xhcmdl X3Rlc3QuY3N2JyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBXSVRIIChmb3JtYXQgJ2NzdicsIGVz Y2FwZSAnIicsIGRlbGltaXRlciAnLCcsIHF1b3RlICciJyk7CgpDT1BZIGxhcmdlX3Rlc3Qg RlJPTSAnL2hvbWUvYW5hc3Rhc2lhL29yaWdpbmFsX3JlcG8vbGFyZ2VfdGVzdC5jc3YnIFdJ VEggKGZvcm1hdCAnY3N2JywgZXNjYXBlICciJywgZGVsaW1pdGVyICcsJywgcXVvdGUgJyIn LCBGUkVFWkUpOwpDT1BZIGxhcmdlX3Rlc3QgRlJPTSAnL2hvbWUvYW5hc3Rhc2lhL29yaWdp bmFsX3JlcG8vbGFyZ2VfdGVzdC5jc3YnIFdJVEggKGZvcm1hdCAnY3N2JywgZXNjYXBlICci JywgZGVsaW1pdGVyICcsJywgcXVvdGUgJyInLCBGUkVFWkUpIDsKQ09QWSBsYXJnZV90ZXN0 IEZST00gJy9ob21lL2FuYXN0YXNpYS9vcmlnaW5hbF9yZXBvL2xhcmdlX3Rlc3QuY3N2JyBX SVRIIChmb3JtYXQgJ2NzdicsIGVzY2FwZSAnIicsIGRlbGltaXRlciAnLCcsIHF1b3RlICci JywgRlJFRVpFKSA7Cgpjb21taXQ7CgpcZCsKClZBQ1VVTSBsYXJnZV90ZXN0OwoKU0VMRUNU IHF1ZXJ5LCB3YWxfcmVjb3Jkcywgd2FsX2J5dGVzLCBwZ19zaXplX3ByZXR0eSh3YWxfYnl0 ZXMpIGFzIHdhbF9ieXRlc19wcmV0dHkKICAgRlJPTSBwZ19zdGF0X3N0YXRlbWVudHMgd2hl cmUgcXVlcnkgbGlrZSAnVkFDVVVNJScgT1IgcXVlcnkgbGlrZSAnQ09QWSUnOwoK --------------050B3F4A6532E47E6BDA7FDC--