public inbox for [email protected]help / color / mirror / Atom feed
Re: Why is src/test/modules/committs/t/002_standby.pl flaky? 12+ messages / 6 participants [nested] [flat]
* Re: Why is src/test/modules/committs/t/002_standby.pl flaky? @ 2022-03-22 03:24 Thomas Munro <[email protected]> 2022-03-22 14:19 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Tom Lane <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Thomas Munro @ 2022-03-22 03:24 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; Alexander Lakhin <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers On Tue, Mar 22, 2022 at 4:13 PM Tom Lane <[email protected]> wrote: > Thomas Munro <[email protected]> writes: > > I have a new socket abstraction patch that should address the known > > Windows socket/event bugs, but it's a little bigger than I thought it > > would be, not quite ready, and now too late to expect people to review > > for 15, so I think it should go into the next cycle. I've bounced > > https://commitfest.postgresql.org/37/3523/ into the next CF. We'll > > need to do something like 75674c7e for master. > > OK. You want me to push 75674c7e to HEAD? Thanks, yes, please do. ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Why is src/test/modules/committs/t/002_standby.pl flaky? 2022-03-22 03:24 Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]> @ 2022-03-22 14:19 ` Tom Lane <[email protected]> 2023-11-10 03:31 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Tom Lane @ 2022-03-22 14:19 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Noah Misch <[email protected]>; Alexander Lakhin <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers Thomas Munro <[email protected]> writes: > On Tue, Mar 22, 2022 at 4:13 PM Tom Lane <[email protected]> wrote: >> OK. You want me to push 75674c7e to HEAD? > Thanks, yes, please do. Done. regards, tom lane ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Why is src/test/modules/committs/t/002_standby.pl flaky? 2022-03-22 03:24 Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]> 2022-03-22 14:19 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Tom Lane <[email protected]> @ 2023-11-10 03:31 ` Thomas Munro <[email protected]> 2023-11-21 11:00 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Alexander Lakhin <[email protected]> 2024-01-05 17:04 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Robert Haas <[email protected]> 0 siblings, 2 replies; 12+ messages in thread From: Thomas Munro @ 2023-11-10 03:31 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; Alexander Lakhin <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers Here is a new attempt to fix this mess. Disclaimer: this based entirely on reading the manual and vicariously hacking a computer I don't have via CI. The two basic ideas are: * keep per-socket event handles in a hash table * add our own level-triggered event memory The socket table entries are reference counted, and exist as long as the socket is currently in at least one WaitEventSet. When creating a new entry, extra polling logic re-checks the initial level-triggered state (an overhead that we had in an ad-hoc way already, and that can be avoided by more widespread use of long lived WaitEventSet). You are not allowed to close a socket while it's in a WaitEventSet, because then a new socket could be allocated with the same number and chaos would ensue. For example, if we revive the idea of hooking libpq connections up to long-lived WaitEventSets, we'll probably need to invent a libpq event callback that says 'I am going to close socket X!', so you have a chance to remove the socket from any WaitEventSet *before* it's closed, to maintain that invariant. Other lazier ideas are possible, but probably become impossible in a hypothetical multi-threaded future. With these changes, AFAIK it should be safe to reinstate graceful socket shutdowns, to fix the field complaints about FATAL error messages being eaten by a grue and the annoying random CI/BF failures. Here are some other ideas that I considered but rejected for now: 1. We could throw the WAIT_USE_WIN32 code away, and hack WAIT_USE_POLL to use WSAPoll() on Windows; we could create a 'self-pipe' using a pair of connected AF_UNIX sockets to implement latches and fake signals. It seems like a lot of work, and makes latches a bit worse (instead of "everything is an event!" we have "everything is a socket!" with a helper thread, and we don't even have socketpair() on this OS). Blah. 2. We could figure out how to do fancy asynchronous sockets and IOCP. That's how NT really wants to talk to the world, it doesn't really want to pretend to be Unix. I expect that is where we'll get to eventually but it's a much bigger cross-platform R&D job. 3. Maybe there is a kind of partial step towards idea 2 that Andres mentioned on another thread somewhere: one could use an IOCP, and then use event callbacks that run on system threads to post IOCP messages (a bit like we do for our fake waitpid()). What I have here is the simplest way I could see to patch up what we already have, with the idea that in the fullness of time we'll eventually get around to idea 2, once someone is ready to do the press-ups. Review/poking-with-a-stick/trying-to-break-it most welcome. Attachments: [text/x-patch] v2-0001-simplehash-Allow-raw-memory-to-be-freed.patch (1.6K, ../../CA+hUKGL0bikWSC2XW-zUgFWNVEpD_gEWXndi2PE5tWqmApkpZQ@mail.gmail.com/2-v2-0001-simplehash-Allow-raw-memory-to-be-freed.patch) download | inline diff: From 4614df55bd0e98b70b942543482e6a9eb767f718 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Wed, 1 Nov 2023 05:53:12 +1300 Subject: [PATCH v2 1/6] simplehash: Allow raw memory to be freed. Commit 48995040d5e introduced SH_RAW_ALLOCATOR, but assumed that memory allocated that way could be freed with pfree(). Allow SH_RAW_FREE to be defined too, for cases where that isn't true. --- src/include/lib/simplehash.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/include/lib/simplehash.h b/src/include/lib/simplehash.h index b7adc16b80..cd354e2f11 100644 --- a/src/include/lib/simplehash.h +++ b/src/include/lib/simplehash.h @@ -42,6 +42,7 @@ * declarations reside * - SH_RAW_ALLOCATOR - if defined, memory contexts are not used; instead, * use this to allocate bytes. The allocator must zero the returned space. + * - SH_RAW_FREE - free operation corresponding to SH_RAW_ALLOCATOR * - SH_USE_NONDEFAULT_ALLOCATOR - if defined no element allocator functions * are defined, so you can supply your own * The following parameters are only relevant when SH_DEFINE is defined: @@ -410,7 +411,11 @@ SH_ALLOCATE(SH_TYPE * type, Size size) static inline void SH_FREE(SH_TYPE * type, void *pointer) { +#ifdef SH_RAW_FREE + SH_RAW_FREE(pointer); +#else pfree(pointer); +#endif } #endif @@ -458,7 +463,11 @@ SH_SCOPE void SH_DESTROY(SH_TYPE * tb) { SH_FREE(tb, tb->data); +#ifdef SH_RAW_FREE + SH_RAW_FREE(tb); +#else pfree(tb); +#endif } /* reset the contents of a previously created hash table */ -- 2.42.0 [text/x-patch] v2-0002-simplehash-Allow-raw-allocation-to-fail.patch (2.9K, ../../CA+hUKGL0bikWSC2XW-zUgFWNVEpD_gEWXndi2PE5tWqmApkpZQ@mail.gmail.com/3-v2-0002-simplehash-Allow-raw-allocation-to-fail.patch) download | inline diff: From 3ee479b182ac53e4839da1156ef30cd9f2523de7 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Wed, 1 Nov 2023 06:51:38 +1300 Subject: [PATCH v2 2/6] simplehash: Allow raw allocation to fail. Commit 48995040d5e allowed for raw allocators to be used instead of the MemoryContext API, but didn't contemplate allocation failure. Teach the grow and insert operations to report failure to the caller. --- src/include/lib/simplehash.h | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/src/include/lib/simplehash.h b/src/include/lib/simplehash.h index cd354e2f11..d1034baf67 100644 --- a/src/include/lib/simplehash.h +++ b/src/include/lib/simplehash.h @@ -205,7 +205,7 @@ SH_SCOPE void SH_DESTROY(SH_TYPE * tb); SH_SCOPE void SH_RESET(SH_TYPE * tb); /* void <prefix>_grow(<prefix>_hash *tb, uint64 newsize) */ -SH_SCOPE void SH_GROW(SH_TYPE * tb, uint64 newsize); +SH_SCOPE bool SH_GROW(SH_TYPE * tb, uint64 newsize); /* <element> *<prefix>_insert(<prefix>_hash *tb, <key> key, bool *found) */ SH_SCOPE SH_ELEMENT_TYPE *SH_INSERT(SH_TYPE * tb, SH_KEY_TYPE key, bool *found); @@ -442,6 +442,8 @@ SH_CREATE(MemoryContext ctx, uint32 nelements, void *private_data) #ifdef SH_RAW_ALLOCATOR tb = (SH_TYPE *) SH_RAW_ALLOCATOR(sizeof(SH_TYPE)); + if (!tb) + return NULL; #else tb = (SH_TYPE *) MemoryContextAllocZero(ctx, sizeof(SH_TYPE)); tb->ctx = ctx; @@ -454,6 +456,17 @@ SH_CREATE(MemoryContext ctx, uint32 nelements, void *private_data) SH_COMPUTE_PARAMETERS(tb, size); tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size); +#ifdef SH_RAW_ALLOCATOR + if (!tb->data) + { +#ifdef SH_RAW_FREE + SH_RAW_FREE(tb); +#else + pfree(tb); +#endif + return NULL; + } +#endif return tb; } @@ -485,7 +498,7 @@ SH_RESET(SH_TYPE * tb) * necessary. But resizing to the exact input size can be advantageous * performance-wise, when known at some point. */ -SH_SCOPE void +SH_SCOPE bool SH_GROW(SH_TYPE * tb, uint64 newsize) { uint64 oldsize = tb->size; @@ -502,9 +515,13 @@ SH_GROW(SH_TYPE * tb, uint64 newsize) /* compute parameters for new table */ SH_COMPUTE_PARAMETERS(tb, newsize); - tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size); + newdata = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size); +#ifdef SH_RAW_ALLOCATOR + if (!newdata) + return false; +#endif - newdata = tb->data; + tb->data = newdata; /* * Copy entries from the old data to newdata. We theoretically could use @@ -589,6 +606,8 @@ SH_GROW(SH_TYPE * tb, uint64 newsize) } SH_FREE(tb, olddata); + + return true; } /* @@ -623,7 +642,11 @@ restart: * When optimizing, it can be very useful to print these out. */ /* SH_STAT(tb); */ - SH_GROW(tb, tb->size * 2); + if (!SH_GROW(tb, tb->size * 2)) + { + *found = false; + return NULL; + } /* SH_STAT(tb); */ } -- 2.42.0 [text/x-patch] v2-0003-Redesign-Windows-socket-event-management.patch (22.1K, ../../CA+hUKGL0bikWSC2XW-zUgFWNVEpD_gEWXndi2PE5tWqmApkpZQ@mail.gmail.com/4-v2-0003-Redesign-Windows-socket-event-management.patch) download | inline diff: From 03a5bd46e5cdf1e551a64b16bb8915aead67ad85 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Sun, 19 Mar 2023 16:07:20 +1300 Subject: [PATCH v2 3/6] Redesign Windows socket event management. Previously, we created a Winsock event handle for each socket in each WaitEventSet, and then we translated an FD_CLOSE event directly to WL_SOCKET_READABLE. Since FD_CLOSE is reported only once when the remote end shuts down gracefully, we could hang in rare scenarios where backend code relies on WL_SOCKET_READABLE being level-triggered. We got away with this in the past when the thing on the other end of the socket was another PostgreSQL server (ie via postgres_fdw, replication etc), because the remote server would exit without shutting down or closing its socket, and that produces a repeating 'abortive' FD_CLOSE. We'd like to change that as it also eats error messages, producing user complaints and random CI failures, but that's a sepaarate issue and we'll need to fix this first. New design: * for each socket, we now create just one event handle to be used by all WaitEventSet objects that are interested in the socket * for each socket, we now track a set of sticky events that are reported as poll() would until they are cleared by either the send()/recv() wrappers, or failing that by an explicit re-check The lifetime management of event handles and associated state is done by reference counting. --- src/backend/port/win32/socket.c | 364 +++++++++++++++++++++++++++++++ src/backend/storage/ipc/latch.c | 212 ++++++------------ src/include/port/win32_port.h | 6 + src/include/storage/latch.h | 3 - src/tools/pgindent/typedefs.list | 1 + 5 files changed, 441 insertions(+), 145 deletions(-) diff --git a/src/backend/port/win32/socket.c b/src/backend/port/win32/socket.c index 9c339397d1..a7fa98cb1d 100644 --- a/src/backend/port/win32/socket.c +++ b/src/backend/port/win32/socket.c @@ -13,6 +13,8 @@ #include "postgres.h" +#include "common/hashfn.h" + /* * Indicate if pgwin32_recv() and pgwin32_send() should operate * in non-blocking mode. @@ -37,6 +39,77 @@ int pgwin32_noblock = 0; #undef recv #undef send +/* + * An entry in our socket table. + */ +typedef struct SocketTableEntry +{ + SOCKET sock; + char status; + + /* + * The reference count for the event handle. Client code that wants to + * use the event functions must acquire a reference and release it when + * finished. + */ + int reference_count; + + /* + * The FD_XXX events that were most recently selected for this socket + * number with WSAEventSelect(). + */ + int selected_events; + + /* + * The FD_XXX events already reported by Winsock, that we'll continue to + * report as long as they are true. They are cleared by our send/recv + * wrappers, because those are 're-enabling' functions that will cause + * Winsock to report them again. The are also cleared by an explicit + * check we perform for the benefit of hypothetical code that might be + * reach Winsock send/recv wrappers without going via our wrappers. + */ + int level_triggered_events; + + /* + * Windows kernel event most recently associated with the socket number. + */ + HANDLE event_handle; +} SocketTableEntry; + +static inline void * +malloc0(size_t size) +{ + void *result; + + result = malloc(size); + if (result) + memset(result, 0, size); + + return result; +} + +/* + * It almost seems feasible to use an array to store our per-socket state, + * based on the observation that Windows socket descriptors seem to be small + * integers as on Unix, but the manual warns against making that assumption. + * So we use a hash table. + */ + +#define SH_PREFIX socket_table +#define SH_ELEMENT_TYPE SocketTableEntry +#define SH_RAW_ALLOCATOR malloc0 +#define SH_RAW_FREE free +#define SH_SCOPE static inline +#define SH_KEY_TYPE SOCKET +#define SH_KEY sock +#define SH_HASH_KEY(tb, key) murmurhash32(key) +#define SH_EQUAL(tb, a, b) (a) == (b) +#define SH_DECLARE +#define SH_DEFINE +#include "lib/simplehash.h" + +static socket_table_hash * socket_table; + /* * Blocking socket functions implemented so they listen on both * the socket and the signal event, required for signal handling. @@ -310,6 +383,265 @@ pgwin32_socket(int af, int type, int protocol) return s; } +/* + * Check if any of FD_READ, FD_WRITE or FD_CLOSE is still true. Used to + * re-check level-triggered events. + */ +static int +pgwin32_socket_poll(SOCKET s, int events) +{ + int revents = 0; + + if (events & (FD_READ | FD_CLOSE)) + { + ssize_t rc; + char c; + + rc = recv(s, &c, 1, MSG_PEEK); + if (rc == 1) + { + /* At least one byte to read. */ + if (events & FD_READ) + revents |= FD_READ; + } + else if (rc == 0 || WSAGetLastError() != WSAEWOULDBLOCK) + { + /* EOF due to graceful shutdown, or error. */ + if (events & FD_CLOSE) + revents |= FD_CLOSE; + } + } + + if (events & FD_WRITE) + { + char c; + + /* If it looks like we could write or get an error, report that. */ + if (send(s, &c, 0, 0) == 0 || WSAGetLastError() != WSAEWOULDBLOCK) + revents |= FD_WRITE; + } + + return revents; +} + +/* + * Adjust the set of FD_XXX events this socket's event handle should wake up + * for. Returns 0 on success, otherwise -1 and sets errno. + */ +int +pgwin32_socket_select_events(SOCKET s, int selected_events) +{ + SocketTableEntry *entry; + + Assert(socket_table); + entry = socket_table_lookup(socket_table, s); + + Assert(entry); + Assert(entry->reference_count > 0); + Assert(entry->event_handle != WSA_INVALID_EVENT); + + /* Do nothing if no change. */ + if (selected_events == entry->selected_events) + return 0; + + /* + * Tell Winsock to link the socket to the event handle, and which events + * we're interested in. + */ + if (WSAEventSelect(s, entry->event_handle, selected_events) == SOCKET_ERROR) + { + TranslateSocketError(); + return -1; + } + + entry->selected_events = selected_events; + + /* + * The manual tells us: "Issuing a WSAEventSelect for a socket cancels any + * previous WSAAsyncSelect or WSAEventSelect for the same socket and + * clears the internal network event record." If that is true, we might + * have wiped an internal flag we're interested in. Close that race by + * triggering an explicit poll before we sleep, by pretending we have seen + * all of these events. + */ + if (selected_events & (FD_READ | FD_WRITE)) + entry->level_triggered_events = selected_events & (FD_READ | FD_WRITE | FD_CLOSE); + else + entry->level_triggered_events = 0; + + return 0; +} + +/* + * Before waiting on the event handle, check if we have pending + * level-triggered events that are still true, and if so take measures to + * prevent the sleep. + */ +void +pgwin32_socket_prepare_to_wait(SOCKET s) +{ + SocketTableEntry *entry; + + Assert(socket_table); + entry = socket_table_lookup(socket_table, s); + + Assert(entry); + Assert(entry->reference_count > 0); + Assert(entry->event_handle != WSA_INVALID_EVENT); + + /* + * If we're not waiting for FD_READ or FD_WRITE, don't try to poll the + * socket. Server sockets and client sockets that haven't connected yet + * can't be polled by that technique. + */ + if ((entry->selected_events & (FD_READ | FD_WRITE)) && + entry->level_triggered_events != 0) + { + /* + * Re-check the level-triggered events we have recorded. This is + * necessary because someone might access WSASend()/WSARecv() directly + * without going via our wrapper functions, so they might never be + * cleared otherwise. + */ + entry->level_triggered_events = + pgwin32_socket_poll(s, + entry->level_triggered_events & entry->selected_events); + if (entry->level_triggered_events) + { + /* + * At least one readiness condition is still true. Prevent + * sleeping, and let pgwin32_socket_enumerate_events() report + * these level-triggered events. + */ + WSASetEvent(entry->event_handle); + } + } +} + +/* + * After the Windows event handle has been signaled, this function can be + * called to find out which socket events occurred, and atomically reset the + * event handle for the next sleep. + * + * The events returned are also remembered in our level-triggered event mask, + * so they'll prevent sleeping and be reported again as long as they remain + * true. + */ +int +pgwin32_socket_enumerate_events(SOCKET s) +{ + WSANETWORKEVENTS new_events = {0}; + SocketTableEntry *entry; + int result; + + Assert(socket_table); + entry = socket_table_lookup(socket_table, s); + + Assert(entry); + Assert(entry->reference_count > 0); + Assert(entry->event_handle != WSA_INVALID_EVENT); + + /* + * Atomically consume the internal network event record and reset the + * associated event handle. This guarantees that we can't miss future + * wakeups. + */ + if (WSAEnumNetworkEvents(s, entry->event_handle, &new_events) != 0) + { + TranslateSocketError(); + return -1; + } + + /* Add any events pgwin32_socket_prepare_to_wait() decided to feed us. */ + result = entry->level_triggered_events | new_events.lNetworkEvents; + + /* Remember certain events for next time around. */ + if (entry->selected_events & (FD_READ | FD_WRITE)) + entry->level_triggered_events = result & (FD_READ | FD_WRITE | FD_CLOSE); + else + entry->level_triggered_events = 0; + + return result; +} + +/* + * Acquire a reference-counted Windows event handle for this socket. This can + * be used for waiting for socket events. Returns NULL and sets errno on + * failure. + */ +HANDLE +pgwin32_socket_acquire_event_handle(SOCKET s) +{ + SocketTableEntry *entry; + bool found; + + /* First-time initialization. */ + if (unlikely(socket_table == NULL)) + { + socket_table = socket_table_create(16, NULL); + if (socket_table == NULL) + { + errno = ENOMEM; + return NULL; + } + } + + /* If we already have it, just bump the count. */ + entry = socket_table_insert(socket_table, s, &found); + if (likely(found)) + { + Assert(entry->event_handle != WSA_INVALID_EVENT); + entry->reference_count++; + return entry->event_handle; + } + + /* Did we run out of memory? */ + if (entry == NULL) + { + errno = ENOMEM; + return NULL; + } + + /* Allocate a new event handle. */ + entry->event_handle = WSACreateEvent(); + if (entry->event_handle == WSA_INVALID_EVENT) + { + socket_table_delete_item(socket_table, entry); + errno = ENOMEM; + return NULL; + } + + entry->selected_events = 0; + entry->level_triggered_events = 0; + entry->reference_count = 1; + + return entry->event_handle; +} + +/* + * Release a reference-counted event handle. + */ +void +pgwin32_socket_release_event_handle(SOCKET s) +{ + SocketTableEntry *entry; + + Assert(socket_table); + entry = socket_table_lookup(socket_table, s); + + Assert(entry); + Assert(entry->reference_count > 0); + Assert(entry->event_handle != WSA_INVALID_EVENT); + + if (--entry->reference_count == 0) + { + WSACloseEvent(entry->event_handle); + socket_table_delete_item(socket_table, entry); + + /* XXX Free socket_table if it is empty? */ + } +} + int pgwin32_bind(SOCKET s, struct sockaddr *addr, int addrlen) { @@ -402,6 +734,22 @@ pgwin32_recv(SOCKET s, char *buf, int len, int f) return -1; } + /* + * WSARecv() is a re-enabling function for Winsock's FD_READ event, so it + * is now safe to clear our level-triggered flag. This is only an + * optimization for a common case, and not required for correctness. If + * someone calls WSARecv() directly instead of going through this wrapper, + * pgwin32_socket_prepare_to_wait() will figure that out and clear it + * anyway. + */ + if (socket_table) + { + SocketTableEntry *entry = socket_table_lookup(socket_table, s); + + if (entry) + entry->level_triggered_events &= ~FD_READ; + } + if (pgwin32_noblock) { /* @@ -485,6 +833,22 @@ pgwin32_send(SOCKET s, const void *buf, int len, int flags) return -1; } + /* + * WSASend() is a re-enabling function for Winsock's FD_WRITE event, + * so it is now safe to clear our level-triggered flag. This is only + * an optimization for a common case, and not required for + * correctness. If someone calls WSASend() directly instead of going + * through this wrapper, pgwin32_socket_prepare_to_wait() will figure + * that out and clear it anyway. + */ + if (socket_table) + { + SocketTableEntry *entry = socket_table_lookup(socket_table, s); + + if (entry) + entry->level_triggered_events &= ~FD_WRITE; + } + if (pgwin32_noblock) { /* diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c index 2fd386a4ed..5bf03a3cd9 100644 --- a/src/backend/storage/ipc/latch.c +++ b/src/backend/storage/ipc/latch.c @@ -847,20 +847,9 @@ FreeWaitEventSet(WaitEventSet *set) cur_event < (set->events + set->nevents); cur_event++) { - if (cur_event->events & WL_LATCH_SET) - { - /* uses the latch's HANDLE */ - } - else if (cur_event->events & WL_POSTMASTER_DEATH) - { - /* uses PostmasterHandle */ - } - else - { - /* Clean up the event object we created for the socket */ - WSAEventSelect(cur_event->fd, NULL, 0); - WSACloseEvent(set->handles[cur_event->pos + 1]); - } + /* Release reference to socket's event handle. */ + if (cur_event->events & WL_SOCKET_MASK) + pgwin32_socket_release_event_handle(cur_event->fd); } #endif @@ -955,9 +944,6 @@ AddWaitEventToSet(WaitEventSet *set, uint32 events, pgsocket fd, Latch *latch, event->fd = fd; event->events = events; event->user_data = user_data; -#ifdef WIN32 - event->reset = false; -#endif if (events == WL_LATCH_SET) { @@ -976,10 +962,21 @@ AddWaitEventToSet(WaitEventSet *set, uint32 events, pgsocket fd, Latch *latch, } else if (events == WL_POSTMASTER_DEATH) { -#ifndef WIN32 +#if defined(WAIT_USE_WIN32) + set->handles[event->pos + 1] = PostmasterHandle; + event->fd = PGINVALID_SOCKET; +#else event->fd = postmaster_alive_fds[POSTMASTER_FD_WATCH]; #endif } + else if (events & WL_SOCKET_MASK) + { +#if defined(WAIT_USE_WIN32) + set->handles[event->pos + 1] = pgwin32_socket_acquire_event_handle(fd); + if (!set->handles[event->pos + 1]) + elog(ERROR, "could not acquire socket event handle: %m"); +#endif + } /* perform wait primitive specific initialization, if needed */ #if defined(WAIT_USE_EPOLL) @@ -1322,45 +1319,52 @@ WaitEventAdjustKqueue(WaitEventSet *set, WaitEvent *event, int old_events) #endif #if defined(WAIT_USE_WIN32) +static int +ToWinsockEvents(int pg_events) +{ + int winsock_events = 0; + + if (pg_events & WL_SOCKET_READABLE) + winsock_events |= FD_CLOSE | FD_READ; + if (pg_events & WL_SOCKET_WRITEABLE) + winsock_events |= FD_CLOSE | FD_WRITE; + if (pg_events & WL_SOCKET_CONNECTED) + winsock_events |= FD_CLOSE | FD_CONNECT; + if (pg_events & WL_SOCKET_ACCEPT) + winsock_events |= FD_CLOSE | FD_ACCEPT; + + return winsock_events; +} + +static int +FromWinsockEvents(int winsock_events) +{ + int pg_events = 0; + + if (winsock_events & (FD_CLOSE | FD_READ)) + pg_events |= WL_SOCKET_READABLE; + if (winsock_events & (FD_CLOSE | FD_WRITE)) + pg_events |= WL_SOCKET_WRITEABLE; + if (winsock_events & (FD_CLOSE | FD_CONNECT)) + pg_events |= WL_SOCKET_CONNECTED; + if (winsock_events & (FD_CLOSE | FD_ACCEPT)) + pg_events |= WL_SOCKET_ACCEPT; + + return pg_events; +} + static void WaitEventAdjustWin32(WaitEventSet *set, WaitEvent *event) { - HANDLE *handle = &set->handles[event->pos + 1]; - - if (event->events == WL_LATCH_SET) + if (event->events & WL_LATCH_SET) { - Assert(set->latch != NULL); - *handle = set->latch->event; + set->handles[event->pos + 1] = set->latch->event; } - else if (event->events == WL_POSTMASTER_DEATH) - { - *handle = PostmasterHandle; - } - else + else if (event->events & WL_SOCKET_MASK) { - int flags = FD_CLOSE; /* always check for errors/EOF */ - - if (event->events & WL_SOCKET_READABLE) - flags |= FD_READ; - if (event->events & WL_SOCKET_WRITEABLE) - flags |= FD_WRITE; - if (event->events & WL_SOCKET_CONNECTED) - flags |= FD_CONNECT; - if (event->events & WL_SOCKET_ACCEPT) - flags |= FD_ACCEPT; - - if (*handle == WSA_INVALID_EVENT) - { - *handle = WSACreateEvent(); - if (*handle == WSA_INVALID_EVENT) - elog(ERROR, "failed to create event for socket: error code %d", - WSAGetLastError()); - } - if (WSAEventSelect(event->fd, *handle, flags) != 0) - elog(ERROR, "failed to set up event for socket: error code %d", - WSAGetLastError()); - - Assert(event->fd != PGINVALID_SOCKET); + if (pgwin32_socket_select_events(event->fd, + ToWinsockEvents(event->events)) < 0) + elog(ERROR, "failed to set up event for socket: %m"); } } #endif @@ -1945,48 +1949,16 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout, DWORD rc; WaitEvent *cur_event; - /* Reset any wait events that need it */ + /* + * Allow level-triggered events to be signaled, causing + * WaitForMultipleObjects() to return immediately. + */ for (cur_event = set->events; cur_event < (set->events + set->nevents); cur_event++) { - if (cur_event->reset) - { - WaitEventAdjustWin32(set, cur_event); - cur_event->reset = false; - } - - /* - * Windows does not guarantee to log an FD_WRITE network event - * indicating that more data can be sent unless the previous send() - * failed with WSAEWOULDBLOCK. While our caller might well have made - * such a call, we cannot assume that here. Therefore, if waiting for - * write-ready, force the issue by doing a dummy send(). If the dummy - * send() succeeds, assume that the socket is in fact write-ready, and - * return immediately. Also, if it fails with something other than - * WSAEWOULDBLOCK, return a write-ready indication to let our caller - * deal with the error condition. - */ - if (cur_event->events & WL_SOCKET_WRITEABLE) - { - char c; - WSABUF buf; - DWORD sent; - int r; - - buf.buf = &c; - buf.len = 0; - - r = WSASend(cur_event->fd, &buf, 1, &sent, 0, NULL, NULL); - if (r == 0 || WSAGetLastError() != WSAEWOULDBLOCK) - { - occurred_events->pos = cur_event->pos; - occurred_events->user_data = cur_event->user_data; - occurred_events->events = WL_SOCKET_WRITEABLE; - occurred_events->fd = cur_event->fd; - return 1; - } - } + if (cur_event->events & WL_SOCKET_MASK) + pgwin32_socket_prepare_to_wait(cur_event->fd); } /* @@ -2067,64 +2039,20 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout, } else if (cur_event->events & WL_SOCKET_MASK) { - WSANETWORKEVENTS resEvents; - HANDLE handle = set->handles[cur_event->pos + 1]; + int winsock_events; + int pg_events; Assert(cur_event->fd); - occurred_events->fd = cur_event->fd; + winsock_events = pgwin32_socket_enumerate_events(cur_event->fd); + if (winsock_events < 0) + elog(ERROR, "could not enumerate socket events: %m"); - ZeroMemory(&resEvents, sizeof(resEvents)); - if (WSAEnumNetworkEvents(cur_event->fd, handle, &resEvents) != 0) - elog(ERROR, "failed to enumerate network events: error code %d", - WSAGetLastError()); - if ((cur_event->events & WL_SOCKET_READABLE) && - (resEvents.lNetworkEvents & FD_READ)) - { - /* data available in socket */ - occurred_events->events |= WL_SOCKET_READABLE; - - /*------ - * WaitForMultipleObjects doesn't guarantee that a read event - * will be returned if the latch is set at the same time. Even - * if it did, the caller might drop that event expecting it to - * reoccur on next call. So, we must force the event to be - * reset if this WaitEventSet is used again in order to avoid - * an indefinite hang. - * - * Refer - * https://msdn.microsoft.com/en-us/library/windows/desktop/ms741576(v=vs.85).aspx - * for the behavior of socket events. - *------ - */ - cur_event->reset = true; - } - if ((cur_event->events & WL_SOCKET_WRITEABLE) && - (resEvents.lNetworkEvents & FD_WRITE)) - { - /* writeable */ - occurred_events->events |= WL_SOCKET_WRITEABLE; - } - if ((cur_event->events & WL_SOCKET_CONNECTED) && - (resEvents.lNetworkEvents & FD_CONNECT)) - { - /* connected */ - occurred_events->events |= WL_SOCKET_CONNECTED; - } - if ((cur_event->events & WL_SOCKET_ACCEPT) && - (resEvents.lNetworkEvents & FD_ACCEPT)) - { - /* incoming connection could be accepted */ - occurred_events->events |= WL_SOCKET_ACCEPT; - } - if (resEvents.lNetworkEvents & FD_CLOSE) - { - /* EOF/error, so signal all caller-requested socket flags */ - occurred_events->events |= (cur_event->events & WL_SOCKET_MASK); - } - - if (occurred_events->events != 0) + pg_events = FromWinsockEvents(winsock_events) & cur_event->events; + if (pg_events) { + occurred_events->fd = cur_event->fd; + occurred_events->events = pg_events; occurred_events++; returned_events++; } diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h index 27a11c7868..a0ed6aaeaa 100644 --- a/src/include/port/win32_port.h +++ b/src/include/port/win32_port.h @@ -506,6 +506,12 @@ extern int pgwin32_recv(SOCKET s, char *buf, int len, int flags); extern int pgwin32_send(SOCKET s, const void *buf, int len, int flags); extern int pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout); +extern HANDLE pgwin32_socket_acquire_event_handle(SOCKET s); +extern void pgwin32_socket_release_event_handle(SOCKET s); +extern int pgwin32_socket_select_events(SOCKET s, int events); +extern void pgwin32_socket_prepare_to_wait(SOCKET s); +extern int pgwin32_socket_enumerate_events(SOCKET s); + extern PGDLLIMPORT int pgwin32_noblock; #endif /* FRONTEND */ diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h index 99cc47874a..cbcc5ef23f 100644 --- a/src/include/storage/latch.h +++ b/src/include/storage/latch.h @@ -153,9 +153,6 @@ typedef struct WaitEvent uint32 events; /* triggered events */ pgsocket fd; /* socket fd associated with event */ void *user_data; /* pointer provided in AddWaitEventToSet */ -#ifdef WIN32 - bool reset; /* Is reset of the event required? */ -#endif } WaitEvent; /* forward declaration to avoid exposing latch.c implementation details */ diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index bf50a32119..15dd7fa2b8 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2585,6 +2585,7 @@ Snapshot SnapshotData SnapshotType SockAddr +SocketTableEntry Sort SortBy SortByDir -- 2.42.0 [text/x-patch] v2-0004-Remove-pgwin32_select.patch (7.5K, ../../CA+hUKGL0bikWSC2XW-zUgFWNVEpD_gEWXndi2PE5tWqmApkpZQ@mail.gmail.com/5-v2-0004-Remove-pgwin32_select.patch) download | inline diff: From c561d1f7091368c5e09735ab099eb4a656587dfb Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Fri, 10 Nov 2023 08:41:28 +1300 Subject: [PATCH v2 4/6] Remove pgwin32_select(). pgwin32_select(), used to replace select() in backend code on Windows, would need to be updated to use the new per-socket event handles. Since the last remaining user of select() in the backend is scheduled for replacement with the WaitEventSet API, it seems better to demolish it instead. Any extension code that is relying on select() with fake signals will still compile, but will no longer respond to signals. Hypothetical code like that is probably buggy anyway, because backend code should also be handling interrupts, and should switch to the various WaitEventSet APIs. --- src/backend/port/win32/socket.c | 200 -------------------------------- src/include/port/win32_port.h | 2 - 2 files changed, 202 deletions(-) diff --git a/src/backend/port/win32/socket.c b/src/backend/port/win32/socket.c index a7fa98cb1d..d0cf08392f 100644 --- a/src/backend/port/win32/socket.c +++ b/src/backend/port/win32/socket.c @@ -867,203 +867,3 @@ pgwin32_send(SOCKET s, const void *buf, int len, int flags) return -1; } - - -/* - * Wait for activity on one or more sockets. - * While waiting, allow signals to run - * - * NOTE! Currently does not implement exceptfds check, - * since it is not used in postgresql! - */ -int -pgwin32_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, const struct timeval *timeout) -{ - WSAEVENT events[FD_SETSIZE * 2]; /* worst case is readfds totally - * different from writefds, so - * 2*FD_SETSIZE sockets */ - SOCKET sockets[FD_SETSIZE * 2]; - int numevents = 0; - int i; - int r; - DWORD timeoutval = WSA_INFINITE; - FD_SET outreadfds; - FD_SET outwritefds; - int nummatches = 0; - - Assert(exceptfds == NULL); - - if (pgwin32_poll_signals()) - return -1; - - FD_ZERO(&outreadfds); - FD_ZERO(&outwritefds); - - /* - * Windows does not guarantee to log an FD_WRITE network event indicating - * that more data can be sent unless the previous send() failed with - * WSAEWOULDBLOCK. While our caller might well have made such a call, we - * cannot assume that here. Therefore, if waiting for write-ready, force - * the issue by doing a dummy send(). If the dummy send() succeeds, - * assume that the socket is in fact write-ready, and return immediately. - * Also, if it fails with something other than WSAEWOULDBLOCK, return a - * write-ready indication to let our caller deal with the error condition. - */ - if (writefds != NULL) - { - for (i = 0; i < writefds->fd_count; i++) - { - char c; - WSABUF buf; - DWORD sent; - - buf.buf = &c; - buf.len = 0; - - r = WSASend(writefds->fd_array[i], &buf, 1, &sent, 0, NULL, NULL); - if (r == 0 || WSAGetLastError() != WSAEWOULDBLOCK) - FD_SET(writefds->fd_array[i], &outwritefds); - } - - /* If we found any write-ready sockets, just return them immediately */ - if (outwritefds.fd_count > 0) - { - memcpy(writefds, &outwritefds, sizeof(fd_set)); - if (readfds) - FD_ZERO(readfds); - return outwritefds.fd_count; - } - } - - - /* Now set up for an actual select */ - - if (timeout != NULL) - { - /* timeoutval is in milliseconds */ - timeoutval = timeout->tv_sec * 1000 + timeout->tv_usec / 1000; - } - - if (readfds != NULL) - { - for (i = 0; i < readfds->fd_count; i++) - { - events[numevents] = WSACreateEvent(); - sockets[numevents] = readfds->fd_array[i]; - numevents++; - } - } - if (writefds != NULL) - { - for (i = 0; i < writefds->fd_count; i++) - { - if (!readfds || - !FD_ISSET(writefds->fd_array[i], readfds)) - { - /* If the socket is not in the read list */ - events[numevents] = WSACreateEvent(); - sockets[numevents] = writefds->fd_array[i]; - numevents++; - } - } - } - - for (i = 0; i < numevents; i++) - { - int flags = 0; - - if (readfds && FD_ISSET(sockets[i], readfds)) - flags |= FD_READ | FD_ACCEPT | FD_CLOSE; - - if (writefds && FD_ISSET(sockets[i], writefds)) - flags |= FD_WRITE | FD_CLOSE; - - if (WSAEventSelect(sockets[i], events[i], flags) != 0) - { - TranslateSocketError(); - /* release already-assigned event objects */ - while (--i >= 0) - WSAEventSelect(sockets[i], NULL, 0); - for (i = 0; i < numevents; i++) - WSACloseEvent(events[i]); - return -1; - } - } - - events[numevents] = pgwin32_signal_event; - r = WaitForMultipleObjectsEx(numevents + 1, events, FALSE, timeoutval, TRUE); - if (r != WAIT_TIMEOUT && r != WAIT_IO_COMPLETION && r != (WAIT_OBJECT_0 + numevents)) - { - /* - * We scan all events, even those not signaled, in case more than one - * event has been tagged but Wait.. can only return one. - */ - WSANETWORKEVENTS resEvents; - - for (i = 0; i < numevents; i++) - { - ZeroMemory(&resEvents, sizeof(resEvents)); - if (WSAEnumNetworkEvents(sockets[i], events[i], &resEvents) != 0) - elog(ERROR, "failed to enumerate network events: error code %d", - WSAGetLastError()); - /* Read activity? */ - if (readfds && FD_ISSET(sockets[i], readfds)) - { - if ((resEvents.lNetworkEvents & FD_READ) || - (resEvents.lNetworkEvents & FD_ACCEPT) || - (resEvents.lNetworkEvents & FD_CLOSE)) - { - FD_SET(sockets[i], &outreadfds); - - nummatches++; - } - } - /* Write activity? */ - if (writefds && FD_ISSET(sockets[i], writefds)) - { - if ((resEvents.lNetworkEvents & FD_WRITE) || - (resEvents.lNetworkEvents & FD_CLOSE)) - { - FD_SET(sockets[i], &outwritefds); - - nummatches++; - } - } - } - } - - /* Clean up all the event objects */ - for (i = 0; i < numevents; i++) - { - WSAEventSelect(sockets[i], NULL, 0); - WSACloseEvent(events[i]); - } - - if (r == WSA_WAIT_TIMEOUT) - { - if (readfds) - FD_ZERO(readfds); - if (writefds) - FD_ZERO(writefds); - return 0; - } - - /* Signal-like events. */ - if (r == WAIT_OBJECT_0 + numevents || r == WAIT_IO_COMPLETION) - { - pgwin32_dispatch_queued_signals(); - errno = EINTR; - if (readfds) - FD_ZERO(readfds); - if (writefds) - FD_ZERO(writefds); - return -1; - } - - /* Overwrite socket sets with our resulting values */ - if (readfds) - memcpy(readfds, &outreadfds, sizeof(fd_set)); - if (writefds) - memcpy(writefds, &outwritefds, sizeof(fd_set)); - return nummatches; -} diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h index a0ed6aaeaa..1b605d9403 100644 --- a/src/include/port/win32_port.h +++ b/src/include/port/win32_port.h @@ -492,7 +492,6 @@ extern int pgkill(int pid, int sig); #define listen(s, backlog) pgwin32_listen(s, backlog) #define accept(s, addr, addrlen) pgwin32_accept(s, addr, addrlen) #define connect(s, name, namelen) pgwin32_connect(s, name, namelen) -#define select(n, r, w, e, timeout) pgwin32_select(n, r, w, e, timeout) #define recv(s, buf, len, flags) pgwin32_recv(s, buf, len, flags) #define send(s, buf, len, flags) pgwin32_send(s, buf, len, flags) @@ -501,7 +500,6 @@ extern int pgwin32_bind(SOCKET s, struct sockaddr *addr, int addrlen); extern int pgwin32_listen(SOCKET s, int backlog); extern SOCKET pgwin32_accept(SOCKET s, struct sockaddr *addr, int *addrlen); extern int pgwin32_connect(SOCKET s, const struct sockaddr *name, int namelen); -extern int pgwin32_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, const struct timeval *timeout); extern int pgwin32_recv(SOCKET s, char *buf, int len, int flags); extern int pgwin32_send(SOCKET s, const void *buf, int len, int flags); extern int pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout); -- 2.42.0 [text/x-patch] v2-0005-Refactor-pgwin32_waitforsinglesocket-to-share-eve.patch (4.8K, ../../CA+hUKGL0bikWSC2XW-zUgFWNVEpD_gEWXndi2PE5tWqmApkpZQ@mail.gmail.com/6-v2-0005-Refactor-pgwin32_waitforsinglesocket-to-share-eve.patch) download | inline diff: From 27144fba3c17e3d15e973e250f3ce3151079aea0 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Fri, 10 Nov 2023 08:59:45 +1300 Subject: [PATCH v2 5/6] Refactor pgwin32_waitforsinglesocket() to share events. This function is hardly used, since sockets in the backend are almost always in non-blocking mode. Ideally they would *always* be non-blocking, and if we ever get to that situation by project policy we could just delete this and related code, but in the meantime, we have to adjust it to use the new per-socket event handle or it could lose network events. While here, delete the code paths for UDP which are probably dead code since we retired the UDP-powered stats collector. There is another user of UDP in auth.c, but it's using sendto() and thus not reaching this code. Any other user of UDP that I might have missed probably isn't generating 'high load' like the old stats system that presumably motivated that sleeping logic. --- src/backend/port/win32/socket.c | 103 ++++++-------------------------- 1 file changed, 17 insertions(+), 86 deletions(-) diff --git a/src/backend/port/win32/socket.c b/src/backend/port/win32/socket.c index d0cf08392f..797a1f503e 100644 --- a/src/backend/port/win32/socket.c +++ b/src/backend/port/win32/socket.c @@ -238,120 +238,51 @@ pgwin32_poll_signals(void) return 0; } -static int -isDataGram(SOCKET s) -{ - int type; - int typelen = sizeof(type); - - if (getsockopt(s, SOL_SOCKET, SO_TYPE, (char *) &type, &typelen)) - return 1; - - return (type == SOCK_DGRAM) ? 1 : 0; -} - int pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout) { - static HANDLE waitevent = INVALID_HANDLE_VALUE; - static SOCKET current_socket = INVALID_SOCKET; - static int isUDP = 0; HANDLE events[2]; int r; - /* Create an event object just once and use it on all future calls */ - if (waitevent == INVALID_HANDLE_VALUE) - { - waitevent = CreateEvent(NULL, TRUE, FALSE, NULL); - - if (waitevent == INVALID_HANDLE_VALUE) - ereport(ERROR, - (errmsg_internal("could not create socket waiting event: error code %lu", GetLastError()))); - } - else if (!ResetEvent(waitevent)) - ereport(ERROR, - (errmsg_internal("could not reset socket waiting event: error code %lu", GetLastError()))); - - /* - * Track whether socket is UDP or not. (NB: most likely, this is both - * useless and wrong; there is no reason to think that the behavior of - * WSAEventSelect is different for TCP and UDP.) - */ - if (current_socket != s) - isUDP = isDataGram(s); - current_socket = s; + events[0] = pgwin32_signal_event; + events[1] = pgwin32_socket_acquire_event_handle(s); - /* - * Attach event to socket. NOTE: we must detach it again before - * returning, since other bits of code may try to attach other events to - * the socket. - */ - if (WSAEventSelect(s, waitevent, what) != 0) + if (events[1] == NULL) { - TranslateSocketError(); + /* errno is set */ return 0; } - events[0] = pgwin32_signal_event; - events[1] = waitevent; - - /* - * Just a workaround of unknown locking problem with writing in UDP socket - * under high load: Client's pgsql backend sleeps infinitely in - * WaitForMultipleObjectsEx, pgstat process sleeps in pgwin32_select(). - * So, we will wait with small timeout(0.1 sec) and if socket is still - * blocked, try WSASend (see comments in pgwin32_select) and wait again. - */ - if ((what & FD_WRITE) && isUDP) + if (pgwin32_socket_select_events(s, what) < 0) { - for (;;) - { - r = WaitForMultipleObjectsEx(2, events, FALSE, 100, TRUE); - - if (r == WAIT_TIMEOUT) - { - char c; - WSABUF buf; - DWORD sent; - - buf.buf = &c; - buf.len = 0; - - r = WSASend(s, &buf, 1, &sent, 0, NULL, NULL); - if (r == 0) /* Completed - means things are fine! */ - { - WSAEventSelect(s, NULL, 0); - return 1; - } - else if (WSAGetLastError() != WSAEWOULDBLOCK) - { - TranslateSocketError(); - WSAEventSelect(s, NULL, 0); - return 0; - } - } - else - break; - } + pgwin32_socket_release_event_handle(s); + return 0; } - else - r = WaitForMultipleObjectsEx(2, events, FALSE, timeout, TRUE); - WSAEventSelect(s, NULL, 0); + pgwin32_socket_prepare_to_wait(s); + + r = WaitForMultipleObjectsEx(2, events, FALSE, timeout, TRUE); if (r == WAIT_OBJECT_0 || r == WAIT_IO_COMPLETION) { + pgwin32_socket_release_event_handle(s); pgwin32_dispatch_queued_signals(); errno = EINTR; return 0; } if (r == WAIT_OBJECT_0 + 1) + { + pgwin32_socket_enumerate_events(s); + pgwin32_socket_release_event_handle(s); return 1; + } if (r == WAIT_TIMEOUT) { + pgwin32_socket_release_event_handle(s); errno = EWOULDBLOCK; return 0; } + pgwin32_socket_release_event_handle(s); ereport(ERROR, (errmsg_internal("unrecognized return value from WaitForMultipleObjects: %d (error code %lu)", r, GetLastError()))); return 0; -- 2.42.0 [text/x-patch] v2-0006-Reinstate-graceful-shutdown-changes-for-Windows.patch (2.4K, ../../CA+hUKGL0bikWSC2XW-zUgFWNVEpD_gEWXndi2PE5tWqmApkpZQ@mail.gmail.com/7-v2-0006-Reinstate-graceful-shutdown-changes-for-Windows.patch) download | inline diff: From 86d3a318af19d8fa476e36aae1c6b754912d7c4c Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Fri, 10 Nov 2023 10:24:32 +1300 Subject: [PATCH v2 6/6] Reinstate "graceful shutdown" changes for Windows. This reverts commit 29992a6a509b256efc4ac560a1586b51a64b2637. See the commit messages for 6051857fc and ed52c3707. --- src/backend/libpq/pqcomm.c | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index 522584e597..0ca93fefc8 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -280,15 +280,30 @@ socket_close(int code, Datum arg) secure_close(MyProcPort); /* - * Formerly we did an explicit close() here, but it seems better to - * leave the socket open until the process dies. This allows clients - * to perform a "synchronous close" if they care --- wait till the - * transport layer reports connection closure, and you can be sure the - * backend has exited. + * On most platforms, we leave the socket open until the process dies. + * This allows clients to perform a "synchronous close" if they care + * --- wait till the transport layer reports connection closure, and + * you can be sure the backend has exited. Saves a kernel call, too. * - * We do set sock to PGINVALID_SOCKET to prevent any further I/O, - * though. + * However, that does not work on Windows: if the kernel closes the + * socket it will invoke an "abortive shutdown" that discards any data + * not yet sent to the client. (This is a flat-out violation of the + * TCP RFCs, but count on Microsoft not to care about that.) To get + * the spec-compliant "graceful shutdown" behavior, we must invoke + * closesocket() explicitly. When using OpenSSL, it seems that clean + * shutdown also requires an explicit shutdown() call. + * + * This code runs late enough during process shutdown that we should + * have finished all externally-visible shutdown activities, so that + * in principle it's good enough to act as a synchronous close on + * Windows too. But it's a lot more fragile than the other way. */ +#ifdef WIN32 + shutdown(MyProcPort->sock, SD_SEND); + closesocket(MyProcPort->sock); +#endif + + /* In any case, set sock to PGINVALID_SOCKET to prevent further I/O */ MyProcPort->sock = PGINVALID_SOCKET; } } -- 2.42.0 ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Why is src/test/modules/committs/t/002_standby.pl flaky? 2022-03-22 03:24 Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]> 2022-03-22 14:19 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Tom Lane <[email protected]> 2023-11-10 03:31 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]> @ 2023-11-21 11:00 ` Alexander Lakhin <[email protected]> 2024-01-22 04:26 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Peter Smith <[email protected]> 1 sibling, 1 reply; 12+ messages in thread From: Alexander Lakhin @ 2023-11-21 11:00 UTC (permalink / raw) To: Thomas Munro <[email protected]>; Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers Hello Thomas, 10.11.2023 06:31, Thomas Munro wrote: > Here is a new attempt to fix this mess. Disclaimer: this based > entirely on reading the manual and vicariously hacking a computer I > don't have via CI. As it also might (and I would like it to) be the final attempt, I decided to gather information and all the cases that we had on this topic. At least, for the last 5 years we've seen: [1] 2019-01-18: Re: BUG #15598: PostgreSQL Error Code is not reported when connection terminated due to idle-in-transaction timeout    test 099_case_15598 made (in attachment) no commit [2] 2019-01-22: Rare SSL failures on eelpout    references [1]    test 099_rare_ssl_failures made (in attachment) commit 2019-03-19 1f39a1c06: Restructure libpq's hqandling of send failures. [3] 2019-11-28: pgsql: Add tests for '-f' option in dropdb utility.    test 051_dropdb_force was proposed (included in the attachment) commit 2019-11-30 98a9b37ba: Revert commits 290acac92b and 8a7e9e9dad. [4] 2019-12-06: closesocket behavior in different platforms    references [1], [2], [3]; a documentation change proposed no commit [5] 2020-06-03: libpq copy error handling busted    test 099_pgbench_with_server_off made (in attachment) commit 2020-06-07 7247e243a: Try to read data from the socket in pqSendSome's write_failed paths. (a fix for 1f39a1c06) [6] 2020-10-19: BUG #16678: The ecpg connect/test5 test sometimes fails on Windows no commit [7] 2021-11-17: Windows: Wrong error message at connection termination   references [6] commit: 2021-12-02 6051857fc: On Windows, close the client socket explicitly during backend shutdown. [8] 2021-12-05 17:03:18: MSVC SSL test failure commit: 2021-12-07 ed52c3707: On Windows, also call shutdown() while closing the client socket. [9] 2021-12-30: Why is src/test/modules/committs/t/002_standby.pl flaky?   additional test 099_postgres_fdw_disconnect made (in attachment) commit 2022-01-26 75674c7ec: Revert "graceful shutdown" changes for Windows, in back branches only. (REL_14_STABLE) commit 2022-03-22 29992a6a5: Revert "graceful shutdown" changes for Windows. (master) [10] 2022-02-02 19:19:22: BUG #17391: While using --with-ssl=openssl and PG_TEST_EXTRA='ssl' options, SSL tests fail on OpenBSD 7.0 commit 2022-02-12 335fa5a26: Fix thinko in PQisBusy(). (a fix for 1f39a1c06) commit 2022-02-12 faa189c93: Move libpq's write_failed mechanism down to pqsecure_raw_write(). (a fix for 1f39a1c06) As it becomes difficult to test/check all those cases scattered around, I decided to retell the whole story by means of tests. Please look at the script win-sock-tests-01.cmd attached, which can be executed both on Windows (as regular cmd) and on Linux (bash win-sock-tests-01.cmd). At the end of the script we can see several things. First, the last patchset you posted, applied to b282fa88d~1, fixes the issue discussed in this thread (it eliminates failures of commit_ts_002_standby (and also 099_postgres_fdw_disconnect)). Second, with the sleep added (see [6]), I had the same results of `meson test` on Windows and on Linux. Namely, there are some tests failing (see win-sock-tests-01.cmd) due to walsender preventing server stop. I describe this issue separately (see details in walsender-cannot-exit.txt; maybe it's worth to discuss it in a separate thread) as it's kind of off-topic. With the supplementary sleep() added to WalSndLoop(), the complete `meson test` passes successfully both on Windows and on Linux. Third, cases [1] and [3] are still broken, due to a Windows peculiarity. Please see server.c and client.c attached, which demonstrate: Case "no shutdown/closesocket" on Windows: C:\src>server Listening for incoming connections...                   C:\src>client Client connected: 127.0.0.1:64395                   Connection to server established. Enter message: msg Client message: msg Sending message...                   Sleeping... Exiting... C:\src>                   Calling recv()...                   recv() failed Case "no shutdown/closesocket" on Linux: $ server Listening for incoming connections...                   $ client Client connected: 127.0.0.1:33044                   Connection to server established. Enter message: msg Client message: msg Sending message...                   Sleeping... Exiting... $                   Calling recv()...                   Server message: MESSAGE Case "shutdown/closesocket" on Windows: C:\src>server shutdown closesocket Listening for incoming connections...                   C:\src>client Client connected: 127.0.0.1:64396                   Connection to server established. Enter message: msg Client message: msg Sending message...                   Sleeping... Calling shutdown()... Calling closesocket()... Exiting... C:\src>                   Calling recv()...                   Server message: MESSAGE That's okay so far, but what makes cases [1]/[3] different from all cases in the whole existing test suite, which now performed successfully, is that psql calls send() before recv() on a socket closed and abandoned by the server. Those programs show on Windows: C:\src>server shutdown closesocket Listening for incoming connections...                   C:\src>client send_before_recv Client connected: 127.0.0.1:64397                   Connection to server established. Enter message: msg Client message: msg Sending message...                   Sleeping... Calling shutdown()... Calling closesocket()... Exiting... C:\src>                   send() returned 4                   Calling recv()...                   recv() failed As known, on Linux the same scenario works just fine. Fourth, tests 099_rare_ssl_failures (and 001_ssl_tests, though more rarely) fail for me with the latest patches (only on Windows again): ...  8/10 postgresql:ssl_1 / ssl_1/099_rare_ssl_failures ERROR          141.34s  exit status 3 ...  9/10 postgresql:ssl_7 / ssl_7/099_rare_ssl_failures OK             142.52s  2000 subtests passed 10/10 postgresql:ssl_6 / ssl_6/099_rare_ssl_failures OK             143.00s  2000 subtests passed Ok:                2 Expected Fail:     0 Fail:              8 ssl_1\099_rare_ssl_failures\log\regress_log_099_rare_ssl_failures.txt: ... iteration 354 [20:57:06.984](0.106s) ok 707 - certificate authorization fails with revoked client cert with server-side CRL directory [20:57:06.984](0.000s) ok 708 - certificate authorization fails with revoked client cert with server-side CRL directory: matches iteration 355 [20:57:07.156](0.172s) ok 709 - certificate authorization fails with revoked client cert with server-side CRL directory [20:57:07.156](0.001s) not ok 710 - certificate authorization fails with revoked client cert with server-side CRL directory: matches [20:57:07.159](0.003s) #  Failed test 'certificate authorization fails with revoked client cert with server-side CRL directory: matches' #  at .../src/test/ssl_1/t/099_rare_ssl_failures.pl line 88. [20:57:07.159](0.000s) #                  'psql: error: connection to server at "127.0.0.1", port 59843 failed: could not receive data from server: Software caused connection abort (0x00002745/10053) # SSL SYSCALL error: Software caused connection abort (0x00002745/10053)' #    doesn't match '(?^:SSL error: sslv3 alert certificate revoked)' ... It seems to me that it can have the same explanation (if openssl can call send() before recv() under the hood), but maybe it should be investigated further. > Review/poking-with-a-stick/trying-to-break-it most welcome. I could not find anything suspicious in the code, except for maybe a typo "The are ...". [1] https://www.postgresql.org/message-id/flat/87k1iy44fd.fsf%40news-spur.riddles.org.uk#ba0c07f13c300d4... [2] https://www.postgresql.org/message-id/flat/CAEepm%3D2n6Nv%2B5tFfe8YnkUm1fXgvxR0Mm1FoD%2BQKG-vLNGLyKg... [3] https://www.postgresql.org/message-id/flat/[email protected] [4] https://www.postgresql.org/message-id/flat/CALDaNm2tEvr_Kum7SyvFn0%3D6H3P0P-Zkhnd%3DdkkX%2BQ%3DwKutZ... [5] https://www.postgresql.org/message-id/flat/20200603201242.ofvm4jztpqytwfye%40alap3.anarazel.de [6] https://www.postgresql.org/message-id/[email protected] [7] https://www.postgresql.org/message-id/flat/90b34057-4176-7bb0-0dbb-9822a5f6425b%40greiz-reinsdorf.de [8] https://www.postgresql.org/message-id/flat/af5e0bf3-6a61-bb97-6cba-061ddf22ff6b%40dunslane.net [9] https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BOeoETZQ%3DQw5Ub5h3tmwQhBmDA%3DnuNO3KG%3DzWfUy... [10] https://www.postgresql.org/message-id/flat/17391-304f81bcf724b58b%40postgresql.org Best regards, Alexander With pg_usleep(10000L) added in pqReadData() you can see several tests failed when running make check-world/meson test. For example: meson test recovery/019_replslot_limit results in: [16:25:18.510](6.771s) ok 16 - check if checkpoint command is not blocked ### Stopping node "primary2" using mode fast # Running: pg_ctl -D .../build/testrun/recovery/019_replslot_limit/data/t_019_replslot_limit_primary2_data/pgdata -m fast stop waiting for server to shut down............................................................... failed pg_ctl: server does not shut down # pg_ctl stop failed: 256 # Postmaster PID for node "primary2" is 209253 [16:26:18.567](60.057s) Bail out! pg_ctl stop failed As I understand, it happens because WalSndLoop() performs this in a loop: ... if (got_SIGUSR2) WalSndDone(send_data); ... /* Sleep until something happens or we time out */ WalSndWait(wakeEvents, sleeptime, WAIT_EVENT_WAL_SENDER_MAIN); ... where WalSndDone() does this: replicatedPtr = XLogRecPtrIsInvalid(MyWalSnd->flush) ? MyWalSnd->write : MyWalSnd->flush; if (WalSndCaughtUp && sentPtr == replicatedPtr && !pq_is_send_pending()) { ... proc_exit(0); } if (!waiting_for_ping_response) WalSndKeepalive(true, InvalidXLogRecPtr); (WalSndWait's call chain is: WalSndWait -> WaitEventSetWait -> WaitEventSetWaitBlock -> epoll_wait) So, WalSndDone waits here for replicatedPtr to become equal to sentPtr. In other words, walsender sends keepalive to walreceiver and waits for a new flush position from it. And the standby loop is as follows: WalReceiverMain: for (;;) { if (len > 0) { ... XLogWalRcvProcessMsg(buf[0], &buf[1], len - 1, startpointTLI); ... } else if (len == 0) break; } ... XLogWalRcvFlush(false, startpointTLI); where XLogWalRcvProcessMsg() calls XLogWalRcvSendReply() if a keepalive message received. So if walreceiver constantly receives some messages (keepalive messages in this case), it just can't get out of the loop and perform XLogWalRcvFlush() /move Flush position what primary is waiting for. But if something delays walsender for more than 10 ms between epoll_wait() and WalSndDone() on a next iteration, standby manages to get len = 0 and to perform Flush. : # git reset --hard b282fa88d~1 && git clean -dfx : # git apply .../win-sock-tests-01.patch : # cp .../v2-*.patch . : # Setup an alias for python3 on Windows : # cd .\\. && mklink c:\Python3\python3.exe c:\Python3\python.exe REM or something like that cd .\\. && @echo off : # Multiply ssl tests perl -i.bak -ne "print unless /ssl_/" "src/test/meson.build" && python3 -c "for i in range(1,10+1):import os;import shutil;sd=\"src/test/ssl/\";td=f\"src/test/ssl_{i}\";shutil.rmtree(td,ignore_errors=1);shutil.copytree(sd, td);assert(os.system(f'perl -i.bak -pe \"s#( subdir.\'ssl\'.)# subdir(\'ssl_{i}\'){chr(92)}n{chr(92)}1#\" \"src/test/meson.build\"')==0);assert(os.system(f'perl -i.bak -pe \"s#\'ssl\',#\'ssl_{i}\',#\" \"{td}/meson.build\"')==0)" : # Multiply test_misc tests perl -i.bak -ne "print unless /test_misc_/" "src/test/modules/meson.build" && python3 -c "for i in range(1,10+1):import os;import shutil;pd=\"src/test/modules\";m=\"test_misc\";td=f\"{pd}/{m}_{i}\";shutil.rmtree(td,ignore_errors=1);shutil.copytree(f\"{pd}/{m}\", td);assert(os.system(f'perl -i.bak -pe \"s#(subdir.\'{m}\'.)#subdir(\'{m}_{i}\'){chr(92)}n{chr(92)}1#\" \"{pd}/meson.build\"')==0);assert(os.system(f'perl -i.bak -pe \"s#(\'name\': \'{m})#{chr(92)}1_{i}#\" \"{td}/meson.build\"')==0)" : # Multiply postgres_fdw tests perl -i.bak -ne "print unless /postgres_fdw_/" "contrib/meson.build" && python3 -c "for i in range(1,10+1):import os;import shutil;pd=\"contrib\";m=\"postgres_fdw\";td=f\"{pd}/{m}_{i}\";shutil.rmtree(td,ignore_errors=1);shutil.copytree(f\"{pd}/{m}\", td);assert(os.system(f'perl -i.bak -pe \"s#(subdir.\'{m}\'.)#subdir(\'{m}_{i}\'){chr(92)}n{chr(92)}1#\" \"{pd}/meson.build\"')==0);assert(os.system(f'perl -i.bak -pe \"s#(\'name\': \'{m})#{chr(92)}1_{i}#\" \"{td}/meson.build\"')==0)" cd .\\. && meson setup --wipe build -Dcassert=true -Db_pch=true -Dextra_lib_dirs="C:\Program Files\OpenSSL-Win64\lib" -Dextra_include_dirs="C:\Program Files\OpenSSL-Win64\include" -DPG_TEST_EXTRA="ssl" : && meson setup build -Dcassert=true -DPG_TEST_EXTRA="ssl" cd build && ninja >ninja.log && meson test --suite setup echo # echo ======================================== echo Re: BUG 15598: PostgreSQL Error Code is not reported when connection terminated due to idle-in-transaction timeout meson test test_misc/099_case_15598 : # this test fails on Windows without 6051857fc due to "psql:<stdin>:6: server closed the connection unexpectedly" grep -r "psql stderr:" "testrun/test_misc/099_case_15598" : # this test fails on Windows with 6051857fc due to "psql:<stdin>:6: could not receive data from server: Software caused connection abort (0x00002745/10053)" echo # echo ======================================== echo Rare SSL failures on eelpout python3 -c "NUMITERATIONS=10;NUMTESTS=10;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"ssl_{i}/099_rare_ssl_failures \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test --num-processes {NUMTESTS} {tsts}\") == 0)')" : # this test fails on Windows sporadically with "server closed the connection unexpectedly" : # this test (v12-compatible variation) fails on 1f39a1c06~1 (on iterations 1, 1, 1 for me) : # PG_TEST_EXTRA=ssl make -s check -C src/test/ssl; rm -rf src/test/ssl_*; for i in `seq 20`; do cp -r src/test/ssl/ src/test/ssl_$i; sed "s|src/test/ssl|src/test/ssl_$i|" -i src/test/ssl_$i/Makefile; done; for ((i=1;i<=10;i++)); do echo "ITERATION $i"; parallel --halt now,fail=1 -j20 --linebuffer --tag PG_TEST_EXTRA=ssl make -s check -C src/test/ssl_{} PROVE_TESTS="t/099_rare_ssl_failures-v12.pl" NO_TEMP_INSTALL=1 ::: `seq 20` || break; done echo # echo ======================================== echo pgsql: Add tests for '-f' option in dropdb utility. meson test test_misc/051_dropdb_force : # this test fails on Windows without 6051857fc due to "psql:<stdin>:4: server closed the connection unexpectedly" grep -r "psql:<stdin>:" "testrun/test_misc/051_dropdb_force" : # this test fails on Windows with 6051857fc due to "psql:<stdin>:4: could not receive data from server: Software caused connection abort (0x00002745/10053)" echo # echo ======================================== echo libpq copy error handling busted meson test test_misc/099_pgbench_with_server_off : # this test fails before 7247e243a, to revert that commit: : # git show 0245f8db3 src/interfaces/libpq/fe-secure.c | git apply -R && git show a9e9a9f32 src/interfaces/libpq/fe-secure.c | git apply -R && git show faa189c932d | git apply -R : # git show 7247e243a | git apply -R : # this test passes after 7247e243a, also survives 50 iterations x 10 parallel processes : # python3 -c "NUMITERATIONS=10;NUMTESTS=10;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"test_misc_{i}/099_pgbench_with_server_off \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test --num-processes {NUMTESTS} {tsts}\") == 0)')" echo # echo ======================================== echo BUG 16678: The ecpg connect/test5 test sometimes fails on Windows perl -i.bak -pe "s/(.*OK, try to read some data)/pg_usleep(10000L);\n\1/" "../src/interfaces/libpq/fe-misc.c" perl -i.bak -pe "s/(.*WalSndWait\(wakeEvents, sleeptime, WAIT_EVENT_WAL_SENDER_MAIN\);)/\1\npg_usleep(11000L);/" "../src/backend/replication/walsender.c" : # sleep in walsender.c needed due to the anomaly "walsender cannot exit" : # Without it, these tests fail on Linux and on Windows: recovery/001_stream_rep recovery/009_twophase recovery/012_subtransactions recovery/021_row_visibility recovery/032_relfilenode_reuse recovery/035_standby_logical_decoding pg_rewind/008_min_recovery_point ninja && meson test --suite setup meson test ecpg/ecpg meson test pg_dump/002_pg_dump : # Failed tests, fixed with 6051857fc: pg_upgrade/002_pg_upgrade isolation/isolation recovery/019_replslot_limit recovery/037_invalid_database test_decoding/isolation ecpg/ecpg pg_amcheck/002_nonesuch pg_dump/002_pg_dump pgbench/001_pgbench_with_server psql/001_basic scripts/011_clusterdb_all scripts/091_reindexdb_all scripts/101_vacuumdb_all cd .. && git show 6051857fc | git apply && cd build && ninja && meson test --suite setup meson test pg_upgrade/002_pg_upgrade isolation/isolation recovery/019_replslot_limit recovery/037_invalid_database test_decoding/isolation ecpg/ecpg pg_amcheck/002_nonesuch pg_dump/002_pg_dump pgbench/001_pgbench_with_server psql/001_basic scripts/011_clusterdb_all scripts/091_reindexdb_all scripts/101_vacuumdb_all : # Test recovery/019_replslot_limit fails on Linux with the delay added, but doesn't fail on Windows (with 6051857fc) echo # echo ======================================== echo MSVC SSL test failure meson test ssl/001_ssltests : # this test fails (on a first run, with the sleep added) after 6051857fc with an error "server closed the connection unexpectedly" cd .. && git show ed52c3707 | git apply && cd build && ninja && meson test --suite setup meson test ssl/001_ssltests : # this test passes (with the sleep added) after 6051857fc + ed52c3707 echo # echo ======================================== echo Why is src/test/modules/committs/t/002_standby.pl flaky? cd .. git restore "src/interfaces/libpq/fe-misc.c" "src/backend/replication/walsender.c" git show 6051857fc | git apply && git show ed52c3707 | git apply cd build && ninja >ninja.log && meson test --suite setup python3 -c "NUMITERATIONS=10;NUMTESTS=10;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"test_misc_{i}/099_commit_ts_002_standby \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test --num-processes {NUMTESTS} {tsts}\") == 0)')" : # this test failed on iterations 1, 3, 1 for me (without the sleep but with 6051857fc + ed52c3707) python3 -c "NUMITERATIONS=10;NUMTESTS=1;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"postgres_fdw_{i}/099_postgres_fdw_disconnect \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test -t 0.2 --num-processes {NUMTESTS} {tsts}\") == 0)')" : # this test also failed (with TIMEOUT) on iterations 4, 6, 2 for me cd .\\. && echo Killing postgres processes left behind failing tests && taskkill /F /FI "IMAGENAME eq postgres.exe" cd .. && git show ed52c3707 | git apply -R && git show 6051857fc | git apply -R && cd build && ninja >ninja.log && meson test --suite setup python3 -c "NUMITERATIONS=10;NUMTESTS=10;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"test_misc_{i}/099_commit_ts_002_standby \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test --num-processes {NUMTESTS} {tsts}\") == 0)')" : # 10 iterations succeeded (without the sleep and without 6051857fc + ed52c3707) echo # echo ======================================== echo BUG 17391: While using --with-ssl=openssl and PG_TEST_EXTRA='ssl' options, SSL tests fail on OpenBSD 7.0 : # Linux-only part; use make because meson.build doesn't play well with libressl for now : && cd .. : # Revert faa189c93 (we also need non-essential changes a9e9a9f32 and 0245f8db3 above) : && git show 0245f8db3 src/interfaces/libpq/fe-secure.c | git apply -R && git show a9e9a9f32 src/interfaces/libpq/fe-secure.c | git apply -R && git show faa189c93 | git apply -R : && ( cd /tmp/ && rm -rf libressl*; wget https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-3.4.1.tar.gz && tar fax libressl-3.4.1.tar.gz && cd libressl-3.4.1/ && ./configure -q --prefix=/tmp/libressl && make -j`nproc` >/dev/null && make install >/dev/null ) : && PATH="/tmp/libressl/bin/:$PATH" sh -c "./configure --enable-tap-tests --with-ssl=openssl --with-includes=/tmp/libressl/include --with-libs=/tmp/libressl/lib && make -s -j8" : && PG_TEST_EXTRA='ssl' LD_LIBRARY_PATH="/tmp/libressl/lib" make -s check -C src/test/ssl PROVE_TESTS="t/001*" : # this test fails without faa189c93 : && grep -r '# skip SSL_CERT_FILE is not supported' src/test/ssl/tmp_check/log/ # make sure that LibreSSL is used : && make -s clean : # Undo all the changes : && git show faa189c93 | git apply && git show a9e9a9f32 src/interfaces/libpq/fe-secure.c | git apply && git show 0245f8db3 src/interfaces/libpq/fe-secure.c | git apply : && PATH="/tmp/libressl/bin/:$PATH" sh -c "./configure --enable-tap-tests --with-ssl=openssl --with-includes=/tmp/libressl/include --with-libs=/tmp/libressl/lib && make -s -j8" : && PG_TEST_EXTRA='ssl' LD_LIBRARY_PATH="/tmp/libressl/lib" make -s check -C src/test/ssl PROVE_TESTS="t/001*" : # this test passes with faa189c93 (on current master) : && grep -r '# skip SSL_CERT_FILE is not supported' src/test/ssl/tmp_check/log/ # make sure that LibreSSL is used : && make -s clean && make -s distclean && cd build echo # echo ======================================== echo Applying all the latest patches cd .. git apply v2-0001-simplehash-Allow-raw-memory-to-be-freed.patch v2-0002-simplehash-Allow-raw-allocation-to-fail.patch git apply v2-0003-Redesign-Windows-socket-event-management.patch git apply v2-0004-Remove-pgwin32_select.patch git apply v2-0005-Refactor-pgwin32_waitforsinglesocket-to-share-eve.patch git apply v2-0006-Reinstate-graceful-shutdown-changes-for-Windows.patch cd build && ninja >ninja.log && meson test --suite setup echo # echo ======================================== echo Checking whether commit_ts_002_standby is still flaky... python3 -c "NUMITERATIONS=10;NUMTESTS=10;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"test_misc_{i}/099_commit_ts_002_standby \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test --num-processes {NUMTESTS} {tsts}\") == 0)')" : # no failures on Linux and Windows echo Checking whether 099_postgres_fdw_disconnect is still flaky... python3 -c "NUMITERATIONS=10;NUMTESTS=1;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"postgres_fdw_{i}/099_postgres_fdw_disconnect \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test -t 0.2 --num-processes {NUMTESTS} {tsts}\") == 0)')" : # no failures on Linux and Windows : # tests 051_dropdb_force, 099_case_15598 still fail on Windows due to the anomaly "send() before recv() invalidates a closed socket" Attachments: [text/plain] walsender-cannot-exit.txt (2.2K, ../../[email protected]/3-walsender-cannot-exit.txt) download | inline: With pg_usleep(10000L) added in pqReadData() you can see several tests failed when running make check-world/meson test. For example: meson test recovery/019_replslot_limit results in: [16:25:18.510](6.771s) ok 16 - check if checkpoint command is not blocked ### Stopping node "primary2" using mode fast # Running: pg_ctl -D .../build/testrun/recovery/019_replslot_limit/data/t_019_replslot_limit_primary2_data/pgdata -m fast stop waiting for server to shut down............................................................... failed pg_ctl: server does not shut down # pg_ctl stop failed: 256 # Postmaster PID for node "primary2" is 209253 [16:26:18.567](60.057s) Bail out! pg_ctl stop failed As I understand, it happens because WalSndLoop() performs this in a loop: ... if (got_SIGUSR2) WalSndDone(send_data); ... /* Sleep until something happens or we time out */ WalSndWait(wakeEvents, sleeptime, WAIT_EVENT_WAL_SENDER_MAIN); ... where WalSndDone() does this: replicatedPtr = XLogRecPtrIsInvalid(MyWalSnd->flush) ? MyWalSnd->write : MyWalSnd->flush; if (WalSndCaughtUp && sentPtr == replicatedPtr && !pq_is_send_pending()) { ... proc_exit(0); } if (!waiting_for_ping_response) WalSndKeepalive(true, InvalidXLogRecPtr); (WalSndWait's call chain is: WalSndWait -> WaitEventSetWait -> WaitEventSetWaitBlock -> epoll_wait) So, WalSndDone waits here for replicatedPtr to become equal to sentPtr. In other words, walsender sends keepalive to walreceiver and waits for a new flush position from it. And the standby loop is as follows: WalReceiverMain: for (;;) { if (len > 0) { ... XLogWalRcvProcessMsg(buf[0], &buf[1], len - 1, startpointTLI); ... } else if (len == 0) break; } ... XLogWalRcvFlush(false, startpointTLI); where XLogWalRcvProcessMsg() calls XLogWalRcvSendReply() if a keepalive message received. So if walreceiver constantly receives some messages (keepalive messages in this case), it just can't get out of the loop and perform XLogWalRcvFlush() /move Flush position what primary is waiting for. But if something delays walsender for more than 10 ms between epoll_wait() and WalSndDone() on a next iteration, standby manages to get len = 0 and to perform Flush. [text/x-csrc] server.c (2.1K, ../../[email protected]/4-server.c) download | inline: #include <stdio.h> #include <string.h> #if defined(_WIN32) #include <winsock.h> #pragma comment(lib, "Ws2_32.lib") #define SD_SEND 1 #else #include <sys/socket.h> #include <arpa/inet.h> #endif int main(int argc, char *argv[]) { char *server_message = "MESSAGE"; char client_message[2000] = { 0 }; #if defined(_WIN32) WSADATA wsaData; int iResult = WSAStartup(MAKEWORD(2,2), &wsaData); if (iResult != NO_ERROR) { printf("WSAStartup failed: %d\n", iResult); return 1; } #endif int socket_desc = socket(AF_INET, SOCK_STREAM, 0); if (socket_desc < 0) {printf("Could not create socket\n"); return 1;} struct sockaddr_in server_addr; server_addr.sin_family = AF_INET; server_addr.sin_port = htons(2000); server_addr.sin_addr.s_addr = inet_addr("127.0.0.1"); if (bind(socket_desc, (struct sockaddr *)&server_addr, sizeof(server_addr)) < 0) { printf("bind() failed\n"); return 1; } if (listen(socket_desc, 1) < 0) { printf("listen() failed\n"); return 1; } printf("Listening for incoming connections...\n"); struct sockaddr_in client_addr; int client_size = sizeof(client_addr); int client_sock = accept(socket_desc, (struct sockaddr*)&client_addr, &client_size); if (client_sock < 0) { printf("accept() failed\n"); return 1; } printf("Client connected: %s:%i\n", inet_ntoa(client_addr.sin_addr), ntohs(client_addr.sin_port)); if (recv(client_sock, client_message, sizeof(client_message), 0) < 0) { printf("recv() failed\n"); return -1; } printf("Client message: %s\n", client_message); printf("Sending message...\n"); if (send(client_sock, server_message, strlen(server_message), 0) < 0) { printf("send() failed\n"); return 1; } #if defined(_WIN32) if (argc > 1 && strcmp(argv[1], "shutdown") == 0) { printf("Calling shutdown()...\n"); shutdown(client_sock, SD_SEND); } if ((argc > 1 && strcmp(argv[argc - 1], "closesocket") == 0)) { printf("Calling closesocket()...\n"); closesocket(client_sock); } #endif printf("Exiting...\n"); return 0; } [text/x-csrc] client.c (1.7K, ../../[email protected]/5-client.c) download | inline: #include <stdio.h> #include <string.h> #if defined(_WIN32) #include <windows.h> #include <winsock.h> #pragma comment (lib, "Ws2_32.lib") #define sleep(s) Sleep((s) * 1000) #else #include <sys/socket.h> #include <arpa/inet.h> #include <unistd.h> #endif int main(int argc, char *argv[]) { char server_message[2000] = { 0 }; char client_message[2000] = { 0 }; #if defined(_WIN32) WSADATA wsaData; int iResult = WSAStartup(MAKEWORD(2,2), &wsaData); if (iResult != NO_ERROR) { printf("WSAStartup failed: %d\n", iResult); return 1; } #endif int socket_desc = socket(AF_INET, SOCK_STREAM, 0); if (socket_desc < 0) { printf("Could not create socket\n"); return -1; } struct sockaddr_in server_addr; server_addr.sin_family = AF_INET; server_addr.sin_port = htons(2000); server_addr.sin_addr.s_addr = inet_addr("127.0.0.1"); if (connect(socket_desc, (struct sockaddr*)&server_addr, sizeof(server_addr)) < 0) { printf("Could not connect\n"); return -1; } printf("Connection to server established. Enter message: "); fgets(client_message, sizeof(client_message), stdin); if (send(socket_desc, client_message, strlen(client_message), 0) < 0) { printf("send() failed\n"); return -1; } printf("Sleeping...\n"); sleep(3); if (argc > 1 && strcmp(argv[1], "send_before_recv") == 0) { int r = send(socket_desc, client_message, strlen(client_message), 0); printf("send() returned %d\n", r); } printf("Calling recv()...\n"); if (recv(socket_desc, server_message, sizeof(server_message), 0) < 0) { printf("recv() failed\n"); return 1; } printf("Server message: %s\n", server_message); return 0; } [text/x-patch] win-sock-tests-01.patch (19.5K, ../../[email protected]/6-win-sock-tests-01.patch) download | inline diff: diff --git a/contrib/postgres_fdw/meson.build b/contrib/postgres_fdw/meson.build index 2b451f165e..674ab0a7b9 100644 --- a/contrib/postgres_fdw/meson.build +++ b/contrib/postgres_fdw/meson.build @@ -39,4 +39,9 @@ tests += { ], 'regress_args': ['--dlpath', meson.build_root() / 'src/test/regress'], }, + 'tap': { + 'tests': [ + 't/099_postgres_fdw_disconnect.pl', + ], + }, } diff --git a/contrib/postgres_fdw/t/099_postgres_fdw_disconnect.pl b/contrib/postgres_fdw/t/099_postgres_fdw_disconnect.pl new file mode 100644 index 0000000000..f1b51fb448 --- /dev/null +++ b/contrib/postgres_fdw/t/099_postgres_fdw_disconnect.pl @@ -0,0 +1,87 @@ +# Test postgres_fdw reaction on target server disconnection + +use strict; +use warnings; + +use PostgreSQL::Test::Utils; +use Test::More tests => 1; +use PostgreSQL::Test::Cluster; + +use Time::HiRes qw(time usleep); +use DateTime; + +use threads; +use threads::shared; + +sub restart_thread { + my @args = @_; + my ($server) = @args; + $SIG{'KILL'} = sub { threads->exit(); }; + sleep(3); + for (my $i = 0; $i < 100; $i++) { + sleep(5); + $server->restart(); + } +} + +my $foreign = PostgreSQL::Test::Cluster->new('foreign'); +$foreign->init(); + +$foreign->append_conf( + 'postgresql.conf', qq{ +log_min_messages = DEBUG1 +log_min_error_statement = log +log_connections = on +log_disconnections = on +log_line_prefix = '%m|%u|%d|%c|' +log_statement = 'all' + }); +$foreign->start; +my $foreign_port = $foreign->port; + +my $local = PostgreSQL::Test::Cluster->new('local'); +$local->init(); + +$local->append_conf( + 'postgresql.conf', qq{ +log_min_messages = DEBUG1 +log_min_error_statement = log +log_connections = on +log_disconnections = on +log_line_prefix = '%m|%u|%d|%c|' +log_statement = 'all' + }); +$local->start(); + +$foreign->safe_psql('postgres', qq{ +CREATE TABLE large(a int, t text); +INSERT INTO large SELECT x, rpad(x::text, 100) FROM generate_series(0, 999999) x; +}); + + +$local->safe_psql('postgres', qq{ +CREATE EXTENSION postgres_fdw; +CREATE SERVER fpg FOREIGN DATA WRAPPER postgres_fdw OPTIONS (dbname 'postgres', port '$foreign_port'); +CREATE USER MAPPING FOR CURRENT_USER SERVER fpg; +CREATE FUNCTION fx2(i integer) RETURNS int AS 'begin return i * 2; end;' LANGUAGE plpgsql; +}); + +my $outputdir = $PostgreSQL::Test::Utils::tmp_check; + +$local->safe_psql('postgres', 'IMPORT FOREIGN SCHEMA public FROM SERVER fpg INTO public;'); +print($local->psql('postgres', 'EXPLAIN (VERBOSE) SELECT * FROM large WHERE a = fx2(a)') . "\n"); +my $thread = threads->create('restart_thread', $foreign); +$ENV{PGHOST} = $local->host; +$ENV{PGPORT} = $local->port; +$ENV{PGCTLTIMEOUT} = 180; +for (my $i = 1; $i <= 50; $i++) { + diag(" executing query ($i)..."); +# Avoid using IPC::Run due to it's own quirks +# my ($ret, $stdout, $stderr) = $local->psql('postgres', 'SELECT * FROM large WHERE a = fx2(a)'); + my $ret = system("psql postgres -c \"SELECT $i i, * FROM large WHERE a = fx2(a)\" >>\"$outputdir/psql.log\" 2>&1"); + diag(" result: \t$ret"); +} +$thread->kill('KILL')->detach; +sleep(2); +$foreign->_update_pid(1); +ok(1); diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build index 911084ac0f..bee227eabe 100644 --- a/src/test/modules/test_misc/meson.build +++ b/src/test/modules/test_misc/meson.build @@ -10,6 +10,11 @@ tests += { 't/002_tablespace.pl', 't/003_check_guc.pl', 't/004_io_direct.pl', + 't/099_case_15598.pl', + 't/051_dropdb_force.pl', + 't/099_pgbench_with_server_off.pl', + 't/099_ecpg_16678.pl', + 't/099_commit_ts_002_standby.pl', ], }, } diff --git a/src/test/modules/test_misc/t/051_dropdb_force.pl b/src/test/modules/test_misc/t/051_dropdb_force.pl new file mode 100644 index 0000000000..94d170f081 --- /dev/null +++ b/src/test/modules/test_misc/t/051_dropdb_force.pl @@ -0,0 +1,106 @@ +# +# Tests the force option of drop database command. +# +use strict; +use warnings; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->start; + +# Create a database that will be dropped. This will test that the force +# option works when no other backend is connected to the database being +# dropped. +$node->safe_psql('postgres', 'CREATE DATABASE foobar'); +$node->issues_sql_like( + [ 'dropdb', '--force', 'foobar' ], + qr/statement: DROP DATABASE foobar WITH \(FORCE\);/, + 'SQL DROP DATABASE (FORCE) run'); + +# database foobar must not exist. +is( $node->safe_psql( + 'postgres', + qq[SELECT EXISTS(SELECT * FROM pg_database WHERE datname='foobar');] + ), + 'f', + 'database foobar was removed'); + +# Create a database that will be dropped. This will test that the force +# option works when one other backend is connected to the database being +# dropped. +$node->safe_psql('postgres', 'CREATE DATABASE foobar1'); + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('foobar1') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Ensure killme process is active. +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok( pump_until( + $killme, $psql_timeout, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGTERM'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Check the connections on foobar1 database. +is( $node->safe_psql( + 'postgres', + qq[SELECT pid FROM pg_stat_activity WHERE datname='foobar1' AND pid = $pid;] + ), + $pid, + 'database foobar1 is used'); + +# Now drop database with dropdb --force command. +$node->issues_sql_like( + [ 'dropdb', '--force', 'foobar1' ], + qr/statement: DROP DATABASE foobar1 WITH \(FORCE\);/, + 'SQL DROP DATABASE (FORCE) run'); + +# Check that psql sees the killed backend as having been terminated. +$killme_stdin .= q[ +SELECT 1; +]; +ok( pump_until( + $killme, $psql_timeout, \$killme_stderr, + qr/FATAL: terminating connection due to administrator command/m), + "psql query died successfully after SIGTERM"); +$killme_stderr = ''; +$killme_stdout = ''; +$killme->finish; + +# database foobar1 must not exist. +is( $node->safe_psql( + 'postgres', + qq[SELECT EXISTS(SELECT * FROM pg_database WHERE datname='foobar1');] + ), + 'f', + 'database foobar1 was removed'); + +$node->stop(); + +done_testing(); diff --git a/src/test/modules/test_misc/t/099_case_15598.pl b/src/test/modules/test_misc/t/099_case_15598.pl new file mode 100644 index 0000000000..57243b04d7 --- /dev/null +++ b/src/test/modules/test_misc/t/099_case_15598.pl @@ -0,0 +1,61 @@ +# Run ecpg regression tests in a loop +use strict; +use warnings; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; +use File::Basename; + +# Initialize node +my $node = PostgreSQL::Test::Cluster->new('p1'); +$node->init( +); + +$node->append_conf( + 'postgresql.conf', + qq{ +}); + +$node->start; + +$node->safe_psql('postgres', 'select 1'); + +my $psql_timeout = IPC::Run::timer(10); +# Run psql, ... +my ($psql_stdin, $psql_stdout, $psql_stderr) = ('', '', ''); +my $psql = IPC::Run::start( + [ + 'psql', '-X', '-At', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$psql_stdin, + '>', + \$psql_stdout, + '2>', + \$psql_stderr, + $psql_timeout); + +$psql_stdin .= q[ +SET idle_in_transaction_session_timeout=500; +BEGIN; +SELECT * FROM pg_class; +]; + +$psql->pump_nb(); +sleep(1); + +$psql_stdin .= q[ +SELECT 'OK'; +]; + +ok(pump_until( + $psql, $psql_timeout, \$psql_stderr, qr/FATAL: terminating connection due to idle-in-transaction timeout/m), "expected FATAL message received"); + +print($psql_stdin . "\n"); +print("psql stdout: ". $psql_stdout . "\npsql stderr: " . $psql_stderr. "\n"); + +ok(1); +$node->stop; + +done_testing(); diff --git a/src/test/modules/test_misc/t/099_commit_ts_002_standby.pl b/src/test/modules/test_misc/t/099_commit_ts_002_standby.pl new file mode 100644 index 0000000000..59cc2b1244 --- /dev/null +++ b/src/test/modules/test_misc/t/099_commit_ts_002_standby.pl @@ -0,0 +1,68 @@ + +# Copyright (c) 2021-2023, PostgreSQL Global Development Group + +# Test simple scenario involving a standby + +use strict; +use warnings; + +use PostgreSQL::Test::Utils; +use Test::More; +use PostgreSQL::Test::Cluster; + +my $bkplabel = 'backup'; +my $primary = PostgreSQL::Test::Cluster->new('primary'); +$primary->init(allows_streaming => 1); + +$primary->append_conf( + 'postgresql.conf', qq{ + track_commit_timestamp = on + max_wal_senders = 5 + }); +$primary->start; +$primary->backup($bkplabel); + +my $standby = PostgreSQL::Test::Cluster->new('standby'); +$standby->init_from_backup($primary, $bkplabel, has_streaming => 1); +$standby->start; + +for my $i (1 .. 10) +{ + $primary->safe_psql('postgres', "create table t$i()"); +} +my $primary_ts = $primary->safe_psql('postgres', + qq{SELECT ts.* FROM pg_class, pg_xact_commit_timestamp(xmin) AS ts WHERE relname = 't10'} +); +my $primary_lsn = + $primary->safe_psql('postgres', 'select pg_current_wal_lsn()'); +$standby->poll_query_until('postgres', + qq{SELECT '$primary_lsn'::pg_lsn <= pg_last_wal_replay_lsn()}) + or die "standby never caught up"; + +my $standby_ts = $standby->safe_psql('postgres', + qq{select ts.* from pg_class, pg_xact_commit_timestamp(xmin) ts where relname = 't10'} +); +is($primary_ts, $standby_ts, "standby gives same value as primary"); + +$primary->append_conf('postgresql.conf', 'track_commit_timestamp = off'); +$primary->restart; +$primary->safe_psql('postgres', 'checkpoint'); +$primary_lsn = $primary->safe_psql('postgres', 'select pg_current_wal_lsn()'); +$standby->poll_query_until('postgres', + qq{SELECT '$primary_lsn'::pg_lsn <= pg_last_wal_replay_lsn()}) + or die "standby never caught up"; +$standby->safe_psql('postgres', 'checkpoint'); + +# This one should raise an error now +my ($ret, $standby_ts_stdout, $standby_ts_stderr) = $standby->psql('postgres', + 'select ts.* from pg_class, pg_xact_commit_timestamp(xmin) ts where relname = \'t10\'' +); +is($ret, 3, 'standby errors when primary turned feature off'); +is($standby_ts_stdout, '', + "standby gives no value when primary turned feature off"); +like( + $standby_ts_stderr, + qr/could not get commit timestamp data/, + 'expected error when primary turned feature off'); + +done_testing(); diff --git a/src/test/modules/test_misc/t/099_ecpg_16678.pl b/src/test/modules/test_misc/t/099_ecpg_16678.pl new file mode 100755 index 0000000000..53d1af7fe2 --- /dev/null +++ b/src/test/modules/test_misc/t/099_ecpg_16678.pl @@ -0,0 +1,56 @@ +# Run ecpg regression tests in a loop +use strict; +use warnings; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; +use File::Basename; + +# Initialize primary node +my $node_primary = PostgreSQL::Test::Cluster->new('p1'); +$node_primary->init( + 'auth_extra' => [ '--create-role', 'regress_ecpg_user1,regress_ecpg_user2' ] +); + +$node_primary->start; + +my $outputdir = $PostgreSQL::Test::Utils::tmp_check; +my $NUM_ITERATIONS = 2; + +for (my $i = 0; $i < $NUM_ITERATIONS; $i++) +{ +my $extra_opts = $ENV{EXTRA_REGRESS_OPTS} || ""; + +my $rc = + system("\"$outputdir/../../../../src/interfaces/ecpg/test/pg_regress_ecpg\" " + . " $extra_opts " + . "--dbname=ecpg1_regression,ecpg2_regression --create-role=regress_ecpg_user1,regress_ecpg_user2 " + . "--bindir= " + . "--host=" + . $node_primary->host . " " + . "--port=" + . $node_primary->port . " " + . "--outputdir=$outputdir " + . "--inputdir=$outputdir/../../../../src/interfaces/ecpg/test/ " + . "--expecteddir=../../../../src/interfaces/ecpg/test/ " + . ( "connect/test5 " x 50) +); + +if ($rc != 0) +{ + # Dump out the regression diffs file, if there is one + my $diffs = "$outputdir/regression.diffs"; + if (-e $diffs) + { + print "=== dumping $diffs ===\n"; + print slurp_file($diffs); + print "=== EOF ===\n"; + } +} +is($rc, 0, 'ecpg regression tests pass'); +($rc == 0) || last; +} + +$node_primary->stop; + +done_testing(); diff --git a/src/test/modules/test_misc/t/099_pgbench_with_server_off.pl b/src/test/modules/test_misc/t/099_pgbench_with_server_off.pl new file mode 100644 index 0000000000..409ef1621f --- /dev/null +++ b/src/test/modules/test_misc/t/099_pgbench_with_server_off.pl @@ -0,0 +1,48 @@ + +# Copyright (c) 2021-2023, PostgreSQL Global Development Group + +# +# test pgbench with server stopped abruptly during the pgbench run +# + +use strict; +use warnings; + +use PostgreSQL::Test::Utils; +use PostgreSQL::Test::Cluster; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('main'); + +$node->init(); +$node->start; + +my $pb_timeout = + IPC::Run::timer($PostgreSQL::Test::Utils::timeout_default); +my ($pb_stdin, $pb_stdout, $pb_stderr) = ('', '', ''); + +$ENV{PGDATABASE} = 'postgres'; +$ENV{PGHOST} = $node->host; +$ENV{PGPORT} = $node->port; + +my $pb = IPC::Run::start( + [ + 'pgbench', + '-i', '-s', '1000' + ], + '<', + \$pb_stdin, + '>', + \$pb_stdout, + '2>', + \$pb_stderr, + $pb_timeout); +sleep(5); +$node->stop('immediate'); +ok( pump_until( + $pb, $pb_timeout, + \$pb_stderr, qr/PQputline failed/), + 'pgbench reacted to server shutdown during copy'); +$pb->finish(); + +done_testing(); diff --git a/src/test/ssl/meson.build b/src/test/ssl/meson.build index abb30ab214..9228acf6fb 100644 --- a/src/test/ssl/meson.build +++ b/src/test/ssl/meson.build @@ -13,6 +13,7 @@ tests += { 't/001_ssltests.pl', 't/002_scram.pl', 't/003_sslinfo.pl', + 't/099_rare_ssl_failures.pl', ], }, } diff --git a/src/test/ssl/t/099_rare_ssl_failures-v12.pl b/src/test/ssl/t/099_rare_ssl_failures-v12.pl new file mode 100644 index 0000000000..1c43f00301 --- /dev/null +++ b/src/test/ssl/t/099_rare_ssl_failures-v12.pl @@ -0,0 +1,84 @@ +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More tests => 100; + +use File::Copy; + +use FindBin; +use lib $FindBin::RealBin; + +use SSLServer; + +#### Some configuration + +# This is the hostname used to connect to the server. This cannot be a +# hostname, because the server certificate is always for the domain +# postgresql-ssl-regression.test. +my $SERVERHOSTADDR = '127.0.0.1'; + +my $common_connstr; + +sub connect_fails +{ + my ($node, $connstr, $test_name, %params) = @_; + + my $cmd = [ + 'psql', '-X', '-A', '-t', '-c', "SELECT 'connected with $connstr'", + '-d', "$common_connstr $connstr" ]; + + my ($stdout, $stderr); + + print("# Running: " . join(" ", @{$cmd}) . "\n"); + my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr; + + if (defined($params{expected_stderr})) + { + like($stderr, $params{expected_stderr}, "$test_name: matches"); + } +} + +# The client's private key must not be world-readable, so take a copy +# of the key stored in the code tree and update its permissions. +copy("ssl/client.key", "ssl/client_tmp.key"); +chmod 0600, "ssl/client_tmp.key"; + +#### Part 0. Set up the server. + +note "setting up data directory"; +my $node = get_new_node('master'); +$node->init; + +# PGHOST is enforced here to set up the node, subsequent connections +# will use a dedicated connection string. +$ENV{PGHOST} = $node->host; +$ENV{PGPORT} = $node->port; +$node->start; +configure_test_server_for_ssl($node, $SERVERHOSTADDR, 'trust'); +switch_server_cert($node, 'server-cn-only'); + +### Part 1. Run client-side tests. +### +### Test that libpq accepts/rejects the connection correctly, depending +### on sslmode and whether the server's certificate looks correct. No +### client certificate is used in these tests. + +note "running client tests"; + +$common_connstr = +"user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test"; + + +for (my $i = 1; $i <= 100; $i++) { +print("iteration $i\n"); +connect_fails( + $node, + "user=ssltestuser sslcert=ssl/client-revoked.crt sslkey=ssl/client-revoked.key", + "certificate authorization fails with revoked client cert", + expected_stderr => qr/SSL error: sslv3 alert certificate revoked/, +); +} + +# clean up +unlink "ssl/client_tmp.key"; diff --git a/src/test/ssl/t/099_rare_ssl_failures.pl b/src/test/ssl/t/099_rare_ssl_failures.pl new file mode 100644 index 0000000000..0ca70c0ff6 --- /dev/null +++ b/src/test/ssl/t/099_rare_ssl_failures.pl @@ -0,0 +1,101 @@ +use strict; +use warnings; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +use File::Copy; + +use FindBin; +use lib $FindBin::RealBin; + +use SSL::Server; + +#### Some configuration + +# This is the hostname used to connect to the server. This cannot be a +# hostname, because the server certificate is always for the domain +# postgresql-ssl-regression.test. +my $SERVERHOSTADDR = '127.0.0.1'; +# This is the pattern to use in pg_hba.conf to match incoming connections. +my $SERVERHOSTCIDR = '127.0.0.1/32'; + +my $common_connstr; + +sub connect_fails +{ + my ($node, $connstr, $test_name, %params) = @_; + + my $cmd = [ + 'psql', '-X', '-A', '-t', '-c', "SELECT 'connected with $connstr'", + '-d', "$common_connstr $connstr" ]; + + my ($stdout, $stderr); + + print("# Running: " . join(" ", @{$cmd}) . "\n"); + my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr; + + if (defined($params{expected_stderr})) + { + like($stderr, $params{expected_stderr}, "$test_name: matches"); + } +} + +my $ssl_server = SSL::Server->new(); + +# The client's private key must not be world-readable, so take a copy +# of the key stored in the code tree and update its permissions. +copy("ssl/client.key", "ssl/client_tmp.key"); +chmod 0600, "ssl/client_tmp.key"; + +#### Part 0. Set up the server. + +note "setting up data directory"; +my $node = PostgreSQL::Test::Cluster->new('primary'); +$node->init; + +# PGHOST is enforced here to set up the node, subsequent connections +# will use a dedicated connection string. +$ENV{PGHOST} = $node->host; +$ENV{PGPORT} = $node->port; +$node->start; + +$ssl_server->configure_test_server_for_ssl($node, $SERVERHOSTADDR, + $SERVERHOSTCIDR, 'trust'); + +$ssl_server->switch_server_cert( + $node, + certfile => 'server-cn-only', + crldir => 'root+client-crldir'); + +### Part 1. Run client-side tests. +### +### Test that libpq accepts/rejects the connection correctly, depending +### on sslmode and whether the server's certificate looks correct. No +### client certificate is used in these tests. + +note "running client tests"; + +$common_connstr = +"user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test"; + + +for (my $i = 1; $i <= 1000; $i++) { +print("iteration $i\n"); +$node->connect_fails( + "$common_connstr user=ssltestuser sslcert=ssl/client-revoked.crt " + . $ssl_server->sslkey('client-revoked.key'), + "certificate authorization fails with revoked client cert with server-side CRL directory", + expected_stderr => qr/SSL error: sslv3 alert certificate revoked/, + # temporarily(?) skip this check due to timing issue + # log_like => [ + # qr{Client certificate verification failed at depth 0: certificate revoked}, + # qr{Failed certificate data \(unverified\): subject "/CN=ssltestuser", serial number 2315134995201656577, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"}, + # ] +); +} + +# clean up +unlink "ssl/client_tmp.key"; + +done_testing(); [text/plain] win-sock-tests-01.cmd.txt (12.0K, ../../[email protected]/7-win-sock-tests-01.cmd.txt) download | inline: : # git reset --hard b282fa88d~1 && git clean -dfx : # git apply .../win-sock-tests-01.patch : # cp .../v2-*.patch . : # Setup an alias for python3 on Windows : # cd .\\. && mklink c:\Python3\python3.exe c:\Python3\python.exe REM or something like that cd .\\. && @echo off : # Multiply ssl tests perl -i.bak -ne "print unless /ssl_/" "src/test/meson.build" && python3 -c "for i in range(1,10+1):import os;import shutil;sd=\"src/test/ssl/\";td=f\"src/test/ssl_{i}\";shutil.rmtree(td,ignore_errors=1);shutil.copytree(sd, td);assert(os.system(f'perl -i.bak -pe \"s#( subdir.\'ssl\'.)# subdir(\'ssl_{i}\'){chr(92)}n{chr(92)}1#\" \"src/test/meson.build\"')==0);assert(os.system(f'perl -i.bak -pe \"s#\'ssl\',#\'ssl_{i}\',#\" \"{td}/meson.build\"')==0)" : # Multiply test_misc tests perl -i.bak -ne "print unless /test_misc_/" "src/test/modules/meson.build" && python3 -c "for i in range(1,10+1):import os;import shutil;pd=\"src/test/modules\";m=\"test_misc\";td=f\"{pd}/{m}_{i}\";shutil.rmtree(td,ignore_errors=1);shutil.copytree(f\"{pd}/{m}\", td);assert(os.system(f'perl -i.bak -pe \"s#(subdir.\'{m}\'.)#subdir(\'{m}_{i}\'){chr(92)}n{chr(92)}1#\" \"{pd}/meson.build\"')==0);assert(os.system(f'perl -i.bak -pe \"s#(\'name\': \'{m})#{chr(92)}1_{i}#\" \"{td}/meson.build\"')==0)" : # Multiply postgres_fdw tests perl -i.bak -ne "print unless /postgres_fdw_/" "contrib/meson.build" && python3 -c "for i in range(1,10+1):import os;import shutil;pd=\"contrib\";m=\"postgres_fdw\";td=f\"{pd}/{m}_{i}\";shutil.rmtree(td,ignore_errors=1);shutil.copytree(f\"{pd}/{m}\", td);assert(os.system(f'perl -i.bak -pe \"s#(subdir.\'{m}\'.)#subdir(\'{m}_{i}\'){chr(92)}n{chr(92)}1#\" \"{pd}/meson.build\"')==0);assert(os.system(f'perl -i.bak -pe \"s#(\'name\': \'{m})#{chr(92)}1_{i}#\" \"{td}/meson.build\"')==0)" cd .\\. && meson setup --wipe build -Dcassert=true -Db_pch=true -Dextra_lib_dirs="C:\Program Files\OpenSSL-Win64\lib" -Dextra_include_dirs="C:\Program Files\OpenSSL-Win64\include" -DPG_TEST_EXTRA="ssl" : && meson setup build -Dcassert=true -DPG_TEST_EXTRA="ssl" cd build && ninja >ninja.log && meson test --suite setup echo # echo ======================================== echo Re: BUG 15598: PostgreSQL Error Code is not reported when connection terminated due to idle-in-transaction timeout meson test test_misc/099_case_15598 : # this test fails on Windows without 6051857fc due to "psql:<stdin>:6: server closed the connection unexpectedly" grep -r "psql stderr:" "testrun/test_misc/099_case_15598" : # this test fails on Windows with 6051857fc due to "psql:<stdin>:6: could not receive data from server: Software caused connection abort (0x00002745/10053)" echo # echo ======================================== echo Rare SSL failures on eelpout python3 -c "NUMITERATIONS=10;NUMTESTS=10;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"ssl_{i}/099_rare_ssl_failures \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test --num-processes {NUMTESTS} {tsts}\") == 0)')" : # this test fails on Windows sporadically with "server closed the connection unexpectedly" : # this test (v12-compatible variation) fails on 1f39a1c06~1 (on iterations 1, 1, 1 for me) : # PG_TEST_EXTRA=ssl make -s check -C src/test/ssl; rm -rf src/test/ssl_*; for i in `seq 20`; do cp -r src/test/ssl/ src/test/ssl_$i; sed "s|src/test/ssl|src/test/ssl_$i|" -i src/test/ssl_$i/Makefile; done; for ((i=1;i<=10;i++)); do echo "ITERATION $i"; parallel --halt now,fail=1 -j20 --linebuffer --tag PG_TEST_EXTRA=ssl make -s check -C src/test/ssl_{} PROVE_TESTS="t/099_rare_ssl_failures-v12.pl" NO_TEMP_INSTALL=1 ::: `seq 20` || break; done echo # echo ======================================== echo pgsql: Add tests for '-f' option in dropdb utility. meson test test_misc/051_dropdb_force : # this test fails on Windows without 6051857fc due to "psql:<stdin>:4: server closed the connection unexpectedly" grep -r "psql:<stdin>:" "testrun/test_misc/051_dropdb_force" : # this test fails on Windows with 6051857fc due to "psql:<stdin>:4: could not receive data from server: Software caused connection abort (0x00002745/10053)" echo # echo ======================================== echo libpq copy error handling busted meson test test_misc/099_pgbench_with_server_off : # this test fails before 7247e243a, to revert that commit: : # git show 0245f8db3 src/interfaces/libpq/fe-secure.c | git apply -R && git show a9e9a9f32 src/interfaces/libpq/fe-secure.c | git apply -R && git show faa189c932d | git apply -R : # git show 7247e243a | git apply -R : # this test passes after 7247e243a, also survives 50 iterations x 10 parallel processes : # python3 -c "NUMITERATIONS=10;NUMTESTS=10;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"test_misc_{i}/099_pgbench_with_server_off \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test --num-processes {NUMTESTS} {tsts}\") == 0)')" echo # echo ======================================== echo BUG 16678: The ecpg connect/test5 test sometimes fails on Windows perl -i.bak -pe "s/(.*OK, try to read some data)/pg_usleep(10000L);\n\1/" "../src/interfaces/libpq/fe-misc.c" perl -i.bak -pe "s/(.*WalSndWait\(wakeEvents, sleeptime, WAIT_EVENT_WAL_SENDER_MAIN\);)/\1\npg_usleep(11000L);/" "../src/backend/replication/walsender.c" : # sleep in walsender.c needed due to the anomaly "walsender cannot exit" : # Without it, these tests fail on Linux and on Windows: recovery/001_stream_rep recovery/009_twophase recovery/012_subtransactions recovery/021_row_visibility recovery/032_relfilenode_reuse recovery/035_standby_logical_decoding pg_rewind/008_min_recovery_point ninja && meson test --suite setup meson test ecpg/ecpg meson test pg_dump/002_pg_dump : # Failed tests, fixed with 6051857fc: pg_upgrade/002_pg_upgrade isolation/isolation recovery/019_replslot_limit recovery/037_invalid_database test_decoding/isolation ecpg/ecpg pg_amcheck/002_nonesuch pg_dump/002_pg_dump pgbench/001_pgbench_with_server psql/001_basic scripts/011_clusterdb_all scripts/091_reindexdb_all scripts/101_vacuumdb_all cd .. && git show 6051857fc | git apply && cd build && ninja && meson test --suite setup meson test pg_upgrade/002_pg_upgrade isolation/isolation recovery/019_replslot_limit recovery/037_invalid_database test_decoding/isolation ecpg/ecpg pg_amcheck/002_nonesuch pg_dump/002_pg_dump pgbench/001_pgbench_with_server psql/001_basic scripts/011_clusterdb_all scripts/091_reindexdb_all scripts/101_vacuumdb_all : # Test recovery/019_replslot_limit fails on Linux with the delay added, but doesn't fail on Windows (with 6051857fc) echo # echo ======================================== echo MSVC SSL test failure meson test ssl/001_ssltests : # this test fails (on a first run, with the sleep added) after 6051857fc with an error "server closed the connection unexpectedly" cd .. && git show ed52c3707 | git apply && cd build && ninja && meson test --suite setup meson test ssl/001_ssltests : # this test passes (with the sleep added) after 6051857fc + ed52c3707 echo # echo ======================================== echo Why is src/test/modules/committs/t/002_standby.pl flaky? cd .. git restore "src/interfaces/libpq/fe-misc.c" "src/backend/replication/walsender.c" git show 6051857fc | git apply && git show ed52c3707 | git apply cd build && ninja >ninja.log && meson test --suite setup python3 -c "NUMITERATIONS=10;NUMTESTS=10;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"test_misc_{i}/099_commit_ts_002_standby \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test --num-processes {NUMTESTS} {tsts}\") == 0)')" : # this test failed on iterations 1, 3, 1 for me (without the sleep but with 6051857fc + ed52c3707) python3 -c "NUMITERATIONS=10;NUMTESTS=1;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"postgres_fdw_{i}/099_postgres_fdw_disconnect \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test -t 0.2 --num-processes {NUMTESTS} {tsts}\") == 0)')" : # this test also failed (with TIMEOUT) on iterations 4, 6, 2 for me cd .\\. && echo Killing postgres processes left behind failing tests && taskkill /F /FI "IMAGENAME eq postgres.exe" cd .. && git show ed52c3707 | git apply -R && git show 6051857fc | git apply -R && cd build && ninja >ninja.log && meson test --suite setup python3 -c "NUMITERATIONS=10;NUMTESTS=10;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"test_misc_{i}/099_commit_ts_002_standby \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test --num-processes {NUMTESTS} {tsts}\") == 0)')" : # 10 iterations succeeded (without the sleep and without 6051857fc + ed52c3707) echo # echo ======================================== echo BUG 17391: While using --with-ssl=openssl and PG_TEST_EXTRA='ssl' options, SSL tests fail on OpenBSD 7.0 : # Linux-only part; use make because meson.build doesn't play well with libressl for now : && cd .. : # Revert faa189c93 (we also need non-essential changes a9e9a9f32 and 0245f8db3 above) : && git show 0245f8db3 src/interfaces/libpq/fe-secure.c | git apply -R && git show a9e9a9f32 src/interfaces/libpq/fe-secure.c | git apply -R && git show faa189c93 | git apply -R : && ( cd /tmp/ && rm -rf libressl*; wget https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-3.4.1.tar.gz && tar fax libressl-3.4.1.tar.gz && cd libressl-3.4.1/ && ./configure -q --prefix=/tmp/libressl && make -j`nproc` >/dev/null && make install >/dev/null ) : && PATH="/tmp/libressl/bin/:$PATH" sh -c "./configure --enable-tap-tests --with-ssl=openssl --with-includes=/tmp/libressl/include --with-libs=/tmp/libressl/lib && make -s -j8" : && PG_TEST_EXTRA='ssl' LD_LIBRARY_PATH="/tmp/libressl/lib" make -s check -C src/test/ssl PROVE_TESTS="t/001*" : # this test fails without faa189c93 : && grep -r '# skip SSL_CERT_FILE is not supported' src/test/ssl/tmp_check/log/ # make sure that LibreSSL is used : && make -s clean : # Undo all the changes : && git show faa189c93 | git apply && git show a9e9a9f32 src/interfaces/libpq/fe-secure.c | git apply && git show 0245f8db3 src/interfaces/libpq/fe-secure.c | git apply : && PATH="/tmp/libressl/bin/:$PATH" sh -c "./configure --enable-tap-tests --with-ssl=openssl --with-includes=/tmp/libressl/include --with-libs=/tmp/libressl/lib && make -s -j8" : && PG_TEST_EXTRA='ssl' LD_LIBRARY_PATH="/tmp/libressl/lib" make -s check -C src/test/ssl PROVE_TESTS="t/001*" : # this test passes with faa189c93 (on current master) : && grep -r '# skip SSL_CERT_FILE is not supported' src/test/ssl/tmp_check/log/ # make sure that LibreSSL is used : && make -s clean && make -s distclean && cd build echo # echo ======================================== echo Applying all the latest patches cd .. git apply v2-0001-simplehash-Allow-raw-memory-to-be-freed.patch v2-0002-simplehash-Allow-raw-allocation-to-fail.patch git apply v2-0003-Redesign-Windows-socket-event-management.patch git apply v2-0004-Remove-pgwin32_select.patch git apply v2-0005-Refactor-pgwin32_waitforsinglesocket-to-share-eve.patch git apply v2-0006-Reinstate-graceful-shutdown-changes-for-Windows.patch cd build && ninja >ninja.log && meson test --suite setup echo # echo ======================================== echo Checking whether commit_ts_002_standby is still flaky... python3 -c "NUMITERATIONS=10;NUMTESTS=10;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"test_misc_{i}/099_commit_ts_002_standby \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test --num-processes {NUMTESTS} {tsts}\") == 0)')" : # no failures on Linux and Windows echo Checking whether 099_postgres_fdw_disconnect is still flaky... python3 -c "NUMITERATIONS=10;NUMTESTS=1;import os;tsts='';exec('for i in range(1,NUMTESTS+1): tsts+=f\"postgres_fdw_{i}/099_postgres_fdw_disconnect \"'); exec('for i in range(1,NUMITERATIONS+1):print(f\"iteration {i}\"); assert(os.system(f\"meson test -t 0.2 --num-processes {NUMTESTS} {tsts}\") == 0)')" : # no failures on Linux and Windows : # tests 051_dropdb_force, 099_case_15598 still fail on Windows due to the anomaly "send() before recv() invalidates a closed socket" ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Why is src/test/modules/committs/t/002_standby.pl flaky? 2022-03-22 03:24 Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]> 2022-03-22 14:19 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Tom Lane <[email protected]> 2023-11-10 03:31 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]> 2023-11-21 11:00 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Alexander Lakhin <[email protected]> @ 2024-01-22 04:26 ` Peter Smith <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Peter Smith @ 2024-01-22 04:26 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Thomas Munro <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers 2024-01 Commitfest. Hi, This patch has a CF status of "Needs Review" [1], but it seems like there were CFbot test failures last time it was run [1]. Please have a look and post an updated version if necessary. ====== 1[] https://commitfest.postgresql.org/46/3523/ [1] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/3523 ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Why is src/test/modules/committs/t/002_standby.pl flaky? 2022-03-22 03:24 Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]> 2022-03-22 14:19 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Tom Lane <[email protected]> 2023-11-10 03:31 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]> @ 2024-01-05 17:04 ` Robert Haas <[email protected]> 1 sibling, 0 replies; 12+ messages in thread From: Robert Haas @ 2024-01-05 17:04 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Tom Lane <[email protected]>; Noah Misch <[email protected]>; Alexander Lakhin <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers On Thu, Nov 9, 2023 at 10:32 PM Thomas Munro <[email protected]> wrote: > Here is a new attempt to fix this mess. Disclaimer: this based > entirely on reading the manual and vicariously hacking a computer I > don't have via CI. I'd first like to congratulate this thread on reaching its second birthday. The CommitFest entry hasn't quite made it to the two year mark yet - expect that in another month or so - but thread itself is over the line. Regarding 0001, I don't know if we really need SH_RAW_FREE. You can just define your own SH_FREE implementation in userspace. That doesn't work for SH_RAW_ALLOCATOR because there's code in simplehash.h that knows about memory contexts apart from the actual definition of SH_ALLOCATE - e.g. we include a MemoryContext pointer in SH_TYPE, and in the signature of SH_CREATE. But SH_FREE doesn't seem to have any similar issues. Maybe it's still worth doing for convenience -- I haven't thought about that very hard -- but it doesn't seem to be required in the same way that SH_RAW_ALLOCATOR was. I wonder whether we really want 0002. It seems like a pretty significant behavior change -- now everybody using simplehash has to worry about whether failure cases are possible. And maybe there's some performance overhead. And most of the changes are restricted to the SH_RAW_ALLOCATOR case, but the changes to SH_GROW are not. And making this contingent on SH_RAW_ALLOCATOR doesn't seem principled. I kind of wonder whether trying to handle OOM here is the wrong direction to go. What if we just bail out hard if we can't insert into the hash table? I think that we don't expect the hash table to ever be very large (right?) and we don't install these kinds of defenses everywhere that OOM on a small memory allocation is a possibility (or at least I don't think we do). I'm actually sort of unclear about why you're trying to force this to use raw malloc/free instead of palloc/pfree. Do we need to use this on the frontend side? Do we need it on the backend side prior to the memory context infrastructure being up? -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v5a 6/7] Vacuum first pass uses Streaming Read interface @ 2023-12-31 16:29 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2023-12-31 16:29 UTC (permalink / raw) Now vacuum's first pass, which HOT prunes and records the TIDs of non-removable dead tuples, uses the streaming read API by implementing a streaming read callback which invokes heap_vac_scan_get_next_block(). --- src/backend/access/heap/vacuumlazy.c | 79 +++++++++++++++++++++------- 1 file changed, 59 insertions(+), 20 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 65d257aab83..fbbc87938e4 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -54,6 +54,7 @@ #include "storage/bufmgr.h" #include "storage/freespace.h" #include "storage/lmgr.h" +#include "storage/streaming_read.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/pg_rusage.h" @@ -168,7 +169,12 @@ typedef struct LVRelState char *relnamespace; char *relname; char *indname; /* Current index name */ - BlockNumber blkno; /* used only for heap operations */ + + /* + * The current block being processed by vacuum. Used only for heap + * operations. Primarily for error reporting and logging. + */ + BlockNumber blkno; OffsetNumber offnum; /* used only for heap operations */ VacErrPhase phase; bool verbose; /* VACUUM VERBOSE? */ @@ -189,6 +195,12 @@ typedef struct LVRelState BlockNumber missed_dead_pages; /* # pages with missed dead tuples */ BlockNumber nonempty_pages; /* actually, last nonempty page + 1 */ + /* + * The most recent block submitted in the streaming read callback by the + * first vacuum pass. + */ + BlockNumber blkno_prefetch; + /* Statistics output by us, for table */ double new_rel_tuples; /* new estimated total # of tuples */ double new_live_tuples; /* new estimated total # of live tuples */ @@ -232,7 +244,7 @@ typedef struct LVSavedErrInfo /* non-export function prototypes */ static void lazy_scan_heap(LVRelState *vacrel); -static bool heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, +static void heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, BlockNumber *blkno, bool *all_visible_according_to_vm); static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, @@ -416,6 +428,9 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, vacrel->nonempty_pages = 0; /* dead_items_alloc allocates vacrel->dead_items later on */ + /* relies on InvalidBlockNumber overflowing to 0 */ + vacrel->blkno_prefetch = InvalidBlockNumber; + /* Allocate/initialize output statistics state */ vacrel->new_rel_tuples = 0; vacrel->new_live_tuples = 0; @@ -776,6 +791,22 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, } } +static BlockNumber +vacuum_scan_pgsr_next(PgStreamingRead *pgsr, + void *pgsr_private, void *per_buffer_data) +{ + LVRelState *vacrel = pgsr_private; + bool *all_visible_according_to_vm = per_buffer_data; + + vacrel->blkno_prefetch++; + + heap_vac_scan_get_next_block(vacrel, + vacrel->blkno_prefetch, &vacrel->blkno_prefetch, + all_visible_according_to_vm); + + return vacrel->blkno_prefetch; +} + /* * lazy_scan_heap() -- workhorse function for VACUUM * @@ -815,12 +846,11 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, static void lazy_scan_heap(LVRelState *vacrel) { + Buffer buf; BlockNumber rel_pages = vacrel->rel_pages, next_fsm_block_to_vacuum = 0; - bool all_visible_according_to_vm; + bool *all_visible_according_to_vm; - /* relies on InvalidBlockNumber overflowing to 0 */ - BlockNumber blkno = InvalidBlockNumber; VacDeadItems *dead_items = vacrel->dead_items; const int initprog_index[] = { PROGRESS_VACUUM_PHASE, @@ -828,6 +858,11 @@ lazy_scan_heap(LVRelState *vacrel) PROGRESS_VACUUM_MAX_DEAD_TUPLES }; int64 initprog_val[3]; + PgStreamingRead *pgsr; + + pgsr = pg_streaming_read_buffer_alloc(PGSR_FLAG_MAINTENANCE, vacrel, + sizeof(bool), vacrel->bstrategy, BMR_REL(vacrel->rel), + MAIN_FORKNUM, vacuum_scan_pgsr_next); /* Report that we're scanning the heap, advertising total # of blocks */ initprog_val[0] = PROGRESS_VACUUM_PHASE_SCAN_HEAP; @@ -838,13 +873,19 @@ lazy_scan_heap(LVRelState *vacrel) vacrel->skip.next_unskippable_block = InvalidBlockNumber; vacrel->skip.vmbuffer = InvalidBuffer; - while (heap_vac_scan_get_next_block(vacrel, blkno + 1, - &blkno, &all_visible_according_to_vm)) + while (BufferIsValid(buf = + pg_streaming_read_buffer_get_next(pgsr, (void **) &all_visible_according_to_vm))) { - Buffer buf; Page page; bool has_lpdead_items; bool got_cleanup_lock = false; + BlockNumber blkno; + + vacrel->blkno = blkno = BufferGetBlockNumber(buf); + + CheckBufferIsPinnedOnce(buf); + + page = BufferGetPage(buf); vacrel->scanned_pages++; @@ -912,9 +953,6 @@ lazy_scan_heap(LVRelState *vacrel) */ visibilitymap_pin(vacrel->rel, blkno, &vacrel->skip.vmbuffer); - buf = ReadBufferExtended(vacrel->rel, MAIN_FORKNUM, blkno, RBM_NORMAL, - vacrel->bstrategy); - page = BufferGetPage(buf); /* * We need a buffer cleanup lock to prune HOT chains and defragment @@ -970,7 +1008,7 @@ lazy_scan_heap(LVRelState *vacrel) */ if (got_cleanup_lock) lazy_scan_prune(vacrel, buf, blkno, page, - all_visible_according_to_vm, + *all_visible_according_to_vm, &has_lpdead_items); /* @@ -1027,7 +1065,7 @@ lazy_scan_heap(LVRelState *vacrel) } /* report that everything is now scanned */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, vacrel->rel_pages); /* now we can compute the new value for pg_class.reltuples */ vacrel->new_live_tuples = vac_estimate_reltuples(vacrel->rel, rel_pages, @@ -1042,6 +1080,8 @@ lazy_scan_heap(LVRelState *vacrel) Max(vacrel->new_live_tuples, 0) + vacrel->recently_dead_tuples + vacrel->missed_dead_tuples; + pg_streaming_read_free(pgsr); + /* * Do index vacuuming (call each index's ambulkdelete routine), then do * related heap vacuuming @@ -1053,11 +1093,11 @@ lazy_scan_heap(LVRelState *vacrel) * Vacuum the remainder of the Free Space Map. We must do this whether or * not there were indexes, and whether or not we bypassed index vacuuming. */ - if (blkno > next_fsm_block_to_vacuum) - FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, blkno); + if (vacrel->rel_pages > next_fsm_block_to_vacuum) + FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, vacrel->rel_pages); /* report all blocks vacuumed */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, vacrel->rel_pages); /* Do final index cleanup (call each index's amvacuumcleanup routine) */ if (vacrel->nindexes > 0 && vacrel->do_index_cleanup) @@ -1090,7 +1130,7 @@ lazy_scan_heap(LVRelState *vacrel) * * The block number and visibility status of the next block to process are set * in blkno and all_visible_according_to_vm. heap_vac_scan_get_next_block() - * returns false if there are no further blocks to process. + * sets blkno to InvalidBlockNumber if there are no further blocks to process. * * vacrel is an in/out parameter here; vacuum options and information about the * relation are read and vacrel->skippedallvis is set to ensure we don't @@ -1110,7 +1150,7 @@ lazy_scan_heap(LVRelState *vacrel) * older XIDs/MXIDs. The vacrel->skippedallvis flag will be set here when the * choice to skip such a range is actually made, making everything safe.) */ -static bool +static void heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, BlockNumber *blkno, bool *all_visible_according_to_vm) { @@ -1119,7 +1159,7 @@ heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, if (next_block >= vacrel->rel_pages) { *blkno = InvalidBlockNumber; - return false; + return; } if (vacrel->skip.next_unskippable_block == InvalidBlockNumber || @@ -1214,7 +1254,6 @@ heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, *all_visible_according_to_vm = true; *blkno = next_block; - return true; } /* -- 2.40.1 --lb5e4aqx4b6ik5lq Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5a-0007-Vacuum-second-pass-uses-Streaming-Read-interface.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v7 6/7] Vacuum first pass uses Streaming Read interface @ 2023-12-31 16:29 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2023-12-31 16:29 UTC (permalink / raw) Now vacuum's first pass, which HOT prunes and records the TIDs of non-removable dead tuples, uses the streaming read API by implementing a streaming read callback which invokes heap_vac_scan_next_block(). --- src/backend/access/heap/vacuumlazy.c | 131 +++++++++++++++++++-------- 1 file changed, 92 insertions(+), 39 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index d2c8f27fc57..d07a2a58b15 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -54,6 +54,7 @@ #include "storage/bufmgr.h" #include "storage/freespace.h" #include "storage/lmgr.h" +#include "storage/streaming_read.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/pg_rusage.h" @@ -168,7 +169,12 @@ typedef struct LVRelState char *relnamespace; char *relname; char *indname; /* Current index name */ - BlockNumber blkno; /* used only for heap operations */ + + /* + * The current block being processed by vacuum. Used only for heap + * operations. Primarily for error reporting and logging. + */ + BlockNumber blkno; OffsetNumber offnum; /* used only for heap operations */ VacErrPhase phase; bool verbose; /* VACUUM VERBOSE? */ @@ -220,6 +226,12 @@ typedef struct LVRelState BlockNumber next_unskippable_block; /* Next unskippable block's visibility status */ bool next_unskippable_allvis; + + /* + * Buffer containing block of VM with visibility information for + * next_unskippable_block. + */ + Buffer next_unskippable_vmbuffer; } next_block_state; } LVRelState; @@ -233,8 +245,7 @@ typedef struct LVSavedErrInfo /* non-export function prototypes */ static void lazy_scan_heap(LVRelState *vacrel); -static bool heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, - BlockNumber *blkno, +static void heap_vac_scan_next_block(LVRelState *vacrel, bool *all_visible_according_to_vm); static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno, Page page, @@ -777,6 +788,47 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, } } +static BlockNumber +vacuum_scan_pgsr_next(PgStreamingRead *pgsr, + void *pgsr_private, void *per_buffer_data) +{ + LVRelState *vacrel = pgsr_private; + bool *all_visible_according_to_vm = per_buffer_data; + + heap_vac_scan_next_block(vacrel, + all_visible_according_to_vm); + + /* + * If there are no further blocks to vacuum in the relation, release the + * vmbuffer. + */ + if (!BlockNumberIsValid(vacrel->next_block_state.current_block) && + BufferIsValid(vacrel->next_block_state.next_unskippable_vmbuffer)) + { + ReleaseBuffer(vacrel->next_block_state.next_unskippable_vmbuffer); + vacrel->next_block_state.next_unskippable_vmbuffer = InvalidBuffer; + } + + return vacrel->next_block_state.current_block; +} + +static inline PgStreamingRead * +vac_scan_pgsr_alloc(LVRelState *vacrel, PgStreamingReadBufferCB next_block_cb) +{ + PgStreamingRead *result = pg_streaming_read_buffer_alloc(PGSR_FLAG_MAINTENANCE, vacrel, + sizeof(bool), vacrel->bstrategy, BMR_REL(vacrel->rel), + MAIN_FORKNUM, next_block_cb); + + /* + * Initialize for first heap_vac_scan_next_block() call. These rely on + * InvalidBlockNumber + 1 = 0 + */ + vacrel->next_block_state.current_block = InvalidBlockNumber; + vacrel->next_block_state.next_unskippable_block = InvalidBlockNumber; + + return result; +} + /* * lazy_scan_heap() -- workhorse function for VACUUM * @@ -816,10 +868,10 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, static void lazy_scan_heap(LVRelState *vacrel) { + Buffer buf; BlockNumber rel_pages = vacrel->rel_pages, - blkno, next_fsm_block_to_vacuum = 0; - bool all_visible_according_to_vm; + bool *all_visible_according_to_vm; VacDeadItems *dead_items = vacrel->dead_items; Buffer vmbuffer = InvalidBuffer; @@ -830,23 +882,27 @@ lazy_scan_heap(LVRelState *vacrel) }; int64 initprog_val[3]; + PgStreamingRead *pgsr = vac_scan_pgsr_alloc(vacrel, vacuum_scan_pgsr_next); + /* Report that we're scanning the heap, advertising total # of blocks */ initprog_val[0] = PROGRESS_VACUUM_PHASE_SCAN_HEAP; initprog_val[1] = rel_pages; initprog_val[2] = dead_items->max_items; pgstat_progress_update_multi_param(3, initprog_index, initprog_val); - /* Initialize for first heap_vac_scan_next_block() call */ - vacrel->next_block_state.current_block = InvalidBlockNumber; - vacrel->next_block_state.next_unskippable_block = InvalidBlockNumber; - - while (heap_vac_scan_next_block(vacrel, &vmbuffer, - &blkno, &all_visible_according_to_vm)) + while (BufferIsValid(buf = pg_streaming_read_buffer_get_next(pgsr, + (void **) &all_visible_according_to_vm))) { - Buffer buf; Page page; bool has_lpdead_items; bool got_cleanup_lock = false; + BlockNumber blkno; + + vacrel->blkno = blkno = BufferGetBlockNumber(buf); + + CheckBufferIsPinnedOnce(buf); + + page = BufferGetPage(buf); vacrel->scanned_pages++; @@ -914,9 +970,6 @@ lazy_scan_heap(LVRelState *vacrel) */ visibilitymap_pin(vacrel->rel, blkno, &vmbuffer); - buf = ReadBufferExtended(vacrel->rel, MAIN_FORKNUM, blkno, RBM_NORMAL, - vacrel->bstrategy); - page = BufferGetPage(buf); /* * We need a buffer cleanup lock to prune HOT chains and defragment @@ -973,7 +1026,7 @@ lazy_scan_heap(LVRelState *vacrel) */ if (got_cleanup_lock) lazy_scan_prune(vacrel, buf, blkno, page, - vmbuffer, all_visible_according_to_vm, + vmbuffer, *all_visible_according_to_vm, &has_lpdead_items); /* @@ -1030,7 +1083,7 @@ lazy_scan_heap(LVRelState *vacrel) } /* report that everything is now scanned */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, vacrel->rel_pages); /* now we can compute the new value for pg_class.reltuples */ vacrel->new_live_tuples = vac_estimate_reltuples(vacrel->rel, rel_pages, @@ -1045,6 +1098,8 @@ lazy_scan_heap(LVRelState *vacrel) Max(vacrel->new_live_tuples, 0) + vacrel->recently_dead_tuples + vacrel->missed_dead_tuples; + pg_streaming_read_free(pgsr); + /* * Do index vacuuming (call each index's ambulkdelete routine), then do * related heap vacuuming @@ -1056,11 +1111,11 @@ lazy_scan_heap(LVRelState *vacrel) * Vacuum the remainder of the Free Space Map. We must do this whether or * not there were indexes, and whether or not we bypassed index vacuuming. */ - if (blkno > next_fsm_block_to_vacuum) - FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, blkno); + if (vacrel->rel_pages > next_fsm_block_to_vacuum) + FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, vacrel->rel_pages); /* report all blocks vacuumed */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, vacrel->rel_pages); /* Do final index cleanup (call each index's amvacuumcleanup routine) */ if (vacrel->nindexes > 0 && vacrel->do_index_cleanup) @@ -1072,12 +1127,13 @@ lazy_scan_heap(LVRelState *vacrel) * * lazy_scan_heap() calls here every time it needs to get the next block to * prune and vacuum, using the visibility map, vacuum options, and various - * thresholds to skip blocks which do not need to be processed and set blkno to - * the next block that actually needs to be processed. + * thresholds to skip blocks which do not need to be processed and set + * current_block to the next block that actually needs to be processed. * - * The block number and visibility status of the next block to process are set - * in blkno and all_visible_according_to_vm. heap_vac_scan_next_block() - * returns false if there are no further blocks to process. + * The number and visibility status of the next block to process are set in + * vacrel->next_block_state->current_block and all_visible_according_to_vm. + * vacrel->next_block_state->current_block is set to InvalidBlockNumber if + * there are no further blocks to process. * * vacrel is an in/out parameter here; vacuum options and information about the * relation are read, members of vacrel->next_block_state are read and set as @@ -1085,12 +1141,10 @@ lazy_scan_heap(LVRelState *vacrel) * don't advance relfrozenxid when we have skipped vacuuming all-visible * blocks. * - * vmbuffer is an output parameter which, upon return, will contain the block - * from the VM containing visibility information for the next unskippable heap - * block. If we decide not to skip this heap block, the caller is responsible - * for fetching the correct VM block into vmbuffer before using it. This is - * okay as providing it as an output parameter is an optimization, not a - * requirement. + * vacrel->next_block_state->vmbuffer will contain visibility information for + * the next unskippable heap block. If we decide not to skip this heap block, + * the caller is responsible for fetching the correct VM block into the + * vmbuffer before using it. * * Note: our opinion of which blocks can be skipped can go stale immediately. * It's okay if caller "misses" a page whose all-visible or all-frozen marking @@ -1100,9 +1154,9 @@ lazy_scan_heap(LVRelState *vacrel) * older XIDs/MXIDs. The vacrel->skippedallvis flag will be set here when the * choice to skip such a range is actually made, making everything safe.) */ -static bool -heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, - BlockNumber *blkno, bool *all_visible_according_to_vm) +static void +heap_vac_scan_next_block(LVRelState *vacrel, + bool *all_visible_according_to_vm) { /* Relies on InvalidBlockNumber + 1 == 0 */ BlockNumber next_block = vacrel->next_block_state.current_block + 1; @@ -1129,8 +1183,8 @@ heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, */ if (next_block >= vacrel->rel_pages) { - vacrel->next_block_state.current_block = *blkno = InvalidBlockNumber; - return false; + vacrel->next_block_state.current_block = InvalidBlockNumber; + return; } if (vacrel->next_block_state.next_unskippable_block == InvalidBlockNumber || @@ -1144,7 +1198,7 @@ heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, { uint8 mapbits = visibilitymap_get_status(vacrel->rel, next_unskippable_block, - vmbuffer); + &vacrel->next_block_state.next_unskippable_vmbuffer); vacrel->next_block_state.next_unskippable_allvis = mapbits & VISIBILITYMAP_ALL_VISIBLE; @@ -1224,8 +1278,7 @@ heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, else *all_visible_according_to_vm = true; - vacrel->next_block_state.current_block = *blkno = next_block; - return true; + vacrel->next_block_state.current_block = next_block; } /* -- 2.40.1 --3uc3jkcn3zcvmfik Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0007-Vacuum-second-pass-uses-Streaming-Read-interface.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v7 6/7] Vacuum first pass uses Streaming Read interface @ 2023-12-31 16:29 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2023-12-31 16:29 UTC (permalink / raw) Now vacuum's first pass, which HOT prunes and records the TIDs of non-removable dead tuples, uses the streaming read API by implementing a streaming read callback which invokes heap_vac_scan_next_block(). --- src/backend/access/heap/vacuumlazy.c | 131 +++++++++++++++++++-------- 1 file changed, 92 insertions(+), 39 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index d2c8f27fc57..d07a2a58b15 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -54,6 +54,7 @@ #include "storage/bufmgr.h" #include "storage/freespace.h" #include "storage/lmgr.h" +#include "storage/streaming_read.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/pg_rusage.h" @@ -168,7 +169,12 @@ typedef struct LVRelState char *relnamespace; char *relname; char *indname; /* Current index name */ - BlockNumber blkno; /* used only for heap operations */ + + /* + * The current block being processed by vacuum. Used only for heap + * operations. Primarily for error reporting and logging. + */ + BlockNumber blkno; OffsetNumber offnum; /* used only for heap operations */ VacErrPhase phase; bool verbose; /* VACUUM VERBOSE? */ @@ -220,6 +226,12 @@ typedef struct LVRelState BlockNumber next_unskippable_block; /* Next unskippable block's visibility status */ bool next_unskippable_allvis; + + /* + * Buffer containing block of VM with visibility information for + * next_unskippable_block. + */ + Buffer next_unskippable_vmbuffer; } next_block_state; } LVRelState; @@ -233,8 +245,7 @@ typedef struct LVSavedErrInfo /* non-export function prototypes */ static void lazy_scan_heap(LVRelState *vacrel); -static bool heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, - BlockNumber *blkno, +static void heap_vac_scan_next_block(LVRelState *vacrel, bool *all_visible_according_to_vm); static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno, Page page, @@ -777,6 +788,47 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, } } +static BlockNumber +vacuum_scan_pgsr_next(PgStreamingRead *pgsr, + void *pgsr_private, void *per_buffer_data) +{ + LVRelState *vacrel = pgsr_private; + bool *all_visible_according_to_vm = per_buffer_data; + + heap_vac_scan_next_block(vacrel, + all_visible_according_to_vm); + + /* + * If there are no further blocks to vacuum in the relation, release the + * vmbuffer. + */ + if (!BlockNumberIsValid(vacrel->next_block_state.current_block) && + BufferIsValid(vacrel->next_block_state.next_unskippable_vmbuffer)) + { + ReleaseBuffer(vacrel->next_block_state.next_unskippable_vmbuffer); + vacrel->next_block_state.next_unskippable_vmbuffer = InvalidBuffer; + } + + return vacrel->next_block_state.current_block; +} + +static inline PgStreamingRead * +vac_scan_pgsr_alloc(LVRelState *vacrel, PgStreamingReadBufferCB next_block_cb) +{ + PgStreamingRead *result = pg_streaming_read_buffer_alloc(PGSR_FLAG_MAINTENANCE, vacrel, + sizeof(bool), vacrel->bstrategy, BMR_REL(vacrel->rel), + MAIN_FORKNUM, next_block_cb); + + /* + * Initialize for first heap_vac_scan_next_block() call. These rely on + * InvalidBlockNumber + 1 = 0 + */ + vacrel->next_block_state.current_block = InvalidBlockNumber; + vacrel->next_block_state.next_unskippable_block = InvalidBlockNumber; + + return result; +} + /* * lazy_scan_heap() -- workhorse function for VACUUM * @@ -816,10 +868,10 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, static void lazy_scan_heap(LVRelState *vacrel) { + Buffer buf; BlockNumber rel_pages = vacrel->rel_pages, - blkno, next_fsm_block_to_vacuum = 0; - bool all_visible_according_to_vm; + bool *all_visible_according_to_vm; VacDeadItems *dead_items = vacrel->dead_items; Buffer vmbuffer = InvalidBuffer; @@ -830,23 +882,27 @@ lazy_scan_heap(LVRelState *vacrel) }; int64 initprog_val[3]; + PgStreamingRead *pgsr = vac_scan_pgsr_alloc(vacrel, vacuum_scan_pgsr_next); + /* Report that we're scanning the heap, advertising total # of blocks */ initprog_val[0] = PROGRESS_VACUUM_PHASE_SCAN_HEAP; initprog_val[1] = rel_pages; initprog_val[2] = dead_items->max_items; pgstat_progress_update_multi_param(3, initprog_index, initprog_val); - /* Initialize for first heap_vac_scan_next_block() call */ - vacrel->next_block_state.current_block = InvalidBlockNumber; - vacrel->next_block_state.next_unskippable_block = InvalidBlockNumber; - - while (heap_vac_scan_next_block(vacrel, &vmbuffer, - &blkno, &all_visible_according_to_vm)) + while (BufferIsValid(buf = pg_streaming_read_buffer_get_next(pgsr, + (void **) &all_visible_according_to_vm))) { - Buffer buf; Page page; bool has_lpdead_items; bool got_cleanup_lock = false; + BlockNumber blkno; + + vacrel->blkno = blkno = BufferGetBlockNumber(buf); + + CheckBufferIsPinnedOnce(buf); + + page = BufferGetPage(buf); vacrel->scanned_pages++; @@ -914,9 +970,6 @@ lazy_scan_heap(LVRelState *vacrel) */ visibilitymap_pin(vacrel->rel, blkno, &vmbuffer); - buf = ReadBufferExtended(vacrel->rel, MAIN_FORKNUM, blkno, RBM_NORMAL, - vacrel->bstrategy); - page = BufferGetPage(buf); /* * We need a buffer cleanup lock to prune HOT chains and defragment @@ -973,7 +1026,7 @@ lazy_scan_heap(LVRelState *vacrel) */ if (got_cleanup_lock) lazy_scan_prune(vacrel, buf, blkno, page, - vmbuffer, all_visible_according_to_vm, + vmbuffer, *all_visible_according_to_vm, &has_lpdead_items); /* @@ -1030,7 +1083,7 @@ lazy_scan_heap(LVRelState *vacrel) } /* report that everything is now scanned */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, vacrel->rel_pages); /* now we can compute the new value for pg_class.reltuples */ vacrel->new_live_tuples = vac_estimate_reltuples(vacrel->rel, rel_pages, @@ -1045,6 +1098,8 @@ lazy_scan_heap(LVRelState *vacrel) Max(vacrel->new_live_tuples, 0) + vacrel->recently_dead_tuples + vacrel->missed_dead_tuples; + pg_streaming_read_free(pgsr); + /* * Do index vacuuming (call each index's ambulkdelete routine), then do * related heap vacuuming @@ -1056,11 +1111,11 @@ lazy_scan_heap(LVRelState *vacrel) * Vacuum the remainder of the Free Space Map. We must do this whether or * not there were indexes, and whether or not we bypassed index vacuuming. */ - if (blkno > next_fsm_block_to_vacuum) - FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, blkno); + if (vacrel->rel_pages > next_fsm_block_to_vacuum) + FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, vacrel->rel_pages); /* report all blocks vacuumed */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, vacrel->rel_pages); /* Do final index cleanup (call each index's amvacuumcleanup routine) */ if (vacrel->nindexes > 0 && vacrel->do_index_cleanup) @@ -1072,12 +1127,13 @@ lazy_scan_heap(LVRelState *vacrel) * * lazy_scan_heap() calls here every time it needs to get the next block to * prune and vacuum, using the visibility map, vacuum options, and various - * thresholds to skip blocks which do not need to be processed and set blkno to - * the next block that actually needs to be processed. + * thresholds to skip blocks which do not need to be processed and set + * current_block to the next block that actually needs to be processed. * - * The block number and visibility status of the next block to process are set - * in blkno and all_visible_according_to_vm. heap_vac_scan_next_block() - * returns false if there are no further blocks to process. + * The number and visibility status of the next block to process are set in + * vacrel->next_block_state->current_block and all_visible_according_to_vm. + * vacrel->next_block_state->current_block is set to InvalidBlockNumber if + * there are no further blocks to process. * * vacrel is an in/out parameter here; vacuum options and information about the * relation are read, members of vacrel->next_block_state are read and set as @@ -1085,12 +1141,10 @@ lazy_scan_heap(LVRelState *vacrel) * don't advance relfrozenxid when we have skipped vacuuming all-visible * blocks. * - * vmbuffer is an output parameter which, upon return, will contain the block - * from the VM containing visibility information for the next unskippable heap - * block. If we decide not to skip this heap block, the caller is responsible - * for fetching the correct VM block into vmbuffer before using it. This is - * okay as providing it as an output parameter is an optimization, not a - * requirement. + * vacrel->next_block_state->vmbuffer will contain visibility information for + * the next unskippable heap block. If we decide not to skip this heap block, + * the caller is responsible for fetching the correct VM block into the + * vmbuffer before using it. * * Note: our opinion of which blocks can be skipped can go stale immediately. * It's okay if caller "misses" a page whose all-visible or all-frozen marking @@ -1100,9 +1154,9 @@ lazy_scan_heap(LVRelState *vacrel) * older XIDs/MXIDs. The vacrel->skippedallvis flag will be set here when the * choice to skip such a range is actually made, making everything safe.) */ -static bool -heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, - BlockNumber *blkno, bool *all_visible_according_to_vm) +static void +heap_vac_scan_next_block(LVRelState *vacrel, + bool *all_visible_according_to_vm) { /* Relies on InvalidBlockNumber + 1 == 0 */ BlockNumber next_block = vacrel->next_block_state.current_block + 1; @@ -1129,8 +1183,8 @@ heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, */ if (next_block >= vacrel->rel_pages) { - vacrel->next_block_state.current_block = *blkno = InvalidBlockNumber; - return false; + vacrel->next_block_state.current_block = InvalidBlockNumber; + return; } if (vacrel->next_block_state.next_unskippable_block == InvalidBlockNumber || @@ -1144,7 +1198,7 @@ heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, { uint8 mapbits = visibilitymap_get_status(vacrel->rel, next_unskippable_block, - vmbuffer); + &vacrel->next_block_state.next_unskippable_vmbuffer); vacrel->next_block_state.next_unskippable_allvis = mapbits & VISIBILITYMAP_ALL_VISIBLE; @@ -1224,8 +1278,7 @@ heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, else *all_visible_according_to_vm = true; - vacrel->next_block_state.current_block = *blkno = next_block; - return true; + vacrel->next_block_state.current_block = next_block; } /* -- 2.40.1 --3uc3jkcn3zcvmfik Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0007-Vacuum-second-pass-uses-Streaming-Read-interface.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v5a 6/7] Vacuum first pass uses Streaming Read interface @ 2023-12-31 16:29 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2023-12-31 16:29 UTC (permalink / raw) Now vacuum's first pass, which HOT prunes and records the TIDs of non-removable dead tuples, uses the streaming read API by implementing a streaming read callback which invokes heap_vac_scan_get_next_block(). --- src/backend/access/heap/vacuumlazy.c | 79 +++++++++++++++++++++------- 1 file changed, 59 insertions(+), 20 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 65d257aab83..fbbc87938e4 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -54,6 +54,7 @@ #include "storage/bufmgr.h" #include "storage/freespace.h" #include "storage/lmgr.h" +#include "storage/streaming_read.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/pg_rusage.h" @@ -168,7 +169,12 @@ typedef struct LVRelState char *relnamespace; char *relname; char *indname; /* Current index name */ - BlockNumber blkno; /* used only for heap operations */ + + /* + * The current block being processed by vacuum. Used only for heap + * operations. Primarily for error reporting and logging. + */ + BlockNumber blkno; OffsetNumber offnum; /* used only for heap operations */ VacErrPhase phase; bool verbose; /* VACUUM VERBOSE? */ @@ -189,6 +195,12 @@ typedef struct LVRelState BlockNumber missed_dead_pages; /* # pages with missed dead tuples */ BlockNumber nonempty_pages; /* actually, last nonempty page + 1 */ + /* + * The most recent block submitted in the streaming read callback by the + * first vacuum pass. + */ + BlockNumber blkno_prefetch; + /* Statistics output by us, for table */ double new_rel_tuples; /* new estimated total # of tuples */ double new_live_tuples; /* new estimated total # of live tuples */ @@ -232,7 +244,7 @@ typedef struct LVSavedErrInfo /* non-export function prototypes */ static void lazy_scan_heap(LVRelState *vacrel); -static bool heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, +static void heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, BlockNumber *blkno, bool *all_visible_according_to_vm); static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, @@ -416,6 +428,9 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, vacrel->nonempty_pages = 0; /* dead_items_alloc allocates vacrel->dead_items later on */ + /* relies on InvalidBlockNumber overflowing to 0 */ + vacrel->blkno_prefetch = InvalidBlockNumber; + /* Allocate/initialize output statistics state */ vacrel->new_rel_tuples = 0; vacrel->new_live_tuples = 0; @@ -776,6 +791,22 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, } } +static BlockNumber +vacuum_scan_pgsr_next(PgStreamingRead *pgsr, + void *pgsr_private, void *per_buffer_data) +{ + LVRelState *vacrel = pgsr_private; + bool *all_visible_according_to_vm = per_buffer_data; + + vacrel->blkno_prefetch++; + + heap_vac_scan_get_next_block(vacrel, + vacrel->blkno_prefetch, &vacrel->blkno_prefetch, + all_visible_according_to_vm); + + return vacrel->blkno_prefetch; +} + /* * lazy_scan_heap() -- workhorse function for VACUUM * @@ -815,12 +846,11 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, static void lazy_scan_heap(LVRelState *vacrel) { + Buffer buf; BlockNumber rel_pages = vacrel->rel_pages, next_fsm_block_to_vacuum = 0; - bool all_visible_according_to_vm; + bool *all_visible_according_to_vm; - /* relies on InvalidBlockNumber overflowing to 0 */ - BlockNumber blkno = InvalidBlockNumber; VacDeadItems *dead_items = vacrel->dead_items; const int initprog_index[] = { PROGRESS_VACUUM_PHASE, @@ -828,6 +858,11 @@ lazy_scan_heap(LVRelState *vacrel) PROGRESS_VACUUM_MAX_DEAD_TUPLES }; int64 initprog_val[3]; + PgStreamingRead *pgsr; + + pgsr = pg_streaming_read_buffer_alloc(PGSR_FLAG_MAINTENANCE, vacrel, + sizeof(bool), vacrel->bstrategy, BMR_REL(vacrel->rel), + MAIN_FORKNUM, vacuum_scan_pgsr_next); /* Report that we're scanning the heap, advertising total # of blocks */ initprog_val[0] = PROGRESS_VACUUM_PHASE_SCAN_HEAP; @@ -838,13 +873,19 @@ lazy_scan_heap(LVRelState *vacrel) vacrel->skip.next_unskippable_block = InvalidBlockNumber; vacrel->skip.vmbuffer = InvalidBuffer; - while (heap_vac_scan_get_next_block(vacrel, blkno + 1, - &blkno, &all_visible_according_to_vm)) + while (BufferIsValid(buf = + pg_streaming_read_buffer_get_next(pgsr, (void **) &all_visible_according_to_vm))) { - Buffer buf; Page page; bool has_lpdead_items; bool got_cleanup_lock = false; + BlockNumber blkno; + + vacrel->blkno = blkno = BufferGetBlockNumber(buf); + + CheckBufferIsPinnedOnce(buf); + + page = BufferGetPage(buf); vacrel->scanned_pages++; @@ -912,9 +953,6 @@ lazy_scan_heap(LVRelState *vacrel) */ visibilitymap_pin(vacrel->rel, blkno, &vacrel->skip.vmbuffer); - buf = ReadBufferExtended(vacrel->rel, MAIN_FORKNUM, blkno, RBM_NORMAL, - vacrel->bstrategy); - page = BufferGetPage(buf); /* * We need a buffer cleanup lock to prune HOT chains and defragment @@ -970,7 +1008,7 @@ lazy_scan_heap(LVRelState *vacrel) */ if (got_cleanup_lock) lazy_scan_prune(vacrel, buf, blkno, page, - all_visible_according_to_vm, + *all_visible_according_to_vm, &has_lpdead_items); /* @@ -1027,7 +1065,7 @@ lazy_scan_heap(LVRelState *vacrel) } /* report that everything is now scanned */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, vacrel->rel_pages); /* now we can compute the new value for pg_class.reltuples */ vacrel->new_live_tuples = vac_estimate_reltuples(vacrel->rel, rel_pages, @@ -1042,6 +1080,8 @@ lazy_scan_heap(LVRelState *vacrel) Max(vacrel->new_live_tuples, 0) + vacrel->recently_dead_tuples + vacrel->missed_dead_tuples; + pg_streaming_read_free(pgsr); + /* * Do index vacuuming (call each index's ambulkdelete routine), then do * related heap vacuuming @@ -1053,11 +1093,11 @@ lazy_scan_heap(LVRelState *vacrel) * Vacuum the remainder of the Free Space Map. We must do this whether or * not there were indexes, and whether or not we bypassed index vacuuming. */ - if (blkno > next_fsm_block_to_vacuum) - FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, blkno); + if (vacrel->rel_pages > next_fsm_block_to_vacuum) + FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, vacrel->rel_pages); /* report all blocks vacuumed */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, vacrel->rel_pages); /* Do final index cleanup (call each index's amvacuumcleanup routine) */ if (vacrel->nindexes > 0 && vacrel->do_index_cleanup) @@ -1090,7 +1130,7 @@ lazy_scan_heap(LVRelState *vacrel) * * The block number and visibility status of the next block to process are set * in blkno and all_visible_according_to_vm. heap_vac_scan_get_next_block() - * returns false if there are no further blocks to process. + * sets blkno to InvalidBlockNumber if there are no further blocks to process. * * vacrel is an in/out parameter here; vacuum options and information about the * relation are read and vacrel->skippedallvis is set to ensure we don't @@ -1110,7 +1150,7 @@ lazy_scan_heap(LVRelState *vacrel) * older XIDs/MXIDs. The vacrel->skippedallvis flag will be set here when the * choice to skip such a range is actually made, making everything safe.) */ -static bool +static void heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, BlockNumber *blkno, bool *all_visible_according_to_vm) { @@ -1119,7 +1159,7 @@ heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, if (next_block >= vacrel->rel_pages) { *blkno = InvalidBlockNumber; - return false; + return; } if (vacrel->skip.next_unskippable_block == InvalidBlockNumber || @@ -1214,7 +1254,6 @@ heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, *all_visible_according_to_vm = true; *blkno = next_block; - return true; } /* -- 2.40.1 --lb5e4aqx4b6ik5lq Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5a-0007-Vacuum-second-pass-uses-Streaming-Read-interface.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v5a 6/7] Vacuum first pass uses Streaming Read interface @ 2023-12-31 16:29 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2023-12-31 16:29 UTC (permalink / raw) Now vacuum's first pass, which HOT prunes and records the TIDs of non-removable dead tuples, uses the streaming read API by implementing a streaming read callback which invokes heap_vac_scan_get_next_block(). --- src/backend/access/heap/vacuumlazy.c | 79 +++++++++++++++++++++------- 1 file changed, 59 insertions(+), 20 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 65d257aab83..fbbc87938e4 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -54,6 +54,7 @@ #include "storage/bufmgr.h" #include "storage/freespace.h" #include "storage/lmgr.h" +#include "storage/streaming_read.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/pg_rusage.h" @@ -168,7 +169,12 @@ typedef struct LVRelState char *relnamespace; char *relname; char *indname; /* Current index name */ - BlockNumber blkno; /* used only for heap operations */ + + /* + * The current block being processed by vacuum. Used only for heap + * operations. Primarily for error reporting and logging. + */ + BlockNumber blkno; OffsetNumber offnum; /* used only for heap operations */ VacErrPhase phase; bool verbose; /* VACUUM VERBOSE? */ @@ -189,6 +195,12 @@ typedef struct LVRelState BlockNumber missed_dead_pages; /* # pages with missed dead tuples */ BlockNumber nonempty_pages; /* actually, last nonempty page + 1 */ + /* + * The most recent block submitted in the streaming read callback by the + * first vacuum pass. + */ + BlockNumber blkno_prefetch; + /* Statistics output by us, for table */ double new_rel_tuples; /* new estimated total # of tuples */ double new_live_tuples; /* new estimated total # of live tuples */ @@ -232,7 +244,7 @@ typedef struct LVSavedErrInfo /* non-export function prototypes */ static void lazy_scan_heap(LVRelState *vacrel); -static bool heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, +static void heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, BlockNumber *blkno, bool *all_visible_according_to_vm); static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, @@ -416,6 +428,9 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, vacrel->nonempty_pages = 0; /* dead_items_alloc allocates vacrel->dead_items later on */ + /* relies on InvalidBlockNumber overflowing to 0 */ + vacrel->blkno_prefetch = InvalidBlockNumber; + /* Allocate/initialize output statistics state */ vacrel->new_rel_tuples = 0; vacrel->new_live_tuples = 0; @@ -776,6 +791,22 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, } } +static BlockNumber +vacuum_scan_pgsr_next(PgStreamingRead *pgsr, + void *pgsr_private, void *per_buffer_data) +{ + LVRelState *vacrel = pgsr_private; + bool *all_visible_according_to_vm = per_buffer_data; + + vacrel->blkno_prefetch++; + + heap_vac_scan_get_next_block(vacrel, + vacrel->blkno_prefetch, &vacrel->blkno_prefetch, + all_visible_according_to_vm); + + return vacrel->blkno_prefetch; +} + /* * lazy_scan_heap() -- workhorse function for VACUUM * @@ -815,12 +846,11 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, static void lazy_scan_heap(LVRelState *vacrel) { + Buffer buf; BlockNumber rel_pages = vacrel->rel_pages, next_fsm_block_to_vacuum = 0; - bool all_visible_according_to_vm; + bool *all_visible_according_to_vm; - /* relies on InvalidBlockNumber overflowing to 0 */ - BlockNumber blkno = InvalidBlockNumber; VacDeadItems *dead_items = vacrel->dead_items; const int initprog_index[] = { PROGRESS_VACUUM_PHASE, @@ -828,6 +858,11 @@ lazy_scan_heap(LVRelState *vacrel) PROGRESS_VACUUM_MAX_DEAD_TUPLES }; int64 initprog_val[3]; + PgStreamingRead *pgsr; + + pgsr = pg_streaming_read_buffer_alloc(PGSR_FLAG_MAINTENANCE, vacrel, + sizeof(bool), vacrel->bstrategy, BMR_REL(vacrel->rel), + MAIN_FORKNUM, vacuum_scan_pgsr_next); /* Report that we're scanning the heap, advertising total # of blocks */ initprog_val[0] = PROGRESS_VACUUM_PHASE_SCAN_HEAP; @@ -838,13 +873,19 @@ lazy_scan_heap(LVRelState *vacrel) vacrel->skip.next_unskippable_block = InvalidBlockNumber; vacrel->skip.vmbuffer = InvalidBuffer; - while (heap_vac_scan_get_next_block(vacrel, blkno + 1, - &blkno, &all_visible_according_to_vm)) + while (BufferIsValid(buf = + pg_streaming_read_buffer_get_next(pgsr, (void **) &all_visible_according_to_vm))) { - Buffer buf; Page page; bool has_lpdead_items; bool got_cleanup_lock = false; + BlockNumber blkno; + + vacrel->blkno = blkno = BufferGetBlockNumber(buf); + + CheckBufferIsPinnedOnce(buf); + + page = BufferGetPage(buf); vacrel->scanned_pages++; @@ -912,9 +953,6 @@ lazy_scan_heap(LVRelState *vacrel) */ visibilitymap_pin(vacrel->rel, blkno, &vacrel->skip.vmbuffer); - buf = ReadBufferExtended(vacrel->rel, MAIN_FORKNUM, blkno, RBM_NORMAL, - vacrel->bstrategy); - page = BufferGetPage(buf); /* * We need a buffer cleanup lock to prune HOT chains and defragment @@ -970,7 +1008,7 @@ lazy_scan_heap(LVRelState *vacrel) */ if (got_cleanup_lock) lazy_scan_prune(vacrel, buf, blkno, page, - all_visible_according_to_vm, + *all_visible_according_to_vm, &has_lpdead_items); /* @@ -1027,7 +1065,7 @@ lazy_scan_heap(LVRelState *vacrel) } /* report that everything is now scanned */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, vacrel->rel_pages); /* now we can compute the new value for pg_class.reltuples */ vacrel->new_live_tuples = vac_estimate_reltuples(vacrel->rel, rel_pages, @@ -1042,6 +1080,8 @@ lazy_scan_heap(LVRelState *vacrel) Max(vacrel->new_live_tuples, 0) + vacrel->recently_dead_tuples + vacrel->missed_dead_tuples; + pg_streaming_read_free(pgsr); + /* * Do index vacuuming (call each index's ambulkdelete routine), then do * related heap vacuuming @@ -1053,11 +1093,11 @@ lazy_scan_heap(LVRelState *vacrel) * Vacuum the remainder of the Free Space Map. We must do this whether or * not there were indexes, and whether or not we bypassed index vacuuming. */ - if (blkno > next_fsm_block_to_vacuum) - FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, blkno); + if (vacrel->rel_pages > next_fsm_block_to_vacuum) + FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, vacrel->rel_pages); /* report all blocks vacuumed */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, vacrel->rel_pages); /* Do final index cleanup (call each index's amvacuumcleanup routine) */ if (vacrel->nindexes > 0 && vacrel->do_index_cleanup) @@ -1090,7 +1130,7 @@ lazy_scan_heap(LVRelState *vacrel) * * The block number and visibility status of the next block to process are set * in blkno and all_visible_according_to_vm. heap_vac_scan_get_next_block() - * returns false if there are no further blocks to process. + * sets blkno to InvalidBlockNumber if there are no further blocks to process. * * vacrel is an in/out parameter here; vacuum options and information about the * relation are read and vacrel->skippedallvis is set to ensure we don't @@ -1110,7 +1150,7 @@ lazy_scan_heap(LVRelState *vacrel) * older XIDs/MXIDs. The vacrel->skippedallvis flag will be set here when the * choice to skip such a range is actually made, making everything safe.) */ -static bool +static void heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, BlockNumber *blkno, bool *all_visible_according_to_vm) { @@ -1119,7 +1159,7 @@ heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, if (next_block >= vacrel->rel_pages) { *blkno = InvalidBlockNumber; - return false; + return; } if (vacrel->skip.next_unskippable_block == InvalidBlockNumber || @@ -1214,7 +1254,6 @@ heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block, *all_visible_according_to_vm = true; *blkno = next_block; - return true; } /* -- 2.40.1 --lb5e4aqx4b6ik5lq Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5a-0007-Vacuum-second-pass-uses-Streaming-Read-interface.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v7 6/7] Vacuum first pass uses Streaming Read interface @ 2023-12-31 16:29 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2023-12-31 16:29 UTC (permalink / raw) Now vacuum's first pass, which HOT prunes and records the TIDs of non-removable dead tuples, uses the streaming read API by implementing a streaming read callback which invokes heap_vac_scan_next_block(). --- src/backend/access/heap/vacuumlazy.c | 131 +++++++++++++++++++-------- 1 file changed, 92 insertions(+), 39 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index d2c8f27fc57..d07a2a58b15 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -54,6 +54,7 @@ #include "storage/bufmgr.h" #include "storage/freespace.h" #include "storage/lmgr.h" +#include "storage/streaming_read.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/pg_rusage.h" @@ -168,7 +169,12 @@ typedef struct LVRelState char *relnamespace; char *relname; char *indname; /* Current index name */ - BlockNumber blkno; /* used only for heap operations */ + + /* + * The current block being processed by vacuum. Used only for heap + * operations. Primarily for error reporting and logging. + */ + BlockNumber blkno; OffsetNumber offnum; /* used only for heap operations */ VacErrPhase phase; bool verbose; /* VACUUM VERBOSE? */ @@ -220,6 +226,12 @@ typedef struct LVRelState BlockNumber next_unskippable_block; /* Next unskippable block's visibility status */ bool next_unskippable_allvis; + + /* + * Buffer containing block of VM with visibility information for + * next_unskippable_block. + */ + Buffer next_unskippable_vmbuffer; } next_block_state; } LVRelState; @@ -233,8 +245,7 @@ typedef struct LVSavedErrInfo /* non-export function prototypes */ static void lazy_scan_heap(LVRelState *vacrel); -static bool heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, - BlockNumber *blkno, +static void heap_vac_scan_next_block(LVRelState *vacrel, bool *all_visible_according_to_vm); static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno, Page page, @@ -777,6 +788,47 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, } } +static BlockNumber +vacuum_scan_pgsr_next(PgStreamingRead *pgsr, + void *pgsr_private, void *per_buffer_data) +{ + LVRelState *vacrel = pgsr_private; + bool *all_visible_according_to_vm = per_buffer_data; + + heap_vac_scan_next_block(vacrel, + all_visible_according_to_vm); + + /* + * If there are no further blocks to vacuum in the relation, release the + * vmbuffer. + */ + if (!BlockNumberIsValid(vacrel->next_block_state.current_block) && + BufferIsValid(vacrel->next_block_state.next_unskippable_vmbuffer)) + { + ReleaseBuffer(vacrel->next_block_state.next_unskippable_vmbuffer); + vacrel->next_block_state.next_unskippable_vmbuffer = InvalidBuffer; + } + + return vacrel->next_block_state.current_block; +} + +static inline PgStreamingRead * +vac_scan_pgsr_alloc(LVRelState *vacrel, PgStreamingReadBufferCB next_block_cb) +{ + PgStreamingRead *result = pg_streaming_read_buffer_alloc(PGSR_FLAG_MAINTENANCE, vacrel, + sizeof(bool), vacrel->bstrategy, BMR_REL(vacrel->rel), + MAIN_FORKNUM, next_block_cb); + + /* + * Initialize for first heap_vac_scan_next_block() call. These rely on + * InvalidBlockNumber + 1 = 0 + */ + vacrel->next_block_state.current_block = InvalidBlockNumber; + vacrel->next_block_state.next_unskippable_block = InvalidBlockNumber; + + return result; +} + /* * lazy_scan_heap() -- workhorse function for VACUUM * @@ -816,10 +868,10 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, static void lazy_scan_heap(LVRelState *vacrel) { + Buffer buf; BlockNumber rel_pages = vacrel->rel_pages, - blkno, next_fsm_block_to_vacuum = 0; - bool all_visible_according_to_vm; + bool *all_visible_according_to_vm; VacDeadItems *dead_items = vacrel->dead_items; Buffer vmbuffer = InvalidBuffer; @@ -830,23 +882,27 @@ lazy_scan_heap(LVRelState *vacrel) }; int64 initprog_val[3]; + PgStreamingRead *pgsr = vac_scan_pgsr_alloc(vacrel, vacuum_scan_pgsr_next); + /* Report that we're scanning the heap, advertising total # of blocks */ initprog_val[0] = PROGRESS_VACUUM_PHASE_SCAN_HEAP; initprog_val[1] = rel_pages; initprog_val[2] = dead_items->max_items; pgstat_progress_update_multi_param(3, initprog_index, initprog_val); - /* Initialize for first heap_vac_scan_next_block() call */ - vacrel->next_block_state.current_block = InvalidBlockNumber; - vacrel->next_block_state.next_unskippable_block = InvalidBlockNumber; - - while (heap_vac_scan_next_block(vacrel, &vmbuffer, - &blkno, &all_visible_according_to_vm)) + while (BufferIsValid(buf = pg_streaming_read_buffer_get_next(pgsr, + (void **) &all_visible_according_to_vm))) { - Buffer buf; Page page; bool has_lpdead_items; bool got_cleanup_lock = false; + BlockNumber blkno; + + vacrel->blkno = blkno = BufferGetBlockNumber(buf); + + CheckBufferIsPinnedOnce(buf); + + page = BufferGetPage(buf); vacrel->scanned_pages++; @@ -914,9 +970,6 @@ lazy_scan_heap(LVRelState *vacrel) */ visibilitymap_pin(vacrel->rel, blkno, &vmbuffer); - buf = ReadBufferExtended(vacrel->rel, MAIN_FORKNUM, blkno, RBM_NORMAL, - vacrel->bstrategy); - page = BufferGetPage(buf); /* * We need a buffer cleanup lock to prune HOT chains and defragment @@ -973,7 +1026,7 @@ lazy_scan_heap(LVRelState *vacrel) */ if (got_cleanup_lock) lazy_scan_prune(vacrel, buf, blkno, page, - vmbuffer, all_visible_according_to_vm, + vmbuffer, *all_visible_according_to_vm, &has_lpdead_items); /* @@ -1030,7 +1083,7 @@ lazy_scan_heap(LVRelState *vacrel) } /* report that everything is now scanned */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, vacrel->rel_pages); /* now we can compute the new value for pg_class.reltuples */ vacrel->new_live_tuples = vac_estimate_reltuples(vacrel->rel, rel_pages, @@ -1045,6 +1098,8 @@ lazy_scan_heap(LVRelState *vacrel) Max(vacrel->new_live_tuples, 0) + vacrel->recently_dead_tuples + vacrel->missed_dead_tuples; + pg_streaming_read_free(pgsr); + /* * Do index vacuuming (call each index's ambulkdelete routine), then do * related heap vacuuming @@ -1056,11 +1111,11 @@ lazy_scan_heap(LVRelState *vacrel) * Vacuum the remainder of the Free Space Map. We must do this whether or * not there were indexes, and whether or not we bypassed index vacuuming. */ - if (blkno > next_fsm_block_to_vacuum) - FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, blkno); + if (vacrel->rel_pages > next_fsm_block_to_vacuum) + FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, vacrel->rel_pages); /* report all blocks vacuumed */ - pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno); + pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, vacrel->rel_pages); /* Do final index cleanup (call each index's amvacuumcleanup routine) */ if (vacrel->nindexes > 0 && vacrel->do_index_cleanup) @@ -1072,12 +1127,13 @@ lazy_scan_heap(LVRelState *vacrel) * * lazy_scan_heap() calls here every time it needs to get the next block to * prune and vacuum, using the visibility map, vacuum options, and various - * thresholds to skip blocks which do not need to be processed and set blkno to - * the next block that actually needs to be processed. + * thresholds to skip blocks which do not need to be processed and set + * current_block to the next block that actually needs to be processed. * - * The block number and visibility status of the next block to process are set - * in blkno and all_visible_according_to_vm. heap_vac_scan_next_block() - * returns false if there are no further blocks to process. + * The number and visibility status of the next block to process are set in + * vacrel->next_block_state->current_block and all_visible_according_to_vm. + * vacrel->next_block_state->current_block is set to InvalidBlockNumber if + * there are no further blocks to process. * * vacrel is an in/out parameter here; vacuum options and information about the * relation are read, members of vacrel->next_block_state are read and set as @@ -1085,12 +1141,10 @@ lazy_scan_heap(LVRelState *vacrel) * don't advance relfrozenxid when we have skipped vacuuming all-visible * blocks. * - * vmbuffer is an output parameter which, upon return, will contain the block - * from the VM containing visibility information for the next unskippable heap - * block. If we decide not to skip this heap block, the caller is responsible - * for fetching the correct VM block into vmbuffer before using it. This is - * okay as providing it as an output parameter is an optimization, not a - * requirement. + * vacrel->next_block_state->vmbuffer will contain visibility information for + * the next unskippable heap block. If we decide not to skip this heap block, + * the caller is responsible for fetching the correct VM block into the + * vmbuffer before using it. * * Note: our opinion of which blocks can be skipped can go stale immediately. * It's okay if caller "misses" a page whose all-visible or all-frozen marking @@ -1100,9 +1154,9 @@ lazy_scan_heap(LVRelState *vacrel) * older XIDs/MXIDs. The vacrel->skippedallvis flag will be set here when the * choice to skip such a range is actually made, making everything safe.) */ -static bool -heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, - BlockNumber *blkno, bool *all_visible_according_to_vm) +static void +heap_vac_scan_next_block(LVRelState *vacrel, + bool *all_visible_according_to_vm) { /* Relies on InvalidBlockNumber + 1 == 0 */ BlockNumber next_block = vacrel->next_block_state.current_block + 1; @@ -1129,8 +1183,8 @@ heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, */ if (next_block >= vacrel->rel_pages) { - vacrel->next_block_state.current_block = *blkno = InvalidBlockNumber; - return false; + vacrel->next_block_state.current_block = InvalidBlockNumber; + return; } if (vacrel->next_block_state.next_unskippable_block == InvalidBlockNumber || @@ -1144,7 +1198,7 @@ heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, { uint8 mapbits = visibilitymap_get_status(vacrel->rel, next_unskippable_block, - vmbuffer); + &vacrel->next_block_state.next_unskippable_vmbuffer); vacrel->next_block_state.next_unskippable_allvis = mapbits & VISIBILITYMAP_ALL_VISIBLE; @@ -1224,8 +1278,7 @@ heap_vac_scan_next_block(LVRelState *vacrel, Buffer *vmbuffer, else *all_visible_according_to_vm = true; - vacrel->next_block_state.current_block = *blkno = next_block; - return true; + vacrel->next_block_state.current_block = next_block; } /* -- 2.40.1 --3uc3jkcn3zcvmfik Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0007-Vacuum-second-pass-uses-Streaming-Read-interface.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2024-01-22 04:26 UTC | newest] Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-03-22 03:24 Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]> 2022-03-22 14:19 ` Tom Lane <[email protected]> 2023-11-10 03:31 ` Thomas Munro <[email protected]> 2023-11-21 11:00 ` Alexander Lakhin <[email protected]> 2024-01-22 04:26 ` Peter Smith <[email protected]> 2024-01-05 17:04 ` Robert Haas <[email protected]> 2023-12-31 16:29 [PATCH v5a 6/7] Vacuum first pass uses Streaming Read interface Melanie Plageman <[email protected]> 2023-12-31 16:29 [PATCH v7 6/7] Vacuum first pass uses Streaming Read interface Melanie Plageman <[email protected]> 2023-12-31 16:29 [PATCH v7 6/7] Vacuum first pass uses Streaming Read interface Melanie Plageman <[email protected]> 2023-12-31 16:29 [PATCH v5a 6/7] Vacuum first pass uses Streaming Read interface Melanie Plageman <[email protected]> 2023-12-31 16:29 [PATCH v5a 6/7] Vacuum first pass uses Streaming Read interface Melanie Plageman <[email protected]> 2023-12-31 16:29 [PATCH v7 6/7] Vacuum first pass uses Streaming Read interface Melanie Plageman <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox