From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
To: Daniel Gustafsson <daniel@yesql.se>
To: Ibrar Ahmed <ibrar.ahmad@gmail.com>
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: Tue, 14 Jul 2020 20:51:37 +0300
Message-ID: <f263b707-fb89-007d-2892-7212a3ecaea4@postgrespro.ru> (raw)
In-Reply-To: <74D1F1CD-40A6-42C8-8FD8-65AB31D6D067@yesql.se>
References: <20190405030604.GA3360@alvherre.pgsql>
<20190405033537.vklegbflesm2c7m3@alap3.anarazel.de>
<959.1554436678@sss.pgh.pa.us>
<20190405040449.5yltddxmlxombova@alap3.anarazel.de>
<20190408010427.4l63qr7h2fjcyp77@alap3.anarazel.de>
<20190528153626.st5yxxri7d7teb5c@alap3.anarazel.de>
<CABOikdOHfJ7ZtXc=8LgJEZCSzAgVacvCF6QpX2SqGE5c+5v8YA@mail.gmail.com>
<CABOikdPa22iZL_EanEWa=Fg3KWLPf6R2pk+XKV8Zus6pNn1PrQ@mail.gmail.com>
<CAA4eK1+OH-VFCn7dBj5TGtEO-Xo4OmTv7-=Zh2qddgPzq89rSQ@mail.gmail.com>
<CALtqXTcLPm3j1gEXRNU81tDhqe9e96dMCidpeYNmub7=D1bdYg@mail.gmail.com>
<20200313015834.GJ29065@telsasoft.com>
<CALtqXTdXh8iUn=-tMkBT5Z0ptBikvehvcahr+qz9WHe+=q0Q2Q@mail.gmail.com>
<CALtqXTdwh06yNn_DzQDOKaGFyY750Vw5WdipZOF=XHGrWKdxOw@mail.gmail.com>
<74D1F1CD-40A6-42C8-8FD8-65AB31D6D067@yesql.se>
On 01.07.2020 12:38, Daniel Gustafsson wrote:
> This patch incurs a compiler warning, which probably is quite simple to fix:
>
> heapam.c: In function ‘heap_multi_insert’:
> heapam.c:2349:4: error: ‘recptr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> visibilitymap_set(relation, BufferGetBlockNumber(buffer), buffer,
> ^
> heapam.c:2136:14: note: ‘recptr’ was declared here
> XLogRecPtr recptr;
> ^
>
> Please fix and submit a new version, I'm marking the entry Waiting on Author in
> the meantime.
>
> cheers ./daniel
>
This patch looks very useful to me, so I want to pick it up.
The patch that fixes the compiler warning is in the attachment. Though,
I'm not
entirely satisfied with this fix. Also, the patch contains some FIXME
comments.
I'll test it more and send fixes this week.
Questions from the first review pass:
1) Do we need XLH_INSERT_ALL_VISIBLE_SET ? IIUC, in the patch it is always
implied by XLH_INSERT_ALL_FROZEN_SET.
2) What does this comment mean? Does XXX refers to the lsn comparison?
Since it
is definitely necessary to update the VM.
+ * XXX: This seems entirely unnecessary?
+ *
+ * FIXME: Theoretically we should only do this after we've
+ * modified the heap - but it's safe to do it here I think,
+ * because this means that the page previously was empty.
+ */
+ if (lsn > PageGetLSN(vmpage))
+ visibilitymap_set(reln, blkno, InvalidBuffer, lsn,
vmbuffer,
+ InvalidTransactionId, vmbits);
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:0005-copy-freeze-should-actually-freeze-right.patch (text/x-patch, 12.8K) 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 d881f4cd46..1ee9dc78dd 100644--- a/src/backend/access/heap/heapam.c+++ b/src/backend/access/heap/heapam.c@@ -2074,6 +2074,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);
@@ -2128,9 +2129,11 @@ 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;
+ XLogRecPtr recptr;
CHECK_FOR_INTERRUPTS();
@@ -2143,6 +2146,11 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
&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();
@@ -2176,7 +2184,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);
@@ -2184,6 +2196,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()
@@ -2194,7 +2212,6 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
/* XLOG stuff */
if (needwal)
{
- XLogRecPtr recptr;
xl_heap_multi_insert *xlrec;
uint8 info = XLOG_HEAP2_MULTI_INSERT;
char *tupledata;
@@ -2207,8 +2224,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;
@@ -2226,7 +2242,16 @@ 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_VISIBLE_SET;+ xlrec->flags |= XLH_INSERT_ALL_FROZEN_SET;+ }+
xlrec->ntuples = nthispage;
/*
@@ -2300,13 +2325,46 @@ 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));++ /*+ * Having to potentially read the page while holding an exclusive+ * lock on the page isn't great. But we only get here if+ * HEAP_INSERT_FROZEN is set, and we only do so if the table isn't+ * readable outside of this session. Therefore doing IO here isn't+ * that bad.+ */+ visibilitymap_pin(relation, BufferGetBlockNumber(buffer), &vmbuffer);++ /*+ * FIXME: setting recptr here is a dirty dirty hack, to prevent+ * visibilitymap_set() from WAL logging.+ *+ * 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,+ needwal ? recptr: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
@@ -8193,6 +8251,7 @@ heap_xlog_multi_insert(XLogReaderState *record)
BlockNumber blkno;
Buffer buffer;
Page page;
+ Page vmpage;
union
{
HeapTupleHeaderData hdr;
@@ -8217,13 +8276,54 @@ 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 |+ XLH_INSERT_ALL_VISIBLE_SET |+ XLH_INSERT_ALL_FROZEN_SET))
{
Relation reln = CreateFakeRelcacheEntry(rnode);
Buffer vmbuffer = InvalidBuffer;
visibilitymap_pin(reln, blkno, &vmbuffer);
- visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS);++ if (xlrec->flags & (XLH_INSERT_ALL_VISIBLE_CLEARED))+ {+ Assert(!(xlrec->flags & (XLH_INSERT_ALL_FROZEN_SET |+ XLH_INSERT_ALL_VISIBLE_SET)));+ visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS);+ }+ else+ {+ int vmbits = 0;++ if (xlrec->flags & (XLH_INSERT_ALL_VISIBLE_SET))+ vmbits |= VISIBILITYMAP_ALL_VISIBLE;+ if (xlrec->flags & (XLH_INSERT_ALL_FROZEN_SET))+ vmbits |= VISIBILITYMAP_ALL_FROZEN;++ vmpage = BufferGetPage(vmbuffer);++ /*+ * Don't set the bit if replay has already passed this point.+ *+ * It might be safe to do this unconditionally; if replay has passed+ * this point, we'll replay at least as far this time as we did+ * before, and if this bit needs to be cleared, the record responsible+ * for doing so should be again replayed, and clear it. For right+ * now, out of an abundance of conservatism, we use the same test here+ * we did for the heap page. If this results in a dropped bit, no+ * real harm is done; and the next VACUUM will fix it.+ *+ * XXX: This seems entirely unnecessary?+ *+ * FIXME: Theoretically we should only do this after we've+ * modified the heap - but it's safe to do it here I think,+ * because this means that the page previously was empty.+ */+ if (lsn > PageGetLSN(vmpage))+ visibilitymap_set(reln, blkno, InvalidBuffer, lsn, vmbuffer,+ InvalidTransactionId, vmbits);+ }+
ReleaseBuffer(vmbuffer);
FreeFakeRelcacheEntry(reln);
}
@@ -8301,6 +8401,8 @@ heap_xlog_multi_insert(XLogReaderState *record)
if (xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED)
PageClearAllVisible(page);
+ if (xlrec->flags & XLH_INSERT_ALL_VISIBLE_SET)+ PageSetAllVisible(page);
MarkBufferDirty(buffer);
}
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.cindex 0a51678c40..812d839612 100644--- a/src/backend/access/heap/visibilitymap.c+++ b/src/backend/access/heap/visibilitymap.c@@ -254,7 +254,6 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
elog(DEBUG1, "vm_set %s %d", RelationGetRelationName(rel), heapBlk);
#endif
- Assert(InRecovery || XLogRecPtrIsInvalid(recptr));
Assert(InRecovery || BufferIsValid(heapBuf));
Assert(flags & VISIBILITYMAP_VALID_BITS);
diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.hindex 95d18cdb12..7426ad5e4f 100644--- a/src/include/access/heapam_xlog.h+++ b/src/include/access/heapam_xlog.h@@ -67,6 +67,8 @@
#define XLH_INSERT_LAST_IN_MULTI (1<<1)
#define XLH_INSERT_IS_SPECULATIVE (1<<2)
#define XLH_INSERT_CONTAINS_NEW_TUPLE (1<<3)
+#define XLH_INSERT_ALL_VISIBLE_SET (1<<4)+#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, daniel@yesql.se, ibrar.ahmad@gmail.com, pryzby@telsasoft.com, andres@anarazel.de
Subject: Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits
In-Reply-To: <f263b707-fb89-007d-2892-7212a3ecaea4@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