Hi,
On 4/27/23 11:53 AM, Amit Kapila wrote:
On Thu, Apr 27, 2023 at 2:16 PM Drouvot, Bertrand
<bertranddrouvot...@gmail.com> wrote:
On 4/27/23 10:11 AM, Yu Shi (Fujitsu) wrote:
Hi hackers,
In 035_standby_logical_decoding.pl, I think that the check in the following test
case should be performed on the standby node, instead of the primary node, as
the slot is created on the standby node.
Oh right, the current test is not done on the right node, thanks!
The test currently passes because it
only checks the return value of psql. It might be more appropriate to check the
error message.
Agree, and it's consistent with what is being done in 006_logical_decoding.pl.
Please see the attached patch.
+
+($result, $stdout, $stderr) = $node_standby->psql(
'otherdb',
"SELECT lsn FROM pg_logical_slot_peek_changes('behaves_ok_activeslot',
NULL, NULL) ORDER BY lsn DESC LIMIT 1;"
- ),
- 3,
- 'replaying logical slot from another database fails');
+ );
+ok( $stderr =~
+ m/replication slot "behaves_ok_activeslot" was not created in this
database/,
+ "replaying logical slot from another database fails");
That does look good to me.
I agree that that the check should be done on standby but how does it
make a difference to check the error message or return value? Won't it
be the same for both primary and standby?
Yes that would be the same. I think the original idea from Shi-san (please
correct me If I'm wrong)
was "while at it" let's also make this test on the right node even better.
Nit: I wonder if while at it (as it was already there) we could not remove the "
ORDER BY lsn DESC LIMIT 1" part of it.
It does not change anything regarding the test but looks more appropriate to me.
It may not make a difference as this is anyway an error case but it
looks more logical to LIMIT by 1 as you are fetching a single LSN
value and it will be consistent with other tests in this file and the
test case in the file 006_logical_decoding.pl.
yeah I think it all depends how one see this test (sort of test a failure or
try to get
a result and see if it fails). That was a Nit so I don't have a strong opinion
on this one.
BTW, in the same test, I see it uses wait_for_catchup() in one place
and wait_for_replay_catchup() in another place after Insert. Isn't it
better to use wait_for_replay_catchup in both places?
They are both using the 'replay' mode so both are perfectly correct but for
consistency (and as
they don't use the same "target_lsn" ('write' vs 'flush')) I think that using
wait_for_replay_catchup()
in both places (which is using the 'flush' target lsn) is a good idea.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
