From 495112ca504f96a1d508ecfac6d70066d326ee53 Mon Sep 17 00:00:00 2001 From: Konstantin Knizhnik Date: Thu, 3 Jul 2025 20:37:05 +0300 Subject: [PATCH] Add GUC for dynamically enable compare local mode (#12424) ## Problem DEBUG_LOCAL_COMPARE mode allows to detect data corruption. But it requires rebuild of neon extension (and so requires special image) and significantly slowdown execution because always fetch pages from page server. ## Summary of changes Introduce new GUC `neon.debug_compare_local`, accepting the following values: " none", "prefetch", "lfc", "all" (by default it is definitely disabled). In mode less than "all", neon SMGR will not fetch page from PS if it is found in local caches. Co-authored-by: Konstantin Knizhnik --- pgxn/neon/neon.c | 18 +++ pgxn/neon/pagestore_client.h | 16 +++ pgxn/neon/pagestore_smgr.c | 249 ++++++++++++++++++----------------- 3 files changed, 163 insertions(+), 120 deletions(-) diff --git a/pgxn/neon/neon.c b/pgxn/neon/neon.c index 8a405f4129..3b2a4d3f2f 100644 --- a/pgxn/neon/neon.c +++ b/pgxn/neon/neon.c @@ -87,6 +87,14 @@ static const struct config_enum_entry running_xacts_overflow_policies[] = { {NULL, 0, false} }; +static const struct config_enum_entry debug_compare_local_modes[] = { + {"none", DEBUG_COMPARE_LOCAL_NONE, false}, + {"prefetch", DEBUG_COMPARE_LOCAL_PREFETCH, false}, + {"lfc", DEBUG_COMPARE_LOCAL_LFC, false}, + {"all", DEBUG_COMPARE_LOCAL_ALL, false}, + {NULL, 0, false} +}; + /* * XXX: These private to procarray.c, but we need them here. */ @@ -519,6 +527,16 @@ _PG_init(void) GUC_UNIT_KB, NULL, NULL, NULL); + DefineCustomEnumVariable( + "neon.debug_compare_local", + "Debug mode for compaing content of pages in prefetch ring/LFC/PS and local disk", + NULL, + &debug_compare_local, + DEBUG_COMPARE_LOCAL_NONE, + debug_compare_local_modes, + PGC_POSTMASTER, + 0, + NULL, NULL, NULL); /* * Important: This must happen after other parts of the extension are * loaded, otherwise any settings to GUCs that were set before the diff --git a/pgxn/neon/pagestore_client.h b/pgxn/neon/pagestore_client.h index 9df202290d..4470d3a94d 100644 --- a/pgxn/neon/pagestore_client.h +++ b/pgxn/neon/pagestore_client.h @@ -177,6 +177,22 @@ extern StringInfoData nm_pack_request(NeonRequest *msg); extern NeonResponse *nm_unpack_response(StringInfo s); extern char *nm_to_string(NeonMessage *msg); +/* + * If debug_compare_local>DEBUG_COMPARE_LOCAL_NONE, we pass through all the SMGR API + * calls to md.c, and *also* do the calls to the Page Server. On every + * read, compare the versions we read from local disk and Page Server, + * and Assert that they are identical. + */ +typedef enum +{ + DEBUG_COMPARE_LOCAL_NONE, /* normal mode - pages are storted locally only for unlogged relations */ + DEBUG_COMPARE_LOCAL_PREFETCH, /* if page is found in prefetch ring, then compare it with local and return */ + DEBUG_COMPARE_LOCAL_LFC, /* if page is found in LFC or prefetch ring, then compare it with local and return */ + DEBUG_COMPARE_LOCAL_ALL /* always fetch page from PS and compare it with local */ +} DebugCompareLocalMode; + +extern int debug_compare_local; + /* * API */ diff --git a/pgxn/neon/pagestore_smgr.c b/pgxn/neon/pagestore_smgr.c index 43fd715bbb..9d25266e10 100644 --- a/pgxn/neon/pagestore_smgr.c +++ b/pgxn/neon/pagestore_smgr.c @@ -76,21 +76,11 @@ typedef PGAlignedBlock PGIOAlignedBlock; #endif -/* - * If DEBUG_COMPARE_LOCAL is defined, we pass through all the SMGR API - * calls to md.c, and *also* do the calls to the Page Server. On every - * read, compare the versions we read from local disk and Page Server, - * and Assert that they are identical. - */ -/* #define DEBUG_COMPARE_LOCAL */ - -#ifdef DEBUG_COMPARE_LOCAL #include "access/nbtree.h" #include "storage/bufpage.h" #include "access/xlog_internal.h" static char *hexdump_page(char *page); -#endif #define IS_LOCAL_REL(reln) (\ NInfoGetDbOid(InfoFromSMgrRel(reln)) != 0 && \ @@ -108,6 +98,8 @@ typedef enum UNLOGGED_BUILD_NOT_PERMANENT } UnloggedBuildPhase; +int debug_compare_local; + static NRelFileInfo unlogged_build_rel_info; static UnloggedBuildPhase unlogged_build_phase = UNLOGGED_BUILD_NOT_IN_PROGRESS; @@ -478,9 +470,10 @@ neon_init(void) old_redo_read_buffer_filter = redo_read_buffer_filter; redo_read_buffer_filter = neon_redo_read_buffer_filter; -#ifdef DEBUG_COMPARE_LOCAL - mdinit(); -#endif + if (debug_compare_local) + { + mdinit(); + } } /* @@ -803,13 +796,16 @@ neon_create(SMgrRelation reln, ForkNumber forkNum, bool isRedo) case RELPERSISTENCE_TEMP: case RELPERSISTENCE_UNLOGGED: -#ifdef DEBUG_COMPARE_LOCAL - mdcreate(reln, forkNum, forkNum == INIT_FORKNUM || isRedo); - if (forkNum == MAIN_FORKNUM) - mdcreate(reln, INIT_FORKNUM, true); -#else - mdcreate(reln, forkNum, isRedo); -#endif + if (debug_compare_local) + { + mdcreate(reln, forkNum, forkNum == INIT_FORKNUM || isRedo); + if (forkNum == MAIN_FORKNUM) + mdcreate(reln, INIT_FORKNUM, true); + } + else + { + mdcreate(reln, forkNum, isRedo); + } return; default: @@ -848,10 +844,11 @@ neon_create(SMgrRelation reln, ForkNumber forkNum, bool isRedo) else set_cached_relsize(InfoFromSMgrRel(reln), forkNum, 0); -#ifdef DEBUG_COMPARE_LOCAL - if (IS_LOCAL_REL(reln)) - mdcreate(reln, forkNum, isRedo); -#endif + if (debug_compare_local) + { + if (IS_LOCAL_REL(reln)) + mdcreate(reln, forkNum, isRedo); + } } /* @@ -877,7 +874,7 @@ neon_unlink(NRelFileInfoBackend rinfo, ForkNumber forkNum, bool isRedo) { /* * Might or might not exist locally, depending on whether it's an unlogged - * or permanent relation (or if DEBUG_COMPARE_LOCAL is set). Try to + * or permanent relation (or if debug_compare_local is set). Try to * unlink, it won't do any harm if the file doesn't exist. */ mdunlink(rinfo, forkNum, isRedo); @@ -973,10 +970,11 @@ neon_extend(SMgrRelation reln, ForkNumber forkNum, BlockNumber blkno, lfc_write(InfoFromSMgrRel(reln), forkNum, blkno, buffer); -#ifdef DEBUG_COMPARE_LOCAL - if (IS_LOCAL_REL(reln)) - mdextend(reln, forkNum, blkno, buffer, skipFsync); -#endif + if (debug_compare_local) + { + if (IS_LOCAL_REL(reln)) + mdextend(reln, forkNum, blkno, buffer, skipFsync); + } /* * smgr_extend is often called with an all-zeroes page, so @@ -1051,10 +1049,11 @@ neon_zeroextend(SMgrRelation reln, ForkNumber forkNum, BlockNumber blocknum, relpath(reln->smgr_rlocator, forkNum), InvalidBlockNumber))); -#ifdef DEBUG_COMPARE_LOCAL - if (IS_LOCAL_REL(reln)) - mdzeroextend(reln, forkNum, blocknum, nblocks, skipFsync); -#endif + if (debug_compare_local) + { + if (IS_LOCAL_REL(reln)) + mdzeroextend(reln, forkNum, blocknum, nblocks, skipFsync); + } /* Don't log any pages if we're not allowed to do so. */ if (!XLogInsertAllowed()) @@ -1265,10 +1264,11 @@ neon_writeback(SMgrRelation reln, ForkNumber forknum, communicator_prefetch_pump_state(); -#ifdef DEBUG_COMPARE_LOCAL - if (IS_LOCAL_REL(reln)) - mdwriteback(reln, forknum, blocknum, nblocks); -#endif + if (debug_compare_local) + { + if (IS_LOCAL_REL(reln)) + mdwriteback(reln, forknum, blocknum, nblocks); + } } /* @@ -1282,7 +1282,6 @@ neon_read_at_lsn(NRelFileInfo rinfo, ForkNumber forkNum, BlockNumber blkno, communicator_read_at_lsnv(rinfo, forkNum, blkno, &request_lsns, &buffer, 1, NULL); } -#ifdef DEBUG_COMPARE_LOCAL static void compare_with_local(SMgrRelation reln, ForkNumber forkNum, BlockNumber blkno, void* buffer, XLogRecPtr request_lsn) { @@ -1364,7 +1363,6 @@ compare_with_local(SMgrRelation reln, ForkNumber forkNum, BlockNumber blkno, voi } } } -#endif #if PG_MAJORVERSION_NUM < 17 @@ -1417,22 +1415,28 @@ neon_read(SMgrRelation reln, ForkNumber forkNum, BlockNumber blkno, void *buffer if (communicator_prefetch_lookupv(InfoFromSMgrRel(reln), forkNum, blkno, &request_lsns, 1, &bufferp, &present)) { /* Prefetch hit */ -#ifdef DEBUG_COMPARE_LOCAL - compare_with_local(reln, forkNum, blkno, buffer, request_lsns.request_lsn); -#else - return; -#endif + if (debug_compare_local >= DEBUG_COMPARE_LOCAL_PREFETCH) + { + compare_with_local(reln, forkNum, blkno, buffer, request_lsns.request_lsn); + } + if (debug_compare_local <= DEBUG_COMPARE_LOCAL_PREFETCH) + { + return; + } } /* Try to read from local file cache */ if (lfc_read(InfoFromSMgrRel(reln), forkNum, blkno, buffer)) { MyNeonCounters->file_cache_hits_total++; -#ifdef DEBUG_COMPARE_LOCAL - compare_with_local(reln, forkNum, blkno, buffer, request_lsns.request_lsn); -#else - return; -#endif + if (debug_compare_local >= DEBUG_COMPARE_LOCAL_LFC) + { + compare_with_local(reln, forkNum, blkno, buffer, request_lsns.request_lsn); + } + if (debug_compare_local <= DEBUG_COMPARE_LOCAL_LFC) + { + return; + } } neon_read_at_lsn(InfoFromSMgrRel(reln), forkNum, blkno, request_lsns, buffer); @@ -1442,15 +1446,15 @@ neon_read(SMgrRelation reln, ForkNumber forkNum, BlockNumber blkno, void *buffer */ communicator_prefetch_pump_state(); -#ifdef DEBUG_COMPARE_LOCAL - compare_with_local(reln, forkNum, blkno, buffer, request_lsns.request_lsn); -#endif + if (debug_compare_local) + { + compare_with_local(reln, forkNum, blkno, buffer, request_lsns.request_lsn); + } } #endif /* PG_MAJORVERSION_NUM <= 16 */ #if PG_MAJORVERSION_NUM >= 17 -#ifdef DEBUG_COMPARE_LOCAL static void compare_with_localv(SMgrRelation reln, ForkNumber forkNum, BlockNumber blkno, void** buffers, BlockNumber nblocks, neon_request_lsns* request_lsns, bits8* read_pages) { @@ -1465,7 +1469,6 @@ compare_with_localv(SMgrRelation reln, ForkNumber forkNum, BlockNumber blkno, vo } } } -#endif static void @@ -1516,13 +1519,19 @@ neon_readv(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, blocknum, request_lsns, nblocks, buffers, read_pages); -#ifdef DEBUG_COMPARE_LOCAL - compare_with_localv(reln, forknum, blocknum, buffers, nblocks, request_lsns, read_pages); - memset(read_pages, 0, sizeof(read_pages)); -#else - if (prefetch_result == nblocks) + if (debug_compare_local >= DEBUG_COMPARE_LOCAL_PREFETCH) + { + compare_with_localv(reln, forknum, blocknum, buffers, nblocks, request_lsns, read_pages); + } + if (debug_compare_local <= DEBUG_COMPARE_LOCAL_PREFETCH && prefetch_result == nblocks) + { return; -#endif + } + if (debug_compare_local > DEBUG_COMPARE_LOCAL_PREFETCH) + { + memset(read_pages, 0, sizeof(read_pages)); + } + /* Try to read from local file cache */ lfc_result = lfc_readv_select(InfoFromSMgrRel(reln), forknum, blocknum, buffers, @@ -1531,14 +1540,19 @@ neon_readv(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, if (lfc_result > 0) MyNeonCounters->file_cache_hits_total += lfc_result; -#ifdef DEBUG_COMPARE_LOCAL - compare_with_localv(reln, forknum, blocknum, buffers, nblocks, request_lsns, read_pages); - memset(read_pages, 0, sizeof(read_pages)); -#else - /* Read all blocks from LFC, so we're done */ - if (prefetch_result + lfc_result == nblocks) + if (debug_compare_local >= DEBUG_COMPARE_LOCAL_LFC) + { + compare_with_localv(reln, forknum, blocknum, buffers, nblocks, request_lsns, read_pages); + } + if (debug_compare_local <= DEBUG_COMPARE_LOCAL_LFC && prefetch_result + lfc_result == nblocks) + { + /* Read all blocks from LFC, so we're done */ return; -#endif + } + if (debug_compare_local > DEBUG_COMPARE_LOCAL_LFC) + { + memset(read_pages, 0, sizeof(read_pages)); + } communicator_read_at_lsnv(InfoFromSMgrRel(reln), forknum, blocknum, request_lsns, buffers, nblocks, read_pages); @@ -1548,14 +1562,14 @@ neon_readv(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, */ communicator_prefetch_pump_state(); -#ifdef DEBUG_COMPARE_LOCAL - memset(read_pages, 0xFF, sizeof(read_pages)); - compare_with_localv(reln, forknum, blocknum, buffers, nblocks, request_lsns, read_pages); -#endif + if (debug_compare_local) + { + memset(read_pages, 0xFF, sizeof(read_pages)); + compare_with_localv(reln, forknum, blocknum, buffers, nblocks, request_lsns, read_pages); + } } #endif -#ifdef DEBUG_COMPARE_LOCAL static char * hexdump_page(char *page) { @@ -1574,7 +1588,6 @@ hexdump_page(char *page) return result.data; } -#endif #if PG_MAJORVERSION_NUM < 17 /* @@ -1596,12 +1609,8 @@ neon_write(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, const vo switch (reln->smgr_relpersistence) { case 0: -#ifndef DEBUG_COMPARE_LOCAL /* This is a bit tricky. Check if the relation exists locally */ - if (mdexists(reln, forknum)) -#else - if (mdexists(reln, INIT_FORKNUM)) -#endif + if (mdexists(reln, debug_compare_local ? INIT_FORKNUM : forknum)) { /* It exists locally. Guess it's unlogged then. */ #if PG_MAJORVERSION_NUM >= 17 @@ -1656,14 +1665,17 @@ neon_write(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, const vo communicator_prefetch_pump_state(); -#ifdef DEBUG_COMPARE_LOCAL - if (IS_LOCAL_REL(reln)) + if (debug_compare_local) + { + if (IS_LOCAL_REL(reln)) + { #if PG_MAJORVERSION_NUM >= 17 - mdwritev(reln, forknum, blocknum, &buffer, 1, skipFsync); + mdwritev(reln, forknum, blocknum, &buffer, 1, skipFsync); #else - mdwrite(reln, forknum, blocknum, buffer, skipFsync); + mdwrite(reln, forknum, blocknum, buffer, skipFsync); #endif -#endif + } + } } #endif @@ -1677,12 +1689,8 @@ neon_writev(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno, switch (reln->smgr_relpersistence) { case 0: -#ifndef DEBUG_COMPARE_LOCAL /* This is a bit tricky. Check if the relation exists locally */ - if (mdexists(reln, forknum)) -#else - if (mdexists(reln, INIT_FORKNUM)) -#endif + if (mdexists(reln, debug_compare_local ? INIT_FORKNUM : forknum)) { /* It exists locally. Guess it's unlogged then. */ mdwritev(reln, forknum, blkno, buffers, nblocks, skipFsync); @@ -1720,10 +1728,11 @@ neon_writev(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno, communicator_prefetch_pump_state(); -#ifdef DEBUG_COMPARE_LOCAL - if (IS_LOCAL_REL(reln)) - mdwritev(reln, forknum, blkno, buffers, nblocks, skipFsync); -#endif + if (debug_compare_local) + { + if (IS_LOCAL_REL(reln)) + mdwritev(reln, forknum, blkno, buffers, nblocks, skipFsync); + } } #endif @@ -1862,10 +1871,11 @@ neon_truncate(SMgrRelation reln, ForkNumber forknum, BlockNumber old_blocks, Blo */ neon_set_lwlsn_relation(lsn, InfoFromSMgrRel(reln), forknum); -#ifdef DEBUG_COMPARE_LOCAL - if (IS_LOCAL_REL(reln)) - mdtruncate(reln, forknum, old_blocks, nblocks); -#endif + if (debug_compare_local) + { + if (IS_LOCAL_REL(reln)) + mdtruncate(reln, forknum, old_blocks, nblocks); + } } /* @@ -1904,10 +1914,11 @@ neon_immedsync(SMgrRelation reln, ForkNumber forknum) communicator_prefetch_pump_state(); -#ifdef DEBUG_COMPARE_LOCAL - if (IS_LOCAL_REL(reln)) - mdimmedsync(reln, forknum); -#endif + if (debug_compare_local) + { + if (IS_LOCAL_REL(reln)) + mdimmedsync(reln, forknum); + } } #if PG_MAJORVERSION_NUM >= 17 @@ -1934,10 +1945,11 @@ neon_registersync(SMgrRelation reln, ForkNumber forknum) neon_log(SmgrTrace, "[NEON_SMGR] registersync noop"); -#ifdef DEBUG_COMPARE_LOCAL - if (IS_LOCAL_REL(reln)) - mdimmedsync(reln, forknum); -#endif + if (debug_compare_local) + { + if (IS_LOCAL_REL(reln)) + mdimmedsync(reln, forknum); + } } #endif @@ -1978,10 +1990,11 @@ neon_start_unlogged_build(SMgrRelation reln) case RELPERSISTENCE_UNLOGGED: unlogged_build_rel_info = InfoFromSMgrRel(reln); unlogged_build_phase = UNLOGGED_BUILD_NOT_PERMANENT; -#ifdef DEBUG_COMPARE_LOCAL - if (!IsParallelWorker()) - mdcreate(reln, INIT_FORKNUM, true); -#endif + if (debug_compare_local) + { + if (!IsParallelWorker()) + mdcreate(reln, INIT_FORKNUM, true); + } return; default: @@ -2009,11 +2022,7 @@ neon_start_unlogged_build(SMgrRelation reln) */ if (!IsParallelWorker()) { -#ifndef DEBUG_COMPARE_LOCAL - mdcreate(reln, MAIN_FORKNUM, false); -#else - mdcreate(reln, INIT_FORKNUM, true); -#endif + mdcreate(reln, debug_compare_local ? INIT_FORKNUM : MAIN_FORKNUM, false); } } @@ -2107,14 +2116,14 @@ neon_end_unlogged_build(SMgrRelation reln) lfc_invalidate(InfoFromNInfoB(rinfob), forknum, nblocks); mdclose(reln, forknum); -#ifndef DEBUG_COMPARE_LOCAL - /* use isRedo == true, so that we drop it immediately */ - mdunlink(rinfob, forknum, true); -#endif + if (!debug_compare_local) + { + /* use isRedo == true, so that we drop it immediately */ + mdunlink(rinfob, forknum, true); + } } -#ifdef DEBUG_COMPARE_LOCAL - mdunlink(rinfob, INIT_FORKNUM, true); -#endif + if (debug_compare_local) + mdunlink(rinfob, INIT_FORKNUM, true); } NRelFileInfoInvalidate(unlogged_build_rel_info); unlogged_build_phase = UNLOGGED_BUILD_NOT_IN_PROGRESS;