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

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

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

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

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: https://postgr.es/m/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: https://postgr.es/m/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

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 

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

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