On Fri, Nov 11, 2022 at 02:11:08PM +0900, Michael Paquier wrote: > Is there a reason why you need a TAP test here? It is by design more > expensive than pg_regress and it does not require --enable-tap-tests. > See for example what we do for snapshot_too_old, commit_ts, > worker_spi, etc., where each module uses a custom configuration file.
I have put my hands on that, and I found that the tests were a bit overengineered. First, SimpleLruDoesPhysicalPageExist() is not that much necessary before and after each operation, like truncation or deletion, as the previous pages were doing equal tests. The hardcoded page number lacks a bit of flexibility and readability IMO, especially when combined with the number of pages per segments, as well. I have reworked that as per the attached, that provides basically the same coverage, going through a SQL interface for the whole thing. Like all the other tests of its kind, this does not use a TAP test, relying on a custom configuration file instead. This still needs some polishing, but the basics are here. What do you think? -- Michael
From 021cff01608b7defc52419168f58584d037e6edf Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 14 Nov 2022 20:16:36 +0900 Subject: [PATCH v6] Add unit tests for SLRU SimpleLruInit() checks whether it is called under postmaster. For this reason the tests can't be implemented in regress.c without changing the interface. We wanted to avoid this. As a result the tests were implemented as an extension in src/test/modules/ directory. Aleksander Alekseev, reviewed by Michael Paquier, Daniel Gustafsson, Noah Misch, Pavel Borisov, Maxim Orlov. Discussion: https://postgr.es/m/CAJ7c6TOFoWcHOW4BVe3BG_uikCrO9B91ayx9d6rh5JZr_tPESg%40mail.gmail.com --- src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + src/test/modules/test_slru/.gitignore | 4 + src/test/modules/test_slru/Makefile | 27 ++ .../modules/test_slru/expected/test_slru.out | 135 ++++++++++ src/test/modules/test_slru/meson.build | 35 +++ src/test/modules/test_slru/sql/test_slru.sql | 38 +++ src/test/modules/test_slru/test_slru--1.0.sql | 21 ++ src/test/modules/test_slru/test_slru.c | 249 ++++++++++++++++++ src/test/modules/test_slru/test_slru.conf | 2 + src/test/modules/test_slru/test_slru.control | 4 + 11 files changed, 517 insertions(+) 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/meson.build 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.conf create mode 100644 src/test/modules/test_slru/test_slru.control diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 7b3f292965..af3eef19b0 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -30,6 +30,7 @@ SUBDIRS = \ test_regex \ test_rls_hooks \ test_shm_mq \ + test_slru \ unsafe_tests \ worker_spi diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index c2e5f5ffd5..0b2dd3134a 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -24,5 +24,6 @@ subdir('test_rbtree') subdir('test_regex') subdir('test_rls_hooks') subdir('test_shm_mq') +subdir('test_slru') subdir('unsafe_tests') subdir('worker_spi') diff --git a/src/test/modules/test_slru/.gitignore b/src/test/modules/test_slru/.gitignore new file mode 100644 index 0000000000..5dcb3ff972 --- /dev/null +++ b/src/test/modules/test_slru/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/test_slru/Makefile b/src/test/modules/test_slru/Makefile new file mode 100644 index 0000000000..936886753b --- /dev/null +++ b/src/test/modules/test_slru/Makefile @@ -0,0 +1,27 @@ +# src/test/modules/test_slru/Makefile + +MODULE_big = test_slru +OBJS = \ + $(WIN32RES) \ + test_slru.o +PGFILEDESC = "test_slru - test module for SLRUs" + +EXTENSION = test_slru +DATA = test_slru--1.0.sql + +REGRESS_OPTS = --temp-config $(top_srcdir)/src/test/modules/test_slru/test_slru.conf +REGRESS = test_slru +# Disabled because these tests require "shared_preload_libraries=test_slru", +# which typical installcheck users do not have (e.g. buildfarm clients). +NO_INSTALLCHECK = 1 + +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..0e66fdc205 --- /dev/null +++ b/src/test/modules/test_slru/expected/test_slru.out @@ -0,0 +1,135 @@ +CREATE EXTENSION test_slru; +SELECT test_slru_page_exists(12345); + test_slru_page_exists +----------------------- + f +(1 row) + +SELECT test_slru_page_write(12345, 'Test SLRU'); + test_slru_page_write +---------------------- + +(1 row) + +SELECT test_slru_page_read(12345); + test_slru_page_read +--------------------- + Test SLRU +(1 row) + +SELECT test_slru_page_exists(12345); + test_slru_page_exists +----------------------- + t +(1 row) + +-- 48 extra pages +SELECT count(test_slru_page_write(a, 'Test SLRU')) + FROM generate_series(12346, 12393, 1) as a; + count +------- + 48 +(1 row) + +-- Reading page in buffer for read and write +SELECT test_slru_page_read(12377, true); + test_slru_page_read +--------------------- + Test SLRU +(1 row) + +-- Reading page in buffer for read-only +SELECT test_slru_page_readonly(12377); + test_slru_page_readonly +------------------------- + Test SLRU +(1 row) + +-- Reading page not in buffer with read-only +SELECT test_slru_page_readonly(12346); + test_slru_page_readonly +------------------------- + Test SLRU +(1 row) + +-- Write all the pages in buffers +SELECT test_slru_page_writeall(); + test_slru_page_writeall +------------------------- + +(1 row) + +-- Flush the last page written out. +SELECT test_slru_page_sync(12393); +NOTICE: Called SlruSyncFileTag() for segment 387 on path pg_test_slru/0183 + test_slru_page_sync +--------------------- + +(1 row) + +SELECT test_slru_page_exists(12393); + test_slru_page_exists +----------------------- + t +(1 row) + +-- Segment deletion +SELECT test_slru_page_delete(12393); +NOTICE: Called SlruDeleteSegment() for segment 387 + test_slru_page_delete +----------------------- + +(1 row) + +SELECT test_slru_page_exists(12393); + test_slru_page_exists +----------------------- + f +(1 row) + +-- Page truncation +SELECT test_slru_page_exists(12377); + test_slru_page_exists +----------------------- + t +(1 row) + +SELECT test_slru_page_truncate(12377); + test_slru_page_truncate +------------------------- + +(1 row) + +SELECT test_slru_page_exists(12377); + test_slru_page_exists +----------------------- + t +(1 row) + +-- Full deletion +SELECT test_slru_delete_all(); +NOTICE: Calling test_slru_scan_cb() + test_slru_delete_all +---------------------- + +(1 row) + +SELECT test_slru_page_exists(12345); + test_slru_page_exists +----------------------- + f +(1 row) + +SELECT test_slru_page_exists(12377); + test_slru_page_exists +----------------------- + f +(1 row) + +SELECT test_slru_page_exists(12393); + test_slru_page_exists +----------------------- + f +(1 row) + +DROP EXTENSION test_slru; diff --git a/src/test/modules/test_slru/meson.build b/src/test/modules/test_slru/meson.build new file mode 100644 index 0000000000..56d9d8a801 --- /dev/null +++ b/src/test/modules/test_slru/meson.build @@ -0,0 +1,35 @@ +# FIXME: prevent install during main install, but not during test :/ + +test_slru_sources = files( + 'test_slru.c', +) + +if host_system == 'windows' + test_slru_sources += rc_lib_gen.process(win32ver_rc, extra_args: [ + '--NAME', 'test_slru', + '--FILEDESC', 'test_slru - test module for SLRUs',]) +endif + +test_slru = shared_module('test_slru', + test_slru_sources, + kwargs: pg_mod_args, +) +testprep_targets += test_slru + +install_data( + 'test_slru.control', + 'test_slru--1.0.sql', + kwargs: contrib_data_args, +) + +tests += { + 'name': 'test_slru', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'regress': { + 'sql': [ + 'test_slru', + ], + 'regress_args': ['--temp-config', files('test_slru.conf')], + }, +} \ No newline at end of file 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..fe0d1342a9 --- /dev/null +++ b/src/test/modules/test_slru/sql/test_slru.sql @@ -0,0 +1,38 @@ +CREATE EXTENSION test_slru; + +SELECT test_slru_page_exists(12345); +SELECT test_slru_page_write(12345, 'Test SLRU'); +SELECT test_slru_page_read(12345); +SELECT test_slru_page_exists(12345); + +-- 48 extra pages +SELECT count(test_slru_page_write(a, 'Test SLRU')) + FROM generate_series(12346, 12393, 1) as a; + +-- Reading page in buffer for read and write +SELECT test_slru_page_read(12377, true); +-- Reading page in buffer for read-only +SELECT test_slru_page_readonly(12377); +-- Reading page not in buffer with read-only +SELECT test_slru_page_readonly(12346); + +-- Write all the pages in buffers +SELECT test_slru_page_writeall(); +-- Flush the last page written out. +SELECT test_slru_page_sync(12393); +SELECT test_slru_page_exists(12393); +-- Segment deletion +SELECT test_slru_page_delete(12393); +SELECT test_slru_page_exists(12393); +-- Page truncation +SELECT test_slru_page_exists(12377); +SELECT test_slru_page_truncate(12377); +SELECT test_slru_page_exists(12377); + +-- Full deletion +SELECT test_slru_delete_all(); +SELECT test_slru_page_exists(12345); +SELECT test_slru_page_exists(12377); +SELECT test_slru_page_exists(12393); + +DROP EXTENSION test_slru; 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..8635e7df01 --- /dev/null +++ b/src/test/modules/test_slru/test_slru--1.0.sql @@ -0,0 +1,21 @@ +-- 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_page_write(int, text) RETURNS VOID + AS 'MODULE_PATHNAME', 'test_slru_page_write' LANGUAGE C; +CREATE OR REPLACE FUNCTION test_slru_page_writeall() RETURNS VOID + AS 'MODULE_PATHNAME', 'test_slru_page_writeall' LANGUAGE C; +CREATE OR REPLACE FUNCTION test_slru_page_sync(int) RETURNS VOID + AS 'MODULE_PATHNAME', 'test_slru_page_sync' LANGUAGE C; +CREATE OR REPLACE FUNCTION test_slru_page_read(int, bool DEFAULT true) RETURNS text + AS 'MODULE_PATHNAME', 'test_slru_page_read' LANGUAGE C; +CREATE OR REPLACE FUNCTION test_slru_page_readonly(int) RETURNS text + AS 'MODULE_PATHNAME', 'test_slru_page_readonly' LANGUAGE C; +CREATE OR REPLACE FUNCTION test_slru_page_exists(int) RETURNS bool + AS 'MODULE_PATHNAME', 'test_slru_page_exists' LANGUAGE C; +CREATE OR REPLACE FUNCTION test_slru_page_delete(int) RETURNS VOID + AS 'MODULE_PATHNAME', 'test_slru_page_delete' LANGUAGE C; +CREATE OR REPLACE FUNCTION test_slru_page_truncate(int) RETURNS VOID + AS 'MODULE_PATHNAME', 'test_slru_page_truncate' LANGUAGE C; +CREATE OR REPLACE FUNCTION test_slru_delete_all() RETURNS VOID + AS 'MODULE_PATHNAME', 'test_slru_delete_all' 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..d9ea637114 --- /dev/null +++ b/src/test/modules/test_slru/test_slru.c @@ -0,0 +1,249 @@ +/*-------------------------------------------------------------------------- + * + * test_slru.c + * Test correctness of SLRU functions. + * + * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * IDENTIFICATION + * src/test/modules/test_slru/test_slru.c + * + * ------------------------------------------------------------------------- + */ + +#include "postgres.h" + +#include "access/slru.h" +#include "access/transam.h" +#include "miscadmin.h" +#include "storage/fd.h" +#include "storage/ipc.h" +#include "storage/shmem.h" +#include "storage/lwlock.h" +#include "utils/builtins.h" + +PG_MODULE_MAGIC; + +/* + * SQL-callable entry points + */ +PG_FUNCTION_INFO_V1(test_slru_page_write); +PG_FUNCTION_INFO_V1(test_slru_page_writeall); +PG_FUNCTION_INFO_V1(test_slru_page_read); +PG_FUNCTION_INFO_V1(test_slru_page_readonly); +PG_FUNCTION_INFO_V1(test_slru_page_exists); +PG_FUNCTION_INFO_V1(test_slru_page_sync); +PG_FUNCTION_INFO_V1(test_slru_page_delete); +PG_FUNCTION_INFO_V1(test_slru_page_truncate); +PG_FUNCTION_INFO_V1(test_slru_delete_all); + +#define NUM_TEST_BUFFERS 16 + +/* SLRU control lock */ +LWLock TestSLRULock; +#define TestSLRULock (&TestSLRULock) + +static SlruCtlData TestSlruCtlData; +#define TestSlruCtl (&TestSlruCtlData) + +static shmem_request_hook_type prev_shmem_request_hook = NULL; +static shmem_startup_hook_type prev_shmem_startup_hook = NULL; + +/* LWLock name */ +const char test_tranche_name[] = "test_slru_tranche"; + +static bool +test_slru_page_precedes_logically(int page1, int page2) +{ + return page1 < page2; +} + +static bool +test_slru_scan_cb(SlruCtl ctl, char *filename, int segpage, void *data) +{ + elog(NOTICE, "Calling test_slru_scan_cb()"); + return SlruScanDirCbDeleteAll(ctl, filename, segpage, data); +} + + +Datum +test_slru_page_write(PG_FUNCTION_ARGS) +{ + int pageno = PG_GETARG_INT32(0); + char *data = text_to_cstring(PG_GETARG_TEXT_PP(1)); + int slotno; + + LWLockAcquire(TestSLRULock, LW_EXCLUSIVE); + + slotno = SimpleLruZeroPage(TestSlruCtl, pageno); + + /* these should match */ + Assert(TestSlruCtl->shared->page_number[slotno] == pageno); + + /* mark the page as dirty so as it would get written */ + TestSlruCtl->shared->page_dirty[slotno] = true; + TestSlruCtl->shared->page_status[slotno] = SLRU_PAGE_VALID; + + /* write given data to the page */ + strcpy(TestSlruCtl->shared->page_buffer[slotno], data); + + SimpleLruWritePage(TestSlruCtl, slotno); + LWLockRelease(TestSLRULock); + + PG_RETURN_VOID(); +} + +Datum +test_slru_page_writeall(PG_FUNCTION_ARGS) +{ + SimpleLruWriteAll(TestSlruCtl, true); + PG_RETURN_VOID(); +} + +Datum +test_slru_page_read(PG_FUNCTION_ARGS) +{ + int pageno = PG_GETARG_INT32(0); + bool write_ok = PG_GETARG_BOOL(1); + char *data = NULL; + int slotno; + + /* find page in shared buffers reading it if necessary */ + LWLockAcquire(TestSLRULock, LW_EXCLUSIVE); + slotno = SimpleLruReadPage(TestSlruCtl, pageno, + write_ok, InvalidTransactionId); + data = (char *) TestSlruCtl->shared->page_buffer[slotno]; + LWLockRelease(TestSLRULock); + + PG_RETURN_TEXT_P(cstring_to_text(data)); +} + +Datum +test_slru_page_readonly(PG_FUNCTION_ARGS) +{ + int pageno = PG_GETARG_INT32(0); + char *data = NULL; + int slotno; + + /* find page in shared buffers reading it if necessary */ + slotno = SimpleLruReadPage_ReadOnly(TestSlruCtl, + pageno, + InvalidTransactionId); + Assert(LWLockHeldByMe(TestSLRULock)); + data = (char *) TestSlruCtl->shared->page_buffer[slotno]; + LWLockRelease(TestSLRULock); + + PG_RETURN_TEXT_P(cstring_to_text(data)); +} + +Datum +test_slru_page_exists(PG_FUNCTION_ARGS) +{ + int pageno = PG_GETARG_INT32(0); + bool found; + + LWLockAcquire(TestSLRULock, LW_EXCLUSIVE); + found = SimpleLruDoesPhysicalPageExist(TestSlruCtl, pageno); + LWLockRelease(TestSLRULock); + + PG_RETURN_BOOL(found); +} + +Datum +test_slru_page_sync(PG_FUNCTION_ARGS) +{ + int pageno = PG_GETARG_INT32(0); + FileTag ftag; + char path[MAXPGPATH]; + + /* note that this flushes the full file a segment is located in */ + ftag.segno = pageno / SLRU_PAGES_PER_SEGMENT; + SlruSyncFileTag(TestSlruCtl, &ftag, path); + + elog(NOTICE, "Called SlruSyncFileTag() for segment %d on path %s", + ftag.segno, path); + + PG_RETURN_VOID(); +} + +Datum +test_slru_page_delete(PG_FUNCTION_ARGS) +{ + int pageno = PG_GETARG_INT32(0); + FileTag ftag; + + ftag.segno = pageno / SLRU_PAGES_PER_SEGMENT; + SlruDeleteSegment(TestSlruCtl, ftag.segno); + + elog(NOTICE, "Called SlruDeleteSegment() for segment %d", ftag.segno); + + PG_RETURN_VOID(); +} + +Datum +test_slru_page_truncate(PG_FUNCTION_ARGS) +{ + int pageno = PG_GETARG_INT32(0); + + SimpleLruTruncate(TestSlruCtl, pageno); + PG_RETURN_VOID(); +} + +Datum +test_slru_delete_all(PG_FUNCTION_ARGS) +{ + /* this calls SlruScanDirCbDeleteAll() internally, ensuring deletion */ + SlruScanDirectory(TestSlruCtl, test_slru_scan_cb, NULL); + + PG_RETURN_VOID(); +} + +static void +test_slru_shmem_request(void) +{ + if (prev_shmem_request_hook) + prev_shmem_request_hook(); + + /* reserve shared memory for the test SLRU */ + RequestAddinShmemSpace(SimpleLruShmemSize(NUM_TEST_BUFFERS, 0)); +} + +static void +test_slru_shmem_startup(void) +{ + const char slru_dir_name[] = "pg_test_slru"; + int test_tranche_id; + + if (prev_shmem_startup_hook) + prev_shmem_startup_hook(); + + /* + * Create the SLRU directory if it does not exist yet, from the + * root of the data directory. + */ + (void) MakePGDirectory(slru_dir_name); + + /* initialize the SLRU facility */ + test_tranche_id = LWLockNewTrancheId(); + LWLockRegisterTranche(test_tranche_id, "test_slru_tranche"); + LWLockInitialize(TestSLRULock, test_tranche_id); + + TestSlruCtl->PagePrecedes = test_slru_page_precedes_logically; + SimpleLruInit(TestSlruCtl, "TestSLRU", + NUM_TEST_BUFFERS, 0, TestSLRULock, slru_dir_name, + test_tranche_id, SYNC_HANDLER_NONE); +} + +void +_PG_init(void) +{ + if (!process_shared_preload_libraries_in_progress) + elog(FATAL, "Please use shared_preload_libraries"); + + prev_shmem_request_hook = shmem_request_hook; + shmem_request_hook = test_slru_shmem_request; + + prev_shmem_startup_hook = shmem_startup_hook; + shmem_startup_hook = test_slru_shmem_startup; +} diff --git a/src/test/modules/test_slru/test_slru.conf b/src/test/modules/test_slru/test_slru.conf new file mode 100644 index 0000000000..ce6bbf0a9d --- /dev/null +++ b/src/test/modules/test_slru/test_slru.conf @@ -0,0 +1,2 @@ +shared_preload_libraries = 'test_slru' + 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.38.1
signature.asc
Description: PGP signature
