Content-Type: multipart/mixed; boundary="------------abJuAEqo7f0coYUPDLcC9oUU"
Message-ID: <64a5bfb0-909e-0a14-3ac9-14dc74673074@enterprisedb.com>
Date: Wed, 26 Jan 2022 04:12:27 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Content-Language: en-US
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Subject: BRIN summarization vs. WAL logging
Archived-At: <https://www.postgresql.org/message-id/64a5bfb0-909e-0a14-3ac9-14dc74673074%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------abJuAEqo7f0coYUPDLcC9oUU
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi,

In a thread about sequences and sync replication [1], I've explained 
that the issue we're observing is due to not waiting for WAL at commit 
if the transaction only did nextval(). In which case we don't flush WAL 
in RecordTransactionCommit, we don't wait for sync replica, etc. The WAL 
may get lost in case of crash, etc.

As I explained in the other thread, there are various other cases where 
a transaction generates WAL but does not have XID, which is sufficient 
for not flushing/waiting at transaction commit. Some of those cases are 
probably fine (e.g. there are comments explaining why this is fine for 
PRUNE record).

But other cases (discovered by running regression tests with extra 
logging) looked a bit suspicious - particularly those that write 
multiple WAL messages, because what if we lose just some of those?

So I looked at two cases related to BRIN, mostly because those were 
fairly simple, and I think at least the brin_summarize_range() is 
somewhat broken.


1) brin_desummarize_range()

This is pretty simple, because this function generates a single WAL 
record, without waiting for it to be flushed:

DESUMMARIZE pagesPerRange 1, heapBlk 0, page offset 9, blkref #0: ...

But if the cluster/VM/... crashes right after you ran the function (and 
it completed just fine, possibly even in an explicit transaciton), that 
change will get lost. Not really a serious data corruption/loss, and you 
can simply run it again, but IMHO rather surprising.

Of course, most people are unlikely to run brin_desummarize_range() very 
often, so maybe it's acceptable? But of course - if we expect this to be 
very rare operation, why skip the WAL at all?


2) brin_summarize_range()

Now, the issue I think is more serious, more likely to happen, and 
harder to fix. When summarizing a range, we write two WAL records:

INSERT heapBlk 2 pagesPerRange 2 offnum 2, blkref #0: rel 1663/63 ...
SAMEPAGE_UPDATE offnum 2, blkref #0: rel 1663/63341/73957 blk 2

So, what happens if we lost the second WAL record, e.g. due to a crash? 
To experiment with this, I wrote a trivial patch (attached) that allows 
crashing on WAL message of certain type by simply setting a GUC.

Now, consider this example:

   create table t (a int);
   insert into t select i from generate_series(1,5000) s(i);
   create index on t using brin (a);
   select brin_desummarize_range('t_a_idx', 1);

   set crash_on_wal_message = 'SAMEPAGE_UPDATE';

   select brin_summarize_range('t_a_idx', 5);

   PANIC:  crashing before 'SAMEPAGE_UPDATE' WAL message
   server closed the connection unexpectedly
   ...

After recovery, this is what we have:

   select * from brin_page_items(get_Raw_page('t_a_idx', 2), 't_a_idx');

    ...  | allnulls | hasnulls | placeholder | value
    ... -+----------+----------+-------------+-------
    ...  | t        | f        | t           |
    (1 row)

So the BRIN tuple is still marked as placeholder, which is a problem 
because that means we'll always consider it as matching, making the 
bitmap index scan less efficient. And we'll *never* fix this, because 
just summarizing the range does nothing:

    select brin_summarize_range('t_a_idx', 5);
    brin_summarize_range
   ----------------------
                       0
   (1 row)

So it's still marked as placeholder, and to fix it you have to 
explicitly desummarize the range first.

The reason for this seems obvious - only the process that created the 
placeholder tuple is expected to mark it as "placeholder=false", but 
this is described as two WAL records. And if we lose the update, the 
tuple will stay marked as a placeholder forever.

Of course, this requires a crash while something is summarizing ranges. 
But consider the summarization is often done by autovacuum, so it's not 
just about hitting this from manually-executed brin_summarize_range.

I'm not quite sure what to do about this. Just doing XLogFlush() does 
not really fix this - it makes it less likely, but the root cause is the 
change is described by multiple WAL messages that are not linked 
together in any way. We may lost the last message without noticing that, 
and the flush does not fix that.

I didn't look at the other cases mentioned in [1], but I would't be 
surprised if some had a similar issue (e.g. the GIN pending list cleanup 
seems like another candidate).


regards

[1] 
https://www.postgresql.org/message-id/0f827a71-a01b-bcf9-fe77-3047a9d4a93c%40enterprisedb.com

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--------------abJuAEqo7f0coYUPDLcC9oUU
Content-Type: text/x-patch; charset=UTF-8; name="trigger-wal-crash.patch"
Content-Disposition: attachment; filename="trigger-wal-crash.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2FjY2Vzcy9icmluL2JyaW5fcGFnZW9wcy5jIGIv
c3JjL2JhY2tlbmQvYWNjZXNzL2JyaW4vYnJpbl9wYWdlb3BzLmMKaW5kZXggMTcyNTc5MTlk
YmYuLmRhZWQ4YmEzYmM4IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9hY2Nlc3MvYnJpbi9i
cmluX3BhZ2VvcHMuYworKysgYi9zcmMvYmFja2VuZC9hY2Nlc3MvYnJpbi9icmluX3BhZ2Vv
cHMuYwpAQCAtMTkwLDYgKzE5MCw4IEBAIGJyaW5fZG91cGRhdGUoUmVsYXRpb24gaWR4cmVs
LCBCbG9ja051bWJlciBwYWdlc1BlclJhbmdlLAogCiAJCQl4bHJlYy5vZmZudW0gPSBvbGRv
ZmY7CiAKKwkJCW1heWJlX2NyYXNoX29uX3dhbCgiU0FNRVBBR0VfVVBEQVRFIik7CisKIAkJ
CVhMb2dCZWdpbkluc2VydCgpOwogCQkJWExvZ1JlZ2lzdGVyRGF0YSgoY2hhciAqKSAmeGxy
ZWMsIFNpemVPZkJyaW5TYW1lcGFnZVVwZGF0ZSk7CiAKZGlmZiAtLWdpdCBhL3NyYy9iYWNr
ZW5kL2FjY2Vzcy90cmFuc2FtL3hhY3QuYyBiL3NyYy9iYWNrZW5kL2FjY2Vzcy90cmFuc2Ft
L3hhY3QuYwppbmRleCBjOTUxNmUwM2ZhZS4uNTYyMTkyZGM1MTQgMTAwNjQ0Ci0tLSBhL3Ny
Yy9iYWNrZW5kL2FjY2Vzcy90cmFuc2FtL3hhY3QuYworKysgYi9zcmMvYmFja2VuZC9hY2Nl
c3MvdHJhbnNhbS94YWN0LmMKQEAgLTg0LDYgKzg0LDggQEAgYm9vbAkJWGFjdERlZmVycmFi
bGU7CiAKIGludAkJCXN5bmNocm9ub3VzX2NvbW1pdCA9IFNZTkNIUk9OT1VTX0NPTU1JVF9P
TjsKIAorY2hhcgkgICAqY3Jhc2hfb25fd2FsX21lc3NhZ2U7CisKIC8qCiAgKiBDaGVja1hp
ZEFsaXZlIGlzIGEgeGlkIHZhbHVlIHBvaW50aW5nIHRvIGEgcG9zc2libHkgb25nb2luZyAo
c3ViKQogICogdHJhbnNhY3Rpb24uICBDdXJyZW50bHksIGl0IGlzIHVzZWQgaW4gbG9naWNh
bCBkZWNvZGluZy4gIEl0J3MgcG9zc2libGUKQEAgLTYxNTcsMyArNjE1OSwxNyBAQCB4YWN0
X3JlZG8oWExvZ1JlYWRlclN0YXRlICpyZWNvcmQpCiAJZWxzZQogCQllbG9nKFBBTklDLCAi
eGFjdF9yZWRvOiB1bmtub3duIG9wIGNvZGUgJXUiLCBpbmZvKTsKIH0KKwordm9pZAorbWF5
YmVfY3Jhc2hfb25fd2FsKGNoYXIgKm1zZykKK3sKKwkvKiBHVUMgbm90IHNldCBvciBub3Qg
dGhlIHJpZ2h0IG1lc3NhZ2UgKi8KKwlpZiAoc3RyY21wKGNyYXNoX29uX3dhbF9tZXNzYWdl
LCBtc2cpICE9IDApCisJCXJldHVybjsKKworCS8qIGZsdXNoIHdoYXRldmVyIHdhcyBnZW5l
cmF0ZWQgaW4gdGhpcyB4YWN0IHNvIGZhciAqLworCVhMb2dGbHVzaChYYWN0TGFzdFJlY0Vu
ZCk7CisKKwllbG9nKFBBTklDLCAiY3Jhc2hpbmcgYmVmb3JlICclcycgV0FMIG1lc3NhZ2Ui
LCBtc2cpOworfQorCmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC91dGlscy9taXNjL2d1Yy5j
IGIvc3JjL2JhY2tlbmQvdXRpbHMvbWlzYy9ndWMuYwppbmRleCA0Yzk0ZjA5YzY0NS4uMzdi
ZjIwZTdmMDQgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3V0aWxzL21pc2MvZ3VjLmMKKysr
IGIvc3JjL2JhY2tlbmQvdXRpbHMvbWlzYy9ndWMuYwpAQCAtMTM5LDYgKzEzOSw3IEBAIGV4
dGVybiBjaGFyICp0ZW1wX3RhYmxlc3BhY2VzOwogZXh0ZXJuIGJvb2wgaWdub3JlX2NoZWNr
c3VtX2ZhaWx1cmU7CiBleHRlcm4gYm9vbCBpZ25vcmVfaW52YWxpZF9wYWdlczsKIGV4dGVy
biBib29sIHN5bmNocm9uaXplX3NlcXNjYW5zOworZXh0ZXJuIGNoYXIgKmNyYXNoX29uX3dh
bF9tZXNzYWdlOwogCiAjaWZkZWYgVFJBQ0VfU1lOQ1NDQU4KIGV4dGVybiBib29sIHRyYWNl
X3N5bmNzY2FuOwpAQCAtNDA3MCw2ICs0MDcxLDE3IEBAIHN0YXRpYyBzdHJ1Y3QgY29uZmln
X3N0cmluZyBDb25maWd1cmVOYW1lc1N0cmluZ1tdID0KIAkJY2hlY2tfZGVmYXVsdF90YWJs
ZXNwYWNlLCBOVUxMLCBOVUxMCiAJfSwKIAorCXsKKwkJeyJjcmFzaF9vbl93YWxfbWVzc2Fn
ZSIsIFBHQ19VU0VSU0VULCBDTElFTlRfQ09OTl9TVEFURU1FTlQsCisJCQlnZXR0ZXh0X25v
b3AoImNyYXNoIG9uIHRoaXMgV0FMIG1lc3NhZ2UiKSwKKwkJCU5VTEwsCisJCQlHVUNfSVNf
TkFNRQorCQl9LAorCQkmY3Jhc2hfb25fd2FsX21lc3NhZ2UsCisJCSIiLAorCQlOVUxMLCBO
VUxMLCBOVUxMCisJfSwKKwogCXsKIAkJeyJ0ZW1wX3RhYmxlc3BhY2VzIiwgUEdDX1VTRVJT
RVQsIENMSUVOVF9DT05OX1NUQVRFTUVOVCwKIAkJCWdldHRleHRfbm9vcCgiU2V0cyB0aGUg
dGFibGVzcGFjZShzKSB0byB1c2UgZm9yIHRlbXBvcmFyeSB0YWJsZXMgYW5kIHNvcnQgZmls
ZXMuIiksCmRpZmYgLS1naXQgYS9zcmMvaW5jbHVkZS9taXNjYWRtaW4uaCBiL3NyYy9pbmNs
dWRlL21pc2NhZG1pbi5oCmluZGV4IDAyMjc2ZDNlZGQ1Li5mYTEzYzYwZGEzMiAxMDA2NDQK
LS0tIGEvc3JjL2luY2x1ZGUvbWlzY2FkbWluLmgKKysrIGIvc3JjL2luY2x1ZGUvbWlzY2Fk
bWluLmgKQEAgLTQ4Nyw0ICs0ODcsNiBAQCBleHRlcm4gdm9pZCBDYW5jZWxCYWNrdXAodm9p
ZCk7CiAvKiBpbiBleGVjdXRvci9ub2RlSGFzaC5jICovCiBleHRlcm4gc2l6ZV90IGdldF9o
YXNoX21lbW9yeV9saW1pdCh2b2lkKTsKIAorZXh0ZXJuIHZvaWQgbWF5YmVfY3Jhc2hfb25f
d2FsKGNoYXIgKm1zZyk7CisKICNlbmRpZgkJCQkJCQkvKiBNSVNDQURNSU5fSCAqLwo=
--------------abJuAEqo7f0coYUPDLcC9oUU--