public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] cfe-12-gist_over_cfe-11-persistent squash commit 5+ messages / 3 participants [nested] [flat]
* [PATCH] cfe-12-gist_over_cfe-11-persistent squash commit @ 2021-03-12 01:02 Bruce Momjian <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Bruce Momjian @ 2021-03-12 01:02 UTC (permalink / raw) --- src/backend/access/gist/README | 10 ++++++++++ src/backend/access/gist/gist.c | 21 +++++++++++++++------ src/backend/access/gist/gistbuild.c | 9 ++++++--- src/backend/access/gist/gistvacuum.c | 18 ++++++++++++------ 4 files changed, 43 insertions(+), 15 deletions(-) diff --git a/src/backend/access/gist/README b/src/backend/access/gist/README index 25cab0047b..6b1f84634f 100644 --- a/src/backend/access/gist/README +++ b/src/backend/access/gist/README @@ -461,6 +461,16 @@ value. The page is not recycled, until that XID is no longer visible to anyone. That's much more conservative than necessary, but let's keep it simple. +GiST and Encryption +------------------- + +GiST uses LSNs and NSNs for concurrency. This means that unlogged and +temporary relations also need LSNs. GiST has a mechanism to assign LSNs +to such relations, but sometimes uses fixed LSNs and or calls +gistGetFakeLSN(), which can cause duplicate LSNs to be used. Therefore, +when encryption is enabled and fake LSNs are needed, the GiST code calls +LSNForEncryption(). See src/backend/crypto/README for more details. + Authors: Teodor Sigaev <[email protected]> diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c index 0683f42c25..5ca5f17db3 100644 --- a/src/backend/access/gist/gist.c +++ b/src/backend/access/gist/gist.c @@ -501,13 +501,16 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate, * we don't need to be able to detect concurrent splits yet.) */ if (is_build) - recptr = GistBuildLSN; + recptr = !FileEncryptionEnabled ? GistBuildLSN : + LSNForEncryption(RelationIsPermanent(rel)); else { if (RelationNeedsWAL(rel)) recptr = gistXLogSplit(is_leaf, dist, oldrlink, oldnsn, leftchildbuf, markfollowright); + else if (FileEncryptionEnabled) + recptr = LSNForEncryption(RelationIsPermanent(rel)); else recptr = gistGetFakeLSN(rel); } @@ -568,7 +571,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate, MarkBufferDirty(leftchildbuf); if (is_build) - recptr = GistBuildLSN; + recptr = !FileEncryptionEnabled ? GistBuildLSN : + LSNForEncryption(RelationIsPermanent(rel)); else { if (RelationNeedsWAL(rel)) @@ -586,6 +590,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate, deloffs, ndeloffs, itup, ntup, leftchildbuf); } + else if (FileEncryptionEnabled) + recptr = LSNForEncryption(RelationIsPermanent(rel)); else recptr = gistGetFakeLSN(rel); } @@ -1665,6 +1671,8 @@ gistprunepage(Relation rel, Page page, Buffer buffer, Relation heapRel) if (ndeletable > 0) { + XLogRecPtr recptr; + TransactionId latestRemovedXid = InvalidTransactionId; if (XLogStandbyInfoActive() && RelationNeedsWAL(rel)) @@ -1690,16 +1698,17 @@ gistprunepage(Relation rel, Page page, Buffer buffer, Relation heapRel) /* XLOG stuff */ if (RelationNeedsWAL(rel)) { - XLogRecPtr recptr; - recptr = gistXLogDelete(buffer, deletable, ndeletable, latestRemovedXid); - PageSetLSN(page, recptr); } + else if (FileEncryptionEnabled) + recptr = LSNForEncryption(RelationIsPermanent(rel)); else - PageSetLSN(page, gistGetFakeLSN(rel)); + recptr = gistGetFakeLSN(rel); + + PageSetLSN(page, recptr); END_CRIT_SECTION(); } diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c index 1054f6f1f2..a8b82483e8 100644 --- a/src/backend/access/gist/gistbuild.c +++ b/src/backend/access/gist/gistbuild.c @@ -299,7 +299,8 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo) GISTInitBuffer(buffer, F_LEAF); MarkBufferDirty(buffer); - PageSetLSN(page, GistBuildLSN); + PageSetLSN(page, !FileEncryptionEnabled ? GistBuildLSN : + LSNForEncryption(RelationIsPermanent(index))); UnlockReleaseBuffer(buffer); @@ -451,7 +452,8 @@ gist_indexsortbuild(GISTBuildState *state) /* Write out the root */ RelationOpenSmgr(state->indexrel); - PageSetLSN(pagestate->page, GistBuildLSN); + PageSetLSN(pagestate->page, !FileEncryptionEnabled ? GistBuildLSN : + LSNForEncryption(RelationIsPermanent(state->indexrel))); PageSetChecksumInplace(pagestate->page, GIST_ROOT_BLKNO); smgrwrite(state->indexrel->rd_smgr, MAIN_FORKNUM, GIST_ROOT_BLKNO, pagestate->page, true); @@ -573,7 +575,8 @@ gist_indexsortbuild_flush_ready_pages(GISTBuildState *state) if (blkno != state->pages_written) elog(ERROR, "unexpected block number to flush GiST sorting build"); - PageSetLSN(page, GistBuildLSN); + PageSetLSN(page, !FileEncryptionEnabled ? GistBuildLSN : + LSNForEncryption(RelationIsPermanent(state->indexrel))); PageSetChecksumInplace(page, blkno); smgrextend(state->indexrel->rd_smgr, MAIN_FORKNUM, blkno, page, true); diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.c index 0663193531..f783edd627 100644 --- a/src/backend/access/gist/gistvacuum.c +++ b/src/backend/access/gist/gistvacuum.c @@ -174,6 +174,8 @@ gistvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats, vstate.callback_state = callback_state; if (RelationNeedsWAL(rel)) vstate.startNSN = GetInsertRecPtr(); + else if (FileEncryptionEnabled) + vstate.startNSN = LSNForEncryption(RelationIsPermanent(rel)); else vstate.startNSN = gistGetFakeLSN(rel); @@ -353,6 +355,8 @@ restart: */ if (ntodelete > 0) { + XLogRecPtr recptr; + START_CRIT_SECTION(); MarkBufferDirty(buffer); @@ -361,16 +365,15 @@ restart: GistMarkTuplesDeleted(page); if (RelationNeedsWAL(rel)) - { - XLogRecPtr recptr; - recptr = gistXLogUpdate(buffer, todelete, ntodelete, NULL, 0, InvalidBuffer); - PageSetLSN(page, recptr); - } + else if (FileEncryptionEnabled) + recptr = LSNForEncryption(RelationIsPermanent(rel)); else - PageSetLSN(page, gistGetFakeLSN(rel)); + recptr = gistGetFakeLSN(rel); + + PageSetLSN(page, recptr); END_CRIT_SECTION(); @@ -657,8 +660,11 @@ gistdeletepage(IndexVacuumInfo *info, IndexBulkDeleteResult *stats, if (RelationNeedsWAL(info->index)) recptr = gistXLogPageDelete(leafBuffer, txid, parentBuffer, downlink); + else if (FileEncryptionEnabled) + recptr = LSNForEncryption(RelationIsPermanent(info->index)); else recptr = gistGetFakeLSN(info->index); + PageSetLSN(parentPage, recptr); PageSetLSN(leafPage, recptr); -- 2.20.1 --LQksG6bCIzRHxTLp Content-Type: application/gzip Content-Disposition: attachment; filename="cfe-13-rel_over_cfe-12-gist.diff.gz" Content-Transfer-Encoding: base64 H4sICMS9SmAAA2NmZS0xMy1yZWxfb3Zlcl9jZmUtMTItZ2lzdC5kaWZmAOQ8aXfbRpKfpV/R 9mw8pHiBN+msneigZI51ragk3mTz8BpAg0QEAgwOU0qc+e1T1d0AcZKUk+xk3/LZPIDq6q6j 6+qCzj13SQbUGHdNpvUVY6z0espgyGjbHPU6w06HKkaHDoZDpa0MyZXrkBlbkfaQKMpr/o90 FKV9eA5oXpMTL9QZAC1/sqhD/lPDn18vxc9m6L89PKMBe03uF2GdtNvkinowutOGt9dKF3E1 lL6iHM5C7SemB6/JD7fH96fvfiS6yRrtbsNjtup+ZJ7Kf3cac8sPiP9zSP0F0d3l0goODxuN xiH8cALP0lqa7bpLeLccn3lBUyep1yf43yW1Q+J7ekuj+gNzjBbVdeb7LUTN3zaDALzfJjV4 Nfgk20ZpoWUbfCiMandgVCH8AlbO31Z0zsRMuKh2yaIWjK5aHlt7VsDwO4xA8GEZekcLPMbk xwZ9Gc0bcN+V3ELwHqkVU+uvOL34EfEX4Qcl6AOPOj5dth5tdy65iuCdLLjmuoEPwKvNtwR4 jjc6DShgbPmB68VcjMGHucXr3tMqcFtX9IGZls0y+pBHL8C10GSODsgrDltXJXh72OP6UPxK 41m5frCkfsC8xNeYxzkmSGpwXhOGwMdyHoN3UKGydGVG2K5ObfguhTLK8SGCRya0dHf1ZFhe rCOd4ZYZ+AiPWY4VpOVYAr+Sy0opudxK+VkCWAtn0dxjfmrLphhlObodGiy585qL3Pbux4uK 4NPyXKTkSTojkhBdNCTBWU7CIjlFG4c0igdEXF1kSQD4To8gH32iL6gzZ0addMcdIjaS5Tp+ pVatk96QGMxm4kKjenhoWKZJGmD4AkJbpVZOK711aDkGeyRGd6gzZg773WZT15Qepfp40CVt RRn0emhCt2A/BP5sm+Hrr0mjPRzWQYPwo60QuKLZ3CKy5Sp4qtwxmyJJhC+mekgOyBH5cHlz oZ6dHM8m6und5Ph+QlxPXLw/uZzdnkZXPaa7ntEk4ESYx0zXY3WyZsRhzBCIgoXlE/aROWS9 wDdqqzb8tN8sQWGX1G5ysNZh7eAWZDRxuEJMnZVNdVZZsoCijOtkej29V89v7t5ff3NVJ9GS p/4t85bUYU5QEauvAyJ8gWBPLm9urtSryf3x7fHFRD25fH99U/0SpsOJZiw4XTD9wQ+X+clK RoIeHviw77nBF/M13nqGiteyKyxCUYfxuLIK6JhHjqpkM2PghYyvDUynClsAr1ZepadovMV3 1XNcgzXxLTNnWht3OFFtB4DUzL5O+2Z7aGigmX0Dvipjpd9Pa+YOREI/dwChlvaVbh0CmRr/ 7KGa4k0um8DlDIk1FSKPOplZvzBigov0VwBSJxfT2f3sHpXyCAf6AQQ3nOWWSSqWr3KVr3IZ gNauAo+8IS/OYdNLpQPEE4dqNjPIV+QCMJzggMvZNXnNB8ELfpy73ga+UqSIsLgqasvBAbN9 RnD2COwatoX/3fGlgOGqGi8F1/zh0p3PVrYV4HptRs1InYVGGwBSJ65teLC/H/g3x3fqxGZm oC9gsWAS0yOW1HswXdt2QWfniyCzrELiM8t6DslE4D5sHBz8im8HW2jH2zuJbxw8m/rUkDz5 eHsX/emlPYcBEfYC6i5YcA4xDmDjwBz2t7wG5MAIchXsKqkIIOTClwS+8ze4AO+Ntw57DIRq S+OGCPgdYV/EHICOb7Rht46utT9U6p3B//mN1pA832PDkT0VUwAd3Jimz4LrcKkxj4A9tF24 AMQoGzWTF39o/8i31rYhtfwIIvDgapIDp/631LaMCmg4gDrhMtovcl0xDuVHQB1DfSnvJqZt y2u/HTZKdtw3KwPEVxFhanL7RJPUYzLqxArCFfzG9yRkcgcKE7MXSTVOUW0bQbU8PUSQ85fZ x7ViM57ha21fttbKuEqkvfjTrHdts492kkzShiZtY0hkCyCQ0ewHx63uH5pEmfr2iEFCySCF jrRRpzfqslGzqRldA4Jnpo33D1IibLsjlQgSrWiv36tDbo0f7U5kRFW+IszXOWgFTSW3bjM0 kuSI20q0Q5EcblbMmUFYV+F3Gm/5eGn5syyWIILbzzGo4An3UoTsGoRWtI4IZufEDz2GeHxC 4ctHClPUCXUMVEYWI67zu44bYNUHEDpBU4T3xz5mJJUXuDnumH4bgE2YOh+5VUBCLwoJrVbJ q1eokkBDpYTmT5+4zm5BQl68SfFFEFaQcOTYfHU8vd6ed2SZlshAUPrq3c3N/e7kIzdx0dhN 8pGZNJGGpBecwRJlILnZZP4hooMh6nV/1C3Xa9W0Q3+heowaTypHVqrphabiWdHA79PeP0p9 n6O/sdLuobVZtS3R1TJl/R0aCmsT1vnLhIyK9LKehEMtg2ATrOPeSshH10k6093pErLF2ELL nAGSDqE3puPRYDgcDZtNRhn8MpUe3ekQsshK/UEWkNdYlE4f9w3/HOK+URFCpZB96BASA4LA zwTWJ3DrQcaKpuWBf8ELdRJaTtDtEIf/9KsyKI45V+A9pF/+PVrCNYPYVK5il7HKgib0Ypsu xIPq5BfmuVgRhTiJ1olJIfbYTzHyZfdiKeXgpHpAsNDW6MjstpvNXlfrmR1Gx93d6pHHV64h eVhUkq7QEfwYo4rACBVvqxIU9IN/ChsaBwsp8R+Uxg6ej6UjNcobi43GBlaGpmkBRfEnGIct piQxU3JEAjeKeIdpya+kcHyx0dmsoFTVctjqXAaD0RhlMBh3ZDHUo2shBFEzLZBBnbyD+/fh ymYEgnQhEnQL+Lm/NNAVffjwgZy5UYmUBG7kewglWJ8mrkmCBePGkmBJ9Svpfvbc2nH+sFOA kGWlBbhFgtsNQanYPktuO/d/9hitcANmgOTO14ewxYfGwOzCzu+MKe0Pxmx3ppBFVrrts4Dc MQw6vPg+iPRNC/4/FN/v7yaTuP69Z9k9O+ZZBffU4H93qT13drtDXQSU1NKO3jOG5oB1+s3m wND0tjYwO7uL7jlse6iphOR2sT+st7tgGPGzzwMYLYAdzZnPmXRy/93GMB6tpWlEscrYLhnP yMSfHPwaVQNerKUN0IK1r6L35/Eul1ThLfKG8Mi6CugxilIqJ5en72ffiwINGFPDdf4eEJ9B sC6VimClFIAbiEMYUWE+i6EBeBP6F49Ne591MuYttWBR2pKiiqdJKnITMNVqG6hYRwuZsIGL Ffc37k5+R6iXoiK5azdx/n7pAAC2jnZuhoLGhEK1zMPJDaHodNBp9/oKazZHBh12B9Ro6zs3 RAG+0i1RACuM94gH9fAxwi0B93cZb+HALdCsR8BoOXPe/gJsFzYYnTtYbY/5K9cx+G1Io2FD CWPOs1OByGPA8ydmbDXSn22gZ7e8NrD/8aiYqXzYvoa6EEPOXufKEm1RlmgPx/tJAjb8pfXA 1pbP+MZGvnsu5PU8ZxM7e7bCRHvqWAHSnKbwcnJ8LgO3P4Ht+xeF6iVDnsnufA1oC6tHA87q sfL5rHZCMKQBRsz+s9lOPkW/rr+5vJz9eVJA9M+UQmbIM6WwGb1dCrssaqZ3q9CipWGkJe2O dYUOtL4GoUWnOxgOTYP2d6e+GVylVjQDJ8rkokrOayKH5G+yKYe8xA5B1xGNK5CL0ICqYWDZ fnPxMgUmGoQesN8qfYc9Mj0MXK8FFhv4tgRJI0QtNzZqLkqNXlq+Tg2IgzPXV3MVqNBZ7jL6 TLzIa6S9tsKLpL12WxR7TkDHZ9gbh2F65aNr8QNQHi+dCgqxCliRTjOGfo8JI6/c4HawqG39 wk54NpwodspBGEMFoNOWA2G8aWCdsNEWEVapvhQ372m7YaJSSZ8qem/U64O+sLHJxr3RqG2W 60sxrry+FMMhazvc4cJ7Tl9kYyEISHdtscmzurIBCZ5WWRFKhcPmgYZOHeo9PUNdbEtb/dxa /exbc2D/nrqEmYPfWmJrY+joXLNLJZXvm9S2349SWo0O6ZgaVGk2h2Z7pHQUbbxFQnk8eenk YfhO7nXBH9TwgxevIkt76q6eZgKyMruae7GDAJRg+ZJXDD+obyo6eHxQ4UDgNTBviEvFUVmw Ko/IefqA5nnqf8s8y7SYMeEBOTOSIWnyUPp2+q2KGfN3x3fX0+sL8Cl45W5ye3N3r2L3gjiA 3o05WlviZNZj9op5PjgxUBRIU96Qu8nl7eRuBjZ+cn06UeH71fH15Po+OUrSts/C0DdARr+C 7Kwyubu7uZNZwEGFeZ4OWoVXT2/OJurZ8f2xenpzd/fN7f3krBrBEYBb+vPKS0ucUIh0xnII r9CQL0IMU71ILF/4L0VtrDdoc2s96AjP/9kiFowNfaauqS3oSaXbAFWQY2fVQAogrgxvOwbJ igm8/DPE9PyTEITclvtk+5q1bXejvdyB1KajdMYG5DljsL2DwWi4JfHPYinYyRkIHt51eXTX FY4rcFeiYcew8Ly/2Wzxf4cksmL/UUmCVDn6CF9zbrsatZEbNyf/mMH4/wH+SwPqwg9ywB23 ixBphIAmRhevllvo5vJhJ1vj/u9CcqO7h6BvvK2XLEG/Ij7KnDJ+NZuKPlT0drfN2pzJLYN9 bGEAW87QeAZkqFJXgJ91bD8Hl1VrHTX+qNdhjRzhfxLPF13AHcl72iCCrOJTGwpYFtGlPfuv S3LB5ULOsNjnrjA0IheeG66i8RzH9AxUf3o+PT2+n95c4yXYNduIlQP/UPJa2ECTiLOiTnN0 zckbKR9bSzlm8FMeJNKWY7q5Yal+9JcFd3iouiMOSNzLNZ8X3jUNuZCW4PR3jIAt3BT2L2fX wrpBnoTVMnEQvYoSloYGeh+4mGFZOtYGwJhSsIMPjGPDs/E19RElP0GQBQRKHBftHKZgjOoL kXJJHv/NgA3rMHLyzTmYPhU8zmz6/eTgoD3AZWJsa+kkdDC8AaQ8KQES1U11TLU+/pAe/CPv fbqdn1qrBfNOg0dyBKErGGf8+oZgrvNl/v4ZS9+PJ8eYGatz8jQoPXVlW5Ux6WQJxJN27NlU y1djropeLZzmsLYl2haxew0b03gLADnl6vCePZGjB/YkGr542FDegOWxIPQcAQpjgFoM9C9Y AFgq768u7tT3k/9Wp2fg9i9lC1mSdRjicqapevCoCvFWbi/U0+ntu8mdejyZqaf3dymyK2nZ QUIJ8zbewlsaTFy1mVONk01JTbyAqugEc+cy9MC42sGOCMuRPEsWTXlx6zF4uSEjlvD/EhlR fLChQ6xgDzoMVkjHb2LrEilcvm/nFh6j8D27toKFqCdFQQ/KGHeZUC45jBc3EnpbrpmFlXOu gRl2iJ68PJP4smokERyJ3ktkCk4L4Ziv8j4ehFRdT/WtpWVTT2qzE2DACVaMSaWVzS4bpXwh NqyUMpo1LFHGJRbRUUQW9CO21mDAiZ02wqyBVWqsMoChH1LbfuIDBKooUK2ghGDpVY6hSUjl BswWe9SZkBO251xYs3tuDuUsPoP4CiS9blYFsnfuGjyfF5+GyTW5DkyNOGxbrsNMnQBEJ2l8 ksB1BTJY+BPhMaAPUqfiCCFpyjmWLIVglw2Xg/l0KWlEuAJuAAH8RPin0BfIwfrjMpp8lBh6 bPsQEK8XGMxshkfa5JM5DMNohzc4Ie1PJLCWCZdj+ZIa1+BZTr1gNRt0sHSQQ2phIW9o5Qhh EoGMM0vjJ4+4aDxsFMzEqvVm8VxePOziooc3ITcgB9wUMhu2sEvQe4KsIEu5RqZazDYACBnJ AqYHAhcnxsfHBPzcwSeeEq35CvmMmCjhcOrjdpG9X6CQaykOcSAkHvjCUcsE11tSzxMqvuGO v3BD24DpeANcVuuf3+9YrZJPn8iLLV4r3nE8K8F4YNMkAJEABY4wHYwa8dGsQaaD5Uac5WZF fw7ZGW+xoWKRUiZYj+QF+OlZHYQdb0CxuWw6JxCFiGY50EmxlemGTM78SKlA8OhMohM0gUPq toxsFnSF6zRBR+UGwa0km+2EJAUu1wG7YLhrsHEgPkhT5yHEaXhQsvZceLfkUJD3kgfAaDDS yonrEMh8d8lKlFxwT0hfLjOItAmAonht+m1doBLKCqZmCXToaI7Q38faUmZhyRt0Q5HEZ0JM +ESF7PYDWV0dfzi+nF5cV1B4rlnJy27TBYuVc4lL3BZoGm83c4N8pAJE8o0GV0pHo7x98or8 kw87vzy+UE+m91fHs/d8hUqEYT9triaCpEKlBnTkRQnDqlGan+oYzXWAFm8WOXNpGJksFpVH iaJ44djWA7OfKi82cYxEtvGLkHRti2cqIoLM++vAq6NxsZxVGJCow0e8UDduzIQrx67UxP1o LPjqOcQhqbGvhA/n9yGJCm1+/lgEmQvuxXo+pqHSAX9yFRgM1IlSrZZGWVFfE99mX4QvN7WT RIAh1osqVkR2MhY7Y58Vi8lhf5FYbFugJcPmdKD1ZymyDHw38/4bFdkNAxz8l1Vkyas/SpFP XX5cpQe4yOi4NKHRuO5ESgw/fldOvC0hLtZr0OocUwWtV2wJRrmSZzlR6pnKQhy2CKJs3yGV EdGeAnB5XDJLttRXT5VVnbyqcEreQSzFPLF/wCOtDNXGpzulT0SIje8RPfErUntTcj+eXdS7 RZWFVHolK5D7SOJKMDM/UfJmOjS7ot4DL/NAzBInJxjNTr/FOFrGbBAy2jzYiEAgTA0h9fRk KIV/UMWmsCgRfICBs1F8SwqIQscCnw3BEnaRTn0Rr0BQpbEAEGBsFrKvNgFJqfvlqn60qtXw wcTHkZKmAyIxyHkerWW4hIl/Di0MyuUiucKCfWiAfcBIrKP0RlEcb2K4aPEOF4EIe73hl4fZ GbDdj2Ol38rru2V/l0PbByo65DaHw/ZwoHSVZnPUa/f6I6U/3tIuVIYtX/0tgxQPACii/18p O+rGSjQXQvHBpLUqvi6qm887q2xZtqx45s4waRgsCm/w9+iEu93tiiaQbncoDgpuY4KvQBcr 4Mogep/rdWk24PvHH34Uj7YtQYlv5/dsucISmR8dYG+Kbn/IsfeBbrs+S97b1jlR9hdctH2g pGKZg27HHGpdjTWbdDDsGN3xaLhFscqw5RWrDJI/NMAbn6QYkmLLVLALDsLlZ8nd6Hh3f80q abnI6J5m4eE6f+xhyzk5kjbu4mapjbvt6EiZGkIPVKH8mdNG3lnD9S19tlcn5+L4TxMmSpwE iieFRSc6uALe9ckN2Jx6GnolPGJGgxQ9EV52ECy7UIElJ+LRFU06AeFlnzW04Dj5M8+TDzar IJuL+x4v/yoG4dr54RhOenKlfj+5u1FvrlUeA2FVAvtQVYMugQxD9LBWZZ+Gwh98rcGXYb3D /17HOT4VKORXER9nzNfJEf5hhvQpMVDsVGVfIzmIuAPuCIa9M7BSzi9U5KPP0puVFtx/FS2+ PEUFB8b0hWPpvMAIJMXMQydmG5tTFYo9bGAoUCTYcxDwwsLffYnGfaBP4PXlkQoymqxl3Qfc ecDAL4c+rz4knuKIEXJvJz0xEnjv8mYgIDERGuIpXoF2hWbjbUDnzQJd4aEZb9+FbD1JXGJY pBfiwcHc3Imz7cz8Eq4MFT5+Hj0h/gysWaqSOEUDMW93FQ/MR1UZ2aANUjeYjxEIRD0zLos1 EwU3XifywRZAdMKVxxWhB0cWPVHkYohE/lXdtfa2jVzRz/avYL1wV4olWdRbSXYLO5YToc4D fmxQIABBimSsri2qopSs0ea/976GM3xKjrNAu8jaMjkcznvO3HvO1XIV4UoZxGIx8gI2GOGZ 4hY3Mk+gyzwxed5v4jVnRaqcxNoGXb1czb+wNIiWTywjPOBHSanvcbuWwd38QS2V7IQr2DGc eeSs5/gamsvTd1fXl8719O3EuZpcO69uLi9hQNQgEQyU1VpYnd0ec5m7vZEQeGi6qinJ8zWt UBTtGXJOYDLhXtVaroIv82iDsRjgcoTrNPlN8FWz318kD2TvQPq/GvkoRZSSRIlF9Staky0k VGLXbFNEFZFBKM6aHI5pKSmbSSX6KFZBmo+anaBeWkAPKXlxpgv39gzKaEoEtTNyMSLJVWII nU4xHnpet98fBW631bLbXRgPw9Gogl1Snt9W/GKkJTDZH5MEin7BhQu8zQPuBIUdRbt8wYae OeUam983/MFOV2l91kk0ob8RgIR+EFoXp2eT05vXcCOEKbxYh7V4DSdOeNfBxal1cnHx/pVV O9w0Dn34V7cO/U+Lg0YiY8vRlVrQfe+ytCVVJtohbQJu+IuF9D+82jB5zqFv6VFtAEOEw/0g +pUId1uYgJgsWgaLmjlAV0y9evtwyl059RXV6n94P/3uiY8al6oNtGTWP23OQzedoNsJJ32r Re6hrZM9EwayeLalE6lggu7IG868YOa2WmHP7o6GrudXULxLMiuf45mENNQ7NNKZNWgg/peb BYBav3X7q3kxfoiPmU39K460wgONqHB+BJ9bEbetPE8niQjJx5FhD5ep4YBXKbkpDP1wFd3D X+q4u47oDzGuzTarWCm11WP4ADYVejDld6Lg/qZCpVEgnytnenU5eV0L4cQerx2E4xLwCfNy 8OFcbhJmqTyBwZuk152vguAMivXHHdLucGTSytyhKtud76+zgkjRIiCu3T6Zgdl22dTl0zly 6VSWVJn9o90SystlcXBwcXA4AwvPM5gUp/gzXv6w6mxxh2Ec+iJu6XYA/RzZ3R5TXHctocpq QaYszDoKQweuRGLdTy5wbBR1uckCGYqhBkiGmkpW5xq6j9kjTfAQjW1BvI7JPtlUjK1X7z/8 wzm9OSezKhpRn1kig0wcClQHWtfI+2vV8g1k/U2esp5bOotEwkdIV5WRjsVUi6RIgL6na3F+ esECSkbeZtht7qN4jeyINay090h74FbuyY7f1uNqp1b+CVaaeSjFsm7iQLSfvCdQDBcAich2 cNFI6d6x6VrvdwS48fMv8mQt1YA0b7IpUu1XTzcKyvoJf1J4WNFHcTjV0eiRlUvMEbwoOczw dr668zVD9drHk+m1M/kNtiOHin0+vZjA4f3kjDWvPPig4Io6H/qN5KyTr2d1+lytC8uFqle+ SRG9OMOXVruQpE6tM+Y4KONBo9N+ZPPsGcx1BhcIB2iaEI3308Fh/OnguXV4fwDrm+RTr+fK hy5rztCDh39/kYIzH0kKH/BUNDFLcsLzcNSt1jgX4D7J3qzNAnalz4EvmSi0hcHbYjbqoy0f j434OGk3ZzAwyT/J3E1cIROegWRDb0xxZhTEwdoUrHNHKmRe4vzRKmm16nFEOnFM6RZJxNNH e7s42Hn7MJLvIDamZ+DUTWufdZx65TcOGwfdu4jQ71A24ComwsfL6fVEdzbaoOu8dtb8eG2O ba41xVKSjztCriSOdgX2UWlUtKH2bNgf+cPRuNUaj/tdz+sEVRqp4ry24K0kHR3lycdw1BVX AxxkoblvZICqA0U8XZx5FLETt3323/KE8/kybOYL9x66Cy0g0bKuLaXv/450qp+JB4MEJzR6 BCtc4GkldGfrDYx3nD8tFQWFHKd0vuqg5xRu4Qn+kHhwKC9BXvnSXd82UCCCHyTKol4cChIc VSbIACr8pUCO1NBhqLO155duKiJ6cTekE0nfj91Zvz/wei56BNx+uzMYt+1ge99nMivv/ExC VkKzELrMH3C73iwdP1i7eTGlpMB4k8V3dmHD7wyu5bSUuU0iz+P74N4PvM1nBb1HKBA9Gg0Y lJExHca76eImdIIf8YP8GWs6FSNRHKlcJoGhiBf3myXG+UoHOk0L4kXBqrs9g8zBfUH2gHKW iT6PVr+ZcK12j+8t1XJvustJiQxtAOgT0S9eEpTKjkmOYWC3jcZ9QmPwKUc61wlhmCEq+4Vm YzIZ5ZAu9Ju8VIvap4SAo+kVrLGy9pRbmmM5U9AEer9QOJer6Auz9W6pVdAzTTEWZhgwGu3E aIiQfGAVuIsieDZ2kVi8iDafb4klSuRRqCezdgUjLqELW9aFi77vTYxNFIWCcDvDRheatde1 G72kYQusBFUNy4tuGQEiMYMzl141OSaoZYTjiS6NaCZ8j4et6Wp46ngto0YJX0UIJJA/vs3U eOCuMhU7OfYYkbLdxGLeYIqxWPDY2gvwxW/OF4mTjl1APHjfBV+FPom8WbxGQZyvo1NVXRjQ Uq26ochIy+3TtIePzB+eKR4pwUPaRiyS1SBwQ3YHjB/kaODZh1Q28caL8ZS0EIIy2smYzwqD h7bWUM4ZP5O/IUCy8solxwQMywV6M9jabbQiZ+WuVu5DA0czRQc0zkAwUv61IZsbHoBI36Wo RKYXgijQkhfaCQOAASQZcklt89XlACVcS2hr7Sghiu0CieRUIYokZXBdoSd0NwtjDZvZ6Pu3 wX20enjFTgC2d15Hy9TVhgEQDUKO5NHIBkcw0KQpsdDpt60nahSoJxQri3lWBRj3qfOlaHmX KfP0wcyNlm2J7a1AVTYrnVmk/w8qbbI6d610KQrMfsONV3VXMUGCWce327POsNVqdzvtcWiP exW+lGwuecCXTUFybDKr9jpVsRBKAh3AdI+P3fghB9mSm0s4dLmr72J/xMt55tY/5+tj+H87 r6g7tolXBL8NXhHWuYJV1BDn51+m8Q00/0pTkZIAY0Xcoh3IRRXsot3oRcXfe+SV31OHRq89 6A/7/ngEA6gX2nbodQcFkQiL89DDp/g+DR6MTHLUs9XgEXMiEvzffYB1PiYAatvYDrAHNlHO RfiGhAG87cKW4BrqHa0ozSdlBUHxAwzFVQnOKcgPvrxmWy/RlMR0djfUQdd02rPJxeR6cpak til1Sv3iBcGCv5gJQAU8TRFaKJBBn3lFZn5vTq6c1yeXpyevJyrLHmVJUhGJWBQt9BtQouID NEuxbIhiwNIT9eKHgDjMFpF8SecB8DpmLIk2ItpRZ7DfewGLvBZaqcSNR4qnlFo3p8aAtbX9 h32O0BiBMRQDpVI19OFZjEJmSlNcR9yQCIOay80KVhdmnDJfE26L4kqgDOWn+w1LmJeiMA4i roYvWp0N8hMzwqzTh/K5kv3OL6/q7q4a/tlsNJgF3qhTruEve4Oh4e+M/mwJ/21y4UPEXy2W 1/Lbj9LyV32f2p8l2f9pHi7Qpy6k7zc5gfmbtAzf4AdIIKf8nqNCPAlN/m6DC7wpNZaPMQ87 YkSTZzg1ZbZACQpQUUwkQ1GSTke6LG3brDNowjCQACAJPW/TkL9IJ3+8KliRVfJoKpP140Uu lVlDY5IXRtY66AzVqYWu6/Lv5fOq7yu6dn/sDXvjcBi2WuHYHw+D4azdK98K8/nkp3g+DfNQ yBDSz2yJUDsSyr3BrbDQJAYJ4+ygZXI2UjfdHKE6eT02b8m9+TrImsbUrSgMEx72oMcBKPts vEG4hxNPJCTaboBrM5z+M1cI5ahn0resZ/rvF+n5k9ItiYNTNBb6+Ya19B3ZXer6+SL5iyWB W61mPvu64A/ce5b4xRK4EyJ1DKY3757a6JOoUmGDQSCIL/S1THWeTEhfENCIAlbBekpt50fW vykmDf1Xq+ExnrwH6B39cDJ13v82uSQfg/UcUIn1HzMxTB9kNKuk0yvnzeTkAyXEKjTN9tPW tVTUIMiuuVdifTPSNXCNqsyu8rBDK1zlW9I+m6rMGroFAHlspxZLX1Ljd7uEP7sDCXt9RdYN 9krBgnWnxgQstDX1+VipbNiaWa/j7pa+ouLhHsgjCADZZeda96gTwyDtUZh57IDxvrlofo+N +cV+U7KgtfWJllS9iu+YW8FxPEVo3GHBLzE5p26bJVRNlvpeMyzSie9PYQUrKiZet3B5Uy2I jWmi51RmkfFHo/DdlDX+eB2sUVSS3ueE3trrdxp9DDQ5atg2fbdCtmlxs6EY/u/VvDcLnS3T Is2rpzoBFKVv6qJawR/KcS6v4iNrxgqctbsW7bi5cflYK7IeSboMu9t9k85/pCw2g0IebT/T 760curu9+VFGrKe82VIklRQ8YgCxEz7SX0NcjFmS+0k4+O64O/DavttqDQZjwEZ+f2Rvx0c6 n3J8pNOwT4OwBv408RE64M+ml4yPzHZ/NFtMr5/J8zswQzK9/USqWGEPJlUkClHT2u+0Ou2W vb//X+BdTqWQfwAA --LQksG6bCIzRHxTLp-- ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Missing update of all_hasnulls in BRIN opclasses @ 2023-03-03 12:14 Tomas Vondra <[email protected]> 2023-03-28 14:30 ` Re: Missing update of all_hasnulls in BRIN opclasses Tomas Vondra <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tomas Vondra @ 2023-03-03 12:14 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Matthias van de Meent <[email protected]>; [email protected] On 3/3/23 11:32, Alvaro Herrera wrote: > > Thanks for doing all this. (Do I understand correctly that this patch > is not in the commitfest?) > > I think my mental model for this was that "allnulls" meant that either > there are no values for the column in question or that the values were > all nulls (For minmax without NULL handling, which is where this all > started, these two things are essentially the same: the range is not to > be returned. So this became a bug the instant I added handling for NULL > values.) I failed to realize that these were two different things, and > this is likely the origin of all these troubles. > > What do you think of using the unused bit in BrinTuple->bt_info to > denote a range that contains no heap tuples? This also means we need it > in BrinMemTuple, I think we can do this: > > @@ -44,6 +44,7 @@ typedef struct BrinValues > typedef struct BrinMemTuple > { > bool bt_placeholder; /* this is a placeholder tuple */ > + bool bt_empty_range; /* range has no tuples */ > BlockNumber bt_blkno; /* heap blkno that the tuple is for */ > MemoryContext bt_context; /* memcxt holding the bt_columns values */ > /* output arrays for brin_deform_tuple: */ > @@ -69,7 +70,7 @@ typedef struct BrinTuple > * > * 7th (high) bit: has nulls > * 6th bit: is placeholder tuple > - * 5th bit: unused > + * 5th bit: range has no tuples > * 4-0 bit: offset of data > * --------------- > */ > @@ -82,7 +83,7 @@ typedef struct BrinTuple > * bt_info manipulation macros > */ > #define BRIN_OFFSET_MASK 0x1F > -/* bit 0x20 is not used at present */ > +#define BRIN_EMPTY_RANGE 0x20 > #define BRIN_PLACEHOLDER_MASK 0x40 > #define BRIN_NULLS_MASK 0x80 > > (Note that bt_empty_range uses a hole in the struct, so there's no ABI > change.) > > This is BRIN-tuple-level, not column-level, so conceptually it seems > more appropriate. (In the case where both are empty in union_tuples, we > can return without entering the per-attribute loop at all, though I > admit it's not a very interesting case.) This approach avoids having to > invent the strange combination of all+has to mean empty. > Oh, that's an interesting idea! I haven't realized there's an unused bit at the tuple level, and I absolutely agree it'd be a better match than having this in individual summaries (like now). It'd mean we'd not have the option to fix this withing the opclasses, because we only pass them the BrinValue and not the tuple. But if you think that's reasonable, that'd be OK. The other thing I was unsure is if the bit could be set for any existing tuples, but AFAICS that shouldn't be possible - brin_form_tuple does palloc0, so it should be 0. I suspect doing this might make the patch quite a bit simpler, actually. > > On 2023-Feb-24, Tomas Vondra wrote: > >> I wonder what's the best >> way to test this in an automated way - it's very dependent on timing of >> the concurrent updated. For example we need to do something like this: >> >> T1: run pg_summarize_range() until it inserts the placeholder tuple >> T2: do an insert into the page range (updates placeholder) >> T1: continue pg_summarize_range() to merge into the placeholder >> >> But there are no convenient ways to do this, I think. I had to check the >> various cases using breakpoints in gdb etc. > > Yeah, I struggled with this during initial development but in the end > did nothing. I think we would need to introduce some new framework, > perhaps Korotkov stop-events stuff at > https://postgr.es/m/CAPpHfdsTeb+hBT5=qxghjNG_cHcJLDaNQ9sdy9vNwBF2E2PuZA@mail.gmail.com > which seemed to me a good fit -- we would add a stop point after the > placeholder tuple is inserted. > Yeah, but we don't have that at the moment. I actually ended up adding a couple sleeps during development, which allowed me to hit just the right order of operations - a poor-man's version of those stop-events. Did work but hardly an acceptable approach. >> I'm not very happy with the union_tuples() changes - it's quite verbose, >> perhaps a bit too verbose. We have to check for empty ranges first, and >> then various combinations of allnulls/hasnulls flags for both BRIN >> tuples. There are 9 combinations, and the current code just checks them >> one by one - I was getting repeatedly confused by the original code, but >> maybe it's too much. > > I think it's okay. I tried to make it more compact (by saying "these > two combinations here are case 2, and these two other are case 4", and > keeping each of the other combinations a separate case; so there are > really 7 cases). But that doesn't make it any easier to follow, on the > contrary it was more convoluted. I think a dozen extra lines of source > is not a problem. > OK >> The alternative is to apply the same fix to every BRIN_PROCNUM_UNION >> opclass procedure out there. I guess doing that for minmax+inclusion is >> not a huge deal, but what about external opclasses? And without the fix >> the indexes are effectively broken. Fixing this outside in brin.c (in >> the union procedure) fixes this for every opclass procedure, without any >> actual limitation of functinality (14+ does that anyway). > > About the hypothetical question, you could as well ask what about > unicorns. I have never seen any hint that any external opclass exist. > I am all for maintaining compatibility, but I think this concern is > overblown for BRIN. Anyway, I think your proposed fix is better than > changing individual 'union' support procs, so it doesn't matter. > OK > As far as I understood, you're now worried that there will be an > incompatibility because we will fail to call the 'union' procedure in > cases where we previously called it? In other words, you fear that some > hypothetical opclass was handling the NULL values in some way that's > incompatible with this? I haven't thought terribly hard about this, but > I can't see a way for this to cause incompatibilities. > Yeah, the possible incompatibility is one concern - I have a hard time imagining such an opclass, because it'd have to handle NULLs in some strange way. But and as you noted, we're not aware of any external BRIN opclasses, so maybe this is OK. The other concern is more generic - as I mentioned, moving the NULL handling from opclasses to brin.c is what we did in PG14, so this feels a bit like a backport, and I dislike that a little bit. >> But maybe someone thinks this is a bad idea and we should do something >> else in the backbranches? > > I think the new handling of NULLs in commit 72ccf55cb99c ("Move IS [NOT] > NULL handling from BRIN support functions") is better than what was > there before, so I don't object to backpatching it now that we know it's > necessary to fix a bug, and also we have field experience that the > approach is solid. > OK, good to hear. > The attached patch is just a pointer to comments that I think need light > edition. There's also a typo "bot" (for "both") in a comment that I > think would go away if you accept my suggestion to store 'empty' at the > tuple level. Note that I worked with the REL_14_STABLE sources, because > for some reason I thought that that was the newest that needed > backpatching of 72ccf55cb99c, but now that I'm finishing this email I > realize that I should have used 13 instead /facepalm > Thanks. I'll try to rework the patches to use the bt_info unused bit, and report back in a week or two. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Missing update of all_hasnulls in BRIN opclasses 2023-03-03 12:14 Re: Missing update of all_hasnulls in BRIN opclasses Tomas Vondra <[email protected]> @ 2023-03-28 14:30 ` Tomas Vondra <[email protected]> 2023-04-23 20:43 ` Re: Missing update of all_hasnulls in BRIN opclasses Tomas Vondra <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tomas Vondra @ 2023-03-28 14:30 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Matthias van de Meent <[email protected]>; [email protected] Hi, It took me a while but I finally got back to reworking this to use the bt_info bit, as proposed by Alvaro. And it turned out to work great, because (a) it's a tuple-level flag, i.e. the right place, and (b) it does not overload existing flags. This greatly simplified the code in add_values_to_range and (especially) union_tuples, making it much easier to understand, I think. One disadvantage is we are unable to see which ranges are empty in current pageinspect, but 0002 addresses that by adding "empty" column to the brin_page_items() output. That's a matter for master only, though. It's a trivial patch and it makes it easier/possible to test this, so we should consider to squeeze it into PG16. I did quite a bit of testing - the attached 0003 adds extra tests, but I don't propose to get this committed as is - it's rather overkill. Maybe some reduced version of it ... The hardest thing to test is the union_tuples() part, as it requires concurrent operations with "correct" timing. Easy to simulate by breakpoints in GDB, not so much in plain regression/TAP tests. There's also a stress tests, doing a lot of randomized summarizations, etc. Without the fix this failed in maybe 30% of runs, now I did ~100 runs without a single failure. I haven't done any backporting, but I think it should be simpler than with the earlier approach. I wonder if we need to care about starting to use the previously unused bit - I don't think so, in the worst case we'll just ignore it, but maybe I'm missing something (e.g. when using physical replication). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company Attachments: [text/x-patch] 0001-Fix-handling-of-NULLs-in-BRIN-indexes.patch (13.8K, ../../[email protected]/2-0001-Fix-handling-of-NULLs-in-BRIN-indexes.patch) download | inline diff: From 10efaf9964806e5a30818994e3cfda879bb90171 Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Sun, 8 Jan 2023 16:43:06 +0100 Subject: [PATCH 1/3] Fix handling of NULLs in BRIN indexes BRIN indexes did not properly distinguish between summaries for empty (no rows) and all-NULL ranges. All summaries were initialized with allnulls=true, and the opclasses simply reset allnulls to false when processing the first non-NULL value. This however fails if the range starts with a NULL value (or a sequence of NULL values), in which case we forget the range contains NULL values. This happens because the allnulls flag is used for two separate purposes - to mark empty ranges (not representing any rows yet) and ranges containing only NULL values. Opclasses don't know which of these cases it is, and so don't know whether to set hasnulls=true. Setting hasnulls=true in both cases would make it correct, but it would also make BRIN indexes useless for queries with IS NULL clauses - all ranges start empty (and thus allnulls=true), so all ranges would end up with either allnulls=true or hasnulls=true. The severity of the issue is somewhat reduced by the fact that it only happens when adding values to an existing summary with allnulls=true, not when the summarization is processing values in bulk (e.g. during CREATE INDEX or automatic summarization). In this case the flags were updated in a slightly different way, not forgetting the NULL values. This introduces a new a new flag marking index tuples representing ranges with no rows. Luckily we have an unused tuple in the BRIN tuple header that we can use for this. We still store index tuples for empty ranges, because otherwise we'd not be able to say whether a range is empty or not yet summarized, and we'd have to process them for any query. Backpatch to 11. The issue exists since BRIN indexes were introduced in 9.5, but older releases are already EOL. Backpatch-through: 11 Reviewed-by: Alvaro Herrera, Justin Pryzby, Matthias van de Meent Discussion: https://postgr.es/m/[email protected] --- src/backend/access/brin/brin.c | 115 +++++++++++++++++- src/backend/access/brin/brin_tuple.c | 15 ++- src/include/access/brin_tuple.h | 6 +- ...summarization-and-inprogress-insertion.out | 8 +- ...ummarization-and-inprogress-insertion.spec | 1 + 5 files changed, 137 insertions(+), 8 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 53e4721a54e..162a0c052aa 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -592,6 +592,17 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) bval = &dtup->bt_columns[attno - 1]; + /* + * If the BRIN tuple indicates that this range is empty, + * we can skip it: there's nothing to match. We don't + * need to examine the next columns. + */ + if (dtup->bt_empty_range) + { + addrange = false; + break; + } + /* * First check if there are any IS [NOT] NULL scan keys, * and if we're violating them. In that case we can @@ -1590,6 +1601,8 @@ form_and_insert_tuple(BrinBuildState *state) /* * Given two deformed tuples, adjust the first one so that it's consistent * with the summary values in both. + * + * XXX I'm not sure we can actually get empty "b". */ static void union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b) @@ -1607,6 +1620,64 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b) db = brin_deform_tuple(bdesc, b, NULL); MemoryContextSwitchTo(oldcxt); + /* + * Check if the ranges are empty. + * + * If at least one of them is empty, we don't need to call per-key union + * functions at all. If "b" is empty, we just use "a" as the result (it + * might be empty fine, but that's fine). If "a" is empty but "b" is not, + * we use "b" as the result (but we have to copy the data into "a" first). + * + * Only when both ranges are non-empty, we actually do the per-key merge. + */ + + /* If "b" is empty - ignore it and just use "a" (even if it's empty etc.). */ + if (db->bt_empty_range) + { + /* skip the per-key merge */ + MemoryContextDelete(cxt); + return; + } + + /* + * Now we know "b" is not empty. If "a" is empty, then "b" is the result. + * But we need to copy the data from "b" to "a" first, because that's how + * we pass result out. + * + * We have to copy all the global/per-key flags etc. too. + */ + if (a->bt_empty_range) + { + for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++) + { + int i; + BrinValues *col_a = &a->bt_columns[keyno]; + BrinValues *col_b = &db->bt_columns[keyno]; + BrinOpcInfo *opcinfo = bdesc->bd_info[keyno]; + + col_a->bv_allnulls = col_b->bv_allnulls; + col_a->bv_hasnulls = col_b->bv_hasnulls; + + /* If "b" has no data, we're done. */ + if (col_b->bv_allnulls) + continue; + + for (i = 0; i < opcinfo->oi_nstored; i++) + col_a->bv_values[i] = + datumCopy(col_b->bv_values[i], + opcinfo->oi_typcache[i]->typbyval, + opcinfo->oi_typcache[i]->typlen); + } + + /* "a" started empty, but "b" was not empty, so remember that */ + a->bt_empty_range = false; + + /* skip the per-key merge */ + MemoryContextDelete(cxt); + return; + } + + /* Now we know neither range is empty. */ for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++) { FmgrInfo *unionFn; @@ -1704,7 +1775,9 @@ add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup, Datum *values, bool *nulls) { int keyno; - bool modified = false; + + /* If the range starts empty, we're certainly going to modify it. */ + bool modified = dtup->bt_empty_range; /* * Compare the key values of the new tuple to the stored index values; our @@ -1718,9 +1791,24 @@ add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup, Datum result; BrinValues *bval; FmgrInfo *addValue; + bool has_nulls; bval = &dtup->bt_columns[keyno]; + /* + * Does the range have actual NULL values? Either of the flags can + * be set, but we ignore the state before adding first row. + * + * We have to remember this, because we'll modify the flags and we + * need to know if the range started as empty. + */ + has_nulls = ((!dtup->bt_empty_range) && + (bval->bv_hasnulls || bval->bv_allnulls)); + + /* + * If the value we're adding is NULL, handle it locally. Otherwise + * call the BRIN_PROCNUM_ADDVALUE procedure. + */ if (bdesc->bd_info[keyno]->oi_regular_nulls && nulls[keyno]) { /* @@ -1746,8 +1834,33 @@ add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup, nulls[keyno]); /* if that returned true, we need to insert the updated tuple */ modified |= DatumGetBool(result); + + /* + * If the range was had actual NULL values (i.e. did not start empty), + * make sure we don't forget about the NULL values. Either the allnulls + * flag is still set to true, or (if the opclass cleared it) we need to + * set hasnulls=true. + * + * XXX This can only happen when the opclass modified the tuple, so the + * modified flag should be set. + */ + if (has_nulls && !(bval->bv_hasnulls || bval->bv_allnulls)) + { + Assert(modified); + bval->bv_hasnulls = true; + } } + /* + * After updating summaries for all the keys, mark it as not empty. + * + * If we're actually changing the flag value (i.e. tuple started as empty), + * we should have modified the tuple. So we should not see empty range that + * was not modified. + */ + Assert(!dtup->bt_empty_range || modified); + dtup->bt_empty_range = false; + return modified; } diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c index 84b79dbfc0d..b81247a262c 100644 --- a/src/backend/access/brin/brin_tuple.c +++ b/src/backend/access/brin/brin_tuple.c @@ -372,6 +372,9 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple, if (tuple->bt_placeholder) rettuple->bt_info |= BRIN_PLACEHOLDER_MASK; + if (tuple->bt_empty_range) + rettuple->bt_info |= BRIN_EMPTY_RANGE_MASK; + *size = len; return rettuple; } @@ -399,7 +402,7 @@ brin_form_placeholder_tuple(BrinDesc *brdesc, BlockNumber blkno, Size *size) rettuple = palloc0(len); rettuple->bt_blkno = blkno; rettuple->bt_info = hoff; - rettuple->bt_info |= BRIN_NULLS_MASK | BRIN_PLACEHOLDER_MASK; + rettuple->bt_info |= BRIN_NULLS_MASK | BRIN_PLACEHOLDER_MASK | BRIN_EMPTY_RANGE_MASK; bitP = ((bits8 *) ((char *) rettuple + SizeOfBrinTuple)) - 1; bitmask = HIGHBIT; @@ -489,6 +492,8 @@ brin_new_memtuple(BrinDesc *brdesc) dtup->bt_allnulls = palloc(sizeof(bool) * brdesc->bd_tupdesc->natts); dtup->bt_hasnulls = palloc(sizeof(bool) * brdesc->bd_tupdesc->natts); + dtup->bt_empty_range = true; + dtup->bt_context = AllocSetContextCreate(CurrentMemoryContext, "brin dtuple", ALLOCSET_DEFAULT_SIZES); @@ -527,6 +532,9 @@ brin_memtuple_initialize(BrinMemTuple *dtuple, BrinDesc *brdesc) currdatum += sizeof(Datum) * brdesc->bd_info[i]->oi_nstored; } + /* FIXME Shouldn't this reset bt_placeholder too? */ + dtuple->bt_empty_range = true; + return dtuple; } @@ -560,6 +568,11 @@ brin_deform_tuple(BrinDesc *brdesc, BrinTuple *tuple, BrinMemTuple *dMemtuple) if (BrinTupleIsPlaceholder(tuple)) dtup->bt_placeholder = true; + + /* ranges start as empty, depends on the BrinTuple */ + if (!BrinTupleIsEmptyRange(tuple)) + dtup->bt_empty_range = false; + dtup->bt_blkno = tuple->bt_blkno; values = dtup->bt_values; diff --git a/src/include/access/brin_tuple.h b/src/include/access/brin_tuple.h index 732f91edf11..c56747aca4a 100644 --- a/src/include/access/brin_tuple.h +++ b/src/include/access/brin_tuple.h @@ -44,6 +44,7 @@ typedef struct BrinValues typedef struct BrinMemTuple { bool bt_placeholder; /* this is a placeholder tuple */ + bool bt_empty_range; /* range represents no tuples */ BlockNumber bt_blkno; /* heap blkno that the tuple is for */ MemoryContext bt_context; /* memcxt holding the bt_columns values */ /* output arrays for brin_deform_tuple: */ @@ -69,7 +70,7 @@ typedef struct BrinTuple * * 7th (high) bit: has nulls * 6th bit: is placeholder tuple - * 5th bit: unused + * 5th bit: range is empty * 4-0 bit: offset of data * --------------- */ @@ -82,13 +83,14 @@ typedef struct BrinTuple * bt_info manipulation macros */ #define BRIN_OFFSET_MASK 0x1F -/* bit 0x20 is not used at present */ +#define BRIN_EMPTY_RANGE_MASK 0x20 #define BRIN_PLACEHOLDER_MASK 0x40 #define BRIN_NULLS_MASK 0x80 #define BrinTupleDataOffset(tup) ((Size) (((BrinTuple *) (tup))->bt_info & BRIN_OFFSET_MASK)) #define BrinTupleHasNulls(tup) (((((BrinTuple *) (tup))->bt_info & BRIN_NULLS_MASK)) != 0) #define BrinTupleIsPlaceholder(tup) (((((BrinTuple *) (tup))->bt_info & BRIN_PLACEHOLDER_MASK)) != 0) +#define BrinTupleIsEmptyRange(tup) (((((BrinTuple *) (tup))->bt_info & BRIN_EMPTY_RANGE_MASK)) != 0) extern BrinTuple *brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, diff --git a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out index 2a4755d0998..584ac2602f7 100644 --- a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out +++ b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out @@ -4,7 +4,7 @@ starting permutation: s2check s1b s2b s1i s2summ s1c s2c s2check step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+-------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} (1 row) step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ; @@ -26,7 +26,7 @@ step s2c: COMMIT; step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+----------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} 2| 1| 1|f |f |f |{1 .. 1000} (2 rows) @@ -35,7 +35,7 @@ starting permutation: s2check s1b s1i s2vacuum s1c s2check step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+-------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} (1 row) step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ; @@ -45,7 +45,7 @@ step s1c: COMMIT; step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+----------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} 2| 1| 1|f |f |f |{1 .. 1000} (2 rows) diff --git a/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec b/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec index 19ac18a2e88..18ba92b7ba1 100644 --- a/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec +++ b/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec @@ -9,6 +9,7 @@ setup ) WITH (fillfactor=10); CREATE INDEX brinidx ON brin_iso USING brin (value) WITH (pages_per_range=1); -- this fills the first page + INSERT INTO brin_iso VALUES (NULL); DO $$ DECLARE curtid tid; BEGIN -- 2.39.2 [text/x-patch] 0002-Show-empty-ranges-in-brin_page_items.patch (6.9K, ../../[email protected]/3-0002-Show-empty-ranges-in-brin_page_items.patch) download | inline diff: From a7ed39285a7c8f3655091346755b81b0d79b2f3e Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Mon, 27 Mar 2023 22:47:12 +0200 Subject: [PATCH 2/3] Show empty ranges in brin_page_items Show which BRIN ranges are empty (no rows), as indicated by the newly introduced flag. --- contrib/pageinspect/brinfuncs.c | 10 ++++--- contrib/pageinspect/expected/brin.out | 6 ++-- .../pageinspect/pageinspect--1.11--1.12.sql | 17 +++++++++++ ...summarization-and-inprogress-insertion.out | 28 +++++++++---------- 4 files changed, 40 insertions(+), 21 deletions(-) diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c index 000dcd8f5d8..a781f265514 100644 --- a/contrib/pageinspect/brinfuncs.c +++ b/contrib/pageinspect/brinfuncs.c @@ -201,8 +201,8 @@ brin_page_items(PG_FUNCTION_ARGS) dtup = NULL; for (;;) { - Datum values[7]; - bool nulls[7] = {0}; + Datum values[8]; + bool nulls[8] = {0}; /* * This loop is called once for every attribute of every tuple in the @@ -239,6 +239,7 @@ brin_page_items(PG_FUNCTION_ARGS) nulls[4] = true; nulls[5] = true; nulls[6] = true; + nulls[7] = true; } else { @@ -261,6 +262,7 @@ brin_page_items(PG_FUNCTION_ARGS) values[3] = BoolGetDatum(dtup->bt_columns[att].bv_allnulls); values[4] = BoolGetDatum(dtup->bt_columns[att].bv_hasnulls); values[5] = BoolGetDatum(dtup->bt_placeholder); + values[6] = BoolGetDatum(dtup->bt_empty_range); if (!dtup->bt_columns[att].bv_allnulls) { BrinValues *bvalues = &dtup->bt_columns[att]; @@ -286,12 +288,12 @@ brin_page_items(PG_FUNCTION_ARGS) } appendStringInfoChar(&s, '}'); - values[6] = CStringGetTextDatum(s.data); + values[7] = CStringGetTextDatum(s.data); pfree(s.data); } else { - nulls[6] = true; + nulls[7] = true; } } diff --git a/contrib/pageinspect/expected/brin.out b/contrib/pageinspect/expected/brin.out index e12fbeb4774..098ddc202f4 100644 --- a/contrib/pageinspect/expected/brin.out +++ b/contrib/pageinspect/expected/brin.out @@ -43,9 +43,9 @@ SELECT * FROM brin_revmap_data(get_raw_page('test1_a_idx', 1)) LIMIT 5; SELECT * FROM brin_page_items(get_raw_page('test1_a_idx', 2), 'test1_a_idx') ORDER BY blknum, attnum LIMIT 5; - itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | value -------------+--------+--------+----------+----------+-------------+---------- - 1 | 0 | 1 | f | f | f | {1 .. 1} + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | f | f | f | {1 .. 1} (1 row) -- Mask DETAIL messages as these are not portable across architectures. diff --git a/contrib/pageinspect/pageinspect--1.11--1.12.sql b/contrib/pageinspect/pageinspect--1.11--1.12.sql index 70c3abccf57..a20d67a9e82 100644 --- a/contrib/pageinspect/pageinspect--1.11--1.12.sql +++ b/contrib/pageinspect/pageinspect--1.11--1.12.sql @@ -21,3 +21,20 @@ CREATE FUNCTION bt_multi_page_stats(IN relname text, IN blkno int8, IN blk_count RETURNS SETOF record AS 'MODULE_PATHNAME', 'bt_multi_page_stats' LANGUAGE C STRICT PARALLEL RESTRICTED; + +-- +-- add information about BRIN empty ranges +-- +DROP FUNCTION brin_page_items(IN page bytea, IN index_oid regclass); +CREATE FUNCTION brin_page_items(IN page bytea, IN index_oid regclass, + OUT itemoffset int, + OUT blknum int8, + OUT attnum int, + OUT allnulls bool, + OUT hasnulls bool, + OUT placeholder bool, + OUT empty bool, + OUT value text) +RETURNS SETOF record +AS 'MODULE_PATHNAME', 'brin_page_items' +LANGUAGE C STRICT PARALLEL RESTRICTED; diff --git a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out index 584ac2602f7..201786c82c0 100644 --- a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out +++ b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out @@ -2,9 +2,9 @@ Parsed test spec with 2 sessions starting permutation: s2check s1b s2b s1i s2summ s1c s2c s2check step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); -itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value -----------+------+------+--------+--------+-----------+-------- - 1| 0| 1|f |t |f |{1 .. 1} +itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|empty|value +----------+------+------+--------+--------+-----------+-----+-------- + 1| 0| 1|f |t |f |f |{1 .. 1} (1 row) step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ; @@ -24,18 +24,18 @@ brin_summarize_new_values step s1c: COMMIT; step s2c: COMMIT; step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); -itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value -----------+------+------+--------+--------+-----------+----------- - 1| 0| 1|f |t |f |{1 .. 1} - 2| 1| 1|f |f |f |{1 .. 1000} +itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|empty|value +----------+------+------+--------+--------+-----------+-----+----------- + 1| 0| 1|f |t |f |f |{1 .. 1} + 2| 1| 1|f |f |f |f |{1 .. 1000} (2 rows) starting permutation: s2check s1b s1i s2vacuum s1c s2check step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); -itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value -----------+------+------+--------+--------+-----------+-------- - 1| 0| 1|f |t |f |{1 .. 1} +itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|empty|value +----------+------+------+--------+--------+-----------+-----+-------- + 1| 0| 1|f |t |f |f |{1 .. 1} (1 row) step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ; @@ -43,9 +43,9 @@ step s1i: INSERT INTO brin_iso VALUES (1000); step s2vacuum: VACUUM brin_iso; step s1c: COMMIT; step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); -itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value -----------+------+------+--------+--------+-----------+----------- - 1| 0| 1|f |t |f |{1 .. 1} - 2| 1| 1|f |f |f |{1 .. 1000} +itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|empty|value +----------+------+------+--------+--------+-----------+-----+----------- + 1| 0| 1|f |t |f |f |{1 .. 1} + 2| 1| 1|f |f |f |f |{1 .. 1000} (2 rows) -- 2.39.2 [text/x-patch] 0003-extra-tests.patch (25.1K, ../../[email protected]/4-0003-extra-tests.patch) download | inline diff: From 3906763f4cd896c5fad68248eb77fbd59299f162 Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Mon, 27 Mar 2023 22:50:46 +0200 Subject: [PATCH 3/3] extra tests --- contrib/pageinspect/Makefile | 2 +- contrib/pageinspect/expected/brin-fails.out | 152 +++++++++++++++ contrib/pageinspect/expected/brin2.out | 201 ++++++++++++++++++++ contrib/pageinspect/sql/brin-fails.sql | 86 +++++++++ contrib/pageinspect/sql/brin2.sql | 117 ++++++++++++ 5 files changed, 557 insertions(+), 1 deletion(-) create mode 100644 contrib/pageinspect/expected/brin-fails.out create mode 100644 contrib/pageinspect/expected/brin2.out create mode 100644 contrib/pageinspect/sql/brin-fails.sql create mode 100644 contrib/pageinspect/sql/brin2.sql diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile index 95e030b3969..6a5795fdfd9 100644 --- a/contrib/pageinspect/Makefile +++ b/contrib/pageinspect/Makefile @@ -22,7 +22,7 @@ DATA = pageinspect--1.11--1.12.sql pageinspect--1.10--1.11.sql \ pageinspect--1.0--1.1.sql PGFILEDESC = "pageinspect - functions to inspect contents of database pages" -REGRESS = page btree brin gin gist hash checksum oldextversions +REGRESS = page btree brin brin2 gin gist hash checksum brin-fails oldextversions ifdef USE_PGXS PG_CONFIG = pg_config diff --git a/contrib/pageinspect/expected/brin-fails.out b/contrib/pageinspect/expected/brin-fails.out new file mode 100644 index 00000000000..08479894ec3 --- /dev/null +++ b/contrib/pageinspect/expected/brin-fails.out @@ -0,0 +1,152 @@ +create table t (a int); +-- works +drop index if exists t_a_idx; +NOTICE: index "t_a_idx" does not exist, skipping +truncate t; +insert into t values (null), (1); +create index on t using brin (a); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 1} +(1 row) + +-- works +drop index if exists t_a_idx; +truncate t; +insert into t values (1), (null); +create index on t using brin (a); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 1} +(1 row) + +-- fails +drop index if exists t_a_idx; +truncate t; +insert into t values (null); +create index on t using brin (a); +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 1} +(1 row) + +-- fails +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a); +insert into t values (null); +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 1} +(1 row) + +-- works +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a) with (pages_per_range=1); +insert into t select null from generate_series(1,291); -- fill first page +insert into t values (null), (1); +select brin_summarize_new_values('t_a_idx'); + brin_summarize_new_values +--------------------------- + 1 +(1 row) + +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | t | f | f | f | + 2 | 1 | 1 | f | t | f | f | {1 .. 1} +(2 rows) + +-- fails +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a) with (pages_per_range=1); +insert into t select null from generate_series(1,291); -- fill first page +insert into t values (null); +select brin_summarize_new_values('t_a_idx'); + brin_summarize_new_values +--------------------------- + 1 +(1 row) + +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | t | f | f | f | + 2 | 1 | 1 | f | t | f | f | {1 .. 1} +(2 rows) + +-- works +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a) with (pages_per_range=1); +insert into t select null from generate_series(1,291); -- fill first page +insert into t values (null); +insert into t values (1); +select brin_summarize_new_values('t_a_idx'); + brin_summarize_new_values +--------------------------- + 1 +(1 row) + +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | t | f | f | f | + 2 | 1 | 1 | f | t | f | f | {1 .. 1} +(2 rows) + +-- fails +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a) with (pages_per_range=1); +insert into t select null from generate_series(1,291); -- fill first page +insert into t values (null); +insert into t values (null); +select brin_summarize_new_values('t_a_idx'); + brin_summarize_new_values +--------------------------- + 1 +(1 row) + +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | t | f | f | f | + 2 | 1 | 1 | f | t | f | f | {1 .. 1} +(2 rows) + +-- fails +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a) with (pages_per_range=1); +insert into t select null from generate_series(1,291); -- fill first page +insert into t values (null); +insert into t values (null); +select brin_summarize_new_values('t_a_idx'); + brin_summarize_new_values +--------------------------- + 1 +(1 row) + +insert into t values (null); +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | t | f | f | f | + 2 | 1 | 1 | f | t | f | f | {1 .. 1} +(2 rows) + +drop table t; diff --git a/contrib/pageinspect/expected/brin2.out b/contrib/pageinspect/expected/brin2.out new file mode 100644 index 00000000000..8406a792cd3 --- /dev/null +++ b/contrib/pageinspect/expected/brin2.out @@ -0,0 +1,201 @@ +create table t (a int); +-- +drop index if exists t_a_idx; +NOTICE: index "t_a_idx" does not exist, skipping +truncate t; +create index on t using brin (a); +-- empty range, all_nulls=true (default) +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+------- + 1 | 0 | 1 | t | f | f | t | +(1 row) + +-- insert NULL value, range no longer empty, all_nulls=true +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+------- + 1 | 0 | 1 | t | f | f | f | +(1 row) + +-- another NULL value, still all_nulls=true +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+------- + 1 | 0 | 1 | t | f | f | f | +(1 row) + +-- not-NULL value, switches to has_nulls=true +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 1} +(1 row) + +-- reinsert the not-NULL value, stays the same +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 1} +(1 row) + +-- another not-NULL value, still has_nulls=true, range extends +insert into t values (2); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 2} +(1 row) + +-- another NULL value, still has_nulls=true +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 2} +(1 row) + +-- +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a); +-- empty range, all_nulls=true (default) +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+------- + 1 | 0 | 1 | t | f | f | t | +(1 row) + +-- insert non-NULL value, range no longer empty, all_nulls=false +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | f | f | f | {1 .. 1} +(1 row) + +-- re-insert non-NULL value, stays the same +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | f | f | f | {1 .. 1} +(1 row) + +-- insert NULL value, has_nulls=true +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 1} +(1 row) + +-- another not-NULL value, still has_nulls=true, range expands +insert into t values (2); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 2} +(1 row) + +-- another NULL value, still has_nulls=true +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 2} +(1 row) + +-- +drop index if exists t_a_idx; +truncate t; +insert into t values (1); -- start with one non-NULL value +create index on t using brin (a); +-- non-empty range, all_nulls/has_nulls=false +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | f | f | f | {1 .. 1} +(1 row) + +-- re-insert the non-NULL value, stays the same +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | f | f | f | {1 .. 1} +(1 row) + +-- insert another non-NULL value, null flags stay the same +insert into t values (2); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | f | f | f | {1 .. 2} +(1 row) + +-- insert NULL value, has_nulls=true +insert into t values (null); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 2} +(1 row) + +-- +drop index if exists t_a_idx; +truncate t; +insert into t values (NULL); -- start with one non-NULL value +create index on t using brin (a); +-- non-empty range, all_nulls=true +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+------- + 1 | 0 | 1 | t | f | f | f | +(1 row) + +-- re-insert NULL, stays the same +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+------- + 1 | 0 | 1 | t | f | f | f | +(1 row) + +-- insert a non-NULL value, switches to has_nulls=true +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 1} +(1 row) + +-- re-insert the non-NULL value, stays the same +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 1} +(1 row) + +-- insert another the non-NULL value, stays the same, range updated +insert into t values (2); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 2} +(1 row) + +-- insert NULL value, stays the same +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | t | f | f | {1 .. 2} +(1 row) + +drop table t; diff --git a/contrib/pageinspect/sql/brin-fails.sql b/contrib/pageinspect/sql/brin-fails.sql new file mode 100644 index 00000000000..e5b37fa6b12 --- /dev/null +++ b/contrib/pageinspect/sql/brin-fails.sql @@ -0,0 +1,86 @@ +create table t (a int); + +-- works +drop index if exists t_a_idx; +truncate t; +insert into t values (null), (1); +create index on t using brin (a); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- works +drop index if exists t_a_idx; +truncate t; +insert into t values (1), (null); +create index on t using brin (a); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- fails +drop index if exists t_a_idx; +truncate t; +insert into t values (null); +create index on t using brin (a); +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- fails +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a); +insert into t values (null); +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- works +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a) with (pages_per_range=1); +insert into t select null from generate_series(1,291); -- fill first page +insert into t values (null), (1); +select brin_summarize_new_values('t_a_idx'); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- fails +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a) with (pages_per_range=1); +insert into t select null from generate_series(1,291); -- fill first page +insert into t values (null); +select brin_summarize_new_values('t_a_idx'); +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- works +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a) with (pages_per_range=1); +insert into t select null from generate_series(1,291); -- fill first page +insert into t values (null); +insert into t values (1); +select brin_summarize_new_values('t_a_idx'); +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- fails +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a) with (pages_per_range=1); +insert into t select null from generate_series(1,291); -- fill first page +insert into t values (null); +insert into t values (null); +select brin_summarize_new_values('t_a_idx'); +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- fails +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a) with (pages_per_range=1); +insert into t select null from generate_series(1,291); -- fill first page +insert into t values (null); +insert into t values (null); +select brin_summarize_new_values('t_a_idx'); +insert into t values (null); +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +drop table t; diff --git a/contrib/pageinspect/sql/brin2.sql b/contrib/pageinspect/sql/brin2.sql new file mode 100644 index 00000000000..501252e7b8c --- /dev/null +++ b/contrib/pageinspect/sql/brin2.sql @@ -0,0 +1,117 @@ +create table t (a int); + +-- +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a); + +-- empty range, all_nulls=true (default) +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- insert NULL value, range no longer empty, all_nulls=true +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- another NULL value, still all_nulls=true +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- not-NULL value, switches to has_nulls=true +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- reinsert the not-NULL value, stays the same +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- another not-NULL value, still has_nulls=true, range extends +insert into t values (2); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- another NULL value, still has_nulls=true +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + + +-- +drop index if exists t_a_idx; +truncate t; +create index on t using brin (a); + +-- empty range, all_nulls=true (default) +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- insert non-NULL value, range no longer empty, all_nulls=false +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- re-insert non-NULL value, stays the same +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- insert NULL value, has_nulls=true +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- another not-NULL value, still has_nulls=true, range expands +insert into t values (2); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- another NULL value, still has_nulls=true +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + + +-- +drop index if exists t_a_idx; +truncate t; +insert into t values (1); -- start with one non-NULL value +create index on t using brin (a); + +-- non-empty range, all_nulls/has_nulls=false +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- re-insert the non-NULL value, stays the same +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- insert another non-NULL value, null flags stay the same +insert into t values (2); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- insert NULL value, has_nulls=true +insert into t values (null); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + + +-- +drop index if exists t_a_idx; +truncate t; +insert into t values (NULL); -- start with one non-NULL value +create index on t using brin (a); + +-- non-empty range, all_nulls=true +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- re-insert NULL, stays the same +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- insert a non-NULL value, switches to has_nulls=true +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- re-insert the non-NULL value, stays the same +insert into t values (1); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- insert another the non-NULL value, stays the same, range updated +insert into t values (2); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + +-- insert NULL value, stays the same +insert into t values (NULL); +select * from brin_page_items(get_raw_page('t_a_idx', 2), 't_a_idx'::regclass); + + +drop table t; -- 2.39.2 [application/x-shellscript] stress-test.sh (5.9K, ../../[email protected]/5-stress-test.sh) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Missing update of all_hasnulls in BRIN opclasses 2023-03-03 12:14 Re: Missing update of all_hasnulls in BRIN opclasses Tomas Vondra <[email protected]> 2023-03-28 14:30 ` Re: Missing update of all_hasnulls in BRIN opclasses Tomas Vondra <[email protected]> @ 2023-04-23 20:43 ` Tomas Vondra <[email protected]> 2023-04-24 15:36 ` Re: Missing update of all_hasnulls in BRIN opclasses Alvaro Herrera <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tomas Vondra @ 2023-04-23 20:43 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Matthias van de Meent <[email protected]>; [email protected] Hi, here's an updated version of the patch, including a backport version. I ended up making the code yet a bit closer to master by introducing add_values_to_range(). The current pre-14 code has the loop adding data to the BRIN tuple in two places, but with the "fixed" logic handling NULLs and the empty_range flag the amount of duplicated code got too high, so this seem reasonable. Both cases have a patch extending pageinspect to show the new flag, but obviously we should commit that only in master. In backbranches it's meant only to make testing easier. I plan to do a bit more testing, I'd welcome some feedback - it's a long-standing bug, and it'd be good to finally get this fixed. I don't think the patch can be made any simpler. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company Attachments: [text/x-patch] 0001-Fix-handling-of-NULLs-in-BRIN-indexes-20230423-11-13.patch (18.3K, ../../[email protected]/2-0001-Fix-handling-of-NULLs-in-BRIN-indexes-20230423-11-13.patch) download | inline diff: From 42603e32bd0ad456a53a96ac2d05ce714f97e1ba Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Sun, 23 Apr 2023 19:26:18 +0200 Subject: [PATCH 1/2] Fix handling of NULLs in BRIN indexes BRIN indexes did not properly distinguish between summaries for empty (no rows) and all-NULL ranges. All summaries were initialized with allnulls=true, and the opclasses simply reset allnulls to false when processing the first non-NULL value. This however fails if the range starts with a NULL value (or a sequence of NULL values), in which case we forget the range contains NULL values. This happens because the allnulls flag is used for two separate purposes - to mark empty ranges (not representing any rows yet) and ranges containing only NULL values. Opclasses don't know which of these cases it is, and so don't know whether to set hasnulls=true. Setting hasnulls=true in both cases would make it correct, but it would also make BRIN indexes useless for queries with IS NULL clauses - all ranges start empty (and thus allnulls=true), so all ranges would end up with either allnulls=true or hasnulls=true. The severity of the issue is somewhat reduced by the fact that it only happens when adding values to an existing summary with allnulls=true, not when the summarization is processing values in bulk (e.g. during CREATE INDEX or automatic summarization). In this case the flags were updated in a slightly different way, not forgetting the NULL values. The best solution would be to introduce a new flag marking index tuples representing ranges with no rows, but that would break on-disk format and/or ABI, depending on where we put the flag. Considering we need to backpatch this, that's not acceptable. So instead we use an "impossible" combination of both flags (allnulls and hasnulls) set to true, to mark "empty" ranges with no rows. In principle "empty" is a feature of the whole index tuple, which may contain multiple summaries in a multi-column index, but this is where the flags are, unfortunately. We could also skip storing index tuples for empty summaries, but then we'd have to always process such ranges - even if there are no rows in large parts of the table (e.g. after a bulk DELETE), it would still require reading the pages etc. So we store them, but ignore them when building the bitmap. Backpatch to 11. The issue exists since BRIN indexes were introduced in 9.5, but older releases are already EOL. Backpatch-through: 11 Reviewed-by: Justin Pryzby, Matthias van de Meent Discussion: https://postgr.es/m/[email protected] --- src/backend/access/brin/brin.c | 226 ++++++++++++++---- src/backend/access/brin/brin_tuple.c | 15 +- src/include/access/brin_tuple.h | 6 +- ...summarization-and-inprogress-insertion.out | 8 +- ...ummarization-and-inprogress-insertion.spec | 1 + 5 files changed, 201 insertions(+), 55 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 0becfde1133..f99a9ba5cf9 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -35,6 +35,7 @@ #include "storage/freespace.h" #include "utils/acl.h" #include "utils/builtins.h" +#include "utils/datum.h" #include "utils/index_selfuncs.h" #include "utils/memutils.h" #include "utils/rel.h" @@ -77,7 +78,8 @@ static void form_and_insert_tuple(BrinBuildState *state); static void union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b); static void brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy); - +static bool add_values_to_range(Relation idxRel, BrinDesc *bdesc, + BrinMemTuple *dtup, Datum *values, bool *nulls); /* * BRIN handler function: return IndexAmRoutine with access method parameters @@ -173,11 +175,10 @@ brininsert(Relation idxRel, Datum *values, bool *nulls, for (;;) { - bool need_insert = false; + bool need_insert; OffsetNumber off; BrinTuple *brtup; BrinMemTuple *dtup; - int keyno; CHECK_FOR_INTERRUPTS(); @@ -241,31 +242,7 @@ brininsert(Relation idxRel, Datum *values, bool *nulls, dtup = brin_deform_tuple(bdesc, brtup, NULL); - /* - * Compare the key values of the new tuple to the stored index values; - * our deformed tuple will get updated if the new tuple doesn't fit - * the original range (note this means we can't break out of the loop - * early). Make a note of whether this happens, so that we know to - * insert the modified tuple later. - */ - for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++) - { - Datum result; - BrinValues *bval; - FmgrInfo *addValue; - - bval = &dtup->bt_columns[keyno]; - addValue = index_getprocinfo(idxRel, keyno + 1, - BRIN_PROCNUM_ADDVALUE); - result = FunctionCall4Coll(addValue, - idxRel->rd_indcollation[keyno], - PointerGetDatum(bdesc), - PointerGetDatum(bval), - values[keyno], - nulls[keyno]); - /* if that returned true, we need to insert the updated tuple */ - need_insert |= DatumGetBool(result); - } + need_insert = add_values_to_range(idxRel, bdesc, dtup, values, nulls); if (!need_insert) { @@ -508,6 +485,17 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) CurrentMemoryContext); } + /* + * If the BRIN tuple indicates that this range is empty, + * we can skip it: there's nothing to match. We don't + * need to examine the next columns. + */ + if (dtup->bt_empty_range) + { + addrange = false; + break; + } + /* * Check whether the scan key is consistent with the page * range values; if so, have the pages in the range added @@ -611,7 +599,6 @@ brinbuildCallback(Relation index, { BrinBuildState *state = (BrinBuildState *) brstate; BlockNumber thisblock; - int i; thisblock = ItemPointerGetBlockNumber(tid); @@ -640,25 +627,8 @@ brinbuildCallback(Relation index, } /* Accumulate the current tuple into the running state */ - for (i = 0; i < state->bs_bdesc->bd_tupdesc->natts; i++) - { - FmgrInfo *addValue; - BrinValues *col; - Form_pg_attribute attr = TupleDescAttr(state->bs_bdesc->bd_tupdesc, i); - - col = &state->bs_dtuple->bt_columns[i]; - addValue = index_getprocinfo(index, i + 1, - BRIN_PROCNUM_ADDVALUE); - - /* - * Update dtuple state, if and as necessary. - */ - FunctionCall4Coll(addValue, - attr->attcollation, - PointerGetDatum(state->bs_bdesc), - PointerGetDatum(col), - values[i], isnull[i]); - } + (void) add_values_to_range(index, state->bs_bdesc, state->bs_dtuple, + values, isnull); } /* @@ -1448,6 +1418,8 @@ form_and_insert_tuple(BrinBuildState *state) /* * Given two deformed tuples, adjust the first one so that it's consistent * with the summary values in both. + * + * XXX I'm not sure we can actually get empty "b". */ static void union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b) @@ -1465,6 +1437,64 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b) db = brin_deform_tuple(bdesc, b, NULL); MemoryContextSwitchTo(oldcxt); + /* + * Check if the ranges are empty. + * + * If at least one of them is empty, we don't need to call per-key union + * functions at all. If "b" is empty, we just use "a" as the result (it + * might be empty fine, but that's fine). If "a" is empty but "b" is not, + * we use "b" as the result (but we have to copy the data into "a" first). + * + * Only when both ranges are non-empty, we actually do the per-key merge. + */ + + /* If "b" is empty - ignore it and just use "a" (even if it's empty etc.). */ + if (db->bt_empty_range) + { + /* skip the per-key merge */ + MemoryContextDelete(cxt); + return; + } + + /* + * Now we know "b" is not empty. If "a" is empty, then "b" is the result. + * But we need to copy the data from "b" to "a" first, because that's how + * we pass result out. + * + * We have to copy all the global/per-key flags etc. too. + */ + if (a->bt_empty_range) + { + for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++) + { + int i; + BrinValues *col_a = &a->bt_columns[keyno]; + BrinValues *col_b = &db->bt_columns[keyno]; + BrinOpcInfo *opcinfo = bdesc->bd_info[keyno]; + + col_a->bv_allnulls = col_b->bv_allnulls; + col_a->bv_hasnulls = col_b->bv_hasnulls; + + /* If "b" has no data, we're done. */ + if (col_b->bv_allnulls) + continue; + + for (i = 0; i < opcinfo->oi_nstored; i++) + col_a->bv_values[i] = + datumCopy(col_b->bv_values[i], + opcinfo->oi_typcache[i]->typbyval, + opcinfo->oi_typcache[i]->typlen); + } + + /* "a" started empty, but "b" was not empty, so remember that */ + a->bt_empty_range = false; + + /* skip the per-key merge */ + MemoryContextDelete(cxt); + return; + } + + /* Now we know neither range is empty. */ for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++) { FmgrInfo *unionFn; @@ -1523,3 +1553,103 @@ brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy) */ FreeSpaceMapVacuum(idxrel); } + +static bool +add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup, + Datum *values, bool *nulls) +{ + int keyno; + + /* If the range starts empty, we're certainly going to modify it. */ + bool modified = dtup->bt_empty_range; + + /* + * Compare the key values of the new tuple to the stored index values; + * our deformed tuple will get updated if the new tuple doesn't fit + * the original range (note this means we can't break out of the loop + * early). Make a note of whether this happens, so that we know to + * insert the modified tuple later. + */ + for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++) + { + Datum result; + BrinValues *bval; + FmgrInfo *addValue; + bool has_nulls; + + bval = &dtup->bt_columns[keyno]; + + /* + * Does the range have actual NULL values? Either of the flags can + * be set, but we ignore the state before adding first row. + * + * We have to remember this, because we'll modify the flags and we + * need to know if the range started as empty. + */ + has_nulls = ((!dtup->bt_empty_range) && + (bval->bv_hasnulls || bval->bv_allnulls)); + + /* + * If the value we're adding is NULL, handle it locally. Otherwise + * call the BRIN_PROCNUM_ADDVALUE procedure. + */ + if (nulls[keyno]) + { + /* + * We can't check "bv_hasnulls" because then we might end up with + * both flags set to true, which is interpreted as empty range. + * But that'd be wrong, because we've just added a value. + * + * So either the range has allnulls=true, or we have to set the + * hasnulls flag. Check if we're changing the value to determine + * if the index tuple was modified. + */ + if (!bval->bv_allnulls) + { + /* Are we changing the tuple? */ + modified |= (!bval->bv_hasnulls); + bval->bv_hasnulls = true; + } + } + else + { + addValue = index_getprocinfo(idxRel, keyno + 1, + BRIN_PROCNUM_ADDVALUE); + result = FunctionCall4Coll(addValue, + idxRel->rd_indcollation[keyno], + PointerGetDatum(bdesc), + PointerGetDatum(bval), + values[keyno], + nulls[keyno]); + /* if that returned true, we need to insert the updated tuple */ + modified |= DatumGetBool(result); + } + + /* + * If the range was had actual NULL values (i.e. did not start empty), + * make sure we don't forget about the NULL values. Either the allnulls + * flag is still set to true, or (if the opclass cleared it) we need to + * set hasnulls=true. + * + * XXX This can only happen when the opclass modified the tuple, so the + * modified flag should be set. + */ + if (has_nulls && !(bval->bv_hasnulls || bval->bv_allnulls)) + { + Assert(modified); + bval->bv_hasnulls = true; + } + } + + /* + * After updating summaries for all the keys, mark it as not empty. + * + * If we're actually changing the flag value (i.e. tuple started as + * empty), we should have modified the tuple. So we should not see + * empty range that was not modified. + */ + Assert(!dtup->bt_empty_range || modified); + dtup->bt_empty_range = false; + + return modified; +} diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c index b3b453aed12..f9877980f4d 100644 --- a/src/backend/access/brin/brin_tuple.c +++ b/src/backend/access/brin/brin_tuple.c @@ -349,6 +349,9 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple, if (tuple->bt_placeholder) rettuple->bt_info |= BRIN_PLACEHOLDER_MASK; + if (tuple->bt_empty_range) + rettuple->bt_info |= BRIN_EMPTY_RANGE_MASK; + *size = len; return rettuple; } @@ -376,7 +379,7 @@ brin_form_placeholder_tuple(BrinDesc *brdesc, BlockNumber blkno, Size *size) rettuple = palloc0(len); rettuple->bt_blkno = blkno; rettuple->bt_info = hoff; - rettuple->bt_info |= BRIN_NULLS_MASK | BRIN_PLACEHOLDER_MASK; + rettuple->bt_info |= BRIN_NULLS_MASK | BRIN_PLACEHOLDER_MASK | BRIN_EMPTY_RANGE_MASK; bitP = ((bits8 *) ((char *) rettuple + SizeOfBrinTuple)) - 1; bitmask = HIGHBIT; @@ -466,6 +469,8 @@ brin_new_memtuple(BrinDesc *brdesc) dtup->bt_allnulls = palloc(sizeof(bool) * brdesc->bd_tupdesc->natts); dtup->bt_hasnulls = palloc(sizeof(bool) * brdesc->bd_tupdesc->natts); + dtup->bt_empty_range = true; + dtup->bt_context = AllocSetContextCreate(CurrentMemoryContext, "brin dtuple", ALLOCSET_DEFAULT_SIZES); @@ -499,6 +504,9 @@ brin_memtuple_initialize(BrinMemTuple *dtuple, BrinDesc *brdesc) currdatum += sizeof(Datum) * brdesc->bd_info[i]->oi_nstored; } + /* FIXME Shouldn't this reset bt_placeholder too? */ + dtuple->bt_empty_range = true; + return dtuple; } @@ -532,6 +540,11 @@ brin_deform_tuple(BrinDesc *brdesc, BrinTuple *tuple, BrinMemTuple *dMemtuple) if (BrinTupleIsPlaceholder(tuple)) dtup->bt_placeholder = true; + + /* ranges start as empty, depends on the BrinTuple */ + if (!BrinTupleIsEmptyRange(tuple)) + dtup->bt_empty_range = false; + dtup->bt_blkno = tuple->bt_blkno; values = dtup->bt_values; diff --git a/src/include/access/brin_tuple.h b/src/include/access/brin_tuple.h index a9ccc3995b4..5280872707a 100644 --- a/src/include/access/brin_tuple.h +++ b/src/include/access/brin_tuple.h @@ -36,6 +36,7 @@ typedef struct BrinValues typedef struct BrinMemTuple { bool bt_placeholder; /* this is a placeholder tuple */ + bool bt_empty_range; /* range represents no tuples */ BlockNumber bt_blkno; /* heap blkno that the tuple is for */ MemoryContext bt_context; /* memcxt holding the bt_columns values */ /* output arrays for brin_deform_tuple: */ @@ -61,7 +62,7 @@ typedef struct BrinTuple * * 7th (high) bit: has nulls * 6th bit: is placeholder tuple - * 5th bit: unused + * 5th bit: range is empty * 4-0 bit: offset of data * --------------- */ @@ -74,13 +75,14 @@ typedef struct BrinTuple * bt_info manipulation macros */ #define BRIN_OFFSET_MASK 0x1F -/* bit 0x20 is not used at present */ +#define BRIN_EMPTY_RANGE_MASK 0x20 #define BRIN_PLACEHOLDER_MASK 0x40 #define BRIN_NULLS_MASK 0x80 #define BrinTupleDataOffset(tup) ((Size) (((BrinTuple *) (tup))->bt_info & BRIN_OFFSET_MASK)) #define BrinTupleHasNulls(tup) (((((BrinTuple *) (tup))->bt_info & BRIN_NULLS_MASK)) != 0) #define BrinTupleIsPlaceholder(tup) (((((BrinTuple *) (tup))->bt_info & BRIN_PLACEHOLDER_MASK)) != 0) +#define BrinTupleIsEmptyRange(tup) (((((BrinTuple *) (tup))->bt_info & BRIN_EMPTY_RANGE_MASK)) != 0) extern BrinTuple *brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, diff --git a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out index 2a4755d0998..584ac2602f7 100644 --- a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out +++ b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out @@ -4,7 +4,7 @@ starting permutation: s2check s1b s2b s1i s2summ s1c s2c s2check step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+-------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} (1 row) step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ; @@ -26,7 +26,7 @@ step s2c: COMMIT; step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+----------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} 2| 1| 1|f |f |f |{1 .. 1000} (2 rows) @@ -35,7 +35,7 @@ starting permutation: s2check s1b s1i s2vacuum s1c s2check step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+-------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} (1 row) step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ; @@ -45,7 +45,7 @@ step s1c: COMMIT; step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+----------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} 2| 1| 1|f |f |f |{1 .. 1000} (2 rows) diff --git a/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec b/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec index 19ac18a2e88..18ba92b7ba1 100644 --- a/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec +++ b/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec @@ -9,6 +9,7 @@ setup ) WITH (fillfactor=10); CREATE INDEX brinidx ON brin_iso USING brin (value) WITH (pages_per_range=1); -- this fills the first page + INSERT INTO brin_iso VALUES (NULL); DO $$ DECLARE curtid tid; BEGIN -- 2.40.0 [text/x-patch] 0002-pageinspect-tweak-20230423-11-13.patch (2.5K, ../../[email protected]/3-0002-pageinspect-tweak-20230423-11-13.patch) download | inline diff: From 157923fb3d55095d8d45fb1e7fe56a992d667b32 Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Sun, 23 Apr 2023 22:19:05 +0200 Subject: [PATCH 2/2] pageinspect tweak --- contrib/pageinspect/brinfuncs.c | 10 ++++++---- contrib/pageinspect/pageinspect--1.7--1.8.sql | 20 +++++++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c index 04a90c4782d..8db6f1bcc7d 100644 --- a/contrib/pageinspect/brinfuncs.c +++ b/contrib/pageinspect/brinfuncs.c @@ -227,8 +227,8 @@ brin_page_items(PG_FUNCTION_ARGS) dtup = NULL; for (;;) { - Datum values[7]; - bool nulls[7]; + Datum values[8]; + bool nulls[8]; /* * This loop is called once for every attribute of every tuple in the @@ -267,6 +267,7 @@ brin_page_items(PG_FUNCTION_ARGS) nulls[4] = true; nulls[5] = true; nulls[6] = true; + nulls[7] = true; } else { @@ -278,6 +279,7 @@ brin_page_items(PG_FUNCTION_ARGS) values[3] = BoolGetDatum(dtup->bt_columns[att].bv_allnulls); values[4] = BoolGetDatum(dtup->bt_columns[att].bv_hasnulls); values[5] = BoolGetDatum(dtup->bt_placeholder); + values[6] = BoolGetDatum(dtup->bt_empty_range); if (!dtup->bt_columns[att].bv_allnulls) { BrinValues *bvalues = &dtup->bt_columns[att]; @@ -303,12 +305,12 @@ brin_page_items(PG_FUNCTION_ARGS) } appendStringInfoChar(&s, '}'); - values[6] = CStringGetTextDatum(s.data); + values[7] = CStringGetTextDatum(s.data); pfree(s.data); } else { - nulls[6] = true; + nulls[7] = true; } } diff --git a/contrib/pageinspect/pageinspect--1.7--1.8.sql b/contrib/pageinspect/pageinspect--1.7--1.8.sql index 45031a026a6..edfb580a4ec 100644 --- a/contrib/pageinspect/pageinspect--1.7--1.8.sql +++ b/contrib/pageinspect/pageinspect--1.7--1.8.sql @@ -67,3 +67,23 @@ CREATE FUNCTION bt_page_items(IN page bytea, RETURNS SETOF record AS 'MODULE_PATHNAME', 'bt_page_items_bytea' LANGUAGE C STRICT PARALLEL SAFE; + +-- +-- add information about BRIN empty ranges +-- +DROP FUNCTION brin_page_items(IN page bytea, IN index_oid regclass); +-- +-- brin_page_items() +-- +CREATE FUNCTION brin_page_items(IN page bytea, IN index_oid regclass, + OUT itemoffset int, + OUT blknum int, + OUT attnum int, + OUT allnulls bool, + OUT hasnulls bool, + OUT placeholder bool, + OUT empty bool, + OUT value text) +RETURNS SETOF record +AS 'MODULE_PATHNAME', 'brin_page_items' +LANGUAGE C STRICT PARALLEL SAFE; -- 2.40.0 [text/x-patch] 0001-Fix-handling-of-NULLs-in-BRIN-indexe-20230423-master.patch (13.8K, ../../[email protected]/4-0001-Fix-handling-of-NULLs-in-BRIN-indexe-20230423-master.patch) download | inline diff: From 75ca742082cd50cef4609ecf0021e7bf69f34de6 Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Sun, 8 Jan 2023 16:43:06 +0100 Subject: [PATCH 1/2] Fix handling of NULLs in BRIN indexes BRIN indexes did not properly distinguish between summaries for empty (no rows) and all-NULL ranges. All summaries were initialized with allnulls=true, and the opclasses simply reset allnulls to false when processing the first non-NULL value. This however fails if the range starts with a NULL value (or a sequence of NULL values), in which case we forget the range contains NULL values. This happens because the allnulls flag is used for two separate purposes - to mark empty ranges (not representing any rows yet) and ranges containing only NULL values. Opclasses don't know which of these cases it is, and so don't know whether to set hasnulls=true. Setting hasnulls=true in both cases would make it correct, but it would also make BRIN indexes useless for queries with IS NULL clauses - all ranges start empty (and thus allnulls=true), so all ranges would end up with either allnulls=true or hasnulls=true. The severity of the issue is somewhat reduced by the fact that it only happens when adding values to an existing summary with allnulls=true, not when the summarization is processing values in bulk (e.g. during CREATE INDEX or automatic summarization). In this case the flags were updated in a slightly different way, not forgetting the NULL values. This introduces a new a new flag marking index tuples representing ranges with no rows. Luckily we have an unused tuple in the BRIN tuple header that we can use for this. We still store index tuples for empty ranges, because otherwise we'd not be able to say whether a range is empty or not yet summarized, and we'd have to process them for any query. Backpatch to 11. The issue exists since BRIN indexes were introduced in 9.5, but older releases are already EOL. Backpatch-through: 11 Reviewed-by: Alvaro Herrera, Justin Pryzby, Matthias van de Meent Discussion: https://postgr.es/m/[email protected] --- src/backend/access/brin/brin.c | 115 +++++++++++++++++- src/backend/access/brin/brin_tuple.c | 15 ++- src/include/access/brin_tuple.h | 6 +- ...summarization-and-inprogress-insertion.out | 8 +- ...ummarization-and-inprogress-insertion.spec | 1 + 5 files changed, 137 insertions(+), 8 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 41bf950a4af..2e20f318e95 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -592,6 +592,17 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) bval = &dtup->bt_columns[attno - 1]; + /* + * If the BRIN tuple indicates that this range is empty, + * we can skip it: there's nothing to match. We don't + * need to examine the next columns. + */ + if (dtup->bt_empty_range) + { + addrange = false; + break; + } + /* * First check if there are any IS [NOT] NULL scan keys, * and if we're violating them. In that case we can @@ -1589,6 +1600,8 @@ form_and_insert_tuple(BrinBuildState *state) /* * Given two deformed tuples, adjust the first one so that it's consistent * with the summary values in both. + * + * XXX I'm not sure we can actually get empty "b". */ static void union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b) @@ -1606,6 +1619,64 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b) db = brin_deform_tuple(bdesc, b, NULL); MemoryContextSwitchTo(oldcxt); + /* + * Check if the ranges are empty. + * + * If at least one of them is empty, we don't need to call per-key union + * functions at all. If "b" is empty, we just use "a" as the result (it + * might be empty fine, but that's fine). If "a" is empty but "b" is not, + * we use "b" as the result (but we have to copy the data into "a" first). + * + * Only when both ranges are non-empty, we actually do the per-key merge. + */ + + /* If "b" is empty - ignore it and just use "a" (even if it's empty etc.). */ + if (db->bt_empty_range) + { + /* skip the per-key merge */ + MemoryContextDelete(cxt); + return; + } + + /* + * Now we know "b" is not empty. If "a" is empty, then "b" is the result. + * But we need to copy the data from "b" to "a" first, because that's how + * we pass result out. + * + * We have to copy all the global/per-key flags etc. too. + */ + if (a->bt_empty_range) + { + for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++) + { + int i; + BrinValues *col_a = &a->bt_columns[keyno]; + BrinValues *col_b = &db->bt_columns[keyno]; + BrinOpcInfo *opcinfo = bdesc->bd_info[keyno]; + + col_a->bv_allnulls = col_b->bv_allnulls; + col_a->bv_hasnulls = col_b->bv_hasnulls; + + /* If "b" has no data, we're done. */ + if (col_b->bv_allnulls) + continue; + + for (i = 0; i < opcinfo->oi_nstored; i++) + col_a->bv_values[i] = + datumCopy(col_b->bv_values[i], + opcinfo->oi_typcache[i]->typbyval, + opcinfo->oi_typcache[i]->typlen); + } + + /* "a" started empty, but "b" was not empty, so remember that */ + a->bt_empty_range = false; + + /* skip the per-key merge */ + MemoryContextDelete(cxt); + return; + } + + /* Now we know neither range is empty. */ for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++) { FmgrInfo *unionFn; @@ -1703,7 +1774,9 @@ add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup, Datum *values, bool *nulls) { int keyno; - bool modified = false; + + /* If the range starts empty, we're certainly going to modify it. */ + bool modified = dtup->bt_empty_range; /* * Compare the key values of the new tuple to the stored index values; our @@ -1717,9 +1790,24 @@ add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup, Datum result; BrinValues *bval; FmgrInfo *addValue; + bool has_nulls; bval = &dtup->bt_columns[keyno]; + /* + * Does the range have actual NULL values? Either of the flags can + * be set, but we ignore the state before adding first row. + * + * We have to remember this, because we'll modify the flags and we + * need to know if the range started as empty. + */ + has_nulls = ((!dtup->bt_empty_range) && + (bval->bv_hasnulls || bval->bv_allnulls)); + + /* + * If the value we're adding is NULL, handle it locally. Otherwise + * call the BRIN_PROCNUM_ADDVALUE procedure. + */ if (bdesc->bd_info[keyno]->oi_regular_nulls && nulls[keyno]) { /* @@ -1745,8 +1833,33 @@ add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup, nulls[keyno]); /* if that returned true, we need to insert the updated tuple */ modified |= DatumGetBool(result); + + /* + * If the range was had actual NULL values (i.e. did not start empty), + * make sure we don't forget about the NULL values. Either the allnulls + * flag is still set to true, or (if the opclass cleared it) we need to + * set hasnulls=true. + * + * XXX This can only happen when the opclass modified the tuple, so the + * modified flag should be set. + */ + if (has_nulls && !(bval->bv_hasnulls || bval->bv_allnulls)) + { + Assert(modified); + bval->bv_hasnulls = true; + } } + /* + * After updating summaries for all the keys, mark it as not empty. + * + * If we're actually changing the flag value (i.e. tuple started as empty), + * we should have modified the tuple. So we should not see empty range that + * was not modified. + */ + Assert(!dtup->bt_empty_range || modified); + dtup->bt_empty_range = false; + return modified; } diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c index 84b79dbfc0d..b81247a262c 100644 --- a/src/backend/access/brin/brin_tuple.c +++ b/src/backend/access/brin/brin_tuple.c @@ -372,6 +372,9 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple, if (tuple->bt_placeholder) rettuple->bt_info |= BRIN_PLACEHOLDER_MASK; + if (tuple->bt_empty_range) + rettuple->bt_info |= BRIN_EMPTY_RANGE_MASK; + *size = len; return rettuple; } @@ -399,7 +402,7 @@ brin_form_placeholder_tuple(BrinDesc *brdesc, BlockNumber blkno, Size *size) rettuple = palloc0(len); rettuple->bt_blkno = blkno; rettuple->bt_info = hoff; - rettuple->bt_info |= BRIN_NULLS_MASK | BRIN_PLACEHOLDER_MASK; + rettuple->bt_info |= BRIN_NULLS_MASK | BRIN_PLACEHOLDER_MASK | BRIN_EMPTY_RANGE_MASK; bitP = ((bits8 *) ((char *) rettuple + SizeOfBrinTuple)) - 1; bitmask = HIGHBIT; @@ -489,6 +492,8 @@ brin_new_memtuple(BrinDesc *brdesc) dtup->bt_allnulls = palloc(sizeof(bool) * brdesc->bd_tupdesc->natts); dtup->bt_hasnulls = palloc(sizeof(bool) * brdesc->bd_tupdesc->natts); + dtup->bt_empty_range = true; + dtup->bt_context = AllocSetContextCreate(CurrentMemoryContext, "brin dtuple", ALLOCSET_DEFAULT_SIZES); @@ -527,6 +532,9 @@ brin_memtuple_initialize(BrinMemTuple *dtuple, BrinDesc *brdesc) currdatum += sizeof(Datum) * brdesc->bd_info[i]->oi_nstored; } + /* FIXME Shouldn't this reset bt_placeholder too? */ + dtuple->bt_empty_range = true; + return dtuple; } @@ -560,6 +568,11 @@ brin_deform_tuple(BrinDesc *brdesc, BrinTuple *tuple, BrinMemTuple *dMemtuple) if (BrinTupleIsPlaceholder(tuple)) dtup->bt_placeholder = true; + + /* ranges start as empty, depends on the BrinTuple */ + if (!BrinTupleIsEmptyRange(tuple)) + dtup->bt_empty_range = false; + dtup->bt_blkno = tuple->bt_blkno; values = dtup->bt_values; diff --git a/src/include/access/brin_tuple.h b/src/include/access/brin_tuple.h index 732f91edf11..c56747aca4a 100644 --- a/src/include/access/brin_tuple.h +++ b/src/include/access/brin_tuple.h @@ -44,6 +44,7 @@ typedef struct BrinValues typedef struct BrinMemTuple { bool bt_placeholder; /* this is a placeholder tuple */ + bool bt_empty_range; /* range represents no tuples */ BlockNumber bt_blkno; /* heap blkno that the tuple is for */ MemoryContext bt_context; /* memcxt holding the bt_columns values */ /* output arrays for brin_deform_tuple: */ @@ -69,7 +70,7 @@ typedef struct BrinTuple * * 7th (high) bit: has nulls * 6th bit: is placeholder tuple - * 5th bit: unused + * 5th bit: range is empty * 4-0 bit: offset of data * --------------- */ @@ -82,13 +83,14 @@ typedef struct BrinTuple * bt_info manipulation macros */ #define BRIN_OFFSET_MASK 0x1F -/* bit 0x20 is not used at present */ +#define BRIN_EMPTY_RANGE_MASK 0x20 #define BRIN_PLACEHOLDER_MASK 0x40 #define BRIN_NULLS_MASK 0x80 #define BrinTupleDataOffset(tup) ((Size) (((BrinTuple *) (tup))->bt_info & BRIN_OFFSET_MASK)) #define BrinTupleHasNulls(tup) (((((BrinTuple *) (tup))->bt_info & BRIN_NULLS_MASK)) != 0) #define BrinTupleIsPlaceholder(tup) (((((BrinTuple *) (tup))->bt_info & BRIN_PLACEHOLDER_MASK)) != 0) +#define BrinTupleIsEmptyRange(tup) (((((BrinTuple *) (tup))->bt_info & BRIN_EMPTY_RANGE_MASK)) != 0) extern BrinTuple *brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, diff --git a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out index 2a4755d0998..584ac2602f7 100644 --- a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out +++ b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out @@ -4,7 +4,7 @@ starting permutation: s2check s1b s2b s1i s2summ s1c s2c s2check step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+-------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} (1 row) step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ; @@ -26,7 +26,7 @@ step s2c: COMMIT; step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+----------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} 2| 1| 1|f |f |f |{1 .. 1000} (2 rows) @@ -35,7 +35,7 @@ starting permutation: s2check s1b s1i s2vacuum s1c s2check step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+-------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} (1 row) step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ; @@ -45,7 +45,7 @@ step s1c: COMMIT; step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value ----------+------+------+--------+--------+-----------+----------- - 1| 0| 1|f |f |f |{1 .. 1} + 1| 0| 1|f |t |f |{1 .. 1} 2| 1| 1|f |f |f |{1 .. 1000} (2 rows) diff --git a/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec b/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec index 19ac18a2e88..18ba92b7ba1 100644 --- a/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec +++ b/src/test/modules/brin/specs/summarization-and-inprogress-insertion.spec @@ -9,6 +9,7 @@ setup ) WITH (fillfactor=10); CREATE INDEX brinidx ON brin_iso USING brin (value) WITH (pages_per_range=1); -- this fills the first page + INSERT INTO brin_iso VALUES (NULL); DO $$ DECLARE curtid tid; BEGIN -- 2.40.0 [text/x-patch] 0002-Show-empty-ranges-in-brin_page_items-20230423-master.patch (6.9K, ../../[email protected]/5-0002-Show-empty-ranges-in-brin_page_items-20230423-master.patch) download | inline diff: From da108ee9bcfd4b8a4ac8b9fba2cc8b6ab4f757f7 Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Mon, 27 Mar 2023 22:47:12 +0200 Subject: [PATCH 2/2] Show empty ranges in brin_page_items Show which BRIN ranges are empty (no rows), as indicated by the newly introduced flag. --- contrib/pageinspect/brinfuncs.c | 10 ++++--- contrib/pageinspect/expected/brin.out | 6 ++-- .../pageinspect/pageinspect--1.11--1.12.sql | 17 +++++++++++ ...summarization-and-inprogress-insertion.out | 28 +++++++++---------- 4 files changed, 40 insertions(+), 21 deletions(-) diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c index 000dcd8f5d8..a781f265514 100644 --- a/contrib/pageinspect/brinfuncs.c +++ b/contrib/pageinspect/brinfuncs.c @@ -201,8 +201,8 @@ brin_page_items(PG_FUNCTION_ARGS) dtup = NULL; for (;;) { - Datum values[7]; - bool nulls[7] = {0}; + Datum values[8]; + bool nulls[8] = {0}; /* * This loop is called once for every attribute of every tuple in the @@ -239,6 +239,7 @@ brin_page_items(PG_FUNCTION_ARGS) nulls[4] = true; nulls[5] = true; nulls[6] = true; + nulls[7] = true; } else { @@ -261,6 +262,7 @@ brin_page_items(PG_FUNCTION_ARGS) values[3] = BoolGetDatum(dtup->bt_columns[att].bv_allnulls); values[4] = BoolGetDatum(dtup->bt_columns[att].bv_hasnulls); values[5] = BoolGetDatum(dtup->bt_placeholder); + values[6] = BoolGetDatum(dtup->bt_empty_range); if (!dtup->bt_columns[att].bv_allnulls) { BrinValues *bvalues = &dtup->bt_columns[att]; @@ -286,12 +288,12 @@ brin_page_items(PG_FUNCTION_ARGS) } appendStringInfoChar(&s, '}'); - values[6] = CStringGetTextDatum(s.data); + values[7] = CStringGetTextDatum(s.data); pfree(s.data); } else { - nulls[6] = true; + nulls[7] = true; } } diff --git a/contrib/pageinspect/expected/brin.out b/contrib/pageinspect/expected/brin.out index e12fbeb4774..098ddc202f4 100644 --- a/contrib/pageinspect/expected/brin.out +++ b/contrib/pageinspect/expected/brin.out @@ -43,9 +43,9 @@ SELECT * FROM brin_revmap_data(get_raw_page('test1_a_idx', 1)) LIMIT 5; SELECT * FROM brin_page_items(get_raw_page('test1_a_idx', 2), 'test1_a_idx') ORDER BY blknum, attnum LIMIT 5; - itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | value -------------+--------+--------+----------+----------+-------------+---------- - 1 | 0 | 1 | f | f | f | {1 .. 1} + itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | empty | value +------------+--------+--------+----------+----------+-------------+-------+---------- + 1 | 0 | 1 | f | f | f | f | {1 .. 1} (1 row) -- Mask DETAIL messages as these are not portable across architectures. diff --git a/contrib/pageinspect/pageinspect--1.11--1.12.sql b/contrib/pageinspect/pageinspect--1.11--1.12.sql index 70c3abccf57..a20d67a9e82 100644 --- a/contrib/pageinspect/pageinspect--1.11--1.12.sql +++ b/contrib/pageinspect/pageinspect--1.11--1.12.sql @@ -21,3 +21,20 @@ CREATE FUNCTION bt_multi_page_stats(IN relname text, IN blkno int8, IN blk_count RETURNS SETOF record AS 'MODULE_PATHNAME', 'bt_multi_page_stats' LANGUAGE C STRICT PARALLEL RESTRICTED; + +-- +-- add information about BRIN empty ranges +-- +DROP FUNCTION brin_page_items(IN page bytea, IN index_oid regclass); +CREATE FUNCTION brin_page_items(IN page bytea, IN index_oid regclass, + OUT itemoffset int, + OUT blknum int8, + OUT attnum int, + OUT allnulls bool, + OUT hasnulls bool, + OUT placeholder bool, + OUT empty bool, + OUT value text) +RETURNS SETOF record +AS 'MODULE_PATHNAME', 'brin_page_items' +LANGUAGE C STRICT PARALLEL RESTRICTED; diff --git a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out index 584ac2602f7..201786c82c0 100644 --- a/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out +++ b/src/test/modules/brin/expected/summarization-and-inprogress-insertion.out @@ -2,9 +2,9 @@ Parsed test spec with 2 sessions starting permutation: s2check s1b s2b s1i s2summ s1c s2c s2check step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); -itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value -----------+------+------+--------+--------+-----------+-------- - 1| 0| 1|f |t |f |{1 .. 1} +itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|empty|value +----------+------+------+--------+--------+-----------+-----+-------- + 1| 0| 1|f |t |f |f |{1 .. 1} (1 row) step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ; @@ -24,18 +24,18 @@ brin_summarize_new_values step s1c: COMMIT; step s2c: COMMIT; step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); -itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value -----------+------+------+--------+--------+-----------+----------- - 1| 0| 1|f |t |f |{1 .. 1} - 2| 1| 1|f |f |f |{1 .. 1000} +itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|empty|value +----------+------+------+--------+--------+-----------+-----+----------- + 1| 0| 1|f |t |f |f |{1 .. 1} + 2| 1| 1|f |f |f |f |{1 .. 1000} (2 rows) starting permutation: s2check s1b s1i s2vacuum s1c s2check step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); -itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value -----------+------+------+--------+--------+-----------+-------- - 1| 0| 1|f |t |f |{1 .. 1} +itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|empty|value +----------+------+------+--------+--------+-----------+-----+-------- + 1| 0| 1|f |t |f |f |{1 .. 1} (1 row) step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ; @@ -43,9 +43,9 @@ step s1i: INSERT INTO brin_iso VALUES (1000); step s2vacuum: VACUUM brin_iso; step s1c: COMMIT; step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); -itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|value -----------+------+------+--------+--------+-----------+----------- - 1| 0| 1|f |t |f |{1 .. 1} - 2| 1| 1|f |f |f |{1 .. 1000} +itemoffset|blknum|attnum|allnulls|hasnulls|placeholder|empty|value +----------+------+------+--------+--------+-----------+-----+----------- + 1| 0| 1|f |t |f |f |{1 .. 1} + 2| 1| 1|f |f |f |f |{1 .. 1000} (2 rows) -- 2.40.0 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Missing update of all_hasnulls in BRIN opclasses 2023-03-03 12:14 Re: Missing update of all_hasnulls in BRIN opclasses Tomas Vondra <[email protected]> 2023-03-28 14:30 ` Re: Missing update of all_hasnulls in BRIN opclasses Tomas Vondra <[email protected]> 2023-04-23 20:43 ` Re: Missing update of all_hasnulls in BRIN opclasses Tomas Vondra <[email protected]> @ 2023-04-24 15:36 ` Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Alvaro Herrera @ 2023-04-24 15:36 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Matthias van de Meent <[email protected]>; [email protected] On 2023-Apr-23, Tomas Vondra wrote: > here's an updated version of the patch, including a backport version. I > ended up making the code yet a bit closer to master by introducing > add_values_to_range(). The current pre-14 code has the loop adding data > to the BRIN tuple in two places, but with the "fixed" logic handling > NULLs and the empty_range flag the amount of duplicated code got too > high, so this seem reasonable. In backbranches, the new field to BrinMemTuple needs to be at the end of the struct, to avoid ABI breakage. There's a comment in add_values_to_range with a typo "If the range was had". Also, "So we should not see empty range that was not modified" should perhaps be "should not see an empty range". (As for your FIXME comment in brin_memtuple_initialize, I think you're correct: we definitely need to reset bt_placeholder. Otherwise, we risk places that call it in a loop using a BrinMemTuple with one range with the flag set, in a range where that doesn't hold. It might be impossible for this to happen, given how narrow the conditions are on which bt_placeholder is used; but it seems safer to reset it anyway.) Some pgindent noise would be induced by this patch. I think it's worth cleaning up ahead of time. I did a quick experiment of turning the patches over -- applying test first, code fix after (requires some conflict fixing). With that I verified that the behavior of 'hasnulls' indeed changes with the code fix. > Both cases have a patch extending pageinspect to show the new flag, but > obviously we should commit that only in master. In backbranches it's > meant only to make testing easier. In backbranches, I think it should be reasonably easy to add a --1.7--1.7.1.sql file and set the default version to 1.7.1; that then enables us to have the functionality (and the tests) in older branches too. If you just add a --1.X.1--1.12.sql version to each branch that's identical to that branch's current pageinspect version upgrade script, that would let us have working upgrade paths for all branches. This is a bit laborious but straightforward enough. If you don't feel like adding that, I volunteer to add the necessary scripts to all branches after you commit the bugfix, and ensure that all upgrade paths work correctly. > I plan to do a bit more testing, I'd welcome some feedback - it's a > long-standing bug, and it'd be good to finally get this fixed. I don't > think the patch can be made any simpler. The approach looks good to me. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Oh, great altar of passive entertainment, bestow upon me thy discordant images at such speed as to render linear thought impossible" (Calvin a la TV) ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-04-24 15:36 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-12 01:02 [PATCH] cfe-12-gist_over_cfe-11-persistent squash commit Bruce Momjian <[email protected]> 2023-03-03 12:14 Re: Missing update of all_hasnulls in BRIN opclasses Tomas Vondra <[email protected]> 2023-03-28 14:30 ` Re: Missing update of all_hasnulls in BRIN opclasses Tomas Vondra <[email protected]> 2023-04-23 20:43 ` Re: Missing update of all_hasnulls in BRIN opclasses Tomas Vondra <[email protected]> 2023-04-24 15:36 ` Re: Missing update of all_hasnulls in BRIN opclasses Alvaro Herrera <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox