From e1d5c9e7fdb2158c763e321237a797bc30787f0e Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev Date: Wed, 30 Mar 2022 15:17:10 +0300 Subject: [PATCH v1] Unit tests for SLRU This patch adds unit tests for SLRU. While on it, also change the interface a bit. Firstly, move Asserts() outside of SimpleLruInit(). Secondly, rename SlruSyncFileTag() to SlruSyncSegment() and change it's signature. There is no reason why SLRU should know anything about FileTags. Also: rename SimpleLruWritePage to SimpleLruWriteSlot? FIXME Author: Aleksander Alekxeev Reviewed-by: FIXME Discussion: FIXME --- src/backend/access/transam/clog.c | 7 +- src/backend/access/transam/commit_ts.c | 5 +- src/backend/access/transam/multixact.c | 10 ++- src/backend/access/transam/slru.c | 19 +++-- src/backend/access/transam/subtrans.c | 5 +- src/backend/commands/async.c | 5 +- src/backend/storage/lmgr/predicate.c | 4 +- src/include/access/slru.h | 4 +- src/test/modules/Makefile | 1 + src/test/modules/test_slru/.gitignore | 4 + src/test/modules/test_slru/Makefile | 23 +++++ .../modules/test_slru/expected/test_slru.out | 31 +++++++ src/test/modules/test_slru/sql/test_slru.sql | 7 ++ src/test/modules/test_slru/test_slru--1.0.sql | 5 ++ src/test/modules/test_slru/test_slru.c | 85 +++++++++++++++++++ src/test/modules/test_slru/test_slru.control | 4 + 16 files changed, 196 insertions(+), 23 deletions(-) create mode 100644 src/test/modules/test_slru/.gitignore create mode 100644 src/test/modules/test_slru/Makefile create mode 100644 src/test/modules/test_slru/expected/test_slru.out create mode 100644 src/test/modules/test_slru/sql/test_slru.sql create mode 100644 src/test/modules/test_slru/test_slru--1.0.sql create mode 100644 src/test/modules/test_slru/test_slru.c create mode 100644 src/test/modules/test_slru/test_slru.control diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c index 3d9088a704..23be3e4d6d 100644 --- a/src/backend/access/transam/clog.c +++ b/src/backend/access/transam/clog.c @@ -696,10 +696,13 @@ CLOGShmemSize(void) void CLOGShmemInit(void) { + bool found; + XactCtl->PagePrecedes = CLOGPagePrecedes; - SimpleLruInit(XactCtl, "Xact", CLOGShmemBuffers(), CLOG_LSNS_PER_PAGE, + found = SimpleLruInit(XactCtl, "Xact", CLOGShmemBuffers(), CLOG_LSNS_PER_PAGE, XactSLRULock, "pg_xact", LWTRANCHE_XACT_BUFFER, SYNC_HANDLER_CLOG); + Assert(found == IsUnderPostmaster); SlruPagePrecedesUnitTests(XactCtl, CLOG_XACTS_PER_PAGE); } @@ -1026,5 +1029,5 @@ clog_redo(XLogReaderState *record) int clogsyncfiletag(const FileTag *ftag, char *path) { - return SlruSyncFileTag(XactCtl, ftag, path); + return SlruSyncSegment(XactCtl, ftag->segno, path); } diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c index 20950eb1e4..bb33583e3a 100644 --- a/src/backend/access/transam/commit_ts.c +++ b/src/backend/access/transam/commit_ts.c @@ -541,10 +541,11 @@ CommitTsShmemInit(void) bool found; CommitTsCtl->PagePrecedes = CommitTsPagePrecedes; - SimpleLruInit(CommitTsCtl, "CommitTs", CommitTsShmemBuffers(), 0, + found = SimpleLruInit(CommitTsCtl, "CommitTs", CommitTsShmemBuffers(), 0, CommitTsSLRULock, "pg_commit_ts", LWTRANCHE_COMMITTS_BUFFER, SYNC_HANDLER_COMMIT_TS); + Assert(found == IsUnderPostmaster); SlruPagePrecedesUnitTests(CommitTsCtl, COMMIT_TS_XACTS_PER_PAGE); commitTsShared = ShmemInitStruct("CommitTs shared", @@ -1031,5 +1032,5 @@ commit_ts_redo(XLogReaderState *record) int committssyncfiletag(const FileTag *ftag, char *path) { - return SlruSyncFileTag(CommitTsCtl, ftag, path); + return SlruSyncSegment(CommitTsCtl, ftag->segno, path); } diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index 9f65c600d0..d79e6f2d65 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1862,17 +1862,19 @@ MultiXactShmemInit(void) MultiXactOffsetCtl->PagePrecedes = MultiXactOffsetPagePrecedes; MultiXactMemberCtl->PagePrecedes = MultiXactMemberPagePrecedes; - SimpleLruInit(MultiXactOffsetCtl, + found = SimpleLruInit(MultiXactOffsetCtl, "MultiXactOffset", NUM_MULTIXACTOFFSET_BUFFERS, 0, MultiXactOffsetSLRULock, "pg_multixact/offsets", LWTRANCHE_MULTIXACTOFFSET_BUFFER, SYNC_HANDLER_MULTIXACT_OFFSET); + Assert(found == IsUnderPostmaster); SlruPagePrecedesUnitTests(MultiXactOffsetCtl, MULTIXACT_OFFSETS_PER_PAGE); - SimpleLruInit(MultiXactMemberCtl, + found = SimpleLruInit(MultiXactMemberCtl, "MultiXactMember", NUM_MULTIXACTMEMBER_BUFFERS, 0, MultiXactMemberSLRULock, "pg_multixact/members", LWTRANCHE_MULTIXACTMEMBER_BUFFER, SYNC_HANDLER_MULTIXACT_MEMBER); + Assert(found == IsUnderPostmaster); /* doesn't call SimpleLruTruncate() or meet criteria for unit tests */ /* Initialize our shared state struct */ @@ -3427,7 +3429,7 @@ pg_get_multixact_members(PG_FUNCTION_ARGS) int multixactoffsetssyncfiletag(const FileTag *ftag, char *path) { - return SlruSyncFileTag(MultiXactOffsetCtl, ftag, path); + return SlruSyncSegment(MultiXactOffsetCtl, ftag->segno, path); } /* @@ -3436,5 +3438,5 @@ multixactoffsetssyncfiletag(const FileTag *ftag, char *path) int multixactmemberssyncfiletag(const FileTag *ftag, char *path) { - return SlruSyncFileTag(MultiXactMemberCtl, ftag, path); + return SlruSyncSegment(MultiXactMemberCtl, ftag->segno, path); } diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c index 30a476ed5d..c397141c9d 100644 --- a/src/backend/access/transam/slru.c +++ b/src/backend/access/transam/slru.c @@ -182,8 +182,10 @@ SimpleLruShmemSize(int nslots, int nlsns) * ctllock: LWLock to use to control access to the shared control structure. * subdir: PGDATA-relative subdirectory that will contain the files. * tranche_id: LWLock tranche ID to use for the SLRU's per-buffer LWLocks. + * + * Returns false if the cache didn't exist before the call, true otherwise. */ -void +bool SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns, LWLock *ctllock, const char *subdir, int tranche_id, SyncRequestHandler sync_handler) @@ -195,15 +197,13 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns, SimpleLruShmemSize(nslots, nlsns), &found); - if (!IsUnderPostmaster) + if (!found) { /* Initialize locks and shared memory area */ char *ptr; Size offset; int slotno; - Assert(!found); - memset(shared, 0, sizeof(SlruSharedData)); shared->ControlLock = ctllock; @@ -256,8 +256,6 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns, /* Should fit to estimated shmem size */ Assert(ptr - (char *) shared <= SimpleLruShmemSize(nslots, nlsns)); } - else - Assert(found); /* * Initialize the unshared control struct, including directory path. We @@ -266,6 +264,8 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns, ctl->shared = shared; ctl->sync_handler = sync_handler; strlcpy(ctl->Dir, subdir, sizeof(ctl->Dir)); + + return found; } /* @@ -608,7 +608,8 @@ SlruInternalWritePage(SlruCtl ctl, int slotno, SlruWriteAll fdata) /* * Wrapper of SlruInternalWritePage, for external callers. - * fdata is always passed a NULL here. + * + * Control lock must be held at entry, and will be held at exit. */ void SimpleLruWritePage(SlruCtl ctl, int slotno) @@ -1590,13 +1591,13 @@ SlruScanDirectory(SlruCtl ctl, SlruScanCallback callback, void *data) * performs the fsync. */ int -SlruSyncFileTag(SlruCtl ctl, const FileTag *ftag, char *path) +SlruSyncSegment(SlruCtl ctl, int segno, char *path) { int fd; int save_errno; int result; - SlruFileName(ctl, path, ftag->segno); + SlruFileName(ctl, path, segno); fd = OpenTransientFile(path, O_RDWR | PG_BINARY); if (fd < 0) diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c index 66d3548155..61ad1a8e43 100644 --- a/src/backend/access/transam/subtrans.c +++ b/src/backend/access/transam/subtrans.c @@ -31,6 +31,7 @@ #include "access/slru.h" #include "access/subtrans.h" #include "access/transam.h" +#include "miscadmin.h" #include "pg_trace.h" #include "utils/snapmgr.h" @@ -190,10 +191,12 @@ SUBTRANSShmemSize(void) void SUBTRANSShmemInit(void) { + bool found; SubTransCtl->PagePrecedes = SubTransPagePrecedes; - SimpleLruInit(SubTransCtl, "Subtrans", NUM_SUBTRANS_BUFFERS, 0, + found = SimpleLruInit(SubTransCtl, "Subtrans", NUM_SUBTRANS_BUFFERS, 0, SubtransSLRULock, "pg_subtrans", LWTRANCHE_SUBTRANS_BUFFER, SYNC_HANDLER_NONE); + Assert(found == IsUnderPostmaster); SlruPagePrecedesUnitTests(SubTransCtl, SUBTRANS_XACTS_PER_PAGE); } diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c index 455d895a44..27dc3b46d7 100644 --- a/src/backend/commands/async.c +++ b/src/backend/commands/async.c @@ -532,7 +532,7 @@ AsyncShmemSize(void) void AsyncShmemInit(void) { - bool found; + bool found, slru_found; Size size; int max_backends = GetMaxBackends(); @@ -570,9 +570,10 @@ AsyncShmemInit(void) * Set up SLRU management of the pg_notify data. */ NotifyCtl->PagePrecedes = asyncQueuePagePrecedes; - SimpleLruInit(NotifyCtl, "Notify", NUM_NOTIFY_BUFFERS, 0, + slru_found = SimpleLruInit(NotifyCtl, "Notify", NUM_NOTIFY_BUFFERS, 0, NotifySLRULock, "pg_notify", LWTRANCHE_NOTIFY_BUFFER, SYNC_HANDLER_NONE); + Assert(slru_found == IsUnderPostmaster); if (!found) { diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c index e337aad5b2..74c4facd9b 100644 --- a/src/backend/storage/lmgr/predicate.c +++ b/src/backend/storage/lmgr/predicate.c @@ -871,9 +871,11 @@ SerialInit(void) * Set up SLRU management of the pg_serial data. */ SerialSlruCtl->PagePrecedes = SerialPagePrecedesLogically; - SimpleLruInit(SerialSlruCtl, "Serial", + found = SimpleLruInit(SerialSlruCtl, "Serial", NUM_SERIAL_BUFFERS, 0, SerialSLRULock, "pg_serial", LWTRANCHE_SERIAL_BUFFER, SYNC_HANDLER_NONE); + Assert(found == IsUnderPostmaster); + #ifdef USE_ASSERT_CHECKING SerialPagePrecedesLogicallyUnitTests(); #endif diff --git a/src/include/access/slru.h b/src/include/access/slru.h index 130c41c863..ac5b08616d 100644 --- a/src/include/access/slru.h +++ b/src/include/access/slru.h @@ -140,7 +140,7 @@ typedef SlruCtlData *SlruCtl; extern Size SimpleLruShmemSize(int nslots, int nlsns); -extern void SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns, +extern bool SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns, LWLock *ctllock, const char *subdir, int tranche_id, SyncRequestHandler sync_handler); extern int SimpleLruZeroPage(SlruCtl ctl, int pageno); @@ -163,7 +163,7 @@ typedef bool (*SlruScanCallback) (SlruCtl ctl, char *filename, int segpage, extern bool SlruScanDirectory(SlruCtl ctl, SlruScanCallback callback, void *data); extern void SlruDeleteSegment(SlruCtl ctl, int segno); -extern int SlruSyncFileTag(SlruCtl ctl, const FileTag *ftag, char *path); +extern int SlruSyncSegment(SlruCtl ctl, int segno, char *path); /* SlruScanDirectory public callbacks */ extern bool SlruScanDirCbReportPresence(SlruCtl ctl, char *filename, diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 9090226daa..5aad63b39b 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -28,6 +28,7 @@ SUBDIRS = \ test_regex \ test_rls_hooks \ test_shm_mq \ + test_slru \ unsafe_tests \ worker_spi diff --git a/src/test/modules/test_slru/.gitignore b/src/test/modules/test_slru/.gitignore new file mode 100644 index 0000000000..4341ef65fe --- /dev/null +++ b/src/test/modules/test_slru/.gitignore @@ -0,0 +1,4 @@ +*.o +*.so +*.bc +results diff --git a/src/test/modules/test_slru/Makefile b/src/test/modules/test_slru/Makefile new file mode 100644 index 0000000000..0e72c8183c --- /dev/null +++ b/src/test/modules/test_slru/Makefile @@ -0,0 +1,23 @@ +# src/test/modules/test_slru/Makefile + +MODULE_big = test_slru +OBJS = \ + $(WIN32RES) \ + test_slru.o +PGFILEDESC = "test_slru - example use of SLRU" + +EXTENSION = test_slru +DATA = test_slru--1.0.sql + +REGRESS = test_slru + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_slru +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_slru/expected/test_slru.out b/src/test/modules/test_slru/expected/test_slru.out new file mode 100644 index 0000000000..585734b900 --- /dev/null +++ b/src/test/modules/test_slru/expected/test_slru.out @@ -0,0 +1,31 @@ +CREATE EXTENSION test_slru; +-- The test is executed twice to make sure the state is cleaned up properly. +SELECT test_slru(); +NOTICE: Creating TestSLRULock... +NOTICE: Creating directory 'pg_test_slru', if not exists... +NOTICE: Creating SLRU, if not exists... +NOTICE: Calling SimpleLruZeroPage()... +NOTICE: Success, slotno = 0, shared->page_number[0] = 12345 +NOTICE: SimpleLruDoesPhysicalPageExist() returned 0. Now writing the page. +NOTICE: SimpleLruDoesPhysicalPageExist() returned 1 +NOTICE: Cleanig up... + test_slru +----------- + +(1 row) + +SELECT test_slru(); +NOTICE: Creating TestSLRULock... +NOTICE: Creating directory 'pg_test_slru', if not exists... +NOTICE: Creating SLRU, if not exists... +NOTICE: Calling SimpleLruZeroPage()... +NOTICE: Success, slotno = 0, shared->page_number[0] = 12345 +NOTICE: SimpleLruDoesPhysicalPageExist() returned 0. Now writing the page. +NOTICE: SimpleLruDoesPhysicalPageExist() returned 1 +NOTICE: Cleanig up... + test_slru +----------- + +(1 row) + +DROP EXTENSION test_slru; diff --git a/src/test/modules/test_slru/sql/test_slru.sql b/src/test/modules/test_slru/sql/test_slru.sql new file mode 100644 index 0000000000..6ed0407ab0 --- /dev/null +++ b/src/test/modules/test_slru/sql/test_slru.sql @@ -0,0 +1,7 @@ +CREATE EXTENSION test_slru; + +-- The test is executed twice to make sure the state is cleaned up properly. +SELECT test_slru(); +SELECT test_slru(); + +DROP EXTENSION test_slru; \ No newline at end of file diff --git a/src/test/modules/test_slru/test_slru--1.0.sql b/src/test/modules/test_slru/test_slru--1.0.sql new file mode 100644 index 0000000000..f36cbf2f1f --- /dev/null +++ b/src/test/modules/test_slru/test_slru--1.0.sql @@ -0,0 +1,5 @@ +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_slru" to load this file. \quit + +CREATE OR REPLACE FUNCTION test_slru() RETURNS VOID +AS 'MODULE_PATHNAME', 'test_slru' LANGUAGE C; diff --git a/src/test/modules/test_slru/test_slru.c b/src/test/modules/test_slru/test_slru.c new file mode 100644 index 0000000000..15a7c131f0 --- /dev/null +++ b/src/test/modules/test_slru/test_slru.c @@ -0,0 +1,85 @@ +#include +#include +#include "access/slru.h" +#include "storage/fd.h" + +PG_MODULE_MAGIC; + + +/* + * SQL-callable entry point to perform all the tests + */ +PG_FUNCTION_INFO_V1(test_slru); + +#define NUM_TEST_BUFFERS 16 + +int test_tranche_id = -1; +LWLock TestSLRULock; +#define TestSLRULock (&TestSLRULock) + +static SlruCtlData TestSlruCtlData; +#define TestSlruCtl (&TestSlruCtlData) + +static bool +TestPagePrecedesLogically(int page1, int page2) +{ + return page1 < page2; +} + +Datum +test_slru(PG_FUNCTION_ARGS) +{ + bool found; + int slotno; + int test_pageno = 12345; + char test_page_data[] = "Test page data"; + const char slru_dir_name[] = "pg_test_slru"; + + elog(NOTICE, "Creating TestSLRULock..."); + + test_tranche_id = LWLockNewTrancheId(); + LWLockRegisterTranche(test_tranche_id, "test_slru_tranche"); + LWLockInitialize(TestSLRULock, test_tranche_id); + + elog(NOTICE, "Creating directory '%s', if not exists...", + slru_dir_name); + (void)MakePGDirectory(slru_dir_name); + + elog(NOTICE, "Creating SLRU, if not exists..."); + + TestSlruCtl->PagePrecedes = TestPagePrecedesLogically; + (void)SimpleLruInit(TestSlruCtl, "Test", + NUM_TEST_BUFFERS, 0, TestSLRULock, slru_dir_name, + test_tranche_id, SYNC_HANDLER_NONE); + + elog(NOTICE, "Calling SimpleLruZeroPage()..."); + + LWLockAcquire(TestSLRULock, LW_EXCLUSIVE); + slotno = SimpleLruZeroPage(TestSlruCtl, test_pageno); + + TestSlruCtl->shared->page_dirty[slotno] = true; + TestSlruCtl->shared->page_status[slotno] = SLRU_PAGE_VALID; + strcpy(TestSlruCtl->shared->page_buffer[slotno], test_page_data); + + elog(NOTICE, "Success, slotno = %d, shared->page_number[%d] = %d", + slotno, slotno, TestSlruCtl->shared->page_number[slotno]); + + found = SimpleLruDoesPhysicalPageExist(TestSlruCtl, test_pageno); + elog(NOTICE, "SimpleLruDoesPhysicalPageExist() returned %d. " + "Now writing the page.", found); + + /* Should we rename it to SimpleLruWriteSlot? -- a.alekseev */ + SimpleLruWritePage(TestSlruCtl, slotno); + LWLockRelease(TestSLRULock); + + found = SimpleLruDoesPhysicalPageExist(TestSlruCtl, test_pageno); + elog(NOTICE, "SimpleLruDoesPhysicalPageExist() returned %d", found); + + elog(NOTICE, "Cleanig up..."); + //SimpleLruWriteAll(TestSlruCtl, true); + // SimpleLruTruncate(TestSlruCtl, 555555); + SlruDeleteSegment(TestSlruCtl, test_pageno / SLRU_PAGES_PER_SEGMENT); + + // TODO: test SlruReportIOError + PG_RETURN_VOID(); +} diff --git a/src/test/modules/test_slru/test_slru.control b/src/test/modules/test_slru/test_slru.control new file mode 100644 index 0000000000..742541b297 --- /dev/null +++ b/src/test/modules/test_slru/test_slru.control @@ -0,0 +1,4 @@ +comment = 'Test code for SLRU' +default_version = '1.0' +module_pathname = '$libdir/test_slru' +relocatable = false -- 2.35.1