Hi,
On Thu, Feb 22, 2024 at 12:02:01PM +0900, Michael Paquier wrote:
> On Wed, Feb 21, 2024 at 11:50:21AM +0000, Bertrand Drouvot wrote:
> > A few comments:
> >
> > 1 ===
> > I think "up" is missing at several places in the patch where "wake" is used.
> > I could be wrong as non native english speaker though.
>
> Patched up three places to be more consisten.
Thanks!
> > 5 ===
> >
> > +PG_FUNCTION_INFO_V1(injection_points_wakeup);
> > +Datum
> > +injection_points_wakeup(PG_FUNCTION_ARGS)
> >
> > Empty line missing before "Datum"?
>
> I've used the same style as anywhere else.
humm, looking at src/test/regress/regress.c for example, I can see an empty
line between PG_FUNCTION_INFO_V1 and Datum for all the "PG_FUNCTION_INFO_V1"
ones.
> > While at it, should we add a second injection wait point in
> > t/041_invalid_checkpoint_after_promote.pl to check that they are wake up
> > individually?
>
> I'm not sure that's a good idea for the sake of this test, TBH, as
> that would provide coverage outside the original scope of the
> restartpoint/promote check.
Yeah, that was my doubt too.
> I have also looked at if it would be possible to get an isolation test
> out of that, but the arbitrary wait does not make that possible with
> the existing isolation APIs, see GetSafeSnapshotBlockingPids() used in
> pg_isolation_test_session_is_blocked(). isolation/README seems to be
> a bit off here, actually, mentioning pg_locks.. We could use some
> tricks with transactions or locks internally, but I'm not really
> tempted to make the wait callback more complicated for the sake of
> more coverage as I'd rather keep it generic for anybody who wants to
> control the order of events across processes.
Makes sense to me, let's keep it as it is.
>
> Attaching a v3 for reference with everything that has been mentioned
> up to now.
Thanks!
Sorry, I missed those ones previously:
1 ===
+/* Maximum number of wait usable in injection points at once */
s/Maximum number of wait/Maximum number of waits/ ?
2 ===
+# Check the logs that the restart point has started on standby. This is
+# optional, but let's be sure.
+my $log = slurp_file($node_standby->logfile, $logstart);
+my $checkpoint_start = 0;
+if ($log =~ m/restartpoint starting: immediate wait/)
+{
+ $checkpoint_start = 1;
+}
+is($checkpoint_start, 1, 'restartpoint has started');
what about?
ok( $node_standby->log_contains( "restartpoint starting: immediate wait", $logstart),
"restartpoint has started");
Except for the above, v3 looks good to me.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
