Thread: amcheck/verify_heapam doesn't check for interrupts
I just noticed that the new heapam amcheck verification code can take a very long time to respond to cancellations from pg_amcheck -- I saw that it took over 2 minutes on a large database on my workstation. It looks like we neglect to call CHECK_FOR_INTERRUPTS() anywhere inside verify_heapam.c. Is there any reason for this? Can't we just put a CHECK_FOR_INTERRUPTS() at the top of the outermost loop, inside verify_heapam()? Not sure if pg_amcheck itself is a factor here too -- didn't get that far. Thanks -- Peter Geoghegan
> On Aug 26, 2021, at 2:38 PM, Peter Geoghegan <pg@bowt.ie> wrote: > > It looks like we neglect to call CHECK_FOR_INTERRUPTS() anywhere > inside verify_heapam.c. Is there any reason for this? Not any good one that I can see. > Can't we just > put a CHECK_FOR_INTERRUPTS() at the top of the outermost loop, inside > verify_heapam()? I expect we could. > Not sure if pg_amcheck itself is a factor here too -- didn't get that far. That runs an event loop in the client over multiple checks (heap and/or btree) running in backends, just as reindexdb andvacuumdb do over parallel reindexes and vacuums running in backends. It should be just as safe to ctrl-c out of pg_amcheckas out of those two. They all three use fe_utils/cancel.h's setup_cancel_handler(), so I would expect modifyingverify_heapam would be enough. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Aug 26, 2021 at 4:24 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote: > > On Aug 26, 2021, at 2:38 PM, Peter Geoghegan <pg@bowt.ie> wrote: > > It looks like we neglect to call CHECK_FOR_INTERRUPTS() anywhere > > inside verify_heapam.c. Is there any reason for this? > > Not any good one that I can see. Seems that way. Want to post a patch? > > Not sure if pg_amcheck itself is a factor here too -- didn't get that far. > > That runs an event loop in the client over multiple checks (heap and/or btree) running in backends, just as reindexdb andvacuumdb do over parallel reindexes and vacuums running in backends. It should be just as safe to ctrl-c out of pg_amcheckas out of those two. They all three use fe_utils/cancel.h's setup_cancel_handler(), so I would expect modifyingverify_heapam would be enough. Right. I checked that out myself, after sending my email from earlier. We don't have any problems when pg_amcheck happens to be verifying a B-Tree index -- verify_nbtree.c already has CHECK_FOR_INTERRUPTS() at a few key points. -- Peter Geoghegan
> On Aug 26, 2021, at 4:39 PM, Peter Geoghegan <pg@bowt.ie> wrote: > >> Not any good one that I can see. > > Seems that way. Want to post a patch? Sure. I just posted another unrelated patch for amcheck this morning, so it seems a good day for it :) — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> On Aug 26, 2021, at 4:41 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote: > >> Seems that way. Want to post a patch? > > Sure. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
Patch committed. Thanks! -- Peter Geoghegan