public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: suppressing useless wakeups in logical/worker.c
Date: Tue, 24 Jan 2023 18:45:08 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <20230109174217.GA1112698@nathanxps13>
References: <20221202195503.GD2277157@nathanxps13>
<TYAPR01MB58669549D0137CC76A9BB6A0F5189@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<20221205173523.GA2714833@nathanxps13>
<20230109174217.GA1112698@nathanxps13>
Nathan Bossart <[email protected]> writes:
> [ v2-0001-suppress-unnecessary-wakeups-in-logical-worker.c.patch ]
I took a look through this, and have a number of mostly-cosmetic
issues:
* It seems wrong that next_sync_start isn't handled as one of the
wakeup[NUM_LRW_WAKEUPS] entries. I see that it needs to be accessed from
another module; but you could handle that without exposing either enum
LogRepWorkerWakeupReason or the array, by providing getter/setter
functions for process_syncing_tables_for_apply() to call.
* This code is far too intimately familiar with the fact that TimestampTz
is an int64 count of microseconds. (I'm picky about that because I
remember that they were once something else, so I wonder if someday
they will be different again.) You could get rid of the PG_INT64_MAX
usages by replacing those with the timestamp infinity macro DT_NOEND;
and I'd even be on board with adding a less-opaque alternate name for
that to datatype/timestamp.h. The various magic-constant multipliers
could perhaps be made less magic by using TimestampTzPlusMilliseconds().
* I think it might be better to construct the enum like this:
+typedef enum LogRepWorkerWakeupReason
+{
+ LRW_WAKEUP_TERMINATE,
+ LRW_WAKEUP_PING,
+ LRW_WAKEUP_STATUS
+#define NUM_LRW_WAKEUPS (LRW_WAKEUP_STATUS + 1)
+} LogRepWorkerWakeupReason;
so that you don't have to have a default: case in switches on the
enum value. I'm more worried about somebody adding an enum value
and missing updating a switch statement elsewhere than I am about
somebody adding an enum value and neglecting to update the
immediately-adjacent macro.
* The updates of "now" in LogicalRepApplyLoop seem rather
randomly placed, and I'm not entirely convinced that we'll
always be using a reasonably up-to-date value. Can't we
just update it right before each usage?
* This special handling of next_sync_start seems mighty ugly:
+ /* Also consider special wakeup time for starting sync workers. */
+ if (next_sync_start < now)
+ {
+ /*
+ * Instead of spinning while we wait for the sync worker to
+ * start, wait a bit before retrying (unless there's an earlier
+ * wakeup time).
+ */
+ nextWakeup = Min(now + INT64CONST(100000), nextWakeup);
+ }
+ else
+ nextWakeup = Min(next_sync_start, nextWakeup);
Do we really need the slop? If so, is there a reason why it
shouldn't apply to all possible sources of nextWakeup? (It's
going to be hard to fold next_sync_start into the wakeup[]
array unless you can make this not a special case.)
* It'd probably be worth enlarging the comment for
LogRepWorkerComputeNextWakeup to explain why its API is like that,
perhaps "We ask the caller to pass in the value of "now" because
this frequently avoids multiple calls of GetCurrentTimestamp().
It had better be a reasonably-up-to-date value, though."
regards, tom lane
view thread (2+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: suppressing useless wakeups in logical/worker.c
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox