Few comments on commit 857f9c36 (skip full index scans )
Hi,
Review comments on commit 857f9c36:
1.
@@ -2049,6 +2055,10 @@ _bt_newroot(Relation rel, Buffer lbuf, Buffer rbuf)
metapg = BufferGetPage(metabuf);
metad = BTPageGetMeta(metapg);
+ /* upgrade metapage if needed */
+ if (metad->btm_version < BTREE_VERSION)
+ _bt_upgrademetapage(metapg);
+The metapage upgrade should be performed under critical section.
2.
@@ -191,6 +304,10 @@ _bt_getroot(Relation rel, int access)
LockBuffer(metabuf, BUFFER_LOCK_UNLOCK);
LockBuffer(metabuf, BT_WRITE);
+ /* upgrade metapage if needed */
+ if (metad->btm_version < BTREE_VERSION)
+ _bt_upgrademetapage(metapg);
+Same as above.
In other cases like _bt_insertonpg, the upgrade is done inside the
critical section. It seems the above cases are missed.
3.
+ TransactionId btm_oldest_btpo_xact; /* oldest btpo_xact among of
+ * deleted pages *//among of/among all
Attached patch to fix the above comments.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
fix_review_skip_full_index_scan_v1.patchapplication/octet-stream; name=fix_review_skip_full_index_scan_v1.patchDownload
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 3d5936f..907cce0 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -2150,10 +2150,6 @@ _bt_newroot(Relation rel, Buffer lbuf, Buffer rbuf)
metapg = BufferGetPage(metabuf);
metad = BTPageGetMeta(metapg);
- /* upgrade metapage if needed */
- if (metad->btm_version < BTREE_VERSION)
- _bt_upgrademetapage(metapg);
-
/*
* Create downlink item for left page (old root). Since this will be the
* first item in a non-leaf page, it implicitly has minus-infinity key
@@ -2178,6 +2174,10 @@ _bt_newroot(Relation rel, Buffer lbuf, Buffer rbuf)
/* NO EREPORT(ERROR) from here till newroot op is logged */
START_CRIT_SECTION();
+ /* upgrade metapage if needed */
+ if (metad->btm_version < BTREE_VERSION)
+ _bt_upgrademetapage(metapg);
+
/* set btree special data */
rootopaque = (BTPageOpaque) PageGetSpecialPointer(rootpage);
rootopaque->btpo_prev = rootopaque->btpo_next = P_NONE;
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 22b4a75..a24e641 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -341,10 +341,6 @@ _bt_getroot(Relation rel, int access)
LockBuffer(metabuf, BUFFER_LOCK_UNLOCK);
LockBuffer(metabuf, BT_WRITE);
- /* upgrade metapage if needed */
- if (metad->btm_version < BTREE_VERSION)
- _bt_upgrademetapage(metapg);
-
/*
* Race condition: if someone else initialized the metadata between
* the time we released the read lock and acquired the write lock, we
@@ -379,6 +375,10 @@ _bt_getroot(Relation rel, int access)
/* NO ELOG(ERROR) till meta is updated */
START_CRIT_SECTION();
+ /* upgrade metapage if needed */
+ if (metad->btm_version < BTREE_VERSION)
+ _bt_upgrademetapage(metapg);
+
metad->btm_root = rootblkno;
metad->btm_level = 0;
metad->btm_fastroot = rootblkno;
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 15a7b4c..04ecb4c 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -103,7 +103,7 @@ typedef struct BTMetaPageData
BlockNumber btm_fastroot; /* current "fast" root location */
uint32 btm_fastlevel; /* tree level of the "fast" root page */
/* following fields are available since page version 3 */
- TransactionId btm_oldest_btpo_xact; /* oldest btpo_xact among of deleted
+ TransactionId btm_oldest_btpo_xact; /* oldest btpo_xact among all deleted
* pages */
float8 btm_last_cleanup_num_heap_tuples; /* number of heap tuples
* during last cleanup */
Hi!
On Mon, May 28, 2018 at 11:52 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:
Review comments on commit 857f9c36:
1.
@@ -2049,6 +2055,10 @@ _bt_newroot(Relation rel, Buffer lbuf, Buffer rbuf)
metapg = BufferGetPage(metabuf);
metad = BTPageGetMeta(metapg);+ /* upgrade metapage if needed */ + if (metad->btm_version < BTREE_VERSION) + _bt_upgrademetapage(metapg); +The metapage upgrade should be performed under critical section.
2.
@@ -191,6 +304,10 @@ _bt_getroot(Relation rel, int access)
LockBuffer(metabuf, BUFFER_LOCK_UNLOCK);
LockBuffer(metabuf, BT_WRITE);+ /* upgrade metapage if needed */ + if (metad->btm_version < BTREE_VERSION) + _bt_upgrademetapage(metapg); +Same as above.
In other cases like _bt_insertonpg, the upgrade is done inside the
critical section. It seems the above cases are missed.3. + TransactionId btm_oldest_btpo_xact; /* oldest btpo_xact among of + * deleted pages *//among of/among all
Attached patch to fix the above comments.
Thank you for catching this!
Your patch looks good for me.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Mon, May 28, 2018 at 4:52 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Hi,
Review comments on commit 857f9c36:
1.
@@ -2049,6 +2055,10 @@ _bt_newroot(Relation rel, Buffer lbuf, Buffer rbuf)
metapg = BufferGetPage(metabuf);
metad = BTPageGetMeta(metapg);+ /* upgrade metapage if needed */ + if (metad->btm_version < BTREE_VERSION) + _bt_upgrademetapage(metapg); +The metapage upgrade should be performed under critical section.
2.
@@ -191,6 +304,10 @@ _bt_getroot(Relation rel, int access)
LockBuffer(metabuf, BUFFER_LOCK_UNLOCK);
LockBuffer(metabuf, BT_WRITE);+ /* upgrade metapage if needed */ + if (metad->btm_version < BTREE_VERSION) + _bt_upgrademetapage(metapg); +Same as above.
In other cases like _bt_insertonpg, the upgrade is done inside the
critical section. It seems the above cases are missed.3. + TransactionId btm_oldest_btpo_xact; /* oldest btpo_xact among of + * deleted pages *//among of/among all
Attached patch to fix the above comments.
Thank you for reviewing and the patch. All changes looks good to me.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
The metapage upgrade should be performed under critical section.
Agree. But after close look I found that btm_version change isn't wal-logged
(near line 2251 in _bt_newroot). So btm_version is not propagated to
replica/backup/etc.
I believe it should be fixed.
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
On Wed, May 30, 2018 at 9:09 PM, Teodor Sigaev <teodor@sigaev.ru> wrote:
The metapage upgrade should be performed under critical section.
Agree. But after close look I found that btm_version change isn't wal-logged
(near line 2251 in _bt_newroot). So btm_version is not propagated to
replica/backup/etc.
I think it will always be set to BTREE_VERSION (See _bt_restore_meta).
I see that other places like _bt_insertonpg,
_bt_update_meta_cleanup_info, etc. that log meta page contents don't
log version number, so if we want to log it, all such places also need
to be modified, but I don't see the need for same.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
I think it will always be set to BTREE_VERSION (See _bt_restore_meta).
You are right, pushed. Thank you!
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/