Some codes refer slot()->{'slot_name'} but it is not defined

Started by Hayato Kuroda (Fujitsu)9 months ago7 messages

kuroda.hayato@fujitsu.com

9 months ago

1 attachment(s)

Dear hackers,

Cluster.pm defines a function slot()which requires a slot_name as a key
and returns attributes of the given slot, as a hash-ref. ISTM, the hash
does not contain 'slot_name'.

However, I found that some codes access it by using a key 'slot_name'. ISTM it always
becomes 'undef' thus any tests are meaningless.

It looks like that existing codes want to check the existing of given logical slots.
So, it is enough to search with key 'plugin'. The valid value is set if exists, otherwise ''.

How do you think?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Attachments:

0001-Fix-invalid-referring-of-hash-ref-for-replication-sl.patchapplication/octet-stream; name=0001-Fix-invalid-referring-of-hash-ref-for-replication-sl.patchDownload

From 47249c139c7fe7671d02657c6fb5f9bed128af14 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Thu, 3 Apr 2025 12:12:12 +0900
Subject: [PATCH] Fix invalid referring of hash-ref for replication slots

hash-ref gerenated by slot() did not have key 'slot_name', but some codes
referred it. Fix it by referring 'plugin' instead.
---
 src/test/recovery/t/006_logical_decoding.pl           | 8 ++++----
 src/test/recovery/t/010_logical_decoding_timelines.pl | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/test/recovery/t/006_logical_decoding.pl b/src/test/recovery/t/006_logical_decoding.pl
index a5678bc4dc4..2137c4e5e30 100644
--- a/src/test/recovery/t/006_logical_decoding.pl
+++ b/src/test/recovery/t/006_logical_decoding.pl
@@ -161,8 +161,8 @@ SKIP:
 	is($node_primary->psql('postgres', 'DROP DATABASE otherdb'),
 		3, 'dropping a DB with active logical slots fails');
 	$pg_recvlogical->kill_kill;
-	is($node_primary->slot('otherdb_slot')->{'slot_name'},
-		undef, 'logical slot still exists');
+	is($node_primary->slot('otherdb_slot')->{'plugin'},
+		'test_decoding', 'logical slot still exists');
 }
 
 $node_primary->poll_query_until('otherdb',
@@ -171,8 +171,8 @@ $node_primary->poll_query_until('otherdb',
 
 is($node_primary->psql('postgres', 'DROP DATABASE otherdb'),
 	0, 'dropping a DB with inactive logical slots succeeds');
-is($node_primary->slot('otherdb_slot')->{'slot_name'},
-	undef, 'logical slot was actually dropped with DB');
+is($node_primary->slot('otherdb_slot')->{'plugin'},
+	'', 'logical slot was actually dropped with DB');
 
 # Test logical slot advancing and its durability.
 # Passing failover=true (last arg) should not have any impact on advancing.
diff --git a/src/test/recovery/t/010_logical_decoding_timelines.pl b/src/test/recovery/t/010_logical_decoding_timelines.pl
index 08615f1fca8..0199ae95abf 100644
--- a/src/test/recovery/t/010_logical_decoding_timelines.pl
+++ b/src/test/recovery/t/010_logical_decoding_timelines.pl
@@ -94,8 +94,8 @@ is( $node_replica->safe_psql(
 		'postgres', q[SELECT 1 FROM pg_database WHERE datname = 'dropme']),
 	'',
 	'dropped DB dropme on standby');
-is($node_primary->slot('dropme_slot')->{'slot_name'},
-	undef, 'logical slot was actually dropped on standby');
+is($node_primary->slot('dropme_slot')->{'plugin'},
+	'', 'logical slot was actually dropped on standby');
 
 # Back to testing failover...
 $node_primary->safe_psql('postgres',
-- 
2.43.5

Fujii Masao

masao.fujii@oss.nttdata.com

9 months ago

In reply to: Hayato Kuroda (Fujitsu) (#1)

Re: Some codes refer slot()->{'slot_name'} but it is not defined

On 2025/04/03 12:15, Hayato Kuroda (Fujitsu) wrote:

Dear hackers,

Cluster.pm defines a function slot()which requires a slot_name as a key
and returns attributes of the given slot, as a hash-ref. ISTM, the hash
does not contain 'slot_name'.

However, I found that some codes access it by using a key 'slot_name'. ISTM it always
becomes 'undef' thus any tests are meaningless.

It looks like that existing codes want to check the existing of given logical slots.
So, it is enough to search with key 'plugin'. The valid value is set if exists, otherwise ''.

How do you think?

I think you're right. The patch looks good to me.

-is($node_primary->slot('dropme_slot')->{'slot_name'},
-	undef, 'logical slot was actually dropped on standby');
+is($node_primary->slot('dropme_slot')->{'plugin'},
+	'', 'logical slot was actually dropped on standby');

This seems like a separate issue from what your patch is addressing,
but since this test is meant to confirm that the slot was dropped
on the standby, shouldn't node_primary be node_replica instead?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Hayato Kuroda (Fujitsu)

kuroda.hayato@fujitsu.com

9 months ago

In reply to: Fujii Masao (#2)

3 attachment(s)

RE: Some codes refer slot()->{'slot_name'} but it is not defined

Dear Fujii-san,

-is($node_primary->slot('dropme_slot')->{'slot_name'},
-	undef, 'logical slot was actually dropped on standby');
+is($node_primary->slot('dropme_slot')->{'plugin'},
+	'', 'logical slot was actually dropped on standby');
This seems like a separate issue from what your patch is addressing,
but since this test is meant to confirm that the slot was dropped
on the standby, shouldn't node_primary be node_replica instead?

You are right. It has been missed 8 years ago, let's fix now.

BTW, the issue exists for all supported branches. How do you feel
to backpatch them? PSA all patch set.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Attachments:

v2-PG13-0001-Fix-invalid-referring-of-hash-ref-for-replic.patchapplication/octet-stream; name=v2-PG13-0001-Fix-invalid-referring-of-hash-ref-for-replic.patchDownload

From 39823c1bd593c0e31684522c0ae916502e59489f Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Thu, 3 Apr 2025 12:12:12 +0900
Subject: [PATCH v2-PG13] Fix invalid referring of hash-ref for replication
 slots

hash-ref gerenated by slot() did not have key 'slot_name', but some codes
referred it. Fix it by referring 'plugin' instead.
---
 src/test/recovery/t/006_logical_decoding.pl           | 8 ++++----
 src/test/recovery/t/010_logical_decoding_timelines.pl | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/test/recovery/t/006_logical_decoding.pl b/src/test/recovery/t/006_logical_decoding.pl
index f523d8f5933..840cacd73df 100644
--- a/src/test/recovery/t/006_logical_decoding.pl
+++ b/src/test/recovery/t/006_logical_decoding.pl
@@ -144,8 +144,8 @@ SKIP:
 	is($node_master->psql('postgres', 'DROP DATABASE otherdb'),
 		3, 'dropping a DB with active logical slots fails');
 	$pg_recvlogical->kill_kill;
-	is($node_master->slot('otherdb_slot')->{'slot_name'},
-		undef, 'logical slot still exists');
+	is($node_master->slot('otherdb_slot')->{'plugin'},
+		'test_decoding', 'logical slot still exists');
 }
 
 $node_master->poll_query_until('otherdb',
@@ -154,8 +154,8 @@ $node_master->poll_query_until('otherdb',
 
 is($node_master->psql('postgres', 'DROP DATABASE otherdb'),
 	0, 'dropping a DB with inactive logical slots succeeds');
-is($node_master->slot('otherdb_slot')->{'slot_name'},
-	undef, 'logical slot was actually dropped with DB');
+is($node_master->slot('otherdb_slot')->{'plugin'},
+	'', 'logical slot was actually dropped with DB');
 
 # Test logical slot advancing and its durability.
 my $logical_slot = 'logical_slot';
diff --git a/src/test/recovery/t/010_logical_decoding_timelines.pl b/src/test/recovery/t/010_logical_decoding_timelines.pl
index 4c8efb3bafc..519bcc40286 100644
--- a/src/test/recovery/t/010_logical_decoding_timelines.pl
+++ b/src/test/recovery/t/010_logical_decoding_timelines.pl
@@ -90,8 +90,8 @@ is( $node_replica->safe_psql(
 		'postgres', q[SELECT 1 FROM pg_database WHERE datname = 'dropme']),
 	'',
 	'dropped DB dropme on standby');
-is($node_master->slot('dropme_slot')->{'slot_name'},
-	undef, 'logical slot was actually dropped on standby');
+is($node_replica->slot('dropme_slot')->{'plugin'},
+	'', 'logical slot was actually dropped on standby');
 
 # Back to testing failover...
 $node_master->safe_psql('postgres',
-- 
2.43.5

v2-PG16-PG14-0001-Fix-invalid-referring-of-hash-ref-for-replic.patchapplication/octet-stream; name=v2-PG16-PG14-0001-Fix-invalid-referring-of-hash-ref-for-replic.patchDownload

From a63d2f84eb1dd98642dc6825adddf9e7915c4f8f Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Thu, 3 Apr 2025 12:12:12 +0900
Subject: [PATCH v2-PG16] Fix invalid referring of hash-ref for replication
 slots

hash-ref gerenated by slot() did not have key 'slot_name', but some codes
referred it. Fix it by referring 'plugin' instead.
---
 src/test/recovery/t/006_logical_decoding.pl           | 8 ++++----
 src/test/recovery/t/010_logical_decoding_timelines.pl | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/test/recovery/t/006_logical_decoding.pl b/src/test/recovery/t/006_logical_decoding.pl
index 5025d65b1b4..96210b8bab7 100644
--- a/src/test/recovery/t/006_logical_decoding.pl
+++ b/src/test/recovery/t/006_logical_decoding.pl
@@ -158,8 +158,8 @@ SKIP:
 	is($node_primary->psql('postgres', 'DROP DATABASE otherdb'),
 		3, 'dropping a DB with active logical slots fails');
 	$pg_recvlogical->kill_kill;
-	is($node_primary->slot('otherdb_slot')->{'slot_name'},
-		undef, 'logical slot still exists');
+	is($node_primary->slot('otherdb_slot')->{'plugin'},
+		'test_decoding', 'logical slot still exists');
 }
 
 $node_primary->poll_query_until('otherdb',
@@ -168,8 +168,8 @@ $node_primary->poll_query_until('otherdb',
 
 is($node_primary->psql('postgres', 'DROP DATABASE otherdb'),
 	0, 'dropping a DB with inactive logical slots succeeds');
-is($node_primary->slot('otherdb_slot')->{'slot_name'},
-	undef, 'logical slot was actually dropped with DB');
+is($node_primary->slot('otherdb_slot')->{'plugin'},
+	'', 'logical slot was actually dropped with DB');
 
 # Test logical slot advancing and its durability.
 my $logical_slot = 'logical_slot';
diff --git a/src/test/recovery/t/010_logical_decoding_timelines.pl b/src/test/recovery/t/010_logical_decoding_timelines.pl
index 6fbbeedde3b..be518e437ae 100644
--- a/src/test/recovery/t/010_logical_decoding_timelines.pl
+++ b/src/test/recovery/t/010_logical_decoding_timelines.pl
@@ -94,8 +94,8 @@ is( $node_replica->safe_psql(
 		'postgres', q[SELECT 1 FROM pg_database WHERE datname = 'dropme']),
 	'',
 	'dropped DB dropme on standby');
-is($node_primary->slot('dropme_slot')->{'slot_name'},
-	undef, 'logical slot was actually dropped on standby');
+is($node_replica->slot('dropme_slot')->{'plugin'},
+	'', 'logical slot was actually dropped on standby');
 
 # Back to testing failover...
 $node_primary->safe_psql('postgres',
-- 
2.43.5

v2-master-PG17-0001-Fix-invalid-referring-of-hash-ref-for-replic.patchapplication/octet-stream; name=v2-master-PG17-0001-Fix-invalid-referring-of-hash-ref-for-replic.patchDownload

From de0fc201214ed02532fc90f110973a63d2f32e4f Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Thu, 3 Apr 2025 12:12:12 +0900
Subject: [PATCH v2] Fix invalid referring of hash-ref for replication slots

hash-ref gerenated by slot() did not have key 'slot_name', but some codes
referred it. Fix it by referring 'plugin' instead.
---
 src/test/recovery/t/006_logical_decoding.pl           | 8 ++++----
 src/test/recovery/t/010_logical_decoding_timelines.pl | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/test/recovery/t/006_logical_decoding.pl b/src/test/recovery/t/006_logical_decoding.pl
index a5678bc4dc4..2137c4e5e30 100644
--- a/src/test/recovery/t/006_logical_decoding.pl
+++ b/src/test/recovery/t/006_logical_decoding.pl
@@ -161,8 +161,8 @@ SKIP:
 	is($node_primary->psql('postgres', 'DROP DATABASE otherdb'),
 		3, 'dropping a DB with active logical slots fails');
 	$pg_recvlogical->kill_kill;
-	is($node_primary->slot('otherdb_slot')->{'slot_name'},
-		undef, 'logical slot still exists');
+	is($node_primary->slot('otherdb_slot')->{'plugin'},
+		'test_decoding', 'logical slot still exists');
 }
 
 $node_primary->poll_query_until('otherdb',
@@ -171,8 +171,8 @@ $node_primary->poll_query_until('otherdb',
 
 is($node_primary->psql('postgres', 'DROP DATABASE otherdb'),
 	0, 'dropping a DB with inactive logical slots succeeds');
-is($node_primary->slot('otherdb_slot')->{'slot_name'},
-	undef, 'logical slot was actually dropped with DB');
+is($node_primary->slot('otherdb_slot')->{'plugin'},
+	'', 'logical slot was actually dropped with DB');
 
 # Test logical slot advancing and its durability.
 # Passing failover=true (last arg) should not have any impact on advancing.
diff --git a/src/test/recovery/t/010_logical_decoding_timelines.pl b/src/test/recovery/t/010_logical_decoding_timelines.pl
index 08615f1fca8..351a9ef7bdd 100644
--- a/src/test/recovery/t/010_logical_decoding_timelines.pl
+++ b/src/test/recovery/t/010_logical_decoding_timelines.pl
@@ -94,8 +94,8 @@ is( $node_replica->safe_psql(
 		'postgres', q[SELECT 1 FROM pg_database WHERE datname = 'dropme']),
 	'',
 	'dropped DB dropme on standby');
-is($node_primary->slot('dropme_slot')->{'slot_name'},
-	undef, 'logical slot was actually dropped on standby');
+is($node_replica->slot('dropme_slot')->{'plugin'},
+	'', 'logical slot was actually dropped on standby');
 
 # Back to testing failover...
 $node_primary->safe_psql('postgres',
-- 
2.43.5

Fujii Masao

masao.fujii@oss.nttdata.com

9 months ago

In reply to: Hayato Kuroda (Fujitsu) (#3)

Re: Some codes refer slot()->{'slot_name'} but it is not defined

On 2025/04/03 21:23, Hayato Kuroda (Fujitsu) wrote:

Dear Fujii-san,
-is($node_primary->slot('dropme_slot')->{'slot_name'},
-	undef, 'logical slot was actually dropped on standby');
+is($node_primary->slot('dropme_slot')->{'plugin'},
+	'', 'logical slot was actually dropped on standby');
This seems like a separate issue from what your patch is addressing,
but since this test is meant to confirm that the slot was dropped
on the standby, shouldn't node_primary be node_replica instead?
You are right. It has been missed 8 years ago, let's fix now.

Thanks for the patch!

BTW, the issue exists for all supported branches. How do you feel
to backpatch them? PSA all patch set.

I think this fix should be backpatched to all supported versions.
Since the issue you found and the one I found seem different,
I'd prefer committing them separately. If that works for you,
here are the commit log messages I'm considering.

--------------------------------------
Fix logical decoding regression tests to correctly check slot existence.

The regression tests for logical decoding verify whether a logical slot
exists or has been dropped. Previously, these tests attempted to
retrieve "slot_name" from the result of slot(), but since "slot_name" was
not included in the result, slot()->{'slot_name'} always returned undef,
leading to incorrect behavior.

This commit fixes the issue by checking the "plugin" field in the result
of slot() instead, ensuring the tests properly verify slot existence.

Back-patch to all supported versions.

Author: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: /messages/by-id/OSCPR01MB149667EC4E738769CA80B7EA5F5AE2@OSCPR01MB14966.jpnprd01.prod.outlook.com
Backpatch-through: 13
--------------------------------------

--------------------------------------
Fix logical decoding test to correctly check slot removal on standby.

The regression test for logical decoding verifies whether a logical slot
is correctly dropped on a standby when its associated database is dropped.
However, the test mistakenly retrieved slot information from the primary
instead of the standby, causing incorrect behavior.

This commit fixes the issue by ensuring the test correctly checks the slot
on the standby.

Back-patch to all supported versions.

Author: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: /messages/by-id/1fdfd020-a509-403c-bd8f-a04664aba148@oss.nttdata.com
Backpatch-through: 13
--------------------------------------

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Hayato Kuroda (Fujitsu)

kuroda.hayato@fujitsu.com

9 months ago

In reply to: Fujii Masao (#4)

RE: Some codes refer slot()->{'slot_name'} but it is not defined

Dear Fujii-san,

I think this fix should be backpatched to all supported versions.
Since the issue you found and the one I found seem different,
I'd prefer committing them separately.

I have no objections.

If that works for you,
here are the commit log messages I'm considering.

LGTM, thanks.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Fujii Masao

masao.fujii@oss.nttdata.com

9 months ago

In reply to: Hayato Kuroda (Fujitsu) (#5)

Re: Some codes refer slot()->{'slot_name'} but it is not defined

On 2025/04/04 12:06, Hayato Kuroda (Fujitsu) wrote:

Dear Fujii-san,

I think this fix should be backpatched to all supported versions.
Since the issue you found and the one I found seem different,
I'd prefer committing them separately.

I have no objections.

I've pushed the patches. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Hayato Kuroda (Fujitsu)

kuroda.hayato@fujitsu.com

9 months ago

In reply to: Fujii Masao (#6)

RE: Some codes refer slot()->{'slot_name'} but it is not defined

Dear Fujii-san,

I've pushed the patches. Thanks!

This is a closing post. I confirmed at least one BF animal for each version
have been run and said OK. IIUC there are no threads to be forked.

Thanks for pushing!

Best regards,
Hayato Kuroda
FUJITSU LIMITED