Add 'worker_type' to pg_stat_subscription

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

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

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

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]https://github.com/search?q=repo%3Apostgres%2Fpostgres%20%20worker-%3Etype&amp;type=code. 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&amp;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

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

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

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

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

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

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

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.