Thread: Add 'worker_type' to pg_stat_subscription
Hi hackers, Earlier this year I proposed a small change for the pg_stat_subscription view: ------ ...it would be very useful to have an additional "kind" attribute for this view. This will save the user from needing to do mental gymnastics every time just to recognise what kind of process they are looking at. ------ At that time Amit replied [1] that this could be posted as a separate enhancement thread. Now that the LogicalRepWorkerType has been recently pushed [2] (something with changes in the same area of the code) it seemed the right time to resurrect my pg_stat_subscription proposal. PSA patch v1. Thoughts? ------ [1] https://www.postgresql.org/message-id/CAA4eK1JO54%3D3s0KM9iZGSrQmmfzk9PEOKkW8TXjo2OKaKrSGCA%40mail.gmail.com [2] https://github.com/postgres/postgres/commit/2a8b40e3681921943a2989fd4ec6cdbf8766566c Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Wed, Aug 16, 2023 at 07:14:18PM +1000, Peter Smith wrote:
> Earlier this year I proposed a small change for the pg_stat_subscription view:
>
> ------
> ...it would be very useful to have an additional "kind" attribute for
> this view. This will save the user from needing to do mental
> gymnastics every time just to recognise what kind of process they are
> looking at.
> ------
>
> At that time Amit replied [1] that this could be posted as a separate
> enhancement thread.
>
> Now that the LogicalRepWorkerType has been recently pushed [2]
> (something with changes in the same area of the code) it seemed the
> right time to resurrect my pg_stat_subscription proposal.
This sounds generally reasonable to me.
<row>
<entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>worker_type</structfield> <type>text</type>
+ </para>
+ <para>
+ Type of the subscription worker process. Possible values are:
+ <itemizedlist>
+ <listitem>
+ <para>
+ <literal>a</literal>: apply worker
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ <literal>p</literal>: parallel apply worker
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ <literal>t</literal>: tablesync worker
+ </para>
+ </listitem>
+ </itemizedlist>
+ </para></entry>
+ </row>
Is there any reason not to spell out the names? I think that would match
the other system views better (e.g., backend_type in pg_stat_activity).
Also, instead of "tablesync worker", I'd suggest using "synchronization
worker" to match the name used elsewhere in this table.
I see that the table refers to "leader apply workers". Would those show up
as parallel apply workers in the view? Can we add another worker type for
those?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Sat, Sep 2, 2023 at 7:41 AM Nathan Bossart <nathandbossart@gmail.com> wrote: Thanks for your interest in this patch. > Is there any reason not to spell out the names? I think that would match > the other system views better (e.g., backend_type in pg_stat_activity). I had thought it might be simpler in case someone wanted to query by type. But your suggestion for consistency is probably better, so I changed to do it that way. The help is also simplified to match the other 'backend_type' you cited. > Also, instead of "tablesync worker", I'd suggest using "synchronization > worker" to match the name used elsewhere in this table. > Changed to "table synchronization worker". > I see that the table refers to "leader apply workers". Would those show up > as parallel apply workers in the view? Can we add another worker type for > those? Internally there are only 3 worker types: A "leader" apply worker is basically the same as a regular apply worker, except it has other parallel apply workers associated with it. I felt that pretending there are 4 types in the view would be confusing. Instead, I just removed the word "leader". Now there are: "apply worker" "parallel apply worker" "table synchronization worker" PSA patch v2. ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Wed, Sep 06, 2023 at 09:02:21AM +1200, Peter Smith wrote: > On Sat, Sep 2, 2023 at 7:41 AM Nathan Bossart <nathandbossart@gmail.com> wrote: >> I see that the table refers to "leader apply workers". Would those show up >> as parallel apply workers in the view? Can we add another worker type for >> those? > > Internally there are only 3 worker types: A "leader" apply worker is > basically the same as a regular apply worker, except it has other > parallel apply workers associated with it. > > I felt that pretending there are 4 types in the view would be > confusing. Instead, I just removed the word "leader". Now there are: > "apply worker" > "parallel apply worker" > "table synchronization worker" Okay. Should we omit "worker" for each of the types? Since these are the values for the "worker_type" column, it seems a bit redundant. For example, we don't add "backend" to the end of each value for backend_type in pg_stat_activity. I wonder if we could add the new field to the end of pg_stat_get_subscription() so that we could simplify this patch a bit. At the moment, a big chunk of it is dedicated to reordering the values. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, Sep 6, 2023 at 9:49 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Wed, Sep 06, 2023 at 09:02:21AM +1200, Peter Smith wrote: > > On Sat, Sep 2, 2023 at 7:41 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > >> I see that the table refers to "leader apply workers". Would those show up > >> as parallel apply workers in the view? Can we add another worker type for > >> those? > > > > Internally there are only 3 worker types: A "leader" apply worker is > > basically the same as a regular apply worker, except it has other > > parallel apply workers associated with it. > > > > I felt that pretending there are 4 types in the view would be > > confusing. Instead, I just removed the word "leader". Now there are: > > "apply worker" > > "parallel apply worker" > > "table synchronization worker" > > Okay. Should we omit "worker" for each of the types? Since these are the > values for the "worker_type" column, it seems a bit redundant. For > example, we don't add "backend" to the end of each value for backend_type > in pg_stat_activity. > > I wonder if we could add the new field to the end of > pg_stat_get_subscription() so that we could simplify this patch a bit. At > the moment, a big chunk of it is dedicated to reordering the values. > Modified as suggested. PSA v3. ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Thu, Sep 07, 2023 at 12:36:29PM +1200, Peter Smith wrote: > Modified as suggested. PSA v3. Thanks. I've attached v4 with a couple of small changes. Notably, I've moved the worker_type column to before the pid column, as it felt more natural to me to keep the PID columns together. I've also added an elog(ERROR, ...) for WORKERTYPE_UNKNOWN, as that seems to be the standard practice elsewhere. That being said, are we absolutely confident that this really cannot happen? I haven't looked too closely, but if there is a small race or something that could cause us to see a worker with this type, perhaps it would be better to actually list it as "unknown". Thoughts? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Fri, Sep 8, 2023 at 8:28 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Thu, Sep 07, 2023 at 12:36:29PM +1200, Peter Smith wrote: > > Modified as suggested. PSA v3. > > Thanks. I've attached v4 with a couple of small changes. Notably, I've > moved the worker_type column to before the pid column, as it felt more > natural to me to keep the PID columns together. I've also added an > elog(ERROR, ...) for WORKERTYPE_UNKNOWN, as that seems to be the standard > practice elsewhere. > That being said, are we absolutely confident that this > really cannot happen? I haven't looked too closely, but if there is a > small race or something that could cause us to see a worker with this type, > perhaps it would be better to actually list it as "unknown". Thoughts? The type is only assigned during worker process launch, and during process cleanup [1]. It's only possible to be UNKNOWN after logicalrep_worker_cleanup(). AFAIK the stats can never see a worker with an UNKNOWN type, although it was due to excessive caution against something unforeseen that my original code did below instead of the elog. + case WORKERTYPE_UNKNOWN: /* should not be possible */ + nulls[9] = true; Adding "unknown" for something that is supposed to be impossible might be slight overkill, but so long as there is no obligation to write about "unknown" in the PG DOCS then I agree it is probably better to do that, ------ [1] https://github.com/search?q=repo%3Apostgres%2Fpostgres%20%20worker-%3Etype&type=code Kind Regards, Peter Smith. Fujitsu Australia
On Tue, Sep 12, 2023 at 01:07:51PM +1000, Peter Smith wrote: > The type is only assigned during worker process launch, and during > process cleanup [1]. It's only possible to be UNKNOWN after > logicalrep_worker_cleanup(). > > AFAIK the stats can never see a worker with an UNKNOWN type, although > it was due to excessive caution against something unforeseen that my > original code did below instead of the elog. > > + case WORKERTYPE_UNKNOWN: /* should not be possible */ > + nulls[9] = true; > > Adding "unknown" for something that is supposed to be impossible might > be slight overkill, but so long as there is no obligation to write > about "unknown" in the PG DOCS then I agree it is probably better to > do that, Using an elog() is OK IMO. pg_stat_get_subscription() holds LogicalRepWorkerLock in shared mode, and the only code path setting a worker to WORKERTYPE_UNKNOWN requires that this same LWLock is hold in exclusive mode while resetting all the shmem fields of the subscription entry cleaned up, which is what pg_stat_get_subscription() uses to check if a subscription should be included in its SRF. Shouldn't this patch add or tweak some SQL queries in src/test/subscription/ to show some worker types, at least? -- Michael
Attachment
On Tue, Sep 12, 2023 at 1:44 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Sep 12, 2023 at 01:07:51PM +1000, Peter Smith wrote: > > The type is only assigned during worker process launch, and during > > process cleanup [1]. It's only possible to be UNKNOWN after > > logicalrep_worker_cleanup(). > > > > AFAIK the stats can never see a worker with an UNKNOWN type, although > > it was due to excessive caution against something unforeseen that my > > original code did below instead of the elog. > > > > + case WORKERTYPE_UNKNOWN: /* should not be possible */ > > + nulls[9] = true; > > > > Adding "unknown" for something that is supposed to be impossible might > > be slight overkill, but so long as there is no obligation to write > > about "unknown" in the PG DOCS then I agree it is probably better to > > do that, > > Using an elog() is OK IMO. pg_stat_get_subscription() holds > LogicalRepWorkerLock in shared mode, and the only code path setting a > worker to WORKERTYPE_UNKNOWN requires that this same LWLock is hold in > exclusive mode while resetting all the shmem fields of the > subscription entry cleaned up, which is what > pg_stat_get_subscription() uses to check if a subscription should be > included in its SRF. > > Shouldn't this patch add or tweak some SQL queries in > src/test/subscription/ to show some worker types, at least? Right. I found just a single test currently using pg_stat_subscription catalog. I added a worker_type check for that. PSA v5 ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Tue, Sep 12, 2023 at 07:00:14PM +1000, Peter Smith wrote: > Right. I found just a single test currently using pg_stat_subscription > catalog. I added a worker_type check for that. This looks enough to me, thanks! -- Michael
Attachment
Hi!
I did a look at the patch, like the idea. The overall code is in a good condition, implements the described feature.
Side note: this is not a problem of this particular patch, but in pg_stat_get_subscription and many other places, there
is a switch with worker types. Can we use a default section there to have an explicit error instead of the compiler
warnings if somehow we decide to add another one worker type?
So, should we mark this thread as RfC?
--
Best regards,
Maxim Orlov.
On Wed, Sep 13, 2023 at 05:06:28PM +0300, Maxim Orlov wrote: > I did a look at the patch, like the idea. The overall code is in a good > condition, implements the described feature. Thanks for reviewing. > Side note: this is not a problem of this particular patch, but in > pg_stat_get_subscription and many other places, there > is a switch with worker types. Can we use a default section there to have > an explicit error instead of the compiler > warnings if somehow we decide to add another one worker type? -1. We want such compiler warnings to remind us to adjust the code accordingly. If we just rely on an ERROR in the default section, we might miss it if there isn't a relevant test. > So, should we mark this thread as RfC? I've done so. Barring additional feedback, I intend to commit this in the next few days. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, Sep 13, 2023 at 09:59:04AM -0700, Nathan Bossart wrote: > On Wed, Sep 13, 2023 at 05:06:28PM +0300, Maxim Orlov wrote: >> So, should we mark this thread as RfC? > > I've done so. Barring additional feedback, I intend to commit this in the > next few days. Note to self: this needs a catversion bump. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Wed, Sep 13, 2023 at 09:59:04AM -0700, Nathan Bossart wrote: > On Wed, Sep 13, 2023 at 05:06:28PM +0300, Maxim Orlov wrote: >> So, should we mark this thread as RfC? > > I've done so. Barring additional feedback, I intend to commit this in the > next few days. I did some staging work for the patch (attached). The one code change I made was for the new test. Instead of adding a new test, I figured we could modify the preceding test to check for the expected worker type instead of whether relid is NULL. ISTM this relid check is intended to filter for the apply worker, anyway. The only reason I didn't apply this already is because IMHO we should adjust the worker types and the documentation for the view to be consistent. For example, the docs say "leader apply worker" but the view just calls them "apply" workers. The docs say "synchronization worker" but the view calls them "table synchronization" workers. My first instinct is to call apply workers "leader apply" workers in the view, and to call table synchronization workers "table synchronization workers" in the docs. Thoughts? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Thu, Sep 14, 2023 at 03:04:19PM -0700, Nathan Bossart wrote: > The only reason I didn't apply this already is because IMHO we should > adjust the worker types and the documentation for the view to be > consistent. For example, the docs say "leader apply worker" but the view > just calls them "apply" workers. The docs say "synchronization worker" but > the view calls them "table synchronization" workers. My first instinct is > to call apply workers "leader apply" workers in the view, and to call table > synchronization workers "table synchronization workers" in the docs. Concretely, like this. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Fri, Sep 15, 2023 at 09:35:38AM -0700, Nathan Bossart wrote: > Concretely, like this. There are two references to "synchronization worker" in tablesync.c (exit routine and busy loop), and a bit more of "sync worker".. Anyway, these don't matter much, but there are two errmsgs where the term "tablesync worker" is used. Even if they are internal, these could be made more consistent at least? In config.sgml, max_sync_workers_per_subscription's description uses "synchronization workers". In the second case, adding "table" makes little sense, but could it for the two other sentences? -- Michael
Attachment
On Sat, Sep 16, 2023 at 1:06 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Thu, Sep 14, 2023 at 03:04:19PM -0700, Nathan Bossart wrote: > > The only reason I didn't apply this already is because IMHO we should > > adjust the worker types and the documentation for the view to be > > consistent. For example, the docs say "leader apply worker" but the view > > just calls them "apply" workers. The docs say "synchronization worker" but > > the view calls them "table synchronization" workers. My first instinct is > > to call apply workers "leader apply" workers in the view, and to call table > > synchronization workers "table synchronization workers" in the docs. > > Concretely, like this. > I think there is a merit in keeping the worker type as 'sync' or 'synchronization' because these would be used in future for syncing other objects like sequences. One more thing that slightly looks odd is the 'leader apply' type of worker, won't this be confusing when there is no parallel apply worker in the system? In this regard, probably existing documentation could also be improved. -- With Regards, Amit Kapila.
On Sat, Sep 16, 2023 at 06:09:48PM +0530, Amit Kapila wrote: > I think there is a merit in keeping the worker type as 'sync' or > 'synchronization' because these would be used in future for syncing > other objects like sequences. One more thing that slightly looks odd > is the 'leader apply' type of worker, won't this be confusing when > there is no parallel apply worker in the system? In this regard, > probably existing documentation could also be improved. These are good points. I went ahead and adjusted the patch back to using "apply" for [leader] apply workers and to using "synchronization" for synchronization workers. I also adjusted a couple of the error messages that Michael pointed out to say "synchronization worker" instead of "table synchronization worker" or "tablesync worker". This still leaves the possibility for confusion with the documentation's use of "leader apply worker", but I haven't touched that for now. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
IIUC some future feature syncing of sequences is likely to share a lot of the tablesync worker code (maybe it is only differentiated by the relid being for a RELKIND_SEQUENCE?). The original intent of this stats worker-type patch was to be able to easily know the type of the process without having to dig through other attributes (like relid etc.) to infer it. If you feel differentiating kinds of syncing processes won't be of interest to users then just generically calling it "synchronization" is fine by me. OTOH, if users might care what 'kind' of syncing it is, perhaps leaving the stats attribute as "table synchronization" (and some future patch would add "sequence synchronization") is better. TBH, I am not sure which option is best, so I am happy to go with the consensus. ------ Kind Regards, Peter Smith. Fujitsu Australia
On Sun, Sep 17, 2023 at 2:10 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Sat, Sep 16, 2023 at 06:09:48PM +0530, Amit Kapila wrote: > > This still leaves the possibility for confusion with the documentation's > use of "leader apply worker", but I haven't touched that for now. > We may want to fix that separately but as you have raised here, I found the following two places in docs which could be a bit confusing. "Specifies maximum number of logical replication workers. This includes leader apply workers, parallel apply workers, and table synchronization" ""OID of the relation that the worker is synchronizing; NULL for the leader apply worker and parallel apply workers" One simple idea to reduce confusion could be to use (leader) in the above two places. Do you see any other place which could be confusing and what do you suggest to fix it? -- With Regards, Amit Kapila.
On Mon, Sep 18, 2023 at 6:10 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> IIUC some future feature syncing of sequences is likely to share a lot
> of the tablesync worker code (maybe it is only differentiated by the
> relid being for a RELKIND_SEQUENCE?).
>
> The original intent of this stats worker-type patch was to be able to
> easily know the type of the process without having to dig through
> other attributes (like relid etc.) to infer it.
>
That makes sense and I think it will probably be helpful in debugging.
For example, I am not sure the following and similar changes in the
patch are a good idea:
if (am_tablesync_worker())
ereport(LOG,
- (errmsg("logical replication table synchronization worker for
subscription \"%s\", table \"%s\" has started",
+ (errmsg("logical replication synchronization worker for subscription
\"%s\", table \"%s\" has started",
I think it would be sometimes helpful in debugging to know the type of
sync worker, so keeping the type in the above message would be
helpful.
> If you feel
> differentiating kinds of syncing processes won't be of interest to
> users then just generically calling it "synchronization" is fine by
> me. OTOH, if users might care what 'kind' of syncing it is, perhaps
> leaving the stats attribute as "table synchronization" (and some
> future patch would add "sequence synchronization") is better.
>
Earlier, I thought it would be better to keep it generic but after
seeing your point and the latest changes in the patch it seems
differentiating between types of sync workers would be a good idea.
--
With Regards,
Amit Kapila.
On Mon, Sep 18, 2023 at 1:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Sep 17, 2023 at 2:10 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> >
> > On Sat, Sep 16, 2023 at 06:09:48PM +0530, Amit Kapila wrote:
> >
> > This still leaves the possibility for confusion with the documentation's
> > use of "leader apply worker", but I haven't touched that for now.
> >
>
> We may want to fix that separately but as you have raised here, I
> found the following two places in docs which could be a bit confusing.
>
> "Specifies maximum number of logical replication workers. This
> includes leader apply workers, parallel apply workers, and table
> synchronization"
>
> ""OID of the relation that the worker is synchronizing; NULL for the
> leader apply worker and parallel apply workers"
>
> One simple idea to reduce confusion could be to use (leader) in the
> above two places. Do you see any other place which could be confusing
> and what do you suggest to fix it?
>
IIRC we first encountered this problem with the parallel apply workers
were introduced -- "leader" was added wherever we needed to
distinguish the main apply and the parallel apply worker. Perhaps at
that time, we ought to have changed it *everywhere* instead of
changing only the ambiguous places. Lately, I've been thinking it
would have been easier to have *one* rule and always call the (main)
apply worker the "leader apply" worker -- simply because 2 names
("leader apply" and "parallel apply") are easier to explain than 3
names.
A "leader apply" worker with no "parallel apply" workers is a bit like
the "boss" of a company that has no employees -- IMO it's OK to still
say that they are the "boss".
Regardless, I think changing this in other docs and other code is
outside the scope of this simple pg stats patch -- here we can just
change the relevant config docs and the stats attribute value to
"leader apply" and leave it at that.
Changing every other place to consistently say "leader apply" is a
bigger task for another thread because we will find lots more places
to change. For example, there are messages like: "logical replication
apply worker for subscription \"%s\" has started" that perhaps should
say "logical replication leader apply worker for subscription \"%s\"
has started". Such changes don't belong in this stats patch.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Sep 18, 2023 at 04:56:46PM +1000, Peter Smith wrote:
> On Mon, Sep 18, 2023 at 1:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> One simple idea to reduce confusion could be to use (leader) in the
>> above two places. Do you see any other place which could be confusing
>> and what do you suggest to fix it?
>
> IIRC we first encountered this problem with the parallel apply workers
> were introduced -- "leader" was added wherever we needed to
> distinguish the main apply and the parallel apply worker. Perhaps at
> that time, we ought to have changed it *everywhere* instead of
> changing only the ambiguous places. Lately, I've been thinking it
> would have been easier to have *one* rule and always call the (main)
> apply worker the "leader apply" worker -- simply because 2 names
> ("leader apply" and "parallel apply") are easier to explain than 3
> names.
>
> A "leader apply" worker with no "parallel apply" workers is a bit like
> the "boss" of a company that has no employees -- IMO it's OK to still
> say that they are the "boss".
From the latest discussion, it sounds like you (Peter and Amit) are leaning
more towards something like the v7 patch [0]. I'm okay with that. Perhaps
it'd be worth starting a new thread after this one to make the terminology
consistent in the docs, error messages, views, etc. Fortunately, we have
some time to straighten this out for v17.
> Regardless, I think changing this in other docs and other code is
> outside the scope of this simple pg stats patch -- here we can just
> change the relevant config docs and the stats attribute value to
> "leader apply" and leave it at that.
>
> Changing every other place to consistently say "leader apply" is a
> bigger task for another thread because we will find lots more places
> to change. For example, there are messages like: "logical replication
> apply worker for subscription \"%s\" has started" that perhaps should
> say "logical replication leader apply worker for subscription \"%s\"
> has started". Such changes don't belong in this stats patch.
+1
[0] https://postgr.es/m/attachment/150345/v7-0001-Add-worker-type-to-pg_stat_subscription.patch
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Sep 19, 2023 at 1:20 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Mon, Sep 18, 2023 at 04:56:46PM +1000, Peter Smith wrote:
> > On Mon, Sep 18, 2023 at 1:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> One simple idea to reduce confusion could be to use (leader) in the
> >> above two places. Do you see any other place which could be confusing
> >> and what do you suggest to fix it?
> >
> > IIRC we first encountered this problem with the parallel apply workers
> > were introduced -- "leader" was added wherever we needed to
> > distinguish the main apply and the parallel apply worker. Perhaps at
> > that time, we ought to have changed it *everywhere* instead of
> > changing only the ambiguous places. Lately, I've been thinking it
> > would have been easier to have *one* rule and always call the (main)
> > apply worker the "leader apply" worker -- simply because 2 names
> > ("leader apply" and "parallel apply") are easier to explain than 3
> > names.
> >
> > A "leader apply" worker with no "parallel apply" workers is a bit like
> > the "boss" of a company that has no employees -- IMO it's OK to still
> > say that they are the "boss".
>
> From the latest discussion, it sounds like you (Peter and Amit) are leaning
> more towards something like the v7 patch [0]. I'm okay with that. Perhaps
> it'd be worth starting a new thread after this one to make the terminology
> consistent in the docs, error messages, views, etc. Fortunately, we have
> some time to straighten this out for v17.
>
Yes, the v7 patch looked good to me.
> > Regardless, I think changing this in other docs and other code is
> > outside the scope of this simple pg stats patch -- here we can just
> > change the relevant config docs and the stats attribute value to
> > "leader apply" and leave it at that.
> >
> > Changing every other place to consistently say "leader apply" is a
> > bigger task for another thread because we will find lots more places
> > to change. For example, there are messages like: "logical replication
> > apply worker for subscription \"%s\" has started" that perhaps should
> > say "logical replication leader apply worker for subscription \"%s\"
> > has started". Such changes don't belong in this stats patch.
>
> +1
>
> [0] https://postgr.es/m/attachment/150345/v7-0001-Add-worker-type-to-pg_stat_subscription.patch
>
------
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Sep 18, 2023 at 8:50 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Mon, Sep 18, 2023 at 04:56:46PM +1000, Peter Smith wrote:
> > On Mon, Sep 18, 2023 at 1:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> One simple idea to reduce confusion could be to use (leader) in the
> >> above two places. Do you see any other place which could be confusing
> >> and what do you suggest to fix it?
> >
> > IIRC we first encountered this problem with the parallel apply workers
> > were introduced -- "leader" was added wherever we needed to
> > distinguish the main apply and the parallel apply worker. Perhaps at
> > that time, we ought to have changed it *everywhere* instead of
> > changing only the ambiguous places. Lately, I've been thinking it
> > would have been easier to have *one* rule and always call the (main)
> > apply worker the "leader apply" worker -- simply because 2 names
> > ("leader apply" and "parallel apply") are easier to explain than 3
> > names.
> >
> > A "leader apply" worker with no "parallel apply" workers is a bit like
> > the "boss" of a company that has no employees -- IMO it's OK to still
> > say that they are the "boss".
>
> From the latest discussion, it sounds like you (Peter and Amit) are leaning
> more towards something like the v7 patch [0].
>
I am of the opinion that worker_type should be 'apply' instead of
'leader apply' because even when it is a leader for parallel apply
worker, it could perform individual transactions apply. For reference,
I checked pg_stat_activity.backend_type, there is nothing called main
or leader backend even when the backend is involved in parallel query.
--
With Regards,
Amit Kapila.
On Tue, Sep 19, 2023 at 08:36:35AM +0530, Amit Kapila wrote: > I am of the opinion that worker_type should be 'apply' instead of > 'leader apply' because even when it is a leader for parallel apply > worker, it could perform individual transactions apply. For reference, > I checked pg_stat_activity.backend_type, there is nothing called main > or leader backend even when the backend is involved in parallel query. Okay. Here is v9 of the patch with this change. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Thu, Sep 21, 2023 at 5:30 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Tue, Sep 19, 2023 at 08:36:35AM +0530, Amit Kapila wrote: > > I am of the opinion that worker_type should be 'apply' instead of > > 'leader apply' because even when it is a leader for parallel apply > > worker, it could perform individual transactions apply. For reference, > > I checked pg_stat_activity.backend_type, there is nothing called main > > or leader backend even when the backend is involved in parallel query. > > Okay. Here is v9 of the patch with this change. > One question -- the patch comment still says "Bumps catversion.", but catversion.h change is missing from the v9 patch? ------ Kind Regards, Peter Smith. Fujitsu Australia
On Thu, Sep 21, 2023 at 09:01:01AM +1000, Peter Smith wrote: > One question -- the patch comment still says "Bumps catversion.", but > catversion.h change is missing from the v9 patch? Yeah, previous patches did that, but it is no big deal. My take is that it is a good practice to never do a catversion bump in posted patches, and that it is equally a good practice from Nathan to be reminded about that with the addition of a note in the commit message of the patch posted. -- Michael
Attachment
On Thu, Sep 21, 2023 at 08:14:23AM +0900, Michael Paquier wrote: > On Thu, Sep 21, 2023 at 09:01:01AM +1000, Peter Smith wrote: >> One question -- the patch comment still says "Bumps catversion.", but >> catversion.h change is missing from the v9 patch? > > Yeah, previous patches did that, but it is no big deal. My take is > that it is a good practice to never do a catversion bump in posted > patches, and that it is equally a good practice from Nathan to be > reminded about that with the addition of a note in the commit message > of the patch posted. Right, I'll take care of it before committing. I'm trying to make sure I don't forget. :) -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Thu, Sep 21, 2023 at 9:34 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Thu, Sep 21, 2023 at 08:14:23AM +0900, Michael Paquier wrote: > > On Thu, Sep 21, 2023 at 09:01:01AM +1000, Peter Smith wrote: > >> One question -- the patch comment still says "Bumps catversion.", but > >> catversion.h change is missing from the v9 patch? > > > > Yeah, previous patches did that, but it is no big deal. My take is > > that it is a good practice to never do a catversion bump in posted > > patches, and that it is equally a good practice from Nathan to be > > reminded about that with the addition of a note in the commit message > > of the patch posted. > > Right, I'll take care of it before committing. I'm trying to make sure I > don't forget. :) OK, all good. ~~~ This is a bit of a late entry, but looking at the PG DOCS, I felt it might be simpler if we don't always refer to every other worker type when explaining NULLs. The descriptions are already bigger than they need to be, and if more types ever get added they will keep growing. ~ BEFORE leader_pid integer Process ID of the leader apply worker if this process is a parallel apply worker; NULL if this process is a leader apply worker or a table synchronization worker SUGGESTION leader_pid integer Process ID of the leader apply worker; NULL if this process is not a parallel apply worker ~ BEFORE relid oid OID of the relation that the worker is synchronizing; NULL for the leader apply worker and parallel apply workers SUGGESTION relid oid OID of the relation being synchronized; NULL if this process is not a table synchronization worker ------ Kind Regards, Peter Smith. Fujitsu Australia
On Thu, Sep 21, 2023 at 5:36 AM Peter Smith <smithpb2250@gmail.com> wrote: > > On Thu, Sep 21, 2023 at 9:34 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > > > On Thu, Sep 21, 2023 at 08:14:23AM +0900, Michael Paquier wrote: > > > On Thu, Sep 21, 2023 at 09:01:01AM +1000, Peter Smith wrote: > > >> One question -- the patch comment still says "Bumps catversion.", but > > >> catversion.h change is missing from the v9 patch? > > > > > > Yeah, previous patches did that, but it is no big deal. My take is > > > that it is a good practice to never do a catversion bump in posted > > > patches, and that it is equally a good practice from Nathan to be > > > reminded about that with the addition of a note in the commit message > > > of the patch posted. > > > > Right, I'll take care of it before committing. I'm trying to make sure I > > don't forget. :) > > OK, all good. > > ~~~ > > This is a bit of a late entry, but looking at the PG DOCS, I felt it > might be simpler if we don't always refer to every other worker type > when explaining NULLs. The descriptions are already bigger than they > need to be, and if more types ever get added they will keep growing. > > ~ > > BEFORE > leader_pid integer > Process ID of the leader apply worker if this process is a parallel > apply worker; NULL if this process is a leader apply worker or a table > synchronization worker > > SUGGESTION > leader_pid integer > Process ID of the leader apply worker; NULL if this process is not a > parallel apply worker > > ~ > > BEFORE > relid oid > OID of the relation that the worker is synchronizing; NULL for the > leader apply worker and parallel apply workers > > SUGGESTION > relid oid > OID of the relation being synchronized; NULL if this process is not a > table synchronization worker > I find the current descriptions better than the proposed. But I am not opposed to your proposal if others are okay with it. Personally, I feel even if we want to change these descriptions, we can do it as a separate patch. -- With Regards, Amit Kapila.
On Thu, Sep 21, 2023 at 1:00 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Tue, Sep 19, 2023 at 08:36:35AM +0530, Amit Kapila wrote: > > I am of the opinion that worker_type should be 'apply' instead of > > 'leader apply' because even when it is a leader for parallel apply > > worker, it could perform individual transactions apply. For reference, > > I checked pg_stat_activity.backend_type, there is nothing called main > > or leader backend even when the backend is involved in parallel query. > > Okay. Here is v9 of the patch with this change. > The changes looks good to me, though I haven't tested it. But feel free to commit if you are fine with this patch. -- With Regards, Amit Kapila.
On Thu, Sep 21, 2023 at 04:01:20PM +0530, Amit Kapila wrote: > The changes looks good to me, though I haven't tested it. But feel > free to commit if you are fine with this patch. Committed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Tue, Sep 26, 2023 at 7:16 AM Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Thu, Sep 21, 2023 at 04:01:20PM +0530, Amit Kapila wrote: > > The changes looks good to me, though I haven't tested it. But feel > > free to commit if you are fine with this patch. > > Committed. > Thanks for pushing. ------ Kind Regards, Peter Smith. Fujitsu Australia