public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v5 1/2] Move a few ResourceOwnerEnlarge() calls for safety and clarity.
5+ messages / 4 participants
[nested] [flat]
* [PATCH v5 1/2] Move a few ResourceOwnerEnlarge() calls for safety and clarity.
@ 2021-01-13 10:21 Heikki Linnakangas <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Heikki Linnakangas @ 2021-01-13 10:21 UTC (permalink / raw)
These are functions where quite a lot of things happen between the
ResourceOwnerEnlarge and ResourceOwnerRemember calls. It's important that
there are no unrelated ResourceOwnerRemember() calls in the code
inbetween, otherwise the entry reserved by the ResourceOwnerEnlarge() call
might be used up by the intervening ResourceOwnerRemember() and not be
available at the intended ResourceOwnerRemember() call anymore. The longer
the code path between them is, the harder it is to verify that.
In bufmgr.c, there is a function similar to ResourceOwnerEnlarge(),
to ensure that the private refcount array has enough space. The
ReservePrivateRefCountEntry() calls, analogous to ResourceOwnerEnlarge(),
were made at different places than the ResourceOwnerEnlarge() calls.
Move the ResourceOwnerEnlarge() calls together with the
ReservePrivateRefCountEntry() calls for consistency.
---
src/backend/storage/buffer/bufmgr.c | 39 +++++++++++----------------
src/backend/storage/buffer/localbuf.c | 3 +++
src/backend/utils/cache/catcache.c | 13 ++++++---
3 files changed, 28 insertions(+), 27 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 561c212092f..cde869e7d64 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -738,9 +738,6 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
*hit = false;
- /* Make sure we will have room to remember the buffer pin */
- ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
-
isExtend = (blockNum == P_NEW);
TRACE_POSTGRESQL_BUFFER_READ_START(forkNum, blockNum,
@@ -1093,9 +1090,11 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
{
/*
* Ensure, while the spinlock's not yet held, that there's a free
- * refcount entry.
+ * refcount entry and that the resoure owner has room to remember the
+ * pin.
*/
ReservePrivateRefCountEntry();
+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
/*
* Select a victim buffer. The buffer is returned with its header
@@ -1595,8 +1594,6 @@ ReleaseAndReadBuffer(Buffer buffer,
* taking the buffer header lock; instead update the state variable in loop of
* CAS operations. Hopefully it's just a single CAS.
*
- * Note that ResourceOwnerEnlargeBuffers must have been done already.
- *
* Returns true if buffer is BM_VALID, else false. This provision allows
* some callers to avoid an extra spinlock cycle.
*/
@@ -1607,6 +1604,8 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy)
bool result;
PrivateRefCountEntry *ref;
+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
+
ref = GetPrivateRefCountEntry(b, true);
if (ref == NULL)
@@ -1687,7 +1686,8 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy)
* The spinlock is released before return.
*
* As this function is called with the spinlock held, the caller has to
- * previously call ReservePrivateRefCountEntry().
+ * previously call ReservePrivateRefCountEntry() and
+ * ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
*
* Currently, no callers of this function want to modify the buffer's
* usage_count at all, so there's no need for a strategy parameter.
@@ -1857,9 +1857,6 @@ BufferSync(int flags)
int mask = BM_DIRTY;
WritebackContext wb_context;
- /* Make sure we can handle the pin inside SyncOneBuffer */
- ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
-
/*
* Unless this is a shutdown checkpoint or we have been explicitly told,
* we write only permanent, dirty buffers. But at shutdown or end of
@@ -2334,9 +2331,6 @@ BgBufferSync(WritebackContext *wb_context)
* requirements, or hit the bgwriter_lru_maxpages limit.
*/
- /* Make sure we can handle the pin inside SyncOneBuffer */
- ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
-
num_to_scan = bufs_to_lap;
num_written = 0;
reusable_buffers = reusable_buffers_est;
@@ -2418,8 +2412,6 @@ BgBufferSync(WritebackContext *wb_context)
*
* (BUF_WRITTEN could be set in error if FlushBuffer finds the buffer clean
* after locking it, but we don't care all that much.)
- *
- * Note: caller must have done ResourceOwnerEnlargeBuffers.
*/
static int
SyncOneBuffer(int buf_id, bool skip_recently_used, WritebackContext *wb_context)
@@ -2429,7 +2421,9 @@ SyncOneBuffer(int buf_id, bool skip_recently_used, WritebackContext *wb_context)
uint32 buf_state;
BufferTag tag;
+ /* Make sure we can handle the pin */
ReservePrivateRefCountEntry();
+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
/*
* Check whether buffer needs writing.
@@ -3487,9 +3481,6 @@ FlushRelationBuffers(Relation rel)
return;
}
- /* Make sure we can handle the pin inside the loop */
- ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
-
for (i = 0; i < NBuffers; i++)
{
uint32 buf_state;
@@ -3503,7 +3494,9 @@ FlushRelationBuffers(Relation rel)
if (!RelFileNodeEquals(bufHdr->tag.rnode, rel->rd_node))
continue;
+ /* Make sure we can handle the pin */
ReservePrivateRefCountEntry();
+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
buf_state = LockBufHdr(bufHdr);
if (RelFileNodeEquals(bufHdr->tag.rnode, rel->rd_node) &&
@@ -3560,9 +3553,6 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
if (use_bsearch)
pg_qsort(srels, nrels, sizeof(SMgrSortArray), rnode_comparator);
- /* Make sure we can handle the pin inside the loop */
- ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
-
for (i = 0; i < NBuffers; i++)
{
SMgrSortArray *srelent = NULL;
@@ -3599,7 +3589,9 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
if (srelent == NULL)
continue;
+ /* Make sure we can handle the pin */
ReservePrivateRefCountEntry();
+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
buf_state = LockBufHdr(bufHdr);
if (RelFileNodeEquals(bufHdr->tag.rnode, srelent->rnode) &&
@@ -3639,9 +3631,6 @@ FlushDatabaseBuffers(Oid dbid)
int i;
BufferDesc *bufHdr;
- /* Make sure we can handle the pin inside the loop */
- ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
-
for (i = 0; i < NBuffers; i++)
{
uint32 buf_state;
@@ -3655,7 +3644,9 @@ FlushDatabaseBuffers(Oid dbid)
if (bufHdr->tag.rnode.dbNode != dbid)
continue;
+ /* Make sure we can handle the pin */
ReservePrivateRefCountEntry();
+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
buf_state = LockBufHdr(bufHdr);
if (bufHdr->tag.rnode.dbNode == dbid &&
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 04b3558ea33..f7c15ea8a44 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -123,6 +123,9 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
if (LocalBufHash == NULL)
InitLocalBuffers();
+ /* Make sure we will have room to remember the buffer pin */
+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
+
/* See if the desired buffer already exists */
hresult = (LocalBufferLookupEnt *)
hash_search(LocalBufHash, (void *) &newTag, HASH_FIND, NULL);
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index fa2b49c676e..86fdfc454d2 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -1607,8 +1607,6 @@ SearchCatCacheList(CatCache *cache,
* block to ensure we can undo those refcounts if we get an error before
* we finish constructing the CatCList.
*/
- ResourceOwnerEnlargeCatCacheListRefs(CurrentResourceOwner);
-
ctlist = NIL;
PG_TRY();
@@ -1696,13 +1694,22 @@ SearchCatCacheList(CatCache *cache,
table_close(relation, AccessShareLock);
+ /* Make sure the resource owner has room to remember this entry. */
+ ResourceOwnerEnlargeCatCacheListRefs(CurrentResourceOwner);
+
/* Now we can build the CatCList entry. */
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
nmembers = list_length(ctlist);
cl = (CatCList *)
palloc(offsetof(CatCList, members) + nmembers * sizeof(CatCTup *));
- /* Extract key values */
+ /*
+ * Extract key values.
+ *
+ * XXX: If we run out of memory while copying the key values, we will
+ * leak any allocations we had already made in the CacheMemoryContext.
+ * That is unlikely enough that we just accept the risk.
+ */
CatCacheCopyKeys(cache->cc_tupdesc, nkeys, cache->cc_keyno,
arguments, cl->keys);
MemoryContextSwitchTo(oldcxt);
--
2.29.2
--------------1104342304EC6341F5D7C60C
Content-Type: text/x-patch; charset=UTF-8;
name="v5-0002-Make-resowners-more-easily-extensible.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="v5-0002-Make-resowners-more-easily-extensible.patch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points
@ 2024-08-23 22:45 Tom Lane <[email protected]>
0 siblings, 2 replies; 5+ messages in thread
From: Tom Lane @ 2024-08-23 22:45 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Michael Paquier <[email protected]>; Maxim Orlov <[email protected]>; pgsql-hackers
Alvaro Herrera <[email protected]> writes:
> On 2024-Aug-23, Michael Paquier wrote:
>>> I've been curious about what exactly does this Makefile line
>>> export enable_injection_points enable_injection_points
>>> achieve.
>> Without this line, the TAP tests would not be able to know if
>> injection points are enabled or not, no?
> Right, I figured out afterwards that what that does is export the
> make-level variable as an environment variable.
It exports it twice, though, which is pretty confusing.
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points
@ 2024-08-28 06:14 Maxim Orlov <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: Maxim Orlov @ 2024-08-28 06:14 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers
By the way, we have the same kind of "problem" with the meson build. As we
are deliberately
not want to install src/test/modules, after b6a0d469cae and 0d237aeebaee we
must add step
"meson compile install-test-files" in order to "meson test -q --setup
running" to be successful.
To be honest, this step is not obvious. Especially than there was no such
step before. But
docs and https://wiki.postgresql.org/wiki/Meson are completely silenced
about it.
--
Best regards,
Maxim Orlov.
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points
@ 2024-09-02 00:53 Michael Paquier <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2024-09-02 00:53 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Maxim Orlov <[email protected]>; pgsql-hackers
On Fri, Aug 23, 2024 at 06:45:02PM -0400, Tom Lane wrote:
> It exports it twice, though, which is pretty confusing.
Right. I am not sure what was my state of mind back then, but this
pattern has spread a bit.
REL_17_STABLE is frozen for a few more days, so I'll address all the
items of this thread that once the release of this week is tagged: the
export duplicates and the installcheck issue. These are staged on a
local branch for now.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points
@ 2024-09-04 00:40 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Michael Paquier @ 2024-09-04 00:40 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Maxim Orlov <[email protected]>; pgsql-hackers
On Mon, Sep 02, 2024 at 09:53:30AM +0900, Michael Paquier wrote:
> REL_17_STABLE is frozen for a few more days, so I'll address all the
> items of this thread that once the release of this week is tagged: the
> export duplicates and the installcheck issue. These are staged on a
> local branch for now.
There are two TAP tests in REL_17_STABLE that use the module
injection_points, and we have 6 of them on HEAD. For now, I have
applied a patch that addresses the problems for v17 to avoid any
problems with the upcoming release, fixing the two tests that exist in
REL_17_STABLE.
For HEAD, I'd like to be slightly more ambitious and propose a routine
in Cluster.pm that scans for available extensions, as of the attached.
This simplifies the injection point test in libpq, as the
injection_point is one portion of the test so we don't need the check
based on the environment variable. There is no need for checks in the
TAP tests injection_points's 001_stats.pl and test_slru's
001_multixact.pl as these modules disable installcheck.
Any thoughts about the attached? This makes installcheck work here
with and without the configure switch.
--
Michael
Attachments:
[text/x-diff] 0001-Check-availability-of-module-injection_points-in-TAP.patch (6.1K, ../../[email protected]/2-0001-Check-availability-of-module-injection_points-in-TAP.patch)
download | inline diff:
From 74deadf240a521ef774dbb540f411e2bccb5b421 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Wed, 4 Sep 2024 09:38:36 +0900
Subject: [PATCH] Check availability of module injection_points in TAP tests
This fixes various defects with installcheck for TAP tests that expect
the module to exist in an installation, but it is not installed by
default with installcheck.
The check is refactored as a new routine in Cluster.pm.
---
src/interfaces/libpq/Makefile | 2 +-
src/interfaces/libpq/meson.build | 1 -
.../libpq/t/005_negotiate_encryption.pl | 7 ++++--
src/test/modules/test_misc/t/005_timeouts.pl | 5 +----
.../test_misc/t/006_signal_autovacuum.pl | 9 ++++++++
src/test/perl/PostgreSQL/Test/Cluster.pm | 22 +++++++++++++++++++
.../recovery/t/041_checkpoint_at_promote.pl | 5 +----
7 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index 27f8499d8a..c1bf33dbdc 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -15,7 +15,7 @@ subdir = src/interfaces/libpq
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
-export with_ssl with_gssapi with_krb_srvnam enable_injection_points
+export with_ssl with_gssapi with_krb_srvnam
PGFILEDESC = "PostgreSQL Access Library"
diff --git a/src/interfaces/libpq/meson.build b/src/interfaces/libpq/meson.build
index 7623aeadab..ed2a4048d1 100644
--- a/src/interfaces/libpq/meson.build
+++ b/src/interfaces/libpq/meson.build
@@ -121,7 +121,6 @@ tests += {
't/005_negotiate_encryption.pl',
],
'env': {
- 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no',
'with_ssl': ssl_library,
'with_gssapi': gssapi.found() ? 'yes' : 'no',
'with_krb_srvnam': 'postgres',
diff --git a/src/interfaces/libpq/t/005_negotiate_encryption.pl b/src/interfaces/libpq/t/005_negotiate_encryption.pl
index 73f0056f10..06d67de2db 100644
--- a/src/interfaces/libpq/t/005_negotiate_encryption.pl
+++ b/src/interfaces/libpq/t/005_negotiate_encryption.pl
@@ -90,8 +90,6 @@ my $kerberos_enabled =
$ENV{PG_TEST_EXTRA} && $ENV{PG_TEST_EXTRA} =~ /\bkerberos\b/;
my $ssl_supported = $ENV{with_ssl} eq 'openssl';
-my $injection_points_supported = $ENV{enable_injection_points} eq 'yes';
-
###
### Prepare test server for GSSAPI and SSL authentication, with a few
### different test users and helper functions. We don't actually
@@ -151,6 +149,11 @@ if ($ssl_supported != 0)
$node->start;
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+my $injection_points_supported = $node->check_extension('injection_points');
+
$node->safe_psql('postgres', 'CREATE USER localuser;');
$node->safe_psql('postgres', 'CREATE USER testuser;');
$node->safe_psql('postgres', 'CREATE USER ssluser;');
diff --git a/src/test/modules/test_misc/t/005_timeouts.pl b/src/test/modules/test_misc/t/005_timeouts.pl
index 53e44016e3..d9b7219121 100644
--- a/src/test/modules/test_misc/t/005_timeouts.pl
+++ b/src/test/modules/test_misc/t/005_timeouts.pl
@@ -28,10 +28,7 @@ $node->start;
# Check if the extension injection_points is available, as it may be
# possible that this script is run with installcheck, where the module
# would not be installed by default.
-my $result = $node->safe_psql('postgres',
- "SELECT count(*) > 0 FROM pg_available_extensions WHERE name = 'injection_points';"
-);
-if ($result eq 'f')
+if (!$node->check_extension('injection_points'))
{
plan skip_all => 'Extension injection_points not installed';
}
diff --git a/src/test/modules/test_misc/t/006_signal_autovacuum.pl b/src/test/modules/test_misc/t/006_signal_autovacuum.pl
index 929253f754..aaea569c10 100644
--- a/src/test/modules/test_misc/t/006_signal_autovacuum.pl
+++ b/src/test/modules/test_misc/t/006_signal_autovacuum.pl
@@ -25,6 +25,15 @@ $node->init;
# This ensures a quick worker spawn.
$node->append_conf('postgresql.conf', 'autovacuum_naptime = 1');
$node->start;
+
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+if (!$node->check_extension('injection_points'))
+{
+ plan skip_all => 'Extension injection_points not installed';
+}
+
$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
$node->safe_psql(
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index fe6ebf10f7..143dc8c101 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -2837,6 +2837,28 @@ sub lsn
=pod
+=item $node->check_extension(extension_name)
+
+Scan pg_available_extensions to check that an extension is available in an
+installation.
+
+Returns 1 if the extension is available, 0 otherwise.
+
+=cut
+
+sub check_extension
+{
+ my ($self, $extension_name) = @_;
+
+ my $result = $self->safe_psql('postgres',
+ "SELECT count(*) > 0 FROM pg_available_extensions WHERE name = '$extension_name';"
+ );
+
+ return $result eq 't' ? 1 : 0;
+}
+
+=pod
+
=item $node->wait_for_event(wait_event_name, backend_type)
Poll pg_stat_activity until backend_type reaches wait_event_name.
diff --git a/src/test/recovery/t/041_checkpoint_at_promote.pl b/src/test/recovery/t/041_checkpoint_at_promote.pl
index 905662353d..3c21d18e3a 100644
--- a/src/test/recovery/t/041_checkpoint_at_promote.pl
+++ b/src/test/recovery/t/041_checkpoint_at_promote.pl
@@ -38,10 +38,7 @@ $node_primary->start;
# Check if the extension injection_points is available, as it may be
# possible that this script is run with installcheck, where the module
# would not be installed by default.
-my $result = $node_primary->safe_psql('postgres',
- "SELECT count(*) > 0 FROM pg_available_extensions WHERE name = 'injection_points';"
-);
-if ($result eq 'f')
+if (!$node_primary->check_extension('injection_points'))
{
plan skip_all => 'Extension injection_points not installed';
}
--
2.45.2
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-09-04 00:40 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-13 10:21 [PATCH v5 1/2] Move a few ResourceOwnerEnlarge() calls for safety and clarity. Heikki Linnakangas <[email protected]>
2024-08-23 22:45 Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points Tom Lane <[email protected]>
2024-08-28 06:14 ` Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points Maxim Orlov <[email protected]>
2024-09-02 00:53 ` Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points Michael Paquier <[email protected]>
2024-09-04 00:40 ` Re: Test 041_checkpoint_at_promote.pl faild in installcheck due to missing injection_points Michael Paquier <[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