From: | "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, 'shveta malik' <shveta(dot)malik(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Peter Smith <smithpb2250(at)gmail(dot)com>, "Drouvot, Bertrand" <bertranddrouvot(dot)pg(at)gmail(dot)com>, Nisha Moond <nisha(dot)moond412(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Bruce Momjian <bruce(at)momjian(dot)us>, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Ajin Cherian <itsajin(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
Subject: | RE: Synchronizing slots from primary to standby |
Date: | 2023-12-20 12:43:55 |
Message-ID: | OS0PR01MB571651C8E70423CDE8A5DDBD9496A@OS0PR01MB5716.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tuesday, December 19, 2023 9:05 PM Kuroda, Hayato/黒田 隼人 <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Shveta,
>
> I resumed to review the patch. I will play more about it, but I can post some
> cosmetic comments.
Thanks for the comments.
>
> ====
> walsender.c
>
> 01. WalSndWaitForStandbyConfirmation
>
> ```
> + sleeptime = WalSndComputeSleeptime(GetCurrentTimestamp());
> ```
>
> It works well, but I'm not sure whether we should use
> WalSndComputeSleeptime()
> because the function won't be called by walsender.
Changed to a hard-coded value.
>
> 02.WalSndWaitForStandbyConfirmation
>
> ```
> + ConditionVariableTimedSleep(&WalSndCtl->wal_confirm_rcv_cv,
> sleeptime,
> +
> WAIT_EVENT_WAL_SENDER_WAIT_FOR_STANDBY_CONFIRMATION)
> ```
>
> Hmm, is it OK to use the same event as WalSndWaitForWal()? IIUC it should be
> avoided.
As discussed, I change the event name to a more common one,
so that it makes sense to use it in both places.
>
> 03. WalSndShmemInit()
>
> ```
> +
> + ConditionVariableInit(&WalSndCtl->wal_confirm_rcv_cv);
> ```
>
> Unnecessary blank?
Removed.
>
> ~~~~~
> 050_standby_failover_slots_sync.pl
>
> 04. General
>
> My pgperltidy modified your test. Please check.
Will run this in next version.
>
> 05.
>
> ```
> # Create publication on the primary
> ```
>
> Missing "a" before publication?
Changed.
>
> 06.
>
> ```
> $subscriber1->init(allows_streaming => 'logical');
> ...
> $subscriber2->init(allows_streaming => 'logical');
> ```
>
> IIUC, these settings are not needed.
Yeah, removed.
>
> 07.
>
> ```
> my $primary_insert_time = time();
> ```
>
> The variable is not used.
Removed.
>
> 08.
>
> ```
> # Stop the standby associated with the specified physical replication slot so
> # that the logical replication slot won't receive changes until the standby
> # slot's restart_lsn is advanced or the slot is removed from the
> # standby_slot_names list
> ```
>
> Missing comma?
Added.
>
> 09.
>
> ```
> $back_q->query_until(qr//,
> "SELECT pg_logical_slot_get_changes('test_slot', NULL, NULL);\n");
> ```
>
> Not sure, should we have to close the back_q connection?
Added the quit.
>
> 10.
>
> ```
> # Remove the standby from the standby_slot_names list and reload the
> # configuration
> $primary->adjust_conf('postgresql.conf', 'standby_slot_names', "''");
> $primary->psql('postgres', "SELECT pg_reload_conf()");
> ```
> a.
> Missing comma?
>
> b.
> I counted and reload function in perl (e.g., `$primary->reload;`) is more often
> to
> be used. Do you have a reason to use pg_reload_conf()?
I think it was copied from other places, changed to ->reload.
>
> 11.
>
> ```
> # Now that the standby lsn has advanced, the primary must send the decoded
> # changes to the subscription.
> $publisher->wait_for_catchup('regress_mysub1');
> ```
>
> Is the comment correct? I think primary sends data because the GUC is
> modified.
Fixed.
>
> 12.
>
> ```
> # Put the standby back on the primary_slot_name for the rest of the tests
> $primary->adjust_conf('postgresql.conf', 'standby_slot_names', 'sb1_slot');
> $primary->restart();
> ```
>
> Just to confirm - you used restart() here because we must ensure the GUC
> change is
> propagated to all backends, right?
Yes, but I think restart is not necessary, so I changed it to reload.
>
> ~~~~~
> wait_event_names.txt
>
> 13.
>
> ```
> +WAL_SENDER_WAIT_FOR_STANDBY_CONFIRMATION "Waiting for the
> WAL to be received by physical standby in WAL sender process."
> ```
>
> But there is a possibility that backend processes may wait with the event, right?
Adjusted.
Best Regards,
Hou zj
From | Date | Subject | |
---|---|---|---|
Next Message | Daniel Verite | 2023-12-20 12:49:20 | Re: Built-in CTYPE provider |
Previous Message | Zhijie Hou (Fujitsu) | 2023-12-20 12:42:28 | RE: Synchronizing slots from primary to standby |