From: "Wong, Yi Wen" <yiwong@amazon.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>,
        Michael Paquier
	<michael.paquier@gmail.com>
CC: Peter Geoghegan <pg@bowt.ie>, "Wood, Dan" <hexpert@amazon.com>,
        "pgsql-bugs@postgresql.org" <pgsql-bugs@postgresql.org>,
        Andres Freund
	<andres@anarazel.de>
Subject: Re: Old row version in hot chain become visible after a freeze
Thread-Topic: [BUGS] Old row version in hot chain become visible after a
 freeze
Thread-Index: 
 AQHTJ3fzEH9T5bMCDUufad1KxDA7S6Kvvd+AgAEpfwCAAAoegIAAp59AgAAqJpCAACgBjg==
Date: Tue, 12 Sep 2017 23:29:25 +0000
Message-ID: <1505258965245.51728@amazon.com>
References: 
 <CAB7nPqTc0-imDj-5URYW9a5A+yijFv_UUXucDE=3TLyUmZg3AA@mail.gmail.com>,<20170912082203.gq5hutuveqlxpvuc@alvherre.pgsql>,<1505241452245.71279@amazon.com>,<1505249698051.58904@amazon.com>
In-Reply-To: <1505249698051.58904@amazon.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: multipart/mixed; boundary="_002_150525896524551728amazoncom_"
MIME-Version: 1.0
Precedence: bulk
Sender: pgsql-bugs-owner@postgresql.org

--_002_150525896524551728amazoncom_
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable

I've spent some time reviewing the new patch and have made a couple of mino=
r changes.

I've made some changes to this chunk:

        else if (TransactionIdIsNormal(xid))
        {
                if (TransactionIdPrecedes(xid, cutoff_xid))
-                       freeze_xmax =3D true;
+               {
+                       if (HeapTupleHeaderIsHeapOnly(tuple) ||
+                               HeapTupleHeaderIsHotUpdated(tuple))
+                       {
+                               frz->t_infomask |=3D HEAP_XMAX_COMMITTED;
+                               totally_frozen =3D false;
+                               changed =3D true;
+                       }
+                       else
+                               freeze_xmax =3D true;
+               }
                else
                        totally_frozen =3D false;
        }

The changes are:

1) To add the line changed =3D true;=20
This is necessary to actually set the bits of to HEAP_XMAX_COMMITTED. In re=
pro.sql, this is set anyway
because the xmin is frozen. However, consider a scenario where we have xmin=
 frozen and xmax still live in a
previous VACUUM FREEZE pass, if we do a second pass where HEAP_XMAX_COMMITT=
ED needs to be set,
we will lose that change.

2) Flip the order of HeapTupleHeaderIsHeapOnly(tuple) and HeapTupleHeaderIs=
HotUpdated(tuple). This is merely a
microoptimization on a non-performance critical path because HeapOnly is a =
broader condititon than HotUpdated :-)

I've also changed the elog ERROR. This seems necessary for a user to know, =
because it does mean VACUUM is broken on their=20
system:

+       if (vacrelstats->num_dead_tuples >=3D vacrelstats->max_dead_tuples)
+               ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_RESOURCES),
+                               errmsg ("dead tuple array overflow: %d dead=
, %d max",
+                                               vacrelstats->num_dead_tuple=
s,
+                                               vacrelstats->max_dead_tuple=
s),
+                               errhint("Increase maintenance_work_mem")));

I've also made some changes to comments in the code.

Thanks,
Yi Wen

________________________________________
From: pgsql-bugs-owner@postgresql.org <pgsql-bugs-owner@postgresql.org> on =
behalf of Wong, Yi Wen <yiwong@amazon.com>
Sent: Tuesday, September 12, 2017 1:54 PM
To: Alvaro Herrera; Michael Paquier
Cc: Peter Geoghegan; Wood, Dan; pgsql-bugs@postgresql.org; Andres Freund
Subject: Re: [BUGS] Old row version in hot chain become visible after a fre=
eze

> > > I also tweaked lazy_record_dead_tuple to fail with ERROR if the tuple
> > > cannot be recorded, as observed by Yi Wen.  AFAICS that's not reachab=
le
> > > because of the way the array is allocated, so an elog(ERROR) is
> > > sufficient.
>
> I agree the fail is rare (and probably doesn't happen in real cases, alth=
ough the comment
> does imply with a sufficiently low working_memory it might?). However, I'=
d dispute that ERROR
> is sufficient --PANIC is probably appropriate here because FREEZE is not =
WAL-logged until
> the end of the page; so we'd end up with unfrozen Xids hanging around wit=
h an appropriately
> timed crash. I wouldn't worry much about the PANIC tripping because like =
you said,
> this seems unreachable normally.
>
> The errmsg should come with a errhint saying "Increase maintenance_work_m=
em".

On closer inspection, I misread the code. The freezes are collected instead=
 of executed, so a ERROR would
suffice. We don't need to change it to PANIC. errhint comment remains.

Yi Wen

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

--_002_150525896524551728amazoncom_
Content-Type: application/octet-stream; name="update_freeze_v4.patch"
Content-Description: update_freeze_v4.patch
Content-Disposition: attachment; filename="update_freeze_v4.patch"; size=5567;
	creation-date="Tue, 12 Sep 2017 23:25:55 GMT";
	modification-date="Tue, 12 Sep 2017 23:25:55 GMT"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2FjY2Vzcy9oZWFwL2hlYXBhbS5j
IGIvc3JjL2JhY2tlbmQvYWNjZXNzL2hlYXAvaGVhcGFtLmMKaW5kZXggZDIw
ZjAzOC4uNjViM2JhZCAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvYWNjZXNz
L2hlYXAvaGVhcGFtLmMKKysrIGIvc3JjL2JhY2tlbmQvYWNjZXNzL2hlYXAv
aGVhcGFtLmMKQEAgLTY0MDUsMTQgKzY0MDUsMjMgQEAgRnJlZXplTXVsdGlY
YWN0SWQoTXVsdGlYYWN0SWQgbXVsdGksIHVpbnQxNiB0X2luZm9tYXNrLAog
CQkJQXNzZXJ0KFRyYW5zYWN0aW9uSWRJc1ZhbGlkKHhpZCkpOwogCiAJCQkv
KgotCQkJICogSWYgdGhlIHhpZCBpcyBvbGRlciB0aGFuIHRoZSBjdXRvZmYs
IGl0IGhhcyB0byBoYXZlIGFib3J0ZWQsCi0JCQkgKiBvdGhlcndpc2UgdGhl
IHR1cGxlIHdvdWxkIGhhdmUgZ290dGVuIHBydW5lZCBhd2F5LgorCQkJICog
VGhlIHVwZGF0aW5nIHRyYW5zYWN0aW9uIGNhbm5vdCBwb3NzaWJseSBiZSBz
dGlsbCBydW5uaW5nLCBidXQKKwkJCSAqIHZlcmlmeSB3aGV0aGVyIGl0IGhh
cyBjb21taXR0ZWQsIGFuZCByZXF1ZXN0IHRvIHNldCB0aGUKKwkJCSAqIENP
TU1JVFRFRCBmbGFnIGlmIHNvLiAgKFdlIG5vcm1hbGx5IGRvbid0IHNlZSBh
bnkgdHVwbGVzIGluCisJCQkgKiB0aGlzIHN0YXRlLCBiZWNhdXNlIHRoZXkg
YXJlIHJlbW92ZWQgYnkgcGFnZSBwcnVuaW5nIGJlZm9yZSB3ZQorCQkJICog
dHJ5IHRvIGZyZWV6ZSB0aGUgcGFnZTsgYnV0IHRoaXMgY2FuIGhhcHBlbiBp
ZiB0aGUgdXBkYXRpbmcKKwkJCSAqIHRyYW5zYWN0aW9uIGNvbW1pdHMgYWZ0
ZXIgdGhlIHBhZ2UgaXMgcHJ1bmVkIGJ1dCBiZWZvcmUKKwkJCSAqIEhlYXBU
dXBsZVNhdGlzZmllc1ZhY3V1bSkuCiAJCQkgKi8KIAkJCWlmIChUcmFuc2Fj
dGlvbklkUHJlY2VkZXMoeGlkLCBjdXRvZmZfeGlkKSkKIAkJCXsKLQkJCQlB
c3NlcnQoIVRyYW5zYWN0aW9uSWREaWRDb21taXQoeGlkKSk7Ci0JCQkJKmZs
YWdzIHw9IEZSTV9JTlZBTElEQVRFX1hNQVg7Ci0JCQkJeGlkID0gSW52YWxp
ZFRyYW5zYWN0aW9uSWQ7IC8qIG5vdCBzdHJpY3RseSBuZWNlc3NhcnkgKi8K
KwkJCQlpZiAoVHJhbnNhY3Rpb25JZERpZENvbW1pdCh4aWQpKQorCQkJCQkq
ZmxhZ3MgPSBGUk1fTUFSS19DT01NSVRURUQgfCBGUk1fUkVUVVJOX0lTX1hJ
RDsKKwkJCQllbHNlCisJCQkJeworCQkJCQkqZmxhZ3MgfD0gRlJNX0lOVkFM
SURBVEVfWE1BWDsKKwkJCQkJeGlkID0gSW52YWxpZFRyYW5zYWN0aW9uSWQ7
IC8qIG5vdCBzdHJpY3RseSBuZWNlc3NhcnkgKi8KKwkJCQl9CiAJCQl9CiAJ
CQllbHNlCiAJCQl7CkBAIC02NDg1LDEzICs2NDk0LDE2IEBAIEZyZWV6ZU11
bHRpWGFjdElkKE11bHRpWGFjdElkIG11bHRpLCB1aW50MTYgdF9pbmZvbWFz
aywKIAkJCS8qCiAJCQkgKiBJdCdzIGFuIHVwZGF0ZTsgc2hvdWxkIHdlIGtl
ZXAgaXQ/ICBJZiB0aGUgdHJhbnNhY3Rpb24gaXMga25vd24KIAkJCSAqIGFi
b3J0ZWQgb3IgY3Jhc2hlZCB0aGVuIGl0J3Mgb2theSB0byBpZ25vcmUgaXQs
IG90aGVyd2lzZSBub3QuCi0JCQkgKiBOb3RlIHRoYXQgYW4gdXBkYXRlciBv
bGRlciB0aGFuIGN1dG9mZl94aWQgY2Fubm90IHBvc3NpYmx5IGJlCi0JCQkg
KiBjb21taXR0ZWQsIGJlY2F1c2UgSGVhcFR1cGxlU2F0aXNmaWVzVmFjdXVt
IHdvdWxkIGhhdmUgcmV0dXJuZWQKLQkJCSAqIEhFQVBUVVBMRV9ERUFEIGFu
ZCB3ZSB3b3VsZCBub3QgYmUgdHJ5aW5nIHRvIGZyZWV6ZSB0aGUgdHVwbGUu
CiAJCQkgKgogCQkJICogQXMgd2l0aCBhbGwgdHVwbGUgdmlzaWJpbGl0eSBy
b3V0aW5lcywgaXQncyBjcml0aWNhbCB0byB0ZXN0CiAJCQkgKiBUcmFuc2Fj
dGlvbklkSXNJblByb2dyZXNzIGJlZm9yZSBUcmFuc2FjdGlvbklkRGlkQ29t
bWl0LAogCQkJICogYmVjYXVzZSBvZiByYWNlIGNvbmRpdGlvbnMgZXhwbGFp
bmVkIGluIGRldGFpbCBpbiB0cXVhbC5jLgorCQkJICoKKwkJCSAqIFdlIG5v
cm1hbGx5IGRvbid0IHNlZSBjb21taXR0ZWQgdXBkYXRpbmcgdHJhbnNhY3Rp
b25zIGVhcmxpZXIKKwkJCSAqIHRoYW4gdGhlIGN1dG9mZiB4aWQsIGJlY2F1
c2UgdGhleSBhcmUgcmVtb3ZlZCBieSBwYWdlIHBydW5pbmcKKwkJCSAqIGJl
Zm9yZSB3ZSB0cnkgdG8gZnJlZXplIHRoZSBwYWdlOyBidXQgaXQgY2FuIGhh
cHBlbiBpZiB0aGUKKwkJCSAqIHVwZGF0aW5nIHRyYW5zYWN0aW9uIGNvbW1p
dHMgYWZ0ZXIgdGhlIHBhZ2UgaXMgcHJ1bmVkIGJ1dAorCQkJICogYmVmb3Jl
IEhlYXBUdXBsZVNhdGlzZmllc1ZhY3V1bS4KIAkJCSAqLwogCQkJaWYgKFRy
YW5zYWN0aW9uSWRJc0N1cnJlbnRUcmFuc2FjdGlvbklkKHhpZCkgfHwKIAkJ
CQlUcmFuc2FjdGlvbklkSXNJblByb2dyZXNzKHhpZCkpCkBAIC02NTE3LDEz
ICs2NTI5LDYgQEAgRnJlZXplTXVsdGlYYWN0SWQoTXVsdGlYYWN0SWQgbXVs
dGksIHVpbnQxNiB0X2luZm9tYXNrLAogCQkJICovCiAKIAkJCS8qCi0JCQkg
KiBTaW5jZSB0aGUgdHVwbGUgd2Fzbid0IG1hcmtlZCBIRUFQVFVQTEVfREVB
RCBieSB2YWN1dW0sIHRoZQotCQkJICogdXBkYXRlIFhpZCBjYW5ub3QgcG9z
c2libHkgYmUgb2xkZXIgdGhhbiB0aGUgeGlkIGN1dG9mZi4KLQkJCSAqLwot
CQkJQXNzZXJ0KCFUcmFuc2FjdGlvbklkSXNWYWxpZCh1cGRhdGVfeGlkKSB8
fAotCQkJCSAgICFUcmFuc2FjdGlvbklkUHJlY2VkZXModXBkYXRlX3hpZCwg
Y3V0b2ZmX3hpZCkpOwotCi0JCQkvKgogCQkJICogSWYgd2UgZGV0ZXJtaW5l
ZCB0aGF0IGl0J3MgYW4gWGlkIGNvcnJlc3BvbmRpbmcgdG8gYW4gdXBkYXRl
CiAJCQkgKiB0aGF0IG11c3QgYmUgcmV0YWluZWQsIGFkZGl0aW9uYWxseSBh
ZGQgaXQgdG8gdGhlIGxpc3Qgb2YKIAkJCSAqIG1lbWJlcnMgb2YgdGhlIG5l
dyBNdWx0aSwgaW4gY2FzZSB3ZSBlbmQgdXAgdXNpbmcgdGhhdC4gIChXZQpA
QCAtNjYwMSw3ICs2NjA2LDggQEAgRnJlZXplTXVsdGlYYWN0SWQoTXVsdGlY
YWN0SWQgbXVsdGksIHVpbnQxNiB0X2luZm9tYXNrLAogICoKICAqIEl0IGlz
IGFzc3VtZWQgdGhhdCB0aGUgY2FsbGVyIGhhcyBjaGVja2VkIHRoZSB0dXBs
ZSB3aXRoCiAgKiBIZWFwVHVwbGVTYXRpc2ZpZXNWYWN1dW0oKSBhbmQgZGV0
ZXJtaW5lZCB0aGF0IGl0IGlzIG5vdCBIRUFQVFVQTEVfREVBRAotICogKGVs
c2Ugd2Ugc2hvdWxkIGJlIHJlbW92aW5nIHRoZSB0dXBsZSwgbm90IGZyZWV6
aW5nIGl0KS4KKyAqIChlbHNlIHdlIHNob3VsZCBiZSByZW1vdmluZyB0aGUg
dHVwbGUsIG5vdCBmcmVlemluZyBpdCkuIFRoZSBleGNlcHRpb24gdG8KKyAq
IHRoaXMgcnVsZSBpcyBIRUFQVFVQTEVfREVBRCBIT1QgdHVwbGVzaTsgdGhv
c2UgYXJlIHRyZWF0ZWQgYXMgUkVDRU5UTFlfREVBRC4KICAqCiAgKiBOQjog
Y3V0b2ZmX3hpZCAqbXVzdCogYmUgPD0gdGhlIGN1cnJlbnQgZ2xvYmFsIHht
aW4sIHRvIGVuc3VyZSB0aGF0IGFueQogICogWElEIG9sZGVyIHRoYW4gaXQg
Y291bGQgbmVpdGhlciBiZSBydW5uaW5nIG5vciBzZWVuIGFzIHJ1bm5pbmcg
YnkgYW55CkBAIC02NzEyLDcgKzY3MTgsMTcgQEAgaGVhcF9wcmVwYXJlX2Zy
ZWV6ZV90dXBsZShIZWFwVHVwbGVIZWFkZXIgdHVwbGUsIFRyYW5zYWN0aW9u
SWQgY3V0b2ZmX3hpZCwKIAllbHNlIGlmIChUcmFuc2FjdGlvbklkSXNOb3Jt
YWwoeGlkKSkKIAl7CiAJCWlmIChUcmFuc2FjdGlvbklkUHJlY2VkZXMoeGlk
LCBjdXRvZmZfeGlkKSkKLQkJCWZyZWV6ZV94bWF4ID0gdHJ1ZTsKKwkJewor
CQkJaWYgKEhlYXBUdXBsZUhlYWRlcklzSGVhcE9ubHkodHVwbGUpIHx8CisJ
CQkJSGVhcFR1cGxlSGVhZGVySXNIb3RVcGRhdGVkKHR1cGxlKSkKKwkJCXsK
KwkJCQlmcnotPnRfaW5mb21hc2sgfD0gSEVBUF9YTUFYX0NPTU1JVFRFRDsK
KwkJCQl0b3RhbGx5X2Zyb3plbiA9IGZhbHNlOworCQkJCWNoYW5nZWQgPSB0
cnVlOworCQkJfQorCQkJZWxzZQorCQkJCWZyZWV6ZV94bWF4ID0gdHJ1ZTsK
KwkJfQogCQllbHNlCiAJCQl0b3RhbGx5X2Zyb3plbiA9IGZhbHNlOwogCX0K
ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL3ZhY3V1bWxhenku
YyBiL3NyYy9iYWNrZW5kL2NvbW1hbmRzL3ZhY3V1bWxhenkuYwppbmRleCA0
NWIxODU5Li45NzA2NGZlIDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9jb21t
YW5kcy92YWN1dW1sYXp5LmMKKysrIGIvc3JjL2JhY2tlbmQvY29tbWFuZHMv
dmFjdXVtbGF6eS5jCkBAIC0yMDE4LDE3ICsyMDE4LDIxIEBAIGxhenlfcmVj
b3JkX2RlYWRfdHVwbGUoTFZSZWxTdGF0cyAqdmFjcmVsc3RhdHMsCiAJCQkJ
CSAgIEl0ZW1Qb2ludGVyIGl0ZW1wdHIpCiB7CiAJLyoKLQkgKiBUaGUgYXJy
YXkgc2hvdWxkbid0IG92ZXJmbG93IHVuZGVyIG5vcm1hbCBiZWhhdmlvciwg
YnV0IHBlcmhhcHMgaXQKLQkgKiBjb3VsZCBpZiB3ZSBhcmUgZ2l2ZW4gYSBy
ZWFsbHkgc21hbGwgbWFpbnRlbmFuY2Vfd29ya19tZW0uIEluIHRoYXQKLQkg
KiBjYXNlLCBqdXN0IGZvcmdldCB0aGUgbGFzdCBmZXcgdHVwbGVzICh3ZSds
bCBnZXQgJ2VtIG5leHQgdGltZSkuCisJICogVGhlIGFycmF5IG11c3QgbmV2
ZXIgb3ZlcmZsb3csIHNpbmNlIHdlIHJlbHkgb24gYWxsIGRlbGV0YWJsZSB0
dXBsZXMKKwkgKiBiZWluZyByZW1vdmVkOyBpbmFiaWxpdHkgdG8gcmVtb3Zl
IGEgdHVwbGUgbWlnaHQgY2F1c2UgYW4gb2xkIFhJRCB0bworCSAqIHBlcnNp
c3QgYmV5b25kIHRoZSBmcmVlemUgbGltaXQsIHdoaWNoIGNvdWxkIGJlIGRp
c2FzdHJvdXMgbGF0ZXIgb24uCiAJICovCi0JaWYgKHZhY3JlbHN0YXRzLT5u
dW1fZGVhZF90dXBsZXMgPCB2YWNyZWxzdGF0cy0+bWF4X2RlYWRfdHVwbGVz
KQotCXsKLQkJdmFjcmVsc3RhdHMtPmRlYWRfdHVwbGVzW3ZhY3JlbHN0YXRz
LT5udW1fZGVhZF90dXBsZXNdID0gKml0ZW1wdHI7Ci0JCXZhY3JlbHN0YXRz
LT5udW1fZGVhZF90dXBsZXMrKzsKLQkJcGdzdGF0X3Byb2dyZXNzX3VwZGF0
ZV9wYXJhbShQUk9HUkVTU19WQUNVVU1fTlVNX0RFQURfVFVQTEVTLAotCQkJ
CQkJCQkJIHZhY3JlbHN0YXRzLT5udW1fZGVhZF90dXBsZXMpOwotCX0KKwlp
ZiAodmFjcmVsc3RhdHMtPm51bV9kZWFkX3R1cGxlcyA+PSB2YWNyZWxzdGF0
cy0+bWF4X2RlYWRfdHVwbGVzKQorCQllcmVwb3J0KEVSUk9SLCAoZXJyY29k
ZShFUlJDT0RFX0lOU1VGRklDSUVOVF9SRVNPVVJDRVMpLAorCQkJCWVycm1z
ZyAoImRlYWQgdHVwbGUgYXJyYXkgb3ZlcmZsb3c6ICVkIGRlYWQsICVkIG1h
eCIsCisJCQkJCQl2YWNyZWxzdGF0cy0+bnVtX2RlYWRfdHVwbGVzLAorCQkJ
CQkJdmFjcmVsc3RhdHMtPm1heF9kZWFkX3R1cGxlcyksCisJCQkJZXJyaGlu
dCgiSW5jcmVhc2UgbWFpbnRlbmFuY2Vfd29ya19tZW0iKSkpOworCisJdmFj
cmVsc3RhdHMtPmRlYWRfdHVwbGVzW3ZhY3JlbHN0YXRzLT5udW1fZGVhZF90
dXBsZXNdID0gKml0ZW1wdHI7CisJdmFjcmVsc3RhdHMtPm51bV9kZWFkX3R1
cGxlcysrOworCXBnc3RhdF9wcm9ncmVzc191cGRhdGVfcGFyYW0oUFJPR1JF
U1NfVkFDVVVNX05VTV9ERUFEX1RVUExFUywKKwkJCQkJCQkJIHZhY3JlbHN0
YXRzLT5udW1fZGVhZF90dXBsZXMpOwogfQogCiAvKgo=

--_002_150525896524551728amazoncom_
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

--_002_150525896524551728amazoncom_--