pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Avoid improbable PANIC during heap_update.
28+ messages / 5 participants
[nested] [flat]
* pgsql: Avoid improbable PANIC during heap_update.
@ 2021-04-13 16:17 Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2021-04-13 16:17 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Avoid improbable PANIC during heap_update.
heap_update needs to clear any existing "all visible" flag on
the old tuple's page (and on the new page too, if different).
Per coding rules, to do this it must acquire pin on the appropriate
visibility-map page while not holding exclusive buffer lock;
which creates a race condition since someone else could set the
flag whenever we're not holding the buffer lock. The code is
supposed to handle that by re-checking the flag after acquiring
buffer lock and retrying if it became set. However, one code
path through heap_update itself, as well as one in its subroutine
RelationGetBufferForTuple, failed to do this. The end result,
in the unlikely event that a concurrent VACUUM did set the flag
while we're transiently not holding lock, is a non-recurring
"PANIC: wrong buffer passed to visibilitymap_clear" failure.
This has been seen a few times in the buildfarm since recent VACUUM
changes that added code paths that could set the all-visible flag
while holding only exclusive buffer lock. Previously, the flag
was (usually?) set only after doing LockBufferForCleanup, which
would insist on buffer pin count zero, thus preventing the flag
from becoming set partway through heap_update. However, it's
clear that it's heap_update not VACUUM that's at fault here.
What's less clear is whether there is any hazard from these bugs
in released branches. heap_update is certainly violating API
expectations, but if there is no code path that can set all-visible
without a cleanup lock then it's only a latent bug. That's not
100% certain though, besides which we should worry about extensions
or future back-patch fixes that could introduce such code paths.
I chose to back-patch to v12. Fixing RelationGetBufferForTuple
before that would require also back-patching portions of older
fixes (notably 0d1fe9f74), which is more code churn than seems
prudent to fix a hypothetical issue.
Discussion: https://postgr.es/m/2247102.1618008027@sss.pgh.pa.us
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/34f581c39e97e2ea237255cf75cccebccc02d477
Modified Files
--------------
src/backend/access/heap/heapam.c | 44 ++++++++++++++++++++++++----------------
src/backend/access/heap/hio.c | 24 ++++++++++++++++------
2 files changed, 45 insertions(+), 23 deletions(-)
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-09-29 07:55 ` Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-10-06 16:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 2 replies; 28+ messages in thread
From: Jaime Casanova @ 2022-09-29 07:55 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers@lists.postgresql.org
On Tue, Apr 13, 2021 at 04:17:39PM +0000, Tom Lane wrote:
> Avoid improbable PANIC during heap_update.
>
> heap_update needs to clear any existing "all visible" flag on
> the old tuple's page (and on the new page too, if different).
> Per coding rules, to do this it must acquire pin on the appropriate
> visibility-map page while not holding exclusive buffer lock;
> which creates a race condition since someone else could set the
> flag whenever we're not holding the buffer lock. The code is
> supposed to handle that by re-checking the flag after acquiring
> buffer lock and retrying if it became set. However, one code
> path through heap_update itself, as well as one in its subroutine
> RelationGetBufferForTuple, failed to do this. The end result,
> in the unlikely event that a concurrent VACUUM did set the flag
> while we're transiently not holding lock, is a non-recurring
> "PANIC: wrong buffer passed to visibilitymap_clear" failure.
>
Hi,
This doesn't look as improbable because I saw it at least 3 times with
v15beta4.
The first time I thought it was my fault, then I tried with a commit on
september 25 (didn't remember which exactly but that doesn't seems too
relevant).
Finally I saw it again in a build with TRACE_VISIBILITYMAP defined (the
same commit).
But I haven't see it anymore on rc1. Anyway I'm attaching the backtrace
(this is from the build with TRACE_VISIBILITYMAP), the query that was
running at the time was (no changes were made to quad_poly_tbl table
nor any indexes were added to this table):
"""
update public.quad_poly_tbl set
id = public.quad_poly_tbl.id
returning
public.quad_poly_tbl.id as c0
"""
--
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
set = {__val = {4194304, 139906277533872, 139906277529216, 139906277533872, 139906275702592, 0, 895, 94675498488722, 140729237540432,
281470681751485, 0, 0, 0, 0, 0, 0}}
pid = <optimized out>
tid = <optimized out>
ret = <optimized out>
#1 0x00007f3e77dfc535 in __GI_abort () at abort.c:79
save_stage = 1
act = {__sigaction_handler = {sa_handler = 0x3ff0000000000000, sa_sigaction = 0x3ff0000000000000}, sa_mask = {__val = {0,
4609753056924675352, 0, 4728779608772575232, 0, 140729237540784, 94675496496005, 94675512751464, 94675512749920, 12884901888,
94675512751464, 94675512751440, 94675512749920, 140729237540832, 139906277529216, 139906277511840}}, sa_flags = 2011471149,
sa_restorer = 0x0}
sigs = {__val = {32, 0 <repeats 15 times>}}
#2 0x0000561b5b5335c7 in errfinish (filename=0x561b5b5c7180 "visibilitymap.c", lineno=156,
funcname=0x561b5b5c74a0 <__func__.12294> "visibilitymap_clear") at elog.c:675
edata = 0x561b5b8a8960 <errordata>
elevel = 23
oldcontext = 0x561b5d786c00
econtext = 0x0
__func__ = "errfinish"
#3 0x0000561b5af09ef7 in visibilitymap_clear (rel=0x561b5da3eb48, heapBlk=49, buf=0, flags=3 '\003') at visibilitymap.c:156
__errno_location = <optimized out>
mapBlock = 0
mapByte = 12
mapOffset = 2
mask = 12 '\f'
map = 0x7f3e70fc7af8 "\200\326\r"
cleared = false
__func__ = "visibilitymap_clear"
#4 0x0000561b5aee6ea7 in heap_update (relation=0x561b5da3eb48, otid=0x7ffe1436942a, newtup=0x561b5cdcf350, cid=0, crosscheck=0x0, wait=true,
tmfd=0x7ffe14369460, lockmode=0x7ffe14369394) at heapam.c:3913
result = TM_Ok
xid = 906880
hot_attrs = 0x561b5cdcf740
key_attrs = 0x0
id_attrs = 0x0
interesting_attrs = 0x561b5cdcf718
modified_attrs = 0x0
lp = 0x7f3e74a0fc18
oldtup = {t_len = 264, t_self = {ip_blkid = {bi_hi = 0, bi_lo = 49}, ip_posid = 1}, t_tableOid = 17323, t_data = 0x7f3e74a11af8}
heaptup = 0x561b5cdcf350
old_key_tuple = 0x0
old_key_copied = false
page = 0x7f3e74a0fc00 "\215"
block = 49
mxact_status = MultiXactStatusNoKeyUpdate
buffer = 11159
newbuf = 3698
vmbuffer = 0
vmbuffer_new = 0
need_toast = false
newtupsize = 264
pagefree = 112
have_tuple_lock = false
iscombo = false
use_hot_update = false
key_intact = true
all_visible_cleared = true
all_visible_cleared_new = false
checked_lockers = false
locker_remains = false
id_has_external = false
xmax_new_tuple = 0
xmax_old_tuple = 906880
infomask_old_tuple = 0
infomask2_old_tuple = 0
infomask_new_tuple = 2048
infomask2_new_tuple = 0
__func__ = "heap_update"
#5 0x0000561b5aef562b in heapam_tuple_update (relation=0x561b5da3eb48, otid=0x7ffe1436942a, slot=0x561b5d788a58, cid=0, snapshot=0x561b5c51be80,
crosscheck=0x0, wait=true, tmfd=0x7ffe14369460, lockmode=0x7ffe14369394, update_indexes=0x7ffe14369391) at heapam_handler.c:327
shouldFree = false
tuple = 0x561b5cdcf350
result = 32766
#6 0x0000561b5b186b50 in table_tuple_update (rel=0x561b5da3eb48, otid=0x7ffe1436942a, slot=0x561b5d788a58, cid=0, snapshot=0x561b5c51be80,
crosscheck=0x0, wait=true, tmfd=0x7ffe14369460, lockmode=0x7ffe14369394, update_indexes=0x7ffe14369391)
at ../../../src/include/access/tableam.h:1511
No locals.
#7 0x0000561b5b18996d in ExecUpdateAct (context=0x7ffe14369430, resultRelInfo=0x561b5d7871a8, tupleid=0x7ffe1436942a, oldtuple=0x0,
slot=0x561b5d788a58, canSetTag=true, updateCxt=0x7ffe14369390) at nodeModifyTable.c:1971
estate = 0x561b5d786d20
resultRelationDesc = 0x561b5da3eb48
partition_constraint_failed = false
result = 1568173344
#8 0x0000561b5b189e96 in ExecUpdate (context=0x7ffe14369430, resultRelInfo=0x561b5d7871a8, tupleid=0x7ffe1436942a, oldtuple=0x0,
slot=0x561b5d788a58, canSetTag=true) at nodeModifyTable.c:2185
estate = 0x561b5d786d20
resultRelationDesc = 0x561b5da3eb48
updateCxt = {updated = false, updateIndexes = false, crossPartUpdate = false, lockmode = LockTupleNoKeyExclusive}
recheckIndexes = 0x0
result = 22043
__func__ = "ExecUpdate"
#9 0x0000561b5b18c865 in ExecModifyTable (pstate=0x561b5d786f90) at nodeModifyTable.c:3737
node = 0x561b5d786f90
context = {mtstate = 0x561b5d786f90, epqstate = 0x561b5d787078, estate = 0x561b5d786d20, planSlot = 0x561b5d7878e8,
GetUpdateNewTuple = 0x561b5b187d1d <internalGetUpdateNewTuple>, relaction = 0x0, tmfd = {ctid = {ip_blkid = {bi_hi = 49287,
bi_lo = 23320}, ip_posid = 22043}, xmax = 1561021592, cmax = 22043, traversed = 4}, cpUpdateRetrySlot = 0x100000010,
cpUpdateReturningSlot = 0x561b5d0b4c98}
estate = 0x561b5d786d20
operation = CMD_UPDATE
resultRelInfo = 0x561b5d7871a8
subplanstate = 0x561b5d787458
slot = 0x561b5d788a58
oldSlot = 0x561b5d788940
tuple_ctid = {ip_blkid = {bi_hi = 0, bi_lo = 49}, ip_posid = 1}
oldtupdata = {t_len = 1561021608, t_self = {ip_blkid = {bi_hi = 22043, bi_lo = 0}, ip_posid = 1}, t_tableOid = 1, t_data = 0x7ffe14369490}
oldtuple = 0x0
tupleid = 0x7ffe1436942a
proute = 0x0
relinfos = 0x0
lc = 0x561b5d787f48
__func__ = "ExecModifyTable"
#10 0x0000561b5b140cd3 in ExecProcNode (node=0x561b5d786f90) at ../../../src/include/executor/executor.h:259
No locals.
#11 0x0000561b5b143992 in ExecutePlan (estate=0x561b5d786d20, planstate=0x561b5d786f90, use_parallel_mode=false, operation=CMD_UPDATE,
sendTuples=true, numberTuples=0, direction=ForwardScanDirection, dest=0x561b5c535b28, execute_once=true) at execMain.c:1636
slot = 0x561b5d787f48
current_tuple_count = 1471
#12 0x0000561b5b141379 in standard_ExecutorRun (queryDesc=0x561b5c535bc0, direction=ForwardScanDirection, count=0, execute_once=true)
at execMain.c:363
estate = 0x561b5d786d20
operation = CMD_UPDATE
dest = 0x561b5c535b28
sendTuples = true
oldcontext = 0x561b5c5358d0
__func__ = "standard_ExecutorRun"
#13 0x0000561b5b14118f in ExecutorRun (queryDesc=0x561b5c535bc0, direction=ForwardScanDirection, count=0, execute_once=true) at execMain.c:307
No locals.
#14 0x0000561b5b391d88 in ProcessQuery (plan=0x561b6046f6a0,
sourceText=0x561b5c4eb8a0 "update public.quad_poly_tbl set \n id = public.quad_poly_tbl.id\nreturning \n public.quad_poly_tbl.id as c0",
params=0x0, queryEnv=0x0, dest=0x561b5c535b28, qc=0x7ffe14369710) at pquery.c:160
queryDesc = 0x561b5c535bc0
#15 0x0000561b5b393761 in PortalRunMulti (portal=0x561b5f832120, isTopLevel=true, setHoldSnapshot=true, dest=0x561b5c535b28,
altdest=0x561b5b86da00 <donothingDR>, qc=0x7ffe14369710) at pquery.c:1277
pstmt = 0x561b6046f6a0
stmtlist_item__state = {l = 0x561b5cfce990, i = 0}
active_snapshot_set = true
stmtlist_item = 0x561b5cfce9a8
#16 0x0000561b5b3932a5 in FillPortalStore (portal=0x561b5f832120, isTopLevel=true) at pquery.c:1026
treceiver = 0x561b5c535b28
qc = {commandTag = CMDTAG_UNKNOWN, nprocessed = 0}
__func__ = "FillPortalStore"
#17 0x0000561b5b392c4b in PortalRun (portal=0x561b5f832120, count=9223372036854775807, isTopLevel=true, run_once=true, dest=0x561b5cfce9e8,
altdest=0x561b5cfce9e8, qc=0x7ffe143698e0) at pquery.c:763
_save_exception_stack = 0x7ffe143699e0
_save_context_stack = 0x0
_local_sigjmp_buf = {{__jmpbuf = {0, 2860372577420250015, 94675489186000, 140729237544704, 0, 0, 2860372577606896543, 8392611472902654879},
__mask_was_saved = 0, __saved_mask = {__val = {18728835850, 94675524184528, 128, 94675512768384, 94675513071824, 140729237542976,
16256, 94675513072368, 94675524184400, 140729237543008, 94675496542566, 109, 112, 94675524184664, 112, 339122288}}}}
_do_rethrow = false
result = false
nprocessed = 94675524184552
saveTopTransactionResourceOwner = 0x561b5c573850
saveTopTransactionContext = 0x561b5c51bc90
saveActivePortal = 0x0
saveResourceOwner = 0x561b5c573850
savePortalContext = 0x0
saveMemoryContext = 0x561b5c51bc90
__func__ = "PortalRun"
#18 0x0000561b5b38c548 in exec_simple_query (
query_string=0x561b5c4eb8a0 "update public.quad_poly_tbl set \n id = public.quad_poly_tbl.id\nreturning \n public.quad_poly_tbl.id as c0")
at postgres.c:1250
snapshot_set = true
per_parsetree_context = 0x0
plantree_list = 0x561b5cfce990
parsetree = 0x561b5c4ec8f0
commandTag = CMDTAG_UPDATE
qc = {commandTag = CMDTAG_UNKNOWN, nprocessed = 0}
querytree_list = 0x561b5c4ed358
portal = 0x561b5f832120
receiver = 0x561b5cfce9e8
format = 0
parsetree_item__state = {l = 0x561b5c4ec928, i = 0}
dest = DestRemote
oldcontext = 0x561b5c51bc90
parsetree_list = 0x561b5c4ec928
parsetree_item = 0x561b5c4ec940
save_log_statement_stats = false
was_logged = true
use_implicit_block = false
msec_str = "\240\270N\\\033V\000\000j\000\000\000\006\000\000\000\060\231\066\024\376\177\000\000\062\001\034[j\000\000"
__func__ = "exec_simple_query"
#19 0x0000561b5b390d39 in PostgresMain (dbname=0x561b5c5163e0 "regression", username=0x561b5c5163b8 "jcasanov") at postgres.c:4581
query_string = 0x561b5c4eb8a0 "update public.quad_poly_tbl set \n id = public.quad_poly_tbl.id\nreturning \n public.quad_poly_tbl.id as c0"
firstchar = 81
input_message = {
data = 0x561b5c4eb8a0 "update public.quad_poly_tbl set \n id = public.quad_poly_tbl.id\nreturning \n public.quad_poly_tbl.id as c0",
len = 107, maxlen = 1024, cursor = 107}
local_sigjmp_buf = {{__jmpbuf = {0, 2860372577472678815, 94675489186000, 140729237544704, 0, 0, 2860372577384598431, 8392611473924361119},
__mask_was_saved = 1, __saved_mask = {__val = {4194304, 1, 15679, 15680, 979, 18446744073709551536, 0, 0, 139906276254835, 3904, 0,
140729237543680, 94675489186000, 140729237544704, 94675496494014, 15616}}}}
send_ready_for_query = false
idle_in_transaction_timeout_enabled = false
idle_session_timeout_enabled = false
__func__ = "PostgresMain"
#20 0x0000561b5b2c8a02 in BackendRun (port=0x561b5c51a640) at postmaster.c:4504
No locals.
#21 0x0000561b5b2c834e in BackendStartup (port=0x561b5c51a640) at postmaster.c:4232
bn = 0x561b5c50d830
pid = 0
__func__ = "BackendStartup"
#22 0x0000561b5b2c4793 in ServerLoop () at postmaster.c:1806
port = 0x561b5c51a640
i = 2
rmask = {fds_bits = {256, 0 <repeats 15 times>}}
selres = 1
now = 1664163286
readmask = {fds_bits = {448, 0 <repeats 15 times>}}
nSockets = 9
last_lockfile_recheck_time = 1664163252
last_touch_time = 1664163132
__func__ = "ServerLoop"
#23 0x0000561b5b2c3fe2 in PostmasterMain (argc=3, argv=0x561b5c4e4eb0) at postmaster.c:1478
opt = -1
status = 0
userDoption = 0x561b5c5088a0 "data1"
listen_addr_saved = true
i = 64
output_config_variable = 0x0
__func__ = "PostmasterMain"
#24 0x0000561b5b1c33da in main (argc=3, argv=0x561b5c4e4eb0) at main.c:202
do_check_root = true
Attachments:
[text/plain] wrong_buffer_panic_beta4.txt (11.6K, ../../YzVPfCKNrmi0aLEh@ahch-to/2-wrong_buffer_panic_beta4.txt)
download | inline:
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
set = {__val = {4194304, 139906277533872, 139906277529216, 139906277533872, 139906275702592, 0, 895, 94675498488722, 140729237540432,
281470681751485, 0, 0, 0, 0, 0, 0}}
pid = <optimized out>
tid = <optimized out>
ret = <optimized out>
#1 0x00007f3e77dfc535 in __GI_abort () at abort.c:79
save_stage = 1
act = {__sigaction_handler = {sa_handler = 0x3ff0000000000000, sa_sigaction = 0x3ff0000000000000}, sa_mask = {__val = {0,
4609753056924675352, 0, 4728779608772575232, 0, 140729237540784, 94675496496005, 94675512751464, 94675512749920, 12884901888,
94675512751464, 94675512751440, 94675512749920, 140729237540832, 139906277529216, 139906277511840}}, sa_flags = 2011471149,
sa_restorer = 0x0}
sigs = {__val = {32, 0 <repeats 15 times>}}
#2 0x0000561b5b5335c7 in errfinish (filename=0x561b5b5c7180 "visibilitymap.c", lineno=156,
funcname=0x561b5b5c74a0 <__func__.12294> "visibilitymap_clear") at elog.c:675
edata = 0x561b5b8a8960 <errordata>
elevel = 23
oldcontext = 0x561b5d786c00
econtext = 0x0
__func__ = "errfinish"
#3 0x0000561b5af09ef7 in visibilitymap_clear (rel=0x561b5da3eb48, heapBlk=49, buf=0, flags=3 '\003') at visibilitymap.c:156
__errno_location = <optimized out>
mapBlock = 0
mapByte = 12
mapOffset = 2
mask = 12 '\f'
map = 0x7f3e70fc7af8 "\200\326\r"
cleared = false
__func__ = "visibilitymap_clear"
#4 0x0000561b5aee6ea7 in heap_update (relation=0x561b5da3eb48, otid=0x7ffe1436942a, newtup=0x561b5cdcf350, cid=0, crosscheck=0x0, wait=true,
tmfd=0x7ffe14369460, lockmode=0x7ffe14369394) at heapam.c:3913
result = TM_Ok
xid = 906880
hot_attrs = 0x561b5cdcf740
key_attrs = 0x0
id_attrs = 0x0
interesting_attrs = 0x561b5cdcf718
modified_attrs = 0x0
lp = 0x7f3e74a0fc18
oldtup = {t_len = 264, t_self = {ip_blkid = {bi_hi = 0, bi_lo = 49}, ip_posid = 1}, t_tableOid = 17323, t_data = 0x7f3e74a11af8}
heaptup = 0x561b5cdcf350
old_key_tuple = 0x0
old_key_copied = false
page = 0x7f3e74a0fc00 "\215"
block = 49
mxact_status = MultiXactStatusNoKeyUpdate
buffer = 11159
newbuf = 3698
vmbuffer = 0
vmbuffer_new = 0
need_toast = false
newtupsize = 264
pagefree = 112
have_tuple_lock = false
iscombo = false
use_hot_update = false
key_intact = true
all_visible_cleared = true
all_visible_cleared_new = false
checked_lockers = false
locker_remains = false
id_has_external = false
xmax_new_tuple = 0
xmax_old_tuple = 906880
infomask_old_tuple = 0
infomask2_old_tuple = 0
infomask_new_tuple = 2048
infomask2_new_tuple = 0
__func__ = "heap_update"
#5 0x0000561b5aef562b in heapam_tuple_update (relation=0x561b5da3eb48, otid=0x7ffe1436942a, slot=0x561b5d788a58, cid=0, snapshot=0x561b5c51be80,
crosscheck=0x0, wait=true, tmfd=0x7ffe14369460, lockmode=0x7ffe14369394, update_indexes=0x7ffe14369391) at heapam_handler.c:327
shouldFree = false
tuple = 0x561b5cdcf350
result = 32766
#6 0x0000561b5b186b50 in table_tuple_update (rel=0x561b5da3eb48, otid=0x7ffe1436942a, slot=0x561b5d788a58, cid=0, snapshot=0x561b5c51be80,
crosscheck=0x0, wait=true, tmfd=0x7ffe14369460, lockmode=0x7ffe14369394, update_indexes=0x7ffe14369391)
at ../../../src/include/access/tableam.h:1511
No locals.
#7 0x0000561b5b18996d in ExecUpdateAct (context=0x7ffe14369430, resultRelInfo=0x561b5d7871a8, tupleid=0x7ffe1436942a, oldtuple=0x0,
slot=0x561b5d788a58, canSetTag=true, updateCxt=0x7ffe14369390) at nodeModifyTable.c:1971
estate = 0x561b5d786d20
resultRelationDesc = 0x561b5da3eb48
partition_constraint_failed = false
result = 1568173344
#8 0x0000561b5b189e96 in ExecUpdate (context=0x7ffe14369430, resultRelInfo=0x561b5d7871a8, tupleid=0x7ffe1436942a, oldtuple=0x0,
slot=0x561b5d788a58, canSetTag=true) at nodeModifyTable.c:2185
estate = 0x561b5d786d20
resultRelationDesc = 0x561b5da3eb48
updateCxt = {updated = false, updateIndexes = false, crossPartUpdate = false, lockmode = LockTupleNoKeyExclusive}
recheckIndexes = 0x0
result = 22043
__func__ = "ExecUpdate"
#9 0x0000561b5b18c865 in ExecModifyTable (pstate=0x561b5d786f90) at nodeModifyTable.c:3737
node = 0x561b5d786f90
context = {mtstate = 0x561b5d786f90, epqstate = 0x561b5d787078, estate = 0x561b5d786d20, planSlot = 0x561b5d7878e8,
GetUpdateNewTuple = 0x561b5b187d1d <internalGetUpdateNewTuple>, relaction = 0x0, tmfd = {ctid = {ip_blkid = {bi_hi = 49287,
bi_lo = 23320}, ip_posid = 22043}, xmax = 1561021592, cmax = 22043, traversed = 4}, cpUpdateRetrySlot = 0x100000010,
cpUpdateReturningSlot = 0x561b5d0b4c98}
estate = 0x561b5d786d20
operation = CMD_UPDATE
resultRelInfo = 0x561b5d7871a8
subplanstate = 0x561b5d787458
slot = 0x561b5d788a58
oldSlot = 0x561b5d788940
tuple_ctid = {ip_blkid = {bi_hi = 0, bi_lo = 49}, ip_posid = 1}
oldtupdata = {t_len = 1561021608, t_self = {ip_blkid = {bi_hi = 22043, bi_lo = 0}, ip_posid = 1}, t_tableOid = 1, t_data = 0x7ffe14369490}
oldtuple = 0x0
tupleid = 0x7ffe1436942a
proute = 0x0
relinfos = 0x0
lc = 0x561b5d787f48
__func__ = "ExecModifyTable"
#10 0x0000561b5b140cd3 in ExecProcNode (node=0x561b5d786f90) at ../../../src/include/executor/executor.h:259
No locals.
#11 0x0000561b5b143992 in ExecutePlan (estate=0x561b5d786d20, planstate=0x561b5d786f90, use_parallel_mode=false, operation=CMD_UPDATE,
sendTuples=true, numberTuples=0, direction=ForwardScanDirection, dest=0x561b5c535b28, execute_once=true) at execMain.c:1636
slot = 0x561b5d787f48
current_tuple_count = 1471
#12 0x0000561b5b141379 in standard_ExecutorRun (queryDesc=0x561b5c535bc0, direction=ForwardScanDirection, count=0, execute_once=true)
at execMain.c:363
estate = 0x561b5d786d20
operation = CMD_UPDATE
dest = 0x561b5c535b28
sendTuples = true
oldcontext = 0x561b5c5358d0
__func__ = "standard_ExecutorRun"
#13 0x0000561b5b14118f in ExecutorRun (queryDesc=0x561b5c535bc0, direction=ForwardScanDirection, count=0, execute_once=true) at execMain.c:307
No locals.
#14 0x0000561b5b391d88 in ProcessQuery (plan=0x561b6046f6a0,
sourceText=0x561b5c4eb8a0 "update public.quad_poly_tbl set \n id = public.quad_poly_tbl.id\nreturning \n public.quad_poly_tbl.id as c0",
params=0x0, queryEnv=0x0, dest=0x561b5c535b28, qc=0x7ffe14369710) at pquery.c:160
queryDesc = 0x561b5c535bc0
#15 0x0000561b5b393761 in PortalRunMulti (portal=0x561b5f832120, isTopLevel=true, setHoldSnapshot=true, dest=0x561b5c535b28,
altdest=0x561b5b86da00 <donothingDR>, qc=0x7ffe14369710) at pquery.c:1277
pstmt = 0x561b6046f6a0
stmtlist_item__state = {l = 0x561b5cfce990, i = 0}
active_snapshot_set = true
stmtlist_item = 0x561b5cfce9a8
#16 0x0000561b5b3932a5 in FillPortalStore (portal=0x561b5f832120, isTopLevel=true) at pquery.c:1026
treceiver = 0x561b5c535b28
qc = {commandTag = CMDTAG_UNKNOWN, nprocessed = 0}
__func__ = "FillPortalStore"
#17 0x0000561b5b392c4b in PortalRun (portal=0x561b5f832120, count=9223372036854775807, isTopLevel=true, run_once=true, dest=0x561b5cfce9e8,
altdest=0x561b5cfce9e8, qc=0x7ffe143698e0) at pquery.c:763
_save_exception_stack = 0x7ffe143699e0
_save_context_stack = 0x0
_local_sigjmp_buf = {{__jmpbuf = {0, 2860372577420250015, 94675489186000, 140729237544704, 0, 0, 2860372577606896543, 8392611472902654879},
__mask_was_saved = 0, __saved_mask = {__val = {18728835850, 94675524184528, 128, 94675512768384, 94675513071824, 140729237542976,
16256, 94675513072368, 94675524184400, 140729237543008, 94675496542566, 109, 112, 94675524184664, 112, 339122288}}}}
_do_rethrow = false
result = false
nprocessed = 94675524184552
saveTopTransactionResourceOwner = 0x561b5c573850
saveTopTransactionContext = 0x561b5c51bc90
saveActivePortal = 0x0
saveResourceOwner = 0x561b5c573850
savePortalContext = 0x0
saveMemoryContext = 0x561b5c51bc90
__func__ = "PortalRun"
#18 0x0000561b5b38c548 in exec_simple_query (
query_string=0x561b5c4eb8a0 "update public.quad_poly_tbl set \n id = public.quad_poly_tbl.id\nreturning \n public.quad_poly_tbl.id as c0")
at postgres.c:1250
snapshot_set = true
per_parsetree_context = 0x0
plantree_list = 0x561b5cfce990
parsetree = 0x561b5c4ec8f0
commandTag = CMDTAG_UPDATE
qc = {commandTag = CMDTAG_UNKNOWN, nprocessed = 0}
querytree_list = 0x561b5c4ed358
portal = 0x561b5f832120
receiver = 0x561b5cfce9e8
format = 0
parsetree_item__state = {l = 0x561b5c4ec928, i = 0}
dest = DestRemote
oldcontext = 0x561b5c51bc90
parsetree_list = 0x561b5c4ec928
parsetree_item = 0x561b5c4ec940
save_log_statement_stats = false
was_logged = true
use_implicit_block = false
msec_str = "\240\270N\\\033V\000\000j\000\000\000\006\000\000\000\060\231\066\024\376\177\000\000\062\001\034[j\000\000"
__func__ = "exec_simple_query"
#19 0x0000561b5b390d39 in PostgresMain (dbname=0x561b5c5163e0 "regression", username=0x561b5c5163b8 "jcasanov") at postgres.c:4581
query_string = 0x561b5c4eb8a0 "update public.quad_poly_tbl set \n id = public.quad_poly_tbl.id\nreturning \n public.quad_poly_tbl.id as c0"
firstchar = 81
input_message = {
data = 0x561b5c4eb8a0 "update public.quad_poly_tbl set \n id = public.quad_poly_tbl.id\nreturning \n public.quad_poly_tbl.id as c0",
len = 107, maxlen = 1024, cursor = 107}
local_sigjmp_buf = {{__jmpbuf = {0, 2860372577472678815, 94675489186000, 140729237544704, 0, 0, 2860372577384598431, 8392611473924361119},
__mask_was_saved = 1, __saved_mask = {__val = {4194304, 1, 15679, 15680, 979, 18446744073709551536, 0, 0, 139906276254835, 3904, 0,
140729237543680, 94675489186000, 140729237544704, 94675496494014, 15616}}}}
send_ready_for_query = false
idle_in_transaction_timeout_enabled = false
idle_session_timeout_enabled = false
__func__ = "PostgresMain"
#20 0x0000561b5b2c8a02 in BackendRun (port=0x561b5c51a640) at postmaster.c:4504
No locals.
#21 0x0000561b5b2c834e in BackendStartup (port=0x561b5c51a640) at postmaster.c:4232
bn = 0x561b5c50d830
pid = 0
__func__ = "BackendStartup"
#22 0x0000561b5b2c4793 in ServerLoop () at postmaster.c:1806
port = 0x561b5c51a640
i = 2
rmask = {fds_bits = {256, 0 <repeats 15 times>}}
selres = 1
now = 1664163286
readmask = {fds_bits = {448, 0 <repeats 15 times>}}
nSockets = 9
last_lockfile_recheck_time = 1664163252
last_touch_time = 1664163132
__func__ = "ServerLoop"
#23 0x0000561b5b2c3fe2 in PostmasterMain (argc=3, argv=0x561b5c4e4eb0) at postmaster.c:1478
opt = -1
status = 0
userDoption = 0x561b5c5088a0 "data1"
listen_addr_saved = true
i = 64
output_config_variable = 0x0
__func__ = "PostmasterMain"
#24 0x0000561b5b1c33da in main (argc=3, argv=0x561b5c4e4eb0) at main.c:202
do_check_root = true
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
@ 2022-09-30 20:44 ` Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:51 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 2 replies; 28+ messages in thread
From: Jaime Casanova @ 2022-09-30 20:44 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers@lists.postgresql.org
On Thu, Sep 29, 2022 at 02:55:40AM -0500, Jaime Casanova wrote:
> On Tue, Apr 13, 2021 at 04:17:39PM +0000, Tom Lane wrote:
> > Avoid improbable PANIC during heap_update.
> >
> > heap_update needs to clear any existing "all visible" flag on
> > the old tuple's page (and on the new page too, if different).
> > Per coding rules, to do this it must acquire pin on the appropriate
> > visibility-map page while not holding exclusive buffer lock;
> > which creates a race condition since someone else could set the
> > flag whenever we're not holding the buffer lock. The code is
> > supposed to handle that by re-checking the flag after acquiring
> > buffer lock and retrying if it became set. However, one code
> > path through heap_update itself, as well as one in its subroutine
> > RelationGetBufferForTuple, failed to do this. The end result,
> > in the unlikely event that a concurrent VACUUM did set the flag
> > while we're transiently not holding lock, is a non-recurring
> > "PANIC: wrong buffer passed to visibilitymap_clear" failure.
> >
>
> Hi,
>
> This doesn't look as improbable because I saw it at least 3 times with
> v15beta4.
>
> The first time I thought it was my fault, then I tried with a commit on
> september 25 (didn't remember which exactly but that doesn't seems too
> relevant).
> Finally I saw it again in a build with TRACE_VISIBILITYMAP defined (the
> same commit).
>
> But I haven't see it anymore on rc1. Anyway I'm attaching the backtrace
> (this is from the build with TRACE_VISIBILITYMAP), the query that was
> running at the time was (no changes were made to quad_poly_tbl table
> nor any indexes were added to this table):
>
> """
> update public.quad_poly_tbl set
> id = public.quad_poly_tbl.id
> returning
> public.quad_poly_tbl.id as c0
> """
>
Just to confirm I saw this on RC1
--
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
@ 2022-09-30 20:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-03 19:34 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
1 sibling, 1 reply; 28+ messages in thread
From: Tom Lane @ 2022-09-30 20:51 UTC (permalink / raw)
To: Jaime Casanova <jcasanov@systemguards.com.ec>; +Cc: pgsql-committers@lists.postgresql.org
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
> Just to confirm I saw this on RC1
What test case are you using?
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:51 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-10-03 19:34 ` Jaime Casanova <jcasanov@systemguards.com.ec>
0 siblings, 0 replies; 28+ messages in thread
From: Jaime Casanova @ 2022-10-03 19:34 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers@lists.postgresql.org
On Fri, Sep 30, 2022 at 04:51:20PM -0400, Tom Lane wrote:
> Jaime Casanova <jcasanov@systemguards.com.ec> writes:
> > Just to confirm I saw this on RC1
>
> What test case are you using?
>
Hi,
Currently the way I have to reproduce it is:
- install the regression database
- drop all tables but:
hash_i4_heap, hash_name_heap, hash_txt_heap,
quad_poly_tbl, road
- run 10 sqlsmith processes... normally in an hour or less the problem
appears
I have the logs from the last time it happened so maybe I can trace the
exact pattern to reproducite at will... at least to keep a test
somewhere.
BTW, so far so good with your last fix (about 12 hours now)...
--
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
@ 2022-09-30 21:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:52 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 2 replies; 28+ messages in thread
From: Tom Lane @ 2022-09-30 21:28 UTC (permalink / raw)
To: Jaime Casanova <jcasanov@systemguards.com.ec>; +Cc: pgsql-committers@lists.postgresql.org
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
> Just to confirm I saw this on RC1
Ugh ... I think I see the problem. There's still one path through
RelationGetBufferForTuple that fails to guarantee that it's acquired
a vmbuffer pin if the all-visible flag becomes set in the otherBuffer.
Namely, if we're forced to extend the relation, then we deal with
vm pins when ConditionalLockBuffer(otherBuffer) fails ... but not
when it succeeds. I think the fix is just to move the last
GetVisibilityMapPins call out of the "if
(unlikely(!ConditionalLockBuffer(otherBuffer)))" stanza.
It'd still be good to have a test case for this ...
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-09-30 21:39 ` Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 28+ messages in thread
From: Peter Geoghegan @ 2022-09-30 21:39 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org
On Fri, Sep 30, 2022 at 2:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Jaime Casanova <jcasanov@systemguards.com.ec> writes:
> > Just to confirm I saw this on RC1
>
> Ugh ... I think I see the problem. There's still one path through
> RelationGetBufferForTuple that fails to guarantee that it's acquired
> a vmbuffer pin if the all-visible flag becomes set in the otherBuffer.
> It'd still be good to have a test case for this ...
FWIW it seems possible that the Postgres 15 vacuumlazy.c work that
added lazy_scan_noprune() made this scenario more likely in practice
-- even compared to Postgres 14.
Note that VACUUM will collect preexisting LP_DEAD items in heap pages
that cannot be cleanup locked during VACUUM's first heap pass in
Postgres 15 (in lazy_scan_noprune). There is no need for a cleanup
lock in the second heap pass, either (that details is the same as 14,
but not earlier versions). So 15 is the first version that doesn't
need a cleanup lock in either the first heap pass or the second heap
pass to be able to set the heap page all-visible. That difference
seems like it could be "protective" on 14, especially when vacuuming
smaller tables.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
@ 2022-09-30 21:56 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2022-09-30 21:56 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org
Peter Geoghegan <pg@bowt.ie> writes:
> On Fri, Sep 30, 2022 at 2:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Ugh ... I think I see the problem. There's still one path through
>> RelationGetBufferForTuple that fails to guarantee that it's acquired
>> a vmbuffer pin if the all-visible flag becomes set in the otherBuffer.
> FWIW it seems possible that the Postgres 15 vacuumlazy.c work that
> added lazy_scan_noprune() made this scenario more likely in practice
> -- even compared to Postgres 14.
Could be, because we haven't seen field reports of this in v14 yet.
And there's still no hard evidence of a bug pre-14. Nonetheless,
I'm inclined to backpatch to v12 as 34f581c39 was.
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-09-30 22:03 ` Peter Geoghegan <pg@bowt.ie>
2022-09-30 23:52 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 28+ messages in thread
From: Peter Geoghegan @ 2022-09-30 22:03 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org
On Fri, Sep 30, 2022 at 2:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > FWIW it seems possible that the Postgres 15 vacuumlazy.c work that
> > added lazy_scan_noprune() made this scenario more likely in practice
> > -- even compared to Postgres 14.
>
> Could be, because we haven't seen field reports of this in v14 yet.
I would be more confident here were it not for the recent
heap_delete() issue reported by one of my AWS colleagues (and fixed by
another, Jeff Davis). See commit 163b0993 if you missed it before now.
> And there's still no hard evidence of a bug pre-14. Nonetheless,
> I'm inclined to backpatch to v12 as 34f581c39 was.
+1
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
@ 2022-09-30 23:52 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 00:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2022-09-30 23:52 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org
Peter Geoghegan <pg@bowt.ie> writes:
> On Fri, Sep 30, 2022 at 2:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Could be, because we haven't seen field reports of this in v14 yet.
> I would be more confident here were it not for the recent
> heap_delete() issue reported by one of my AWS colleagues (and fixed by
> another, Jeff Davis). See commit 163b0993 if you missed it before now.
Hmm, okay, though that's really a distinct bug of the same ilk.
You're right that I'd not paid close attention to that thread after
Jeff diagnosed the problem. It does seem like Robins' report
shows that there's some way that v13 will set the AV bit without
a cleanup lock ... does that constitute a bug in itself?
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 23:52 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-10-01 00:09 ` Peter Geoghegan <pg@bowt.ie>
2022-10-01 01:29 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 28+ messages in thread
From: Peter Geoghegan @ 2022-10-01 00:09 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org
On Fri, Sep 30, 2022 at 4:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:> >
I would be more confident here were it not for the recent
> > heap_delete() issue reported by one of my AWS colleagues (and fixed by
> > another, Jeff Davis). See commit 163b0993 if you missed it before now.
>
> Hmm, okay, though that's really a distinct bug of the same ilk.
> You're right that I'd not paid close attention to that thread after
> Jeff diagnosed the problem.
I just meant that I don't feel particularly confident about what might
be possible or likely in Postgres 14 with this new issue in
heap_update() on point releases without today's bugfix. My theory
about lazy_scan_noprune() might be correct, but take it with a grain
of salt.
> It does seem like Robins' report
> shows that there's some way that v13 will set the AV bit without
> a cleanup lock ... does that constitute a bug in itself?
We never got to the bottom of that part, strangely enough. I can ask again.
In any case we cannot really treat the information that we have about
that as a bug report -- not as things stand. Why should the question
of whether or not we ever set a page PD_ALL_VISIBLE without a cleanup
lock on v13 be a mystery at all? Why wouldn't a simple grep get to the
bottom of it? I have to imagine that the true explanation is very
simple and boring.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 23:52 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 00:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
@ 2022-10-01 01:29 ` Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:23 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 28+ messages in thread
From: Peter Geoghegan @ 2022-10-01 01:29 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org
On Fri, Sep 30, 2022 at 5:09 PM Peter Geoghegan <pg@bowt.ie> wrote:
> In any case we cannot really treat the information that we have about
> that as a bug report -- not as things stand. Why should the question
> of whether or not we ever set a page PD_ALL_VISIBLE without a cleanup
> lock on v13 be a mystery at all? Why wouldn't a simple grep get to the
> bottom of it? I have to imagine that the true explanation is very
> simple and boring.
I talked to Robins about this privately. I was wrong; there isn't a
simple or boring explanation.
Robins set out to find bugs like this in Postgres via stress-testing.
He used a lab environment for this, and was quite methodical. So there
is no reason to doubt that a PANIC happened on v13 at least once.
There must be some relatively complicated explanation for that, but
right now I can only speculate.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 23:52 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 00:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 01:29 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
@ 2022-10-01 04:23 ` Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:38 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 16:35 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jeff Davis <pgsql@j-davis.com>
0 siblings, 2 replies; 28+ messages in thread
From: Peter Geoghegan @ 2022-10-01 04:23 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org, Jeff Davis <pgsql@j-davis.com>
On Fri, Sep 30, 2022 at 6:29 PM Peter Geoghegan <pg@bowt.ie> wrote:
> I talked to Robins about this privately. I was wrong; there isn't a
> simple or boring explanation.
I think that I figured it out. With or without bugfix commit 163b0993,
we do these steps early in heap_delete() (this is 13 code as of
today):
2490 page = BufferGetPage(buffer);
2491
2492 /*
2493 * Before locking the buffer, pin the visibility map page if
it appears to
2494 * be necessary. Since we haven't got the lock yet, someone
else might be
2495 * in the middle of changing this, so we'll need to recheck
after we have
2496 * the lock.
2497 */
2498 if (PageIsAllVisible(page))
2499 visibilitymap_pin(relation, block, &vmbuffer);
So we're calling visibilitymap_pin() having just acquired a buffer pin
on a heap page buffer for the first time, and without acquiring a
buffer lock on the same heap page (we don't hold one now, and we've
never held one at some earlier point).
Without Jeff's bugfix, nothing stops heap_delete() from getting a pin
on a heap page that happens to have already been cleanup locked by
another session running VACUUM. The same session could then
(correctly) observe that the page does not have PD_ALL_VISIBLE set --
not yet. VACUUM might then set PD_ALL_VISIBLE, without having to
acquire any kind of interlock that heap_delete() will reliably notice.
After all, VACUUM had a cleanup lock before the other session's call
to heap_delete() even began -- so the responsibility has to lie with
heap_delete().
Jeff's bugfix will fix the bug on 13 too. The bugfix doesn't take the
aggressive/conservative approach of simply getting an exclusive lock
to check PageIsAllVisible() at the same point, on performance grounds
(no change there). The bugfix does make this old heap_delete()
no-buffer-lock behavior safe by teaching heap_delete() to not assume
that a page that didn't have PD_ALL_VISIBLE initially set cannot have
it set concurrently.
So 13 is only different to 14 in that there are fewer ways for
essentially the same race to happen. This is probably only true for
the heap_delete() issue, not either of the similar heap_update()
issues.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 23:52 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 00:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 01:29 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:23 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
@ 2022-10-01 04:38 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 05:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
1 sibling, 1 reply; 28+ messages in thread
From: Tom Lane @ 2022-10-01 04:38 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org, Jeff Davis <pgsql@j-davis.com>
Peter Geoghegan <pg@bowt.ie> writes:
> ... nothing stops heap_delete() from getting a pin
> on a heap page that happens to have already been cleanup locked by
> another session running VACUUM. The same session could then
> (correctly) observe that the page does not have PD_ALL_VISIBLE set --
> not yet. VACUUM might then set PD_ALL_VISIBLE, without having to
> acquire any kind of interlock that heap_delete() will reliably notice.
I'm too tired to think this through completely clearly, but this
sounds right, and what it seems to imply is that this race condition
exists in all PG versions. Which would imply that we need to do the
work to back-patch these three fixes into v11/v10. I would rather
not do that, because then we'd have to also back-patch some other
more-invasive changes, and the net risk of introducing new bugs
seems uncomfortably high. (Especially for v10, where there will
be no second chance after the November releases.)
So what is bothering me about this line of thought is: how come
there have not been reports of these failures in older branches?
Is there some aspect we're not thinking about that masks the bug?
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 23:52 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 00:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 01:29 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:23 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:38 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-10-01 05:09 ` Peter Geoghegan <pg@bowt.ie>
2022-10-01 05:13 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 28+ messages in thread
From: Peter Geoghegan @ 2022-10-01 05:09 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org, Jeff Davis <pgsql@j-davis.com>
On Fri, Sep 30, 2022 at 9:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm too tired to think this through completely clearly, but this
> sounds right, and what it seems to imply is that this race condition
> exists in all PG versions.
I think that the heap_delete() issue is probably in all PG versions.
> Which would imply that we need to do the
> work to back-patch these three fixes into v11/v10.
I am not aware of any reason why we should need the heap_update()
fixes to be backpatched any further. Though I will need to think about
it some more.
> So what is bothering me about this line of thought is: how come
> there have not been reports of these failures in older branches?
> Is there some aspect we're not thinking about that masks the bug?
The likely explanation is that Robins was able to find the
heap_delete() bug by throwing lots of resources (human effort and
machine time) into it. It literally took weeks of adversarial
stress-testing to find the bug. It's entirely possible and perhaps
likely that this isn't representative of real world conditions in some
crucial way.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 23:52 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 00:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 01:29 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:23 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:38 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 05:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
@ 2022-10-01 05:13 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 05:25 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2022-10-01 05:13 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org, Jeff Davis <pgsql@j-davis.com>
Peter Geoghegan <pg@bowt.ie> writes:
> I think that the heap_delete() issue is probably in all PG versions.
Yeah, that's what I'm afraid of ...
> I am not aware of any reason why we should need the heap_update()
> fixes to be backpatched any further.
How so? AFAICS these are exactly the same oversight, ie failure
to deal with the all-visible bit getting set partway through the
operation. You've explained how that can happen.
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 23:52 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 00:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 01:29 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:23 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:38 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 05:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 05:13 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-10-01 05:25 ` Peter Geoghegan <pg@bowt.ie>
0 siblings, 0 replies; 28+ messages in thread
From: Peter Geoghegan @ 2022-10-01 05:25 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org, Jeff Davis <pgsql@j-davis.com>
On Fri, Sep 30, 2022 at 10:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> How so? AFAICS these are exactly the same oversight, ie failure
> to deal with the all-visible bit getting set partway through the
> operation. You've explained how that can happen.
I thought that there might have been something protective about how
the loop would work in heap_update(), but perhaps that's not true. It
might just be that heap_update() does lots of stuff in between, so
it's less likely to be affected by this particular race (the race
which seems to be present in all versions).
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 23:52 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 00:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 01:29 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:23 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
@ 2022-10-01 16:35 ` Jeff Davis <pgsql@j-davis.com>
2022-10-01 16:53 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
1 sibling, 1 reply; 28+ messages in thread
From: Jeff Davis @ 2022-10-01 16:35 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org
On Fri, 2022-09-30 at 21:23 -0700, Peter Geoghegan wrote:
> 2490 page = BufferGetPage(buffer);
> 2491
> 2492 /*
> 2493 * Before locking the buffer, pin the visibility map page if
> it appears to
> 2494 * be necessary. Since we haven't got the lock yet, someone
> else might be
> 2495 * in the middle of changing this, so we'll need to recheck
> after we have
> 2496 * the lock.
> 2497 */
> 2498 if (PageIsAllVisible(page))
> 2499 visibilitymap_pin(relation, block, &vmbuffer);
>
> So we're calling visibilitymap_pin() having just acquired a buffer
> pin
> on a heap page buffer for the first time, and without acquiring a
> buffer lock on the same heap page (we don't hold one now, and we've
> never held one at some earlier point).
>
> Without Jeff's bugfix, nothing stops heap_delete() from getting a pin
> on a heap page that happens to have already been cleanup locked by
> another session running VACUUM. The same session could then
> (correctly) observe that the page does not have PD_ALL_VISIBLE set --
> not yet.
With you so far; I had considered this code path and was still unable
to repro.
> VACUUM might then set PD_ALL_VISIBLE, without having to
> acquire any kind of interlock that heap_delete() will reliably
> notice.
> After all, VACUUM had a cleanup lock before the other session's call
> to heap_delete() even began -- so the responsibility has to lie with
> heap_delete().
Directly after the code you reference above, there is (in 5f9dda4c06,
right before my patch):
2501 LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
2502
2503 /*
2504 * If we didn't pin the visibility map page and the page has
become all
2505 * visible while we were busy locking the buffer, we'll have
to unlock and
2506 * re-lock, to avoid holding the buffer lock across an I/O.
That's a bit
2507 * unfortunate, but hopefully shouldn't happen often.
2508 */
2509 if (vmbuffer == InvalidBuffer && PageIsAllVisible(page))
2510 {
2511 LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
2512 visibilitymap_pin(relation, block, &vmbuffer);
2513 LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
2514 }
Doesn't that deal with the case you brought up directly? heap_delete()
can't get the exclusive lock until VACUUM releases its cleanup lock, at
which point all-visible will be set. Then heap_delete() should notice
in line 2509 and then pin the VM buffer. Right?
And I don't think a similar issue exists when the locks are briefly
released on lines 2563 or 2606. The pin is held until after the VM bit
is cleared (aside from an error path and an early return):
2489 buffer = ReadBuffer(relation, block);
...
2717 if (PageIsAllVisible(page))
2718 {
2719 all_visible_cleared = true;
2720 PageClearAllVisible(page);
...
2835 ReleaseBuffer(buffer);
If VACCUM hasn't acquired the cleanup lock before 2489, it can't get
one until heap_delete() is done looking at the all-visible bit anyway.
So I don't see how my patch would have fixed it even if that was the
problem.
Of course, I must be wrong somewhere, because the bug seems to exist.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-09-30 23:52 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 00:09 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 01:29 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:23 ` Re: pgsql: Avoid improbable PANIC during heap_update. Peter Geoghegan <pg@bowt.ie>
2022-10-01 16:35 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jeff Davis <pgsql@j-davis.com>
@ 2022-10-01 16:53 ` Peter Geoghegan <pg@bowt.ie>
0 siblings, 0 replies; 28+ messages in thread
From: Peter Geoghegan @ 2022-10-01 16:53 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Jaime Casanova <jcasanov@systemguards.com.ec>; pgsql-committers@lists.postgresql.org
On Sat, Oct 1, 2022 at 9:35 AM Jeff Davis <pgsql@j-davis.com> wrote:
> Doesn't that deal with the case you brought up directly? heap_delete()
> can't get the exclusive lock until VACUUM releases its cleanup lock, at
> which point all-visible will be set. Then heap_delete() should notice
> in line 2509 and then pin the VM buffer. Right?
I now believe that you're right. I don't think that this code was ever
designed to rely on cleanup locks in any way; that was kind of an
accident all along. Even still, I'm not sure how I missed such an
obvious thing. Sorry for the misdirection.
Still, there has to be *some* reason why the bug could repro on 13.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Re: pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-09-30 21:52 ` Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 28+ messages in thread
From: Tom Lane @ 2022-09-30 21:52 UTC (permalink / raw)
To: Jaime Casanova <jcasanov@systemguards.com.ec>; +Cc: pgsql-committers@lists.postgresql.org
I wrote:
> Ugh ... I think I see the problem. There's still one path through
> RelationGetBufferForTuple that fails to guarantee that it's acquired
> a vmbuffer pin if the all-visible flag becomes set in the otherBuffer.
> Namely, if we're forced to extend the relation, then we deal with
> vm pins when ConditionalLockBuffer(otherBuffer) fails ... but not
> when it succeeds. I think the fix is just to move the last
> GetVisibilityMapPins call out of the "if
> (unlikely(!ConditionalLockBuffer(otherBuffer)))" stanza.
Concretely, about like this.
regards, tom lane
Attachments:
[text/x-diff] fix-RelationGetBufferForTuple-some-more.patch (2.0K, ../../741234.1664574750@sss.pgh.pa.us/2-fix-RelationGetBufferForTuple-some-more.patch)
download | inline diff:
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index ae2e2ce37a..b0ece66629 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -678,29 +678,34 @@ loop:
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
LockBuffer(otherBuffer, BUFFER_LOCK_EXCLUSIVE);
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
+ }
- /*
- * Because the buffers were unlocked for a while, it's possible,
- * although unlikely, that an all-visible flag became set or that
- * somebody used up the available space in the new page. We can
- * use GetVisibilityMapPins to deal with the first case. In the
- * second case, just retry from start.
- */
- GetVisibilityMapPins(relation, otherBuffer, buffer,
- otherBlock, targetBlock, vmbuffer_other,
- vmbuffer);
+ /*
+ * Because the buffers were unlocked for a while, it's possible,
+ * although unlikely, that an all-visible flag became set or that
+ * somebody used up the available space in the new page. We can use
+ * GetVisibilityMapPins to deal with the first case. In the second
+ * case, just retry from start.
+ */
+ GetVisibilityMapPins(relation, otherBuffer, buffer,
+ otherBlock, targetBlock, vmbuffer_other,
+ vmbuffer);
- if (len > PageGetHeapFreeSpace(page))
- {
- LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
- UnlockReleaseBuffer(buffer);
+ /*
+ * Note that we have to check the available space even if our
+ * conditional lock succeeded, because GetVisibilityMapPins might've
+ * transiently released lock on the target buffer to acquire a VM pin
+ * for the otherBuffer.
+ */
+ if (len > PageGetHeapFreeSpace(page))
+ {
+ LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
+ UnlockReleaseBuffer(buffer);
- goto loop;
- }
+ goto loop;
}
}
-
- if (len > PageGetHeapFreeSpace(page))
+ else if (len > PageGetHeapFreeSpace(page))
{
/* We should not get here given the test at the top */
elog(PANIC, "tuple is too big: size %zu", len);
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Avoid improbable PANIC during heap_update.
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Re: pgsql: Avoid improbable PANIC during heap_update. Jaime Casanova <jcasanov@systemguards.com.ec>
@ 2022-10-06 16:28 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 0 replies; 28+ messages in thread
From: Alvaro Herrera @ 2022-10-06 16:28 UTC (permalink / raw)
To: Jaime Casanova <jcasanov@systemguards.com.ec>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers@lists.postgresql.org
On 2022-Sep-29, Jaime Casanova wrote:
> This doesn't look as improbable because I saw it at least 3 times with
> v15beta4.
To further the case of the not-so-low-probability, we have customers
that are hitting this about once per day, with Postgres 14 ... so their
systems are crashing all the time :-( We've collected a bunch of
backtraces, and while I didn't analyze all of them, I hear that they all
look related to this fix.
Not good.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Maybe there's lots of data loss but the records of data loss are also lost.
(Lincoln Yeoh)
^ permalink raw reply [nested|flat] 28+ messages in thread
* pgsql: Avoid improbable PANIC during heap_update.
@ 2021-04-13 16:17 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 28+ messages in thread
From: Tom Lane @ 2021-04-13 16:17 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Avoid improbable PANIC during heap_update.
heap_update needs to clear any existing "all visible" flag on
the old tuple's page (and on the new page too, if different).
Per coding rules, to do this it must acquire pin on the appropriate
visibility-map page while not holding exclusive buffer lock;
which creates a race condition since someone else could set the
flag whenever we're not holding the buffer lock. The code is
supposed to handle that by re-checking the flag after acquiring
buffer lock and retrying if it became set. However, one code
path through heap_update itself, as well as one in its subroutine
RelationGetBufferForTuple, failed to do this. The end result,
in the unlikely event that a concurrent VACUUM did set the flag
while we're transiently not holding lock, is a non-recurring
"PANIC: wrong buffer passed to visibilitymap_clear" failure.
This has been seen a few times in the buildfarm since recent VACUUM
changes that added code paths that could set the all-visible flag
while holding only exclusive buffer lock. Previously, the flag
was (usually?) set only after doing LockBufferForCleanup, which
would insist on buffer pin count zero, thus preventing the flag
from becoming set partway through heap_update. However, it's
clear that it's heap_update not VACUUM that's at fault here.
What's less clear is whether there is any hazard from these bugs
in released branches. heap_update is certainly violating API
expectations, but if there is no code path that can set all-visible
without a cleanup lock then it's only a latent bug. That's not
100% certain though, besides which we should worry about extensions
or future back-patch fixes that could introduce such code paths.
I chose to back-patch to v12. Fixing RelationGetBufferForTuple
before that would require also back-patching portions of older
fixes (notably 0d1fe9f74), which is more code churn than seems
prudent to fix a hypothetical issue.
Discussion: https://postgr.es/m/2247102.1618008027@sss.pgh.pa.us
Branch
------
REL_13_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/37e76546a2ba4cdf1ca14b25b1e729aef7235870
Modified Files
--------------
src/backend/access/heap/heapam.c | 44 ++++++++++++++++++++++++----------------
src/backend/access/heap/hio.c | 24 ++++++++++++++++------
2 files changed, 45 insertions(+), 23 deletions(-)
^ permalink raw reply [nested|flat] 28+ messages in thread
* pgsql: Avoid improbable PANIC during heap_update.
@ 2021-04-13 16:17 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 28+ messages in thread
From: Tom Lane @ 2021-04-13 16:17 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Avoid improbable PANIC during heap_update.
heap_update needs to clear any existing "all visible" flag on
the old tuple's page (and on the new page too, if different).
Per coding rules, to do this it must acquire pin on the appropriate
visibility-map page while not holding exclusive buffer lock;
which creates a race condition since someone else could set the
flag whenever we're not holding the buffer lock. The code is
supposed to handle that by re-checking the flag after acquiring
buffer lock and retrying if it became set. However, one code
path through heap_update itself, as well as one in its subroutine
RelationGetBufferForTuple, failed to do this. The end result,
in the unlikely event that a concurrent VACUUM did set the flag
while we're transiently not holding lock, is a non-recurring
"PANIC: wrong buffer passed to visibilitymap_clear" failure.
This has been seen a few times in the buildfarm since recent VACUUM
changes that added code paths that could set the all-visible flag
while holding only exclusive buffer lock. Previously, the flag
was (usually?) set only after doing LockBufferForCleanup, which
would insist on buffer pin count zero, thus preventing the flag
from becoming set partway through heap_update. However, it's
clear that it's heap_update not VACUUM that's at fault here.
What's less clear is whether there is any hazard from these bugs
in released branches. heap_update is certainly violating API
expectations, but if there is no code path that can set all-visible
without a cleanup lock then it's only a latent bug. That's not
100% certain though, besides which we should worry about extensions
or future back-patch fixes that could introduce such code paths.
I chose to back-patch to v12. Fixing RelationGetBufferForTuple
before that would require also back-patching portions of older
fixes (notably 0d1fe9f74), which is more code churn than seems
prudent to fix a hypothetical issue.
Discussion: https://postgr.es/m/2247102.1618008027@sss.pgh.pa.us
Branch
------
REL_12_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/5f12bc94dcc60403bdfec6ea3d97aa616a512d9e
Modified Files
--------------
src/backend/access/heap/heapam.c | 44 ++++++++++++++++++++++++----------------
src/backend/access/heap/hio.c | 24 ++++++++++++++++------
2 files changed, 45 insertions(+), 23 deletions(-)
^ permalink raw reply [nested|flat] 28+ messages in thread
* pgsql: Avoid improbable PANIC during heap_update, redux.
@ 2022-09-30 23:37 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 28+ messages in thread
From: Tom Lane @ 2022-09-30 23:37 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Avoid improbable PANIC during heap_update, redux.
Commit 34f581c39 intended to ensure that RelationGetBufferForTuple
would acquire a visibility-map page pin in case the otherBuffer's
all-visible bit had become set since we last had lock on that page.
But I missed a case: when we're extending the relation, VM concerns
were dealt with only in the relatively-less-likely case that we
fail to conditionally lock the otherBuffer. I think I'd believed
that we couldn't need to worry about it if the conditional lock
succeeds, which is true for the target buffer; but the otherBuffer
was unlocked for awhile so its bit might be set anyway. So we need
to do the GetVisibilityMapPins dance, and then also recheck the
page's free space, in both cases.
Per report from Jaime Casanova. Back-patch to v12 as the previous
patch was (although there's still no evidence that the bug is
reachable pre-v14).
Discussion: https://postgr.es/m/E1lWLjP-00006Y-Ml@gemulon.postgresql.org
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/2dc2e4e31adb71502074c8c2bf9e0766347aa6e5
Modified Files
--------------
src/backend/access/heap/hio.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
^ permalink raw reply [nested|flat] 28+ messages in thread
* pgsql: Avoid improbable PANIC during heap_update, redux.
@ 2022-09-30 23:37 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 28+ messages in thread
From: Tom Lane @ 2022-09-30 23:37 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Avoid improbable PANIC during heap_update, redux.
Commit 34f581c39 intended to ensure that RelationGetBufferForTuple
would acquire a visibility-map page pin in case the otherBuffer's
all-visible bit had become set since we last had lock on that page.
But I missed a case: when we're extending the relation, VM concerns
were dealt with only in the relatively-less-likely case that we
fail to conditionally lock the otherBuffer. I think I'd believed
that we couldn't need to worry about it if the conditional lock
succeeds, which is true for the target buffer; but the otherBuffer
was unlocked for awhile so its bit might be set anyway. So we need
to do the GetVisibilityMapPins dance, and then also recheck the
page's free space, in both cases.
Per report from Jaime Casanova. Back-patch to v12 as the previous
patch was (although there's still no evidence that the bug is
reachable pre-v14).
Discussion: https://postgr.es/m/E1lWLjP-00006Y-Ml@gemulon.postgresql.org
Branch
------
REL_15_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/2267085c168899b43dadc4ca52fb516773068228
Modified Files
--------------
src/backend/access/heap/hio.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
^ permalink raw reply [nested|flat] 28+ messages in thread
* pgsql: Avoid improbable PANIC during heap_update, redux.
@ 2022-09-30 23:37 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 28+ messages in thread
From: Tom Lane @ 2022-09-30 23:37 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Avoid improbable PANIC during heap_update, redux.
Commit 34f581c39 intended to ensure that RelationGetBufferForTuple
would acquire a visibility-map page pin in case the otherBuffer's
all-visible bit had become set since we last had lock on that page.
But I missed a case: when we're extending the relation, VM concerns
were dealt with only in the relatively-less-likely case that we
fail to conditionally lock the otherBuffer. I think I'd believed
that we couldn't need to worry about it if the conditional lock
succeeds, which is true for the target buffer; but the otherBuffer
was unlocked for awhile so its bit might be set anyway. So we need
to do the GetVisibilityMapPins dance, and then also recheck the
page's free space, in both cases.
Per report from Jaime Casanova. Back-patch to v12 as the previous
patch was (although there's still no evidence that the bug is
reachable pre-v14).
Discussion: https://postgr.es/m/E1lWLjP-00006Y-Ml@gemulon.postgresql.org
Branch
------
REL_14_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/b93d7e6883d683ea35f4f5ea843222f47aacbaee
Modified Files
--------------
src/backend/access/heap/hio.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
^ permalink raw reply [nested|flat] 28+ messages in thread
* pgsql: Avoid improbable PANIC during heap_update, redux.
@ 2022-09-30 23:37 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 28+ messages in thread
From: Tom Lane @ 2022-09-30 23:37 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Avoid improbable PANIC during heap_update, redux.
Commit 34f581c39 intended to ensure that RelationGetBufferForTuple
would acquire a visibility-map page pin in case the otherBuffer's
all-visible bit had become set since we last had lock on that page.
But I missed a case: when we're extending the relation, VM concerns
were dealt with only in the relatively-less-likely case that we
fail to conditionally lock the otherBuffer. I think I'd believed
that we couldn't need to worry about it if the conditional lock
succeeds, which is true for the target buffer; but the otherBuffer
was unlocked for awhile so its bit might be set anyway. So we need
to do the GetVisibilityMapPins dance, and then also recheck the
page's free space, in both cases.
Per report from Jaime Casanova. Back-patch to v12 as the previous
patch was (although there's still no evidence that the bug is
reachable pre-v14).
Discussion: https://postgr.es/m/E1lWLjP-00006Y-Ml@gemulon.postgresql.org
Branch
------
REL_13_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/92941f26435ce3910bb9320fd92bda1e3dd4c6b5
Modified Files
--------------
src/backend/access/heap/hio.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
^ permalink raw reply [nested|flat] 28+ messages in thread
* pgsql: Avoid improbable PANIC during heap_update, redux.
@ 2022-09-30 23:37 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 28+ messages in thread
From: Tom Lane @ 2022-09-30 23:37 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Avoid improbable PANIC during heap_update, redux.
Commit 34f581c39 intended to ensure that RelationGetBufferForTuple
would acquire a visibility-map page pin in case the otherBuffer's
all-visible bit had become set since we last had lock on that page.
But I missed a case: when we're extending the relation, VM concerns
were dealt with only in the relatively-less-likely case that we
fail to conditionally lock the otherBuffer. I think I'd believed
that we couldn't need to worry about it if the conditional lock
succeeds, which is true for the target buffer; but the otherBuffer
was unlocked for awhile so its bit might be set anyway. So we need
to do the GetVisibilityMapPins dance, and then also recheck the
page's free space, in both cases.
Per report from Jaime Casanova. Back-patch to v12 as the previous
patch was (although there's still no evidence that the bug is
reachable pre-v14).
Discussion: https://postgr.es/m/E1lWLjP-00006Y-Ml@gemulon.postgresql.org
Branch
------
REL_12_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/e7bd2d6712495eba037bae0fdbe1bf413110ebe6
Modified Files
--------------
src/backend/access/heap/hio.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
^ permalink raw reply [nested|flat] 28+ messages in thread
end of thread, other threads:[~2022-10-06 16:28 UTC | newest]
Thread overview: 28+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-29 07:55 ` Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:44 ` Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 20:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-03 19:34 ` Jaime Casanova <jcasanov@systemguards.com.ec>
2022-09-30 21:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 21:39 ` Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:56 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 22:03 ` Peter Geoghegan <pg@bowt.ie>
2022-09-30 23:52 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 00:09 ` Peter Geoghegan <pg@bowt.ie>
2022-10-01 01:29 ` Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:23 ` Peter Geoghegan <pg@bowt.ie>
2022-10-01 04:38 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 05:09 ` Peter Geoghegan <pg@bowt.ie>
2022-10-01 05:13 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-01 05:25 ` Peter Geoghegan <pg@bowt.ie>
2022-10-01 16:35 ` Jeff Davis <pgsql@j-davis.com>
2022-10-01 16:53 ` Peter Geoghegan <pg@bowt.ie>
2022-09-30 21:52 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-06 16:28 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2021-04-13 16:17 pgsql: Avoid improbable PANIC during heap_update. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 23:37 pgsql: Avoid improbable PANIC during heap_update, redux. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 23:37 pgsql: Avoid improbable PANIC during heap_update, redux. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 23:37 pgsql: Avoid improbable PANIC during heap_update, redux. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 23:37 pgsql: Avoid improbable PANIC during heap_update, redux. Tom Lane <tgl@sss.pgh.pa.us>
2022-09-30 23:37 pgsql: Avoid improbable PANIC during heap_update, redux. Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox