public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v45 2/7] Add conditional lock feature to dshash 5+ messages / 4 participants [nested] [flat]
* [PATCH v45 2/7] Add conditional lock feature to dshash @ 2020-03-13 07:58 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Kyotaro Horiguchi @ 2020-03-13 07:58 UTC (permalink / raw) Dshash currently waits for lock unconditionally. It is inconvenient when we want to avoid being blocked by other processes. This commit adds alternative functions of dshash_find and dshash_find_or_insert that allows immediate return on lock failure. --- src/backend/lib/dshash.c | 98 +++++++++++++++++++++------------------- src/include/lib/dshash.h | 3 ++ 2 files changed, 55 insertions(+), 46 deletions(-) diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c index 520bfa0979..853d78b528 100644 --- a/src/backend/lib/dshash.c +++ b/src/backend/lib/dshash.c @@ -383,6 +383,10 @@ dshash_get_hash_table_handle(dshash_table *hash_table) * the caller must take care to ensure that the entry is not left corrupted. * The lock mode is either shared or exclusive depending on 'exclusive'. * + * If found is not NULL, *found is set to true if the key is found in the hash + * table. If the key is not found, *found is set to false and a pointer to a + * newly created entry is returned. + * * The caller must not lock a lock already. * * Note that the lock held is in fact an LWLock, so interrupts will be held on @@ -392,36 +396,7 @@ dshash_get_hash_table_handle(dshash_table *hash_table) void * dshash_find(dshash_table *hash_table, const void *key, bool exclusive) { - dshash_hash hash; - size_t partition; - dshash_table_item *item; - - hash = hash_key(hash_table, key); - partition = PARTITION_FOR_HASH(hash); - - Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(!hash_table->find_locked); - - LWLockAcquire(PARTITION_LOCK(hash_table, partition), - exclusive ? LW_EXCLUSIVE : LW_SHARED); - ensure_valid_bucket_pointers(hash_table); - - /* Search the active bucket. */ - item = find_in_bucket(hash_table, key, BUCKET_FOR_HASH(hash_table, hash)); - - if (!item) - { - /* Not found. */ - LWLockRelease(PARTITION_LOCK(hash_table, partition)); - return NULL; - } - else - { - /* The caller will free the lock by calling dshash_release_lock. */ - hash_table->find_locked = true; - hash_table->find_exclusively_locked = exclusive; - return ENTRY_FROM_ITEM(item); - } + return dshash_find_extended(hash_table, key, exclusive, false, false, NULL); } /* @@ -439,31 +414,60 @@ dshash_find_or_insert(dshash_table *hash_table, const void *key, bool *found) { - dshash_hash hash; - size_t partition_index; - dshash_partition *partition; + return dshash_find_extended(hash_table, key, true, false, true, found); +} + + +/* + * Find the key in the hash table. + * + * "exclusive" is the lock mode in which the partition for the returned item + * is locked. If "nowait" is true, the function immediately returns if + * required lock was not acquired. "insert" indicates insert mode. In this + * mode new entry is inserted and set *found to false. *found is set to true if + * found. "found" must be non-null in this mode. + */ +void * +dshash_find_extended(dshash_table *hash_table, const void *key, + bool exclusive, bool nowait, bool insert, bool *found) +{ + dshash_hash hash = hash_key(hash_table, key); + size_t partidx = PARTITION_FOR_HASH(hash); + dshash_partition *partition = &hash_table->control->partitions[partidx]; + LWLockMode lockmode = exclusive ? LW_EXCLUSIVE : LW_SHARED; dshash_table_item *item; - hash = hash_key(hash_table, key); - partition_index = PARTITION_FOR_HASH(hash); - partition = &hash_table->control->partitions[partition_index]; - - Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(!hash_table->find_locked); + /* must be exclusive when insert allowed */ + Assert(!insert || (exclusive && found != NULL)); restart: - LWLockAcquire(PARTITION_LOCK(hash_table, partition_index), - LW_EXCLUSIVE); + if (!nowait) + LWLockAcquire(PARTITION_LOCK(hash_table, partidx), lockmode); + else if (!LWLockConditionalAcquire(PARTITION_LOCK(hash_table, partidx), + lockmode)) + return NULL; + ensure_valid_bucket_pointers(hash_table); /* Search the active bucket. */ item = find_in_bucket(hash_table, key, BUCKET_FOR_HASH(hash_table, hash)); if (item) - *found = true; + { + if (found) + *found = true; + } else { - *found = false; + if (found) + *found = false; + + if (!insert) + { + /* The caller didn't told to add a new entry. */ + LWLockRelease(PARTITION_LOCK(hash_table, partidx)); + return NULL; + } /* Check if we are getting too full. */ if (partition->count > MAX_COUNT_PER_PARTITION(hash_table)) @@ -479,7 +483,8 @@ restart: * Give up our existing lock first, because resizing needs to * reacquire all the locks in the right order to avoid deadlocks. */ - LWLockRelease(PARTITION_LOCK(hash_table, partition_index)); + LWLockRelease(PARTITION_LOCK(hash_table, partidx)); + resize(hash_table, hash_table->size_log2 + 1); goto restart; @@ -493,12 +498,13 @@ restart: ++partition->count; } - /* The caller must release the lock with dshash_release_lock. */ + /* The caller will free the lock by calling dshash_release_lock. */ hash_table->find_locked = true; - hash_table->find_exclusively_locked = true; + hash_table->find_exclusively_locked = exclusive; return ENTRY_FROM_ITEM(item); } + /* * Remove an entry by key. Returns true if the key was found and the * corresponding entry was removed. diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h index a6ea377173..5b8114d041 100644 --- a/src/include/lib/dshash.h +++ b/src/include/lib/dshash.h @@ -91,6 +91,9 @@ extern void *dshash_find(dshash_table *hash_table, const void *key, bool exclusive); extern void *dshash_find_or_insert(dshash_table *hash_table, const void *key, bool *found); +extern void *dshash_find_extended(dshash_table *hash_table, const void *key, + bool exclusive, bool nowait, bool insert, + bool *found); extern bool dshash_delete_key(dshash_table *hash_table, const void *key); extern void dshash_delete_entry(dshash_table *hash_table, void *entry); extern void dshash_release_lock(dshash_table *hash_table, void *entry); -- 2.27.0 ----Next_Part(Fri_Jan__8_10_24_34_2021_185)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v45-0003-Make-archiver-process-an-auxiliary-process.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Implement generalized sub routine find_in_log for tap test @ 2023-05-25 17:34 Dagfinn Ilmari Mannsåker <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Dagfinn Ilmari Mannsåker @ 2023-05-25 17:34 UTC (permalink / raw) To: vignesh C <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> vignesh C <[email protected]> writes: > Hi, > > The recovery tap test has 4 implementations of find_in_log sub routine > for various uses, I felt we can generalize these and have a single > function for the same. The attached patch is an attempt to have a > generalized sub routine find_in_log which can be used by all of them. > Thoughts? +1 on factoring out this common code. Just a few comments on the implementation. > diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm > index a27fac83d2..5c9b2f6c03 100644 > --- a/src/test/perl/PostgreSQL/Test/Utils.pm > +++ b/src/test/perl/PostgreSQL/Test/Utils.pm > @@ -67,6 +67,7 @@ our @EXPORT = qw( > slurp_file > append_to_file > string_replace_file > + find_in_log > check_mode_recursive > chmod_recursive > check_pg_config > @@ -579,6 +580,28 @@ sub string_replace_file > > =pod > > + > +=item find_in_log(node, pattern, offset) > + > +Find pattern in logfile of node after offset byte. > + > +=cut > + > +sub find_in_log > +{ > + my ($node, $pattern, $offset) = @_; > + > + $offset = 0 unless defined $offset; > + my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile); Since this function is in the same package, there's no need to qualify it with the full name. I know the callers you copied it from did, but they wouldn't have had to either, since it's exported by default (in the @EXPORT array above), unless the use statement has an explicit argument list that excludes it. > + return 0 if (length($log) <= 0 || length($log) <= $offset); > + > + $log = substr($log, $offset); Also, the existing callers don't seem to have got the memo that slurp_file() takes an optinal offset parameter, which will cause it to seek to that postion before slurping the file, which is more efficient than reading the whole file in and substr-ing it. There's not much point in the length checks either, since regex-matching against an empty string is very cheap (and if the provide pattern can match the empty string the whole function call is rather pointless). > + return $log =~ m/$pattern/; > +} All in all, it could be simplified to: sub find_in_log { my ($node, $pattern, $offset) = @_; return slurp_file($node->logfile, $offset) =~ $pattern; } However, none of the other functions in ::Utils know anything about node objects, which makes me think it should be a method on the node itself (i.e. in PostgreSQL::Test::Cluster) instead. Also, I think log_contains would be a better name, since it just returns a boolean. The name find_in_log makes me think it would return the log lines matching the pattern, or the position of the match in the file. In that case, the slurp_file() call would have to be fully qualified, since ::Cluster uses an empty import list to avoid polluting the method namespace with imported functions. - ilmari ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Implement generalized sub routine find_in_log for tap test @ 2023-05-25 22:39 Michael Paquier <[email protected]> parent: Dagfinn Ilmari Mannsåker <[email protected]> 1 sibling, 1 reply; 5+ messages in thread From: Michael Paquier @ 2023-05-25 22:39 UTC (permalink / raw) To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, May 25, 2023 at 06:34:20PM +0100, Dagfinn Ilmari Mannsåker wrote: > However, none of the other functions in ::Utils know anything about node > objects, which makes me think it should be a method on the node itself > (i.e. in PostgreSQL::Test::Cluster) instead. Also, I think log_contains > would be a better name, since it just returns a boolean. The name > find_in_log makes me think it would return the log lines matching the > pattern, or the position of the match in the file. > > In that case, the slurp_file() call would have to be fully qualified, > since ::Cluster uses an empty import list to avoid polluting the method > namespace with imported functions. Hmm. connect_ok() and connect_fails() in Cluster.pm have a similar log comparison logic, feeding from the offset of a log file. Couldn't you use the same code across the board for everything? Note that this stuff is parameterized so as it is possible to check if patterns match or do not match, for multiple patterns. It seems to me that we could use the new log finding routine there as well, so how about extending it a bit more? You would need, at least: - One parameter for log entries matching. - One parameter for log entries not matching. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../ZG%[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Implement generalized sub routine find_in_log for tap test @ 2023-05-26 17:00 vignesh C <[email protected]> parent: Dagfinn Ilmari Mannsåker <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: vignesh C @ 2023-05-26 17:00 UTC (permalink / raw) To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Thu, 25 May 2023 at 23:04, Dagfinn Ilmari Mannsåker <[email protected]> wrote: > > vignesh C <[email protected]> writes: > > > Hi, > > > > The recovery tap test has 4 implementations of find_in_log sub routine > > for various uses, I felt we can generalize these and have a single > > function for the same. The attached patch is an attempt to have a > > generalized sub routine find_in_log which can be used by all of them. > > Thoughts? > > +1 on factoring out this common code. Just a few comments on the implementation. > > > > diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm > > index a27fac83d2..5c9b2f6c03 100644 > > --- a/src/test/perl/PostgreSQL/Test/Utils.pm > > +++ b/src/test/perl/PostgreSQL/Test/Utils.pm > > @@ -67,6 +67,7 @@ our @EXPORT = qw( > > slurp_file > > append_to_file > > string_replace_file > > + find_in_log > > check_mode_recursive > > chmod_recursive > > check_pg_config > > @@ -579,6 +580,28 @@ sub string_replace_file > > > > =pod > > > > + > > +=item find_in_log(node, pattern, offset) > > + > > +Find pattern in logfile of node after offset byte. > > + > > +=cut > > + > > +sub find_in_log > > +{ > > + my ($node, $pattern, $offset) = @_; > > + > > + $offset = 0 unless defined $offset; > > + my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile); > > Since this function is in the same package, there's no need to qualify > it with the full name. I know the callers you copied it from did, but > they wouldn't have had to either, since it's exported by default (in the > @EXPORT array above), unless the use statement has an explicit argument > list that excludes it. I have moved this function to Cluster.pm file now, since it is moved, I had to qualify the name with the full name. > > + return 0 if (length($log) <= 0 || length($log) <= $offset); > > + > > + $log = substr($log, $offset); > > Also, the existing callers don't seem to have got the memo that > slurp_file() takes an optinal offset parameter, which will cause it to > seek to that postion before slurping the file, which is more efficient > than reading the whole file in and substr-ing it. There's not much > point in the length checks either, since regex-matching against an empty > string is very cheap (and if the provide pattern can match the empty > string the whole function call is rather pointless). > > > + return $log =~ m/$pattern/; > > +} > > All in all, it could be simplified to: > > sub find_in_log { > my ($node, $pattern, $offset) = @_; > > return slurp_file($node->logfile, $offset) =~ $pattern; > } Modified in similar lines > However, none of the other functions in ::Utils know anything about node > objects, which makes me think it should be a method on the node itself > (i.e. in PostgreSQL::Test::Cluster) instead. Also, I think log_contains > would be a better name, since it just returns a boolean. The name > find_in_log makes me think it would return the log lines matching the > pattern, or the position of the match in the file. Modified > In that case, the slurp_file() call would have to be fully qualified, > since ::Cluster uses an empty import list to avoid polluting the method > namespace with imported functions. Modified. Thanks for the comments, the attached v2 version patch has the changes for the same. Regards, Vignesh Attachments: [text/x-patch] v2-0001-Remove-duplicate-find_in_log-sub-routines-from-ta.patch (7.7K, ../../CALDaNm2W0OKLX6Y0a9=jcZ_WFWuLwSeqxpk67sCzvhNOB674gg@mail.gmail.com/2-v2-0001-Remove-duplicate-find_in_log-sub-routines-from-ta.patch) download | inline diff: From 8df06d233de4e5b5abe7a4dd4bc314c215b2dfc2 Mon Sep 17 00:00:00 2001 From: Vignesh C <[email protected]> Date: Fri, 26 May 2023 20:07:30 +0530 Subject: [PATCH v2] Remove duplicate find_in_log sub routines from tap tests. Remove duplicate find_in_log sub routines from tap tests. --- src/test/authentication/t/003_peer.pl | 17 ++-------- src/test/perl/PostgreSQL/Test/Cluster.pm | 16 +++++++++ src/test/recovery/t/019_replslot_limit.pl | 33 +++++-------------- src/test/recovery/t/033_replay_tsp_drops.pl | 15 ++------- .../t/035_standby_logical_decoding.pl | 26 +++------------ 5 files changed, 33 insertions(+), 74 deletions(-) diff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl index 3272e52cae..2a035c2d0d 100644 --- a/src/test/authentication/t/003_peer.pl +++ b/src/test/authentication/t/003_peer.pl @@ -69,17 +69,6 @@ sub test_role } } -# Find $pattern in log file of $node. -sub find_in_log -{ - my ($node, $offset, $pattern) = @_; - - my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile, $offset); - return 0 if (length($log) <= 0); - - return $log =~ m/$pattern/; -} - my $node = PostgreSQL::Test::Cluster->new('node'); $node->init; $node->append_conf('postgresql.conf', "log_connections = on\n"); @@ -91,9 +80,9 @@ reset_pg_hba($node, 'peer'); # Check if peer authentication is supported on this platform. my $log_offset = -s $node->logfile; $node->psql('postgres'); -if (find_in_log( - $node, $log_offset, - qr/peer authentication is not supported on this platform/)) +if ($node->log_contains( + qr/peer authentication is not supported on this platform/), + $log_offset,) { plan skip_all => 'peer authentication is not supported on this platform'; } diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm index baea0fcd1c..f10e44ba51 100644 --- a/src/test/perl/PostgreSQL/Test/Cluster.pm +++ b/src/test/perl/PostgreSQL/Test/Cluster.pm @@ -2536,6 +2536,22 @@ sub log_content } +=pod + +=item log_contains(pattern, offset) + +Find pattern in logfile of node after offset byte. + +=cut + +sub log_contains +{ + my ($self, $pattern, $offset) = @_; + + $offset = 0 unless defined $offset; + return PostgreSQL::Test::Utils::slurp_file($self->logfile, $offset) =~ m/$pattern/; +} + =pod =item $node->run_log(...) diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl index a1aba16e14..95acf9e357 100644 --- a/src/test/recovery/t/019_replslot_limit.pl +++ b/src/test/recovery/t/019_replslot_limit.pl @@ -161,8 +161,7 @@ $node_primary->wait_for_catchup($node_standby); $node_standby->stop; -ok( !find_in_log( - $node_standby, +ok( !$node_standby->log_contains( "requested WAL segment [0-9A-F]+ has already been removed"), 'check that required WAL segments are still available'); @@ -184,8 +183,8 @@ $node_primary->safe_psql('postgres', "CHECKPOINT;"); my $invalidated = 0; for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) { - if (find_in_log( - $node_primary, 'invalidating obsolete replication slot "rep1"', + if ($node_primary->log_contains( + 'invalidating obsolete replication slot "rep1"', $logstart)) { $invalidated = 1; @@ -207,7 +206,7 @@ is($result, "rep1|f|t|lost|", my $checkpoint_ended = 0; for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) { - if (find_in_log($node_primary, "checkpoint complete: ", $logstart)) + if ($node_primary->log_contains("checkpoint complete: ", $logstart)) { $checkpoint_ended = 1; last; @@ -237,8 +236,7 @@ $node_standby->start; my $failed = 0; for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) { - if (find_in_log( - $node_standby, + if ($node_standby->log_contains( "requested WAL segment [0-9A-F]+ has already been removed", $logstart)) { @@ -381,8 +379,7 @@ my $msg_logged = 0; my $max_attempts = $PostgreSQL::Test::Utils::timeout_default; while ($max_attempts-- >= 0) { - if (find_in_log( - $node_primary3, + if ($node_primary3->log_contains( "terminating process $senderpid to release replication slot \"rep3\"", $logstart)) { @@ -406,8 +403,8 @@ $msg_logged = 0; $max_attempts = $PostgreSQL::Test::Utils::timeout_default; while ($max_attempts-- >= 0) { - if (find_in_log( - $node_primary3, 'invalidating obsolete replication slot "rep3"', + if ($node_primary3->log_contains( + 'invalidating obsolete replication slot "rep3"', $logstart)) { $msg_logged = 1; @@ -446,18 +443,4 @@ sub get_log_size return (stat $node->logfile)[7]; } -# find $pat in logfile of $node after $off-th byte -sub find_in_log -{ - my ($node, $pat, $off) = @_; - - $off = 0 unless defined $off; - my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile); - return 0 if (length($log) <= $off); - - $log = substr($log, $off); - - return $log =~ m/$pat/; -} - done_testing(); diff --git a/src/test/recovery/t/033_replay_tsp_drops.pl b/src/test/recovery/t/033_replay_tsp_drops.pl index 0a35a7bda6..307c30bc6b 100644 --- a/src/test/recovery/t/033_replay_tsp_drops.pl +++ b/src/test/recovery/t/033_replay_tsp_drops.pl @@ -135,22 +135,11 @@ while ($max_attempts-- >= 0) { last if ( - find_in_log( - $node_standby, + $node_standby->log_contains( qr!WARNING: ( [A-Z0-9]+:)? creating missing directory: pg_tblspc/!, $logstart)); usleep(100_000); } ok($max_attempts > 0, "invalid directory creation is detected"); -done_testing(); - -# find $pat in logfile of $node after $off-th byte -sub find_in_log -{ - my ($node, $pat, $off) = @_; - - my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile, $off); - - return $log =~ m/$pat/; -} +done_testing(); \ No newline at end of file diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl b/src/test/recovery/t/035_standby_logical_decoding.pl index 64beec4bd3..480e6d6caa 100644 --- a/src/test/recovery/t/035_standby_logical_decoding.pl +++ b/src/test/recovery/t/035_standby_logical_decoding.pl @@ -28,20 +28,6 @@ my $res; my $primary_slotname = 'primary_physical'; my $standby_physical_slotname = 'standby_physical'; -# find $pat in logfile of $node after $off-th byte -sub find_in_log -{ - my ($node, $pat, $off) = @_; - - $off = 0 unless defined $off; - my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile); - return 0 if (length($log) <= $off); - - $log = substr($log, $off); - - return $log =~ m/$pat/; -} - # Fetch xmin columns from slot's pg_replication_slots row, after waiting for # given boolean condition to be true to ensure we've reached a quiescent state. sub wait_for_xmins @@ -235,14 +221,12 @@ sub check_for_invalidation my $inactive_slot = $slot_prefix . 'inactiveslot'; # message should be issued - ok( find_in_log( - $node_standby, + ok( $node_standby->log_contains( "invalidating obsolete replication slot \"$inactive_slot\"", $log_start), "inactiveslot slot invalidation is logged $test_name"); - ok( find_in_log( - $node_standby, + ok( $node_standby->log_contains( "invalidating obsolete replication slot \"$active_slot\"", $log_start), "activeslot slot invalidation is logged $test_name"); @@ -657,14 +641,12 @@ $node_primary->safe_psql( $node_primary->wait_for_replay_catchup($node_standby); # message should not be issued -ok( !find_in_log( - $node_standby, +ok( !$node_standby->log_contains( "invalidating obsolete slot \"no_conflict_inactiveslot\"", $logstart), 'inactiveslot slot invalidation is not logged with vacuum on conflict_test' ); -ok( !find_in_log( - $node_standby, +ok( !$node_standby->log_contains( "invalidating obsolete slot \"no_conflict_activeslot\"", $logstart), 'activeslot slot invalidation is not logged with vacuum on conflict_test' ); -- 2.34.1 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Implement generalized sub routine find_in_log for tap test @ 2023-05-27 00:35 vignesh C <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: vignesh C @ 2023-05-27 00:35 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, 26 May 2023 at 04:09, Michael Paquier <[email protected]> wrote: > > On Thu, May 25, 2023 at 06:34:20PM +0100, Dagfinn Ilmari Mannsåker wrote: > > However, none of the other functions in ::Utils know anything about node > > objects, which makes me think it should be a method on the node itself > > (i.e. in PostgreSQL::Test::Cluster) instead. Also, I think log_contains > > would be a better name, since it just returns a boolean. The name > > find_in_log makes me think it would return the log lines matching the > > pattern, or the position of the match in the file. > > > > In that case, the slurp_file() call would have to be fully qualified, > > since ::Cluster uses an empty import list to avoid polluting the method > > namespace with imported functions. > > Hmm. connect_ok() and connect_fails() in Cluster.pm have a similar > log comparison logic, feeding from the offset of a log file. Couldn't > you use the same code across the board for everything? Note that this > stuff is parameterized so as it is possible to check if patterns match > or do not match, for multiple patterns. It seems to me that we could > use the new log finding routine there as well, so how about extending > it a bit more? You would need, at least: > - One parameter for log entries matching. > - One parameter for log entries not matching. I felt adding these to log_contains was making the function slightly complex with multiple checks. I was not able to make it simple with the approach I tried. How about having a common function check_connect_log_contents which has the common log contents check for connect_ok and connect_fails function like the v2-0002 patch attached. Regards, Vignesh Attachments: [text/x-patch] v2-0001-Remove-duplicate-find_in_log-sub-routines-from-ta.patch (7.7K, ../../CALDaNm1sDD-MCW6zrxHCC7Ka5AzO-L4ejxeeTMtXPPN+F4vz2Q@mail.gmail.com/2-v2-0001-Remove-duplicate-find_in_log-sub-routines-from-ta.patch) download | inline diff: From 1e835b6d032ea380bc213ff130e87fbb250be77f Mon Sep 17 00:00:00 2001 From: Vignesh C <[email protected]> Date: Fri, 26 May 2023 20:07:30 +0530 Subject: [PATCH v2 1/2] Remove duplicate find_in_log sub routines from tap tests. Remove duplicate find_in_log sub routines from tap tests. --- src/test/authentication/t/003_peer.pl | 17 ++-------- src/test/perl/PostgreSQL/Test/Cluster.pm | 16 +++++++++ src/test/recovery/t/019_replslot_limit.pl | 33 +++++-------------- src/test/recovery/t/033_replay_tsp_drops.pl | 15 ++------- .../t/035_standby_logical_decoding.pl | 26 +++------------ 5 files changed, 33 insertions(+), 74 deletions(-) diff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl index 3272e52cae..2a035c2d0d 100644 --- a/src/test/authentication/t/003_peer.pl +++ b/src/test/authentication/t/003_peer.pl @@ -69,17 +69,6 @@ sub test_role } } -# Find $pattern in log file of $node. -sub find_in_log -{ - my ($node, $offset, $pattern) = @_; - - my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile, $offset); - return 0 if (length($log) <= 0); - - return $log =~ m/$pattern/; -} - my $node = PostgreSQL::Test::Cluster->new('node'); $node->init; $node->append_conf('postgresql.conf', "log_connections = on\n"); @@ -91,9 +80,9 @@ reset_pg_hba($node, 'peer'); # Check if peer authentication is supported on this platform. my $log_offset = -s $node->logfile; $node->psql('postgres'); -if (find_in_log( - $node, $log_offset, - qr/peer authentication is not supported on this platform/)) +if ($node->log_contains( + qr/peer authentication is not supported on this platform/), + $log_offset,) { plan skip_all => 'peer authentication is not supported on this platform'; } diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm index baea0fcd1c..f10e44ba51 100644 --- a/src/test/perl/PostgreSQL/Test/Cluster.pm +++ b/src/test/perl/PostgreSQL/Test/Cluster.pm @@ -2536,6 +2536,22 @@ sub log_content } +=pod + +=item log_contains(pattern, offset) + +Find pattern in logfile of node after offset byte. + +=cut + +sub log_contains +{ + my ($self, $pattern, $offset) = @_; + + $offset = 0 unless defined $offset; + return PostgreSQL::Test::Utils::slurp_file($self->logfile, $offset) =~ m/$pattern/; +} + =pod =item $node->run_log(...) diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl index a1aba16e14..95acf9e357 100644 --- a/src/test/recovery/t/019_replslot_limit.pl +++ b/src/test/recovery/t/019_replslot_limit.pl @@ -161,8 +161,7 @@ $node_primary->wait_for_catchup($node_standby); $node_standby->stop; -ok( !find_in_log( - $node_standby, +ok( !$node_standby->log_contains( "requested WAL segment [0-9A-F]+ has already been removed"), 'check that required WAL segments are still available'); @@ -184,8 +183,8 @@ $node_primary->safe_psql('postgres', "CHECKPOINT;"); my $invalidated = 0; for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) { - if (find_in_log( - $node_primary, 'invalidating obsolete replication slot "rep1"', + if ($node_primary->log_contains( + 'invalidating obsolete replication slot "rep1"', $logstart)) { $invalidated = 1; @@ -207,7 +206,7 @@ is($result, "rep1|f|t|lost|", my $checkpoint_ended = 0; for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) { - if (find_in_log($node_primary, "checkpoint complete: ", $logstart)) + if ($node_primary->log_contains("checkpoint complete: ", $logstart)) { $checkpoint_ended = 1; last; @@ -237,8 +236,7 @@ $node_standby->start; my $failed = 0; for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) { - if (find_in_log( - $node_standby, + if ($node_standby->log_contains( "requested WAL segment [0-9A-F]+ has already been removed", $logstart)) { @@ -381,8 +379,7 @@ my $msg_logged = 0; my $max_attempts = $PostgreSQL::Test::Utils::timeout_default; while ($max_attempts-- >= 0) { - if (find_in_log( - $node_primary3, + if ($node_primary3->log_contains( "terminating process $senderpid to release replication slot \"rep3\"", $logstart)) { @@ -406,8 +403,8 @@ $msg_logged = 0; $max_attempts = $PostgreSQL::Test::Utils::timeout_default; while ($max_attempts-- >= 0) { - if (find_in_log( - $node_primary3, 'invalidating obsolete replication slot "rep3"', + if ($node_primary3->log_contains( + 'invalidating obsolete replication slot "rep3"', $logstart)) { $msg_logged = 1; @@ -446,18 +443,4 @@ sub get_log_size return (stat $node->logfile)[7]; } -# find $pat in logfile of $node after $off-th byte -sub find_in_log -{ - my ($node, $pat, $off) = @_; - - $off = 0 unless defined $off; - my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile); - return 0 if (length($log) <= $off); - - $log = substr($log, $off); - - return $log =~ m/$pat/; -} - done_testing(); diff --git a/src/test/recovery/t/033_replay_tsp_drops.pl b/src/test/recovery/t/033_replay_tsp_drops.pl index 0a35a7bda6..307c30bc6b 100644 --- a/src/test/recovery/t/033_replay_tsp_drops.pl +++ b/src/test/recovery/t/033_replay_tsp_drops.pl @@ -135,22 +135,11 @@ while ($max_attempts-- >= 0) { last if ( - find_in_log( - $node_standby, + $node_standby->log_contains( qr!WARNING: ( [A-Z0-9]+:)? creating missing directory: pg_tblspc/!, $logstart)); usleep(100_000); } ok($max_attempts > 0, "invalid directory creation is detected"); -done_testing(); - -# find $pat in logfile of $node after $off-th byte -sub find_in_log -{ - my ($node, $pat, $off) = @_; - - my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile, $off); - - return $log =~ m/$pat/; -} +done_testing(); \ No newline at end of file diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl b/src/test/recovery/t/035_standby_logical_decoding.pl index 64beec4bd3..480e6d6caa 100644 --- a/src/test/recovery/t/035_standby_logical_decoding.pl +++ b/src/test/recovery/t/035_standby_logical_decoding.pl @@ -28,20 +28,6 @@ my $res; my $primary_slotname = 'primary_physical'; my $standby_physical_slotname = 'standby_physical'; -# find $pat in logfile of $node after $off-th byte -sub find_in_log -{ - my ($node, $pat, $off) = @_; - - $off = 0 unless defined $off; - my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile); - return 0 if (length($log) <= $off); - - $log = substr($log, $off); - - return $log =~ m/$pat/; -} - # Fetch xmin columns from slot's pg_replication_slots row, after waiting for # given boolean condition to be true to ensure we've reached a quiescent state. sub wait_for_xmins @@ -235,14 +221,12 @@ sub check_for_invalidation my $inactive_slot = $slot_prefix . 'inactiveslot'; # message should be issued - ok( find_in_log( - $node_standby, + ok( $node_standby->log_contains( "invalidating obsolete replication slot \"$inactive_slot\"", $log_start), "inactiveslot slot invalidation is logged $test_name"); - ok( find_in_log( - $node_standby, + ok( $node_standby->log_contains( "invalidating obsolete replication slot \"$active_slot\"", $log_start), "activeslot slot invalidation is logged $test_name"); @@ -657,14 +641,12 @@ $node_primary->safe_psql( $node_primary->wait_for_replay_catchup($node_standby); # message should not be issued -ok( !find_in_log( - $node_standby, +ok( !$node_standby->log_contains( "invalidating obsolete slot \"no_conflict_inactiveslot\"", $logstart), 'inactiveslot slot invalidation is not logged with vacuum on conflict_test' ); -ok( !find_in_log( - $node_standby, +ok( !$node_standby->log_contains( "invalidating obsolete slot \"no_conflict_activeslot\"", $logstart), 'activeslot slot invalidation is not logged with vacuum on conflict_test' ); -- 2.34.1 [text/x-patch] v2-0002-Move-common-connection-log-content-verification-c.patch (4.7K, ../../CALDaNm1sDD-MCW6zrxHCC7Ka5AzO-L4ejxeeTMtXPPN+F4vz2Q@mail.gmail.com/3-v2-0002-Move-common-connection-log-content-verification-c.patch) download | inline diff: From aa587e7cedd92d60e93075d643cb8ad93ca07c5b Mon Sep 17 00:00:00 2001 From: Vignesh C <[email protected]> Date: Sat, 27 May 2023 05:36:34 +0530 Subject: [PATCH v2 2/2] Move common connection log content verification code to a common function. Move common connection log content verification code to a common function. --- src/test/perl/PostgreSQL/Test/Cluster.pm | 122 +++++++++++------------ 1 file changed, 60 insertions(+), 62 deletions(-) diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm index f10e44ba51..7458406ee6 100644 --- a/src/test/perl/PostgreSQL/Test/Cluster.pm +++ b/src/test/perl/PostgreSQL/Test/Cluster.pm @@ -2168,21 +2168,19 @@ sub pgbench =pod -=item $node->connect_ok($connstr, $test_name, %params) +=item $node->check_connect_log_contents($offset, $test_name, %parameters) -Attempt a connection with a custom connection string. This is expected -to succeed. +Check connection log contents. =over -=item sql => B<value> +=item $test_name -If this parameter is set, this query is used for the connection attempt -instead of the default. +Name of test for error messages. -=item expected_stdout => B<value> +=item $offset -If this regular expression is set, matches it with the output generated. +Offset of the log file. =item log_like => [ qr/required message/ ] @@ -2200,6 +2198,58 @@ passed to C<Test::More::unlike()>. =cut +sub check_connect_log_contents +{ + my ($self, $test_name, $offset, %params) = @_; + + my (@log_like, @log_unlike); + if (defined($params{log_like})) + { + @log_like = @{ $params{log_like} }; + } + if (defined($params{log_unlike})) + { + @log_unlike = @{ $params{log_unlike} }; + } + + if (@log_like or @log_unlike) + { + my $log_contents = + PostgreSQL::Test::Utils::slurp_file($self->logfile, $offset); + + while (my $regex = shift @log_like) + { + like($log_contents, $regex, "$test_name: log matches"); + } + while (my $regex = shift @log_unlike) + { + unlike($log_contents, $regex, "$test_name: log does not match"); + } + } +} + +=pod + +=item $node->connect_ok($connstr, $test_name, %params) + +Attempt a connection with a custom connection string. This is expected +to succeed. + +=over + +=item sql => B<value> + +If this parameter is set, this query is used for the connection attempt +instead of the default. + +=item expected_stdout => B<value> + +If this regular expression is set, matches it with the output generated. + +=back + +=cut + sub connect_ok { local $Test::Builder::Level = $Test::Builder::Level + 1; @@ -2215,16 +2265,6 @@ sub connect_ok $sql = "SELECT \$\$connected with $connstr\$\$"; } - my (@log_like, @log_unlike); - if (defined($params{log_like})) - { - @log_like = @{ $params{log_like} }; - } - if (defined($params{log_unlike})) - { - @log_unlike = @{ $params{log_unlike} }; - } - my $log_location = -s $self->logfile; # Never prompt for a password, any callers of this routine should @@ -2245,20 +2285,7 @@ sub connect_ok is($stderr, "", "$test_name: no stderr"); - if (@log_like or @log_unlike) - { - my $log_contents = - PostgreSQL::Test::Utils::slurp_file($self->logfile, $log_location); - - while (my $regex = shift @log_like) - { - like($log_contents, $regex, "$test_name: log matches"); - } - while (my $regex = shift @log_unlike) - { - unlike($log_contents, $regex, "$test_name: log does not match"); - } - } + $self->check_connect_log_contents($test_name, $log_location, %params); } =pod @@ -2274,12 +2301,6 @@ to fail. If this regular expression is set, matches it with the output generated. -=item log_like => [ qr/required message/ ] - -=item log_unlike => [ qr/prohibited message/ ] - -See C<connect_ok(...)>, above. - =back =cut @@ -2289,16 +2310,6 @@ sub connect_fails local $Test::Builder::Level = $Test::Builder::Level + 1; my ($self, $connstr, $test_name, %params) = @_; - my (@log_like, @log_unlike); - if (defined($params{log_like})) - { - @log_like = @{ $params{log_like} }; - } - if (defined($params{log_unlike})) - { - @log_unlike = @{ $params{log_unlike} }; - } - my $log_location = -s $self->logfile; # Never prompt for a password, any callers of this routine should @@ -2316,20 +2327,7 @@ sub connect_fails like($stderr, $params{expected_stderr}, "$test_name: matches"); } - if (@log_like or @log_unlike) - { - my $log_contents = - PostgreSQL::Test::Utils::slurp_file($self->logfile, $log_location); - - while (my $regex = shift @log_like) - { - like($log_contents, $regex, "$test_name: log matches"); - } - while (my $regex = shift @log_unlike) - { - unlike($log_contents, $regex, "$test_name: log does not match"); - } - } + $self->check_connect_log_contents($test_name, $log_location, %params); } =pod -- 2.34.1 ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-05-27 00:35 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-13 07:58 [PATCH v45 2/7] Add conditional lock feature to dshash Kyotaro Horiguchi <[email protected]> 2023-05-25 17:34 Re: Implement generalized sub routine find_in_log for tap test Dagfinn Ilmari Mannsåker <[email protected]> 2023-05-25 22:39 ` Re: Implement generalized sub routine find_in_log for tap test Michael Paquier <[email protected]> 2023-05-27 00:35 ` Re: Implement generalized sub routine find_in_log for tap test vignesh C <[email protected]> 2023-05-26 17:00 ` Re: Implement generalized sub routine find_in_log for tap test vignesh C <[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