Design of pg_stat_subscription_workers vs pgstats

Hi,

On Tue, Jan 25, 2022 at 3:31 PM Andres Freund <andres@anarazel.de> wrote:

Hi,

I was looking the shared memory stats patch again. The rebase of which
collided fairly heavily with the addition of pg_stat_subscription_workers.

I'm concerned about the design of pg_stat_subscription_workers. The view was
introduced in

commit 8d74fc96db5fd547e077bf9bf4c3b67f821d71cd
Author: Amit Kapila <akapila@postgresql.org>
Date: 2021-11-30 08:54:30 +0530

Add a view to show the stats of subscription workers.

This commit adds a new system view pg_stat_subscription_workers, that
shows information about any errors which occur during the application of
logical replication changes as well as during performing initial table
synchronization. The subscription statistics entries are removed when the
corresponding subscription is removed.

It also adds an SQL function pg_stat_reset_subscription_worker() to reset
single subscription errors.

The contents of this view can be used by an upcoming patch that skips the
particular transaction that conflicts with the existing data on the
subscriber.

This view can be extended in the future to track other xact related
statistics like the number of xacts committed/aborted for subscription
workers.

Author: Masahiko Sawada
Reviewed-by: Greg Nancarrow, Hou Zhijie, Tang Haiying, Vignesh C, Dilip Kumar, Takamichi Osumi, Amit Kapila
Discussion: /messages/by-id/CAD21AoDeScrsHhLyEPYqN3sydg6PxAPVBboK=30xJfUVihNZDA@mail.gmail.com

I tried to skim-read the discussion leading to its introduction, but it's
extraordinarily long: 474 messages in [1], 131 messages in [2], as well as a
few other associated threads.

From the commit message alone I am concerned that this appears to be intended
to be used to store important state in pgstats. For which pgstats is
fundamentally unsuitable (pgstat can loose state during normal operation,
always looses state during crash restarts, the state can be reset).

The information on pg_stat_subscription_workers view, especially
last_error_xid, can be used to specify XID to "ALTER SUBSCRIPTION ...
SKIP (xid = XXX)" command which is proposed on the same thread, but it
doesn't mean that the new SKIP command relies on this information. The
failure XID is written in the server logs as well and the user
specifies XID manually.

I don't really understand the name "pg_stat_subscription_workers" - what
workers are stats kept about exactly? The columns don't obviously refer to a
single worker or such? From the contents it should be name
pg_stat_subscription_table_stats or such. But no, that'd not quite right,
because apply errors are stored per-susbcription, while initial sync stuff is
per-(subscription, table).

This stores stats for subscription workers namely apply and tablesync
worker, so named as pg_stat_subscription_workers.

Also, there is another proposal to add transaction statistics for
logical replication subscribers[1]/messages/by-id/OSBPR01MB48887CA8F40C8D984A6DC00CED199@OSBPR01MB4888.jpnprd01.prod.outlook.com, and it's reasonable to merge these
statistics and this error information rather than having separate
views[2]/messages/by-id/CAD21AoDF7LmSALzMfmPshRw_xFcRz3WvB-me8T2gO6Ht=3zL2w@mail.gmail.com. There also was an idea to add the transaction statistics to
pg_stat_subscription view, but it doesn't seem a good idea because the
pg_stat_subscription shows dynamic statistics whereas the transaction
statistics are accumulative statistics[3]/messages/by-id/CAA4eK1JqwpsvjhLxV8CMYQ3NrhimZ8AFhWHh0Qn1FrL=LXfY6Q@mail.gmail.com.

The pgstat entries are quite wide (292 bytes), because of the error message
stored. That's nearly twice the size of PgStat_StatTabEntry. And as far as I
can tell, once there was an error, we'll just keep the stats entry around
until the subscription is dropped.

We can drop the particular statistics by
pg_stat_reset_subscription_worker() function.

And that includes stats for long dropped
tables, as far as I can see - except that they're hidden from view, due to a
join to pg_subscription_rel.

We are planning to drop this after successfully apply[4]/messages/by-id/CAA4eK1+9yXkWkJSNtWYV2rG7QNAnoAt+eNH0PexoSP9ZQmXKPg@mail.gmail.com.

To me this looks like it's using pgstat as an extremely poor IPC mechanism.

Why isn't this just storing data in pg_subscription_rel?

These need to be updated on error which means for a failed xact and we
don't want to update the system catalog in that state. There will be
some challenges in a case where updating pg_subscription_rel also
failed too (what to report to the user, etc.). And moreover, we don't
want to consume space for temporary information in the system catalog.

Regards,

[1]: /messages/by-id/OSBPR01MB48887CA8F40C8D984A6DC00CED199@OSBPR01MB4888.jpnprd01.prod.outlook.com
[2]: /messages/by-id/CAD21AoDF7LmSALzMfmPshRw_xFcRz3WvB-me8T2gO6Ht=3zL2w@mail.gmail.com
[3]: /messages/by-id/CAA4eK1JqwpsvjhLxV8CMYQ3NrhimZ8AFhWHh0Qn1FrL=LXfY6Q@mail.gmail.com
[4]: /messages/by-id/CAA4eK1+9yXkWkJSNtWYV2rG7QNAnoAt+eNH0PexoSP9ZQmXKPg@mail.gmail.com

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

Hi,

I didn't quite get to responding in depth, but I wanted to at least respond to
one point today.

On 2022-01-25 20:27:07 +0900, Masahiko Sawada wrote:

The pgstat entries are quite wide (292 bytes), because of the error message
stored. That's nearly twice the size of PgStat_StatTabEntry. And as far as I
can tell, once there was an error, we'll just keep the stats entry around
until the subscription is dropped.

We can drop the particular statistics by
pg_stat_reset_subscription_worker() function.

Only if either the user wants to drop all stats, or somehow knows the oids of
already dropped tables...

Why isn't this just storing data in pg_subscription_rel?

These need to be updated on error which means for a failed xact and we
don't want to update the system catalog in that state.

Rightly so! In fact, I'm concerned with sending a pgstats message in that
state as well. But: You don't need to. Just abort the current transaction,
start a new one, and update the state.

There will be some challenges in a case where updating pg_subscription_rel
also failed too (what to report to the user, etc.). And moreover, we don't
want to consume space for temporary information in the system catalog.

You're consuming resources in a *WAY* worse way right now. The stats file gets
constantly written out, and quite often read back by backends. In contrast to
parts of pg_subscription_rel or such that data can't be removed from
shared_buffers under pressure.

Greetings,

Andres Freund

On Thu, Jan 27, 2022 at 11:16 AM Andres Freund <andres@anarazel.de> wrote:

On 2022-01-25 20:27:07 +0900, Masahiko Sawada wrote:

There will be some challenges in a case where updating pg_subscription_rel
also failed too (what to report to the user, etc.). And moreover, we don't
want to consume space for temporary information in the system catalog.

You're consuming resources in a *WAY* worse way right now. The stats file gets
constantly written out, and quite often read back by backends. In contrast to
parts of pg_subscription_rel or such that data can't be removed from
shared_buffers under pressure.

I don't think pg_subscription_rel is the right place to store error
info as the error can happen say while processing some message type
like BEGIN where we can't map it to pg_subscription_rel entry. There
could be other cases as well where we won't be able to map it to
pg_subscription_rel like some error related to some other table while
processing trigger function.

In general, there doesn't appear to be much advantage in storing all
the error info in system catalogs as we don't want it to be persistent
(crash-safe). Also, this information is not about any system object
that future operations can use, so won't map from that angle as well.

But, I see the point related to the size overhead of each message (296
bytes) and that is because of the error message present in each entry.
I think it would be better to store error_code instead of the message.

--
With Regards,
Amit Kapila.

On Thu, Jan 27, 2022 at 5:08 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jan 27, 2022 at 11:16 AM Andres Freund <andres@anarazel.de> wrote:

On 2022-01-25 20:27:07 +0900, Masahiko Sawada wrote:

There will be some challenges in a case where updating

pg_subscription_rel

also failed too (what to report to the user, etc.). And moreover, we

don't

want to consume space for temporary information in the system catalog.

You're consuming resources in a *WAY* worse way right now. The stats

file gets

constantly written out, and quite often read back by backends. In

contrast to

parts of pg_subscription_rel or such that data can't be removed from
shared_buffers under pressure.

I don't think pg_subscription_rel is the right place to store error
info as the error can happen say while processing some message type
like BEGIN where we can't map it to pg_subscription_rel entry. There
could be other cases as well where we won't be able to map it to
pg_subscription_rel like some error related to some other table while
processing trigger function.

In general, there doesn't appear to be much advantage in storing all
the error info in system catalogs as we don't want it to be persistent
(crash-safe). Also, this information is not about any system object
that future operations can use, so won't map from that angle as well.

Repeating myself here to try and keep complaints regarding
pg_stat_subscription_worker in one place.

This is my specific email with respect to the pg_stat_scription_workers
design.

/messages/by-id/CAKFQuwZbFuPSV1WLiNFuODst1sUZon2Qwbj8d9tT=38hMhJfvw@mail.gmail.com

Specifically,

pg_stat_subscription_workers is defined as showing:
"will contain one row per subscription
worker on which errors have occurred, for workers applying logical
replication changes and workers handling the initial data copy of the
subscribed tables."

The fact that these errors remain (last_error_*) even after they are no
longer relevant is my main gripe regarding this feature. The information
itself is generally useful though last_error_count is not. These fields
should auto-clear and be named current_error_* as they exist for purposes
of describing the current state of any error-encountering logical
replication worker so that ALTER SUBSCRIPTION SKIP, or someone other manual
intervention, can be done with that knowledge without having to scan the
subscriber's server logs.

This is my email trying to understand reality better in order to figure out
what exactly is causing the limitations that are negatively impacting the
design of this feature.

/messages/by-id/CAKFQuwYJ7dsW+Stsw5+ZVoY3nwQ9j6pPt-7oYjGddH-h7uVb+g@mail.gmail.com

In short, it was convenient to use the statistics collector here even if
doing so resulted in a non-user friendly (IMO) design. Given all of the
limitations to the statistics collection infrastructure, and the fact that
this data is not statistical in the usual usage of the term, I find that to
be less than satisfying. To the point that I'd be inclined to revert this
feature and hold up the ALTER SUBSCRIPTION SET patch until a more
user-friendly design can be done using proper IPC techniques. (I also noted
in the first email that pg_stat_archiver, solely by observing the column
names it exposes, shares this same abuse of the statistics collector for
non-statistical data).

In my second email I did some tracing and ended up at the PG_CATCH() block
in src/backend/replication/logical/worker.c:L3629. When mentioning trying
to get rid of the PG_RE_THROW() there apparently doing so completely is
unwarranted due to fatal/panic errors. I am curious that the addition of
the statistic reporting logic doesn't seem to care about the same. And in
any case, while maybe PG_RE_THROW() cannot go away it could maybe be done
conditionally, and the worker still allowed to exit should that be more
desirable than making the routine safe for looping after an error.

Andres, I do not know how to be more precise than your comment "But: You
don't need to. Just abort the current transaction, start a new one, and
update the state.". When I suggested that idea it didn't seem to resonate
with anyone on the other thread. Starting at the main PG_TRY() loop in
worker.c noted above, could you maybe please explain in a bit more detail
whether, and how hard, it would be to go from "just PG_RE_THROW();" to
"abort and start a new transaction"?

David J.

Hi,

On 2022-01-27 13:18:51 -0700, David G. Johnston wrote:

Repeating myself here to try and keep complaints regarding
pg_stat_subscription_worker in one place.

Thanks!

This is my specific email with respect to the pg_stat_scription_workers
design.

/messages/by-id/CAKFQuwZbFuPSV1WLiNFuODst1sUZon2Qwbj8d9tT=38hMhJfvw@mail.gmail.com

Specifically,

pg_stat_subscription_workers is defined as showing:
"will contain one row per subscription
worker on which errors have occurred, for workers applying logical
replication changes and workers handling the initial data copy of the
subscribed tables."

The fact that these errors remain (last_error_*) even after they are no
longer relevant is my main gripe regarding this feature. The information
itself is generally useful though last_error_count is not. These fields
should auto-clear and be named current_error_* as they exist for purposes
of describing the current state of any error-encountering logical
replication worker so that ALTER SUBSCRIPTION SKIP, or someone other manual
intervention, can be done with that knowledge without having to scan the
subscriber's server logs.

Indeed.

Another related thing is that using a 32bit xid for allowing skipping is a bad
idea anyway - we shouldn't adding new interfaces with xid wraparound dangers -
it's getting more and more common to have multiple wraparounds a day. An
easily better alternative would be the LSN at which a transaction starts.

This is my email trying to understand reality better in order to figure out
what exactly is causing the limitations that are negatively impacting the
design of this feature.

/messages/by-id/CAKFQuwYJ7dsW+Stsw5+ZVoY3nwQ9j6pPt-7oYjGddH-h7uVb+g@mail.gmail.com

In short, it was convenient to use the statistics collector here even if
doing so resulted in a non-user friendly (IMO) design.

And importantly, the whole justification for the scheme, namely the inability
to change actual tables in that state, just doesn't hold up. It's a few lines
to abort the failed transaction and log the error after.

Just retrying over and over at full pace doesn't seem like a good thing. We
should start to back off retries - the retries themselves can very well
contribute to making it harder to fix the problem, by holding locks etc. For
that the launcher (or workers) should check whether there's no worker because
it's errored out. With pgstats such a check would need this full sequence:

1) worker sends failure stats message
2) pgstats receive stats message
3) launcher sends ping to pgstats to request file to be written out
4) pgstats writes out the whole database's stats
5) launcher reads the whole stats file

That's a big and expensive cannon for a check whether we should delay the
launcher of a worker.

Given all of the
limitations to the statistics collection infrastructure, and the fact that
this data is not statistical in the usual usage of the term, I find that to
be less than satisfying. To the point that I'd be inclined to revert this
feature and hold up the ALTER SUBSCRIPTION SET patch until a more
user-friendly design can be done using proper IPC techniques.

Same.

In my second email I did some tracing and ended up at the PG_CATCH() block
in src/backend/replication/logical/worker.c:L3629. When mentioning trying
to get rid of the PG_RE_THROW() there apparently doing so completely is
unwarranted due to fatal/panic errors. I am curious that the addition of
the statistic reporting logic doesn't seem to care about the same.

We shouldn't even think about doing stuff like stats updates when we've
PANICed. You could argue its safe to do that in the FATAL case - but where
would such a FATAL validly come from? It'd be something like a user calling
pg_terminate_backend(), which isn't transaction specific, so it'd not make
sense to record details like xid in pg_stat_subscription_workers.

But the argument of needing to do something in PG_CATCH in the fatal/panic
case is bogus, because FATAL/PANIC doesn't reach PG_CATCH. errfinish() only
throws when elevel == ERROR, in the FATAL case we end with proc_exit(1), with
PANIC we abort().

Andres, I do not know how to be more precise than your comment "But: You
don't need to. Just abort the current transaction, start a new one, and
update the state.". When I suggested that idea it didn't seem to resonate
with anyone on the other thread. Starting at the main PG_TRY() loop in
worker.c noted above, could you maybe please explain in a bit more detail
whether, and how hard, it would be to go from "just PG_RE_THROW();" to
"abort and start a new transaction"?

It's pretty easy from the POV of getting into a new transaction.

PG_CATCH():

/* get us out of the failed transaction */
AbortOutOfAnyTransaction();

StartTransactionCommand();
/* do something to remember the error we just got */
CommitTransactionCommand();

It may be a bit harder to afterwards to to not just error out the whole
worker, because we'd need to know what to do instead.

Greetings,

Andres Freund

On Thu, Jan 27, 2022 at 2:15 PM Andres Freund <andres@anarazel.de> wrote:

Another related thing is that using a 32bit xid for allowing skipping is a
bad
idea anyway - we shouldn't adding new interfaces with xid wraparound
dangers -
it's getting more and more common to have multiple wraparounds a day. An
easily better alternative would be the LSN at which a transaction starts.

Interesting idea. I do not think a well-designed skipping feature need
worry about wrap-around though. The XID to be skipped was just seen be a
worker and because it failed it will continue to be the same XID
encountered by that worker until it is resolved. There is no effective
progression in time while the subscriber is stuck for wrap-around to
happen. Since we want to skip the transaction as a whole adding a layer of
hidden indirection to the process seems undesirable. I'm not against the
idea though - to the user it is basically "copy this value from the error
message in order to skip the transaction that caused the error". Then the
system verifies the value and then ensures it skips one, and only one,
transaction.

It's pretty easy from the POV of getting into a new transaction.

PG_CATCH():

/* get us out of the failed transaction */
AbortOutOfAnyTransaction();

StartTransactionCommand();
/* do something to remember the error we just got */
CommitTransactionCommand();

Thank you.

It may be a bit harder to afterwards to to not just error out the whole
worker, because we'd need to know what to do instead.

I imagine the launcher and worker startup code can be made to deal with the
restart adequately. Just wait if the last thing seen was an error.
Require the user to manually resume the worker - unless we really think
a try-until-you-succeed with a backoff protocol is superior. Upon system
restart all error information is cleared and we start from scratch and let
the errors happen (or not depending) as they will.

David J.

On Fri, Jan 28, 2022 at 1:49 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Thu, Jan 27, 2022 at 5:08 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jan 27, 2022 at 11:16 AM Andres Freund <andres@anarazel.de> wrote:

On 2022-01-25 20:27:07 +0900, Masahiko Sawada wrote:

There will be some challenges in a case where updating pg_subscription_rel
also failed too (what to report to the user, etc.). And moreover, we don't
want to consume space for temporary information in the system catalog.

You're consuming resources in a *WAY* worse way right now. The stats file gets
constantly written out, and quite often read back by backends. In contrast to
parts of pg_subscription_rel or such that data can't be removed from
shared_buffers under pressure.

I don't think pg_subscription_rel is the right place to store error
info as the error can happen say while processing some message type
like BEGIN where we can't map it to pg_subscription_rel entry. There
could be other cases as well where we won't be able to map it to
pg_subscription_rel like some error related to some other table while
processing trigger function.

In general, there doesn't appear to be much advantage in storing all
the error info in system catalogs as we don't want it to be persistent
(crash-safe). Also, this information is not about any system object
that future operations can use, so won't map from that angle as well.

Repeating myself here to try and keep complaints regarding pg_stat_subscription_worker in one place.

This is my specific email with respect to the pg_stat_scription_workers design.

/messages/by-id/CAKFQuwZbFuPSV1WLiNFuODst1sUZon2Qwbj8d9tT=38hMhJfvw@mail.gmail.com

Specifically,

pg_stat_subscription_workers is defined as showing:
"will contain one row per subscription
worker on which errors have occurred, for workers applying logical
replication changes and workers handling the initial data copy of the
subscribed tables."

The fact that these errors remain (last_error_*) even after they are no longer relevant is my main gripe regarding this feature. The information itself is generally useful though last_error_count is not. These fields should auto-clear and be named current_error_* as they exist for purposes of describing the current state of any error-encountering logical replication worker so that ALTER SUBSCRIPTION SKIP, or someone other manual intervention, can be done with that knowledge without having to scan the subscriber's server logs.

We can discuss names of columns but the main reason was that tomorrow
say we want to account for total errors not only the current error
then we have to introduce the field error_count or something like that
which will then conflict with names like current_*. Similar for
transaction abort_count. In the initial versions of the patch, we were
not using last_* for column names but similar arguments led us to
change names to last_ terminology and the same was being used in
pg_stat_archiver. But, feel free to suggest better names. Yes, I agree
with an auto-clear point as well and there seems to be an agreement
for doing it after the next successful apply and or after we skipped
the failed xact.

This is my email trying to understand reality better in order to figure out what exactly is causing the limitations that are negatively impacting the design of this feature.

/messages/by-id/CAKFQuwYJ7dsW+Stsw5+ZVoY3nwQ9j6pPt-7oYjGddH-h7uVb+g@mail.gmail.com

In short, it was convenient to use the statistics collector here even if doing so resulted in a non-user friendly (IMO) design. Given all of the limitations to the statistics collection infrastructure, and the fact that this data is not statistical in the usual usage of the term, I find that to be less than satisfying.

I think the failures/conflicts are also important information for
users to know, so having a view of those doesn't appear to be a bad
idea. All this data is less suitable for system catalogs like
pg_subscription_rel or others for the reasons quoted in my previous
email [1]/messages/by-id/CAA4eK1+MDngbOQfMcAMsrf__s2a-MMMHaCR0zwde3GVeEi-bbQ@mail.gmail.com. You have already noted one view pg_stat_archiver and we do
have failure information like checksum_failures, deadlocks in some
other views. Then, we have some information like conflicts available
via pg_stat_database_conflicts. I think the error/conflict info about
apply failures is on similar lines.

To the point that I'd be inclined to revert this feature and hold up the ALTER SUBSCRIPTION SET patch until a more user-friendly design can be done using proper IPC techniques.

If we find a different/better way and that is a conclusion then I will
do it. But, in my humble opinion, let's first discuss and see why this
is incorrect? IIUC, your main argument was to allow auto skip instead
of allowing users to fetch XID (from a view or server logs) and then
use it in the command. We are already trying to brainstorm that and
Sawada-San has already proposed a couple of ideas [2]/messages/by-id/CAD21AoBdEcyXKMCMws7HjcYDbbPyq_KfUbCnTX84rDeP45Hbrw@mail.gmail.com. Also, the other
idea Andres has shared is to use LSN (of the corresponding failed
transaction) instead of XID which I find better.

[1]: /messages/by-id/CAA4eK1+MDngbOQfMcAMsrf__s2a-MMMHaCR0zwde3GVeEi-bbQ@mail.gmail.com
[2]: /messages/by-id/CAD21AoBdEcyXKMCMws7HjcYDbbPyq_KfUbCnTX84rDeP45Hbrw@mail.gmail.com

--
With Regards,
Amit Kapila.

On Fri, Jan 28, 2022 at 2:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Jan 28, 2022 at 1:49 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Thu, Jan 27, 2022 at 5:08 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jan 27, 2022 at 11:16 AM Andres Freund <andres@anarazel.de> wrote:

On 2022-01-25 20:27:07 +0900, Masahiko Sawada wrote:

There will be some challenges in a case where updating pg_subscription_rel
also failed too (what to report to the user, etc.). And moreover, we don't
want to consume space for temporary information in the system catalog.

You're consuming resources in a *WAY* worse way right now. The stats file gets
constantly written out, and quite often read back by backends. In contrast to
parts of pg_subscription_rel or such that data can't be removed from
shared_buffers under pressure.

I don't think pg_subscription_rel is the right place to store error
info as the error can happen say while processing some message type
like BEGIN where we can't map it to pg_subscription_rel entry. There
could be other cases as well where we won't be able to map it to
pg_subscription_rel like some error related to some other table while
processing trigger function.

In general, there doesn't appear to be much advantage in storing all
the error info in system catalogs as we don't want it to be persistent
(crash-safe). Also, this information is not about any system object
that future operations can use, so won't map from that angle as well.

Repeating myself here to try and keep complaints regarding pg_stat_subscription_worker in one place.

This is my specific email with respect to the pg_stat_scription_workers design.

/messages/by-id/CAKFQuwZbFuPSV1WLiNFuODst1sUZon2Qwbj8d9tT=38hMhJfvw@mail.gmail.com

Specifically,

pg_stat_subscription_workers is defined as showing:
"will contain one row per subscription
worker on which errors have occurred, for workers applying logical
replication changes and workers handling the initial data copy of the
subscribed tables."

The fact that these errors remain (last_error_*) even after they are no longer relevant is my main gripe regarding this feature. The information itself is generally useful though last_error_count is not. These fields should auto-clear and be named current_error_* as they exist for purposes of describing the current state of any error-encountering logical replication worker so that ALTER SUBSCRIPTION SKIP, or someone other manual intervention, can be done with that knowledge without having to scan the subscriber's server logs.

We can discuss names of columns but the main reason was that tomorrow
say we want to account for total errors not only the current error
then we have to introduce the field error_count or something like that
which will then conflict with names like current_*. Similar for
transaction abort_count. In the initial versions of the patch, we were
not using last_* for column names but similar arguments led us to
change names to last_ terminology and the same was being used in
pg_stat_archiver. But, feel free to suggest better names. Yes, I agree
with an auto-clear point as well and there seems to be an agreement
for doing it after the next successful apply and or after we skipped
the failed xact.

This is my email trying to understand reality better in order to figure out what exactly is causing the limitations that are negatively impacting the design of this feature.

/messages/by-id/CAKFQuwYJ7dsW+Stsw5+ZVoY3nwQ9j6pPt-7oYjGddH-h7uVb+g@mail.gmail.com

In short, it was convenient to use the statistics collector here even if doing so resulted in a non-user friendly (IMO) design. Given all of the limitations to the statistics collection infrastructure, and the fact that this data is not statistical in the usual usage of the term, I find that to be less than satisfying.

I think the failures/conflicts are also important information for
users to know, so having a view of those doesn't appear to be a bad
idea. All this data is less suitable for system catalogs like
pg_subscription_rel or others for the reasons quoted in my previous
email [1].

I see that it's better to use a better IPC for ALTER SUBSCRIPTION SKIP
feature to pass error-XID or error-LSN information to the worker
whereas I'm also not sure of the advantages in storing all error
information in a system catalog. Since what we need to do for this
purpose is only error-XID/LSN, we can store only error-XID/LSN in the
catalog? That is, the worker stores error-XID/LSN in the catalog on an
error, and ALTER SUBSCRIPTION SKIP command enables the worker to skip
the transaction in question. The worker clears the error-XID/LSN after
successfully applying or skipping the first non-empty transaction.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Feb 1, 2022 at 11:47 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Fri, Jan 28, 2022 at 2:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Jan 28, 2022 at 1:49 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

In short, it was convenient to use the statistics collector here even if doing so resulted in a non-user friendly (IMO) design. Given all of the limitations to the statistics collection infrastructure, and the fact that this data is not statistical in the usual usage of the term, I find that to be less than satisfying.

I think the failures/conflicts are also important information for
users to know, so having a view of those doesn't appear to be a bad
idea. All this data is less suitable for system catalogs like
pg_subscription_rel or others for the reasons quoted in my previous
email [1].

I see that it's better to use a better IPC for ALTER SUBSCRIPTION SKIP
feature to pass error-XID or error-LSN information to the worker
whereas I'm also not sure of the advantages in storing all error
information in a system catalog. Since what we need to do for this
purpose is only error-XID/LSN, we can store only error-XID/LSN in the
catalog? That is, the worker stores error-XID/LSN in the catalog on an
error, and ALTER SUBSCRIPTION SKIP command enables the worker to skip
the transaction in question. The worker clears the error-XID/LSN after
successfully applying or skipping the first non-empty transaction.

Where do you propose to store this information? I think we can't use
pg_subscription_rel for reasons quoted by me in email [1]/messages/by-id/CAA4eK1+MDngbOQfMcAMsrf__s2a-MMMHaCR0zwde3GVeEi-bbQ@mail.gmail.com. We can
store it in pg_subscription but that won't cover tablesync cases. I
think it can work if we store at both places. I think that would be
extendable if one wants to bring parallelism on the apply-side as we
can think of storing the values in the array. The other possibility
could be to invent a new catalog for this info but I guess it will
then have to have some duplicate info from pg_subscription/_rel.

The other point is after this, do we want an interface where the user
can also be allowed to specify error_lsn or error_xid? I think it
would be better to have such flexibility as that can be extended later
to allow users to skip some specific operations like 'update',
'insert', etc., or other similar things.

[1]: /messages/by-id/CAA4eK1+MDngbOQfMcAMsrf__s2a-MMMHaCR0zwde3GVeEi-bbQ@mail.gmail.com

--
With Regards,
Amit Kapila.

On Tue, Feb 1, 2022 at 8:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Feb 1, 2022 at 11:47 AM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:

I see that it's better to use a better IPC for ALTER SUBSCRIPTION SKIP
feature to pass error-XID or error-LSN information to the worker
whereas I'm also not sure of the advantages in storing all error
information in a system catalog. Since what we need to do for this
purpose is only error-XID/LSN, we can store only error-XID/LSN in the
catalog? That is, the worker stores error-XID/LSN in the catalog on an
error, and ALTER SUBSCRIPTION SKIP command enables the worker to skip
the transaction in question. The worker clears the error-XID/LSN after
successfully applying or skipping the first non-empty transaction.

Where do you propose to store this information?

pg_subscription_worker

The error message and context is very important. Just make sure it is only
non-null when the worker state is "syncing failed" (or whatever term we
use).

Records are removed upon server restart (the launcher can handle this).
Consider recording a last activity timestamp (some protection/visibility
against bugs or, say, a worker ending without reporting that fact).
Records stay around even when the worker goes away (the user can filter the
state field to omit inactive rows). I'd consider just removing them when
done and/or having a reset function that the DBA could run (it should never
be wrong to clear the table).

Re: XID and/or LSN, I don't know enough yet to really judge this...

The other possibility

could be to invent a new catalog for this info but I guess it will
then have to have some duplicate info from pg_subscription/_rel.

The other point is after this, do we want an interface where the user
can also be allowed to specify error_lsn or error_xid?

...but whatever is decided, tell me, the user, what my options are, the
limitations, and what info to copy from this catalog into the command(s)
that I issue to the server, that will make the errors go away. This is
generic, not specific to the skipping a commit command or the skip-to-lsn
functions, but also includes considering performing DML on the relevant
table(s) to avoid the error.

I don't think the fields would be duplicated. While some of the fields
seem similar, aside from the key fields the data we would show would be
state info for a given worker. None of the v14 fields do this at the
worker scope.

That all makes the new catalog a generally useful monitoring source and a
standalone patch. I'd personally start a new thread, with a functioning
patch as the first message, and a recap of what and why this rework is
being done. In order for Andres to make progress on the shared memory
statistics patch I would suggest reverting this and building the new patch
as if this statistics collector approach never happened.

I'd still like to get some clarity regarding the observation that our
error-die-restart process seems problematic. Since that process needs to
talk to the new catalog anyway I'd rather commit the changes to the process
(if any, but I hope we can either all agree on the status quo or get
something better in for v15), and the new catalog that provides insight
into that process, as part of this first commit. That includes a probable
user function to restart a halted worker instead of doing so continually
(even with the suggested back-off protocol).

Then the SKIP commit can go in, leveraging the state information exposed in
the catalog. That discussion and work should be restarted on a new thread
with an intro recap message. The existing patch should be adapted to
leverage the new pg_subscription_worker catalog before starting the new
thread.

David J.

On Wed, Feb 2, 2022 at 9:41 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Tue, Feb 1, 2022 at 8:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Feb 1, 2022 at 11:47 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I see that it's better to use a better IPC for ALTER SUBSCRIPTION SKIP
feature to pass error-XID or error-LSN information to the worker
whereas I'm also not sure of the advantages in storing all error
information in a system catalog. Since what we need to do for this
purpose is only error-XID/LSN, we can store only error-XID/LSN in the
catalog? That is, the worker stores error-XID/LSN in the catalog on an
error, and ALTER SUBSCRIPTION SKIP command enables the worker to skip
the transaction in question. The worker clears the error-XID/LSN after
successfully applying or skipping the first non-empty transaction.

Where do you propose to store this information?

pg_subscription_worker

The error message and context is very important. Just make sure it is only non-null when the worker state is "syncing failed" (or whatever term we use).

Sure, but is this the reason you want to store all the error info in
the system catalog? I agree that providing more error info could be
useful and also possibly the previously failed (apply) xacts info as
well but I am not able to see why you want to have that sort of info
in the catalog. I could see storing info like err_lsn/err_xid that can
allow to proceed to apply worker automatically or to slow down the
launch of errored apply worker but not all sort of other error info
(like err_cnt, err_code, err_message, err_time, etc.). I want to know
why you are insisting to make all the error info persistent via the
system catalog?

--
With Regards,
Amit Kapila.

On Tue, Feb 1, 2022 at 11:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Feb 2, 2022 at 9:41 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Tue, Feb 1, 2022 at 8:07 PM Amit Kapila <amit.kapila16@gmail.com>

wrote:

On Tue, Feb 1, 2022 at 11:47 AM Masahiko Sawada <sawada.mshk@gmail.com>

wrote:

I see that it's better to use a better IPC for ALTER SUBSCRIPTION SKIP
feature to pass error-XID or error-LSN information to the worker
whereas I'm also not sure of the advantages in storing all error
information in a system catalog. Since what we need to do for this
purpose is only error-XID/LSN, we can store only error-XID/LSN in the
catalog? That is, the worker stores error-XID/LSN in the catalog on an
error, and ALTER SUBSCRIPTION SKIP command enables the worker to skip
the transaction in question. The worker clears the error-XID/LSN after
successfully applying or skipping the first non-empty transaction.

Where do you propose to store this information?

pg_subscription_worker

The error message and context is very important. Just make sure it is

only non-null when the worker state is "syncing failed" (or whatever term
we use).

Sure, but is this the reason you want to store all the error info in
the system catalog? I agree that providing more error info could be
useful and also possibly the previously failed (apply) xacts info as
well but I am not able to see why you want to have that sort of info
in the catalog. I could see storing info like err_lsn/err_xid that can
allow to proceed to apply worker automatically or to slow down the
launch of errored apply worker but not all sort of other error info
(like err_cnt, err_code, err_message, err_time, etc.). I want to know
why you are insisting to make all the error info persistent via the
system catalog?

I look at the catalog and am informed that the worker has stopped because
of an error. I'd rather simply read the error message right then instead
of having to go look at the log file. And if I am going to take an action
in order to overcome the error I would have to know what that error is; so
the error message is not something I can ignore. The error is an attribute
of system state, and the catalog stores the current state of the (workers)
system.

I already explained that the concept of err_cnt is not useful. The fact
that you include it here makes me think you are still thinking that this
all somehow is meant to keep track of history. It is not. The workers are
state machines and "error" is one of the states - with relevant attributes
to display to the user, and system, while in that state. The state machine
reporting does not care about historical states nor does it report on
them. There is some uncertainty if we continue with the automatic
re-launch; which, now that I write this, I can see where what you call
err_cnt is effectively a count of how many times the worker re-launched
without the underlying problem being resolved and thus encountered the same
error. If we persist with the re-launch behavior then maybe err_cnt should
be left in place - with the description for it basically being the ah-ha!
comment I just made. In a world where we do not typically re-launch and
simply re-try without being informed there is a change - such a count
remains of minimal value.

I don't really understand the confusion here though - this error data
already exists in the pg_stat_subscription_workers stat collector view -
the fact that I want to keep it around (just changing the reset behavior) -
doesn't seem like it should be controversial. I, thinking as a user,
really don't care about all of these implementation details. Whether it is
a pg_stat_* view (collector or shmem IPC) or a pg_* catalog is immaterial
to me. The behavior I observe is what matters. As a developer I don't
want to use the statistics collector because these are not statistics and
the collector is unreliable. I don't know enough about the relevant
differences between shared memory IPC and catalog tables to decide between
them. But catalog tables seem like a lower bar to meet and seem like they
can implement the user-facing requirements as I envision them.

David J.

On Wed, Feb 2, 2022 at 1:06 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Tue, Feb 1, 2022 at 11:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Feb 2, 2022 at 9:41 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Tue, Feb 1, 2022 at 8:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Feb 1, 2022 at 11:47 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I see that it's better to use a better IPC for ALTER SUBSCRIPTION SKIP
feature to pass error-XID or error-LSN information to the worker
whereas I'm also not sure of the advantages in storing all error
information in a system catalog. Since what we need to do for this
purpose is only error-XID/LSN, we can store only error-XID/LSN in the
catalog? That is, the worker stores error-XID/LSN in the catalog on an
error, and ALTER SUBSCRIPTION SKIP command enables the worker to skip
the transaction in question. The worker clears the error-XID/LSN after
successfully applying or skipping the first non-empty transaction.

Where do you propose to store this information?

pg_subscription_worker

The error message and context is very important. Just make sure it is only non-null when the worker state is "syncing failed" (or whatever term we use).

Sure, but is this the reason you want to store all the error info in
the system catalog? I agree that providing more error info could be
useful and also possibly the previously failed (apply) xacts info as
well but I am not able to see why you want to have that sort of info
in the catalog. I could see storing info like err_lsn/err_xid that can
allow to proceed to apply worker automatically or to slow down the
launch of errored apply worker but not all sort of other error info
(like err_cnt, err_code, err_message, err_time, etc.). I want to know
why you are insisting to make all the error info persistent via the
system catalog?

...
...

I already explained that the concept of err_cnt is not useful. The fact that you include it here makes me think you are still thinking that this all somehow is meant to keep track of history. It is not. The workers are state machines and "error" is one of the states - with relevant attributes to display to the user, and system, while in that state. The state machine reporting does not care about historical states nor does it report on them. There is some uncertainty if we continue with the automatic re-launch;

I think automatic retry will help to allow some transient errors say
like network glitches that can be resolved on retry and will keep the
behavior transparent. This is also consistent with what we do in
standby mode where if there is an error on primary due to which
standby is not able to fetch some data it will just retry. We can't
fix any error that occurred on the server-side, so the way is to retry
which is true for both standby and subscribers. Basically, I don't
think every kind of error demands user intervention. We can allow to
control it via some parameter say disable_on_error as is discussed in
CF entry [1]https://commitfest.postgresql.org/36/3407/ but don't think that should be the default.

[1]: https://commitfest.postgresql.org/36/3407/

--
With Regards,
Amit Kapila.

On Wed, Feb 2, 2022 at 5:08 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Feb 2, 2022 at 1:06 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

...

I already explained that the concept of err_cnt is not useful. The fact

that you include it here makes me think you are still thinking that this
all somehow is meant to keep track of history. It is not. The workers are
state machines and "error" is one of the states - with relevant attributes
to display to the user, and system, while in that state. The state machine
reporting does not care about historical states nor does it report on
them. There is some uncertainty if we continue with the automatic
re-launch;

I think automatic retry will help to allow some transient errors say
like network glitches that can be resolved on retry and will keep the
behavior transparent. This is also consistent with what we do in
standby mode where if there is an error on primary due to which
standby is not able to fetch some data it will just retry. We can't
fix any error that occurred on the server-side, so the way is to retry
which is true for both standby and subscribers.

Good points. In short there are two subsets of problems to deal with
here. We should address them separately, though the pg_subscription_worker
table should provide relevant information for both cases. If we are in a
retry situation relevant information, like next_scheduled_retry
(estimated), should be provided (if there is some kind of delay involved).
In a situation like "unique constraint violation" the
"next_scheduled_retry" would be null; or make the field a text field and
print "Manual Intervention Required". Likewise, the XID/LSN would be null
in a retry situation since we haven't received a wholly intact transaction
from the publisher (we may know of such an ID but if the final COMMIT
message is never even seen before the feed dies we should not be exposing
that incomplete information to the user).

A standby is not expected to encounter any user data constraint problems so
even a system with manual intervention for such will work for standbys
because they will never hit that code path. And you cannot simply skip
applying the failed transaction and move onto the next one - that data also
never came over.

David J.

On Wed, Feb 2, 2022 at 4:36 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Tue, Feb 1, 2022 at 11:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Feb 2, 2022 at 9:41 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Tue, Feb 1, 2022 at 8:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Feb 1, 2022 at 11:47 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I see that it's better to use a better IPC for ALTER SUBSCRIPTION SKIP
feature to pass error-XID or error-LSN information to the worker
whereas I'm also not sure of the advantages in storing all error
information in a system catalog. Since what we need to do for this
purpose is only error-XID/LSN, we can store only error-XID/LSN in the
catalog? That is, the worker stores error-XID/LSN in the catalog on an
error, and ALTER SUBSCRIPTION SKIP command enables the worker to skip
the transaction in question. The worker clears the error-XID/LSN after
successfully applying or skipping the first non-empty transaction.

Where do you propose to store this information?

pg_subscription_worker

The error message and context is very important. Just make sure it is only non-null when the worker state is "syncing failed" (or whatever term we use).

Sure, but is this the reason you want to store all the error info in
the system catalog? I agree that providing more error info could be
useful and also possibly the previously failed (apply) xacts info as
well but I am not able to see why you want to have that sort of info
in the catalog. I could see storing info like err_lsn/err_xid that can
allow to proceed to apply worker automatically or to slow down the
launch of errored apply worker but not all sort of other error info
(like err_cnt, err_code, err_message, err_time, etc.). I want to know
why you are insisting to make all the error info persistent via the
system catalog?

I look at the catalog and am informed that the worker has stopped because of an error. I'd rather simply read the error message right then instead of having to go look at the log file. And if I am going to take an action in order to overcome the error I would have to know what that error is; so the error message is not something I can ignore. The error is an attribute of system state, and the catalog stores the current state of the (workers) system.

I already explained that the concept of err_cnt is not useful. The fact that you include it here makes me think you are still thinking that this all somehow is meant to keep track of history. It is not. The workers are state machines and "error" is one of the states - with relevant attributes to display to the user, and system, while in that state. The state machine reporting does not care about historical states nor does it report on them. There is some uncertainty if we continue with the automatic re-launch; which, now that I write this, I can see where what you call err_cnt is effectively a count of how many times the worker re-launched without the underlying problem being resolved and thus encountered the same error. If we persist with the re-launch behavior then maybe err_cnt should be left in place - with the description for it basically being the ah-ha! comment I just made. In a world where we do not typically re-launch and simply re-try without being informed there is a change - such a count remains of minimal value.

I don't really understand the confusion here though - this error data already exists in the pg_stat_subscription_workers stat collector view - the fact that I want to keep it around (just changing the reset behavior) - doesn't seem like it should be controversial. I, thinking as a user, really don't care about all of these implementation details. Whether it is a pg_stat_* view (collector or shmem IPC) or a pg_* catalog is immaterial to me. The behavior I observe is what matters. As a developer I don't want to use the statistics collector because these are not statistics and the collector is unreliable. I don't know enough about the relevant differences between shared memory IPC and catalog tables to decide between them. But catalog tables seem like a lower bar to meet and seem like they can implement the user-facing requirements as I envision them.

I see that important information such as error-XID that can be used
for ALTER SUBSCRIPTION SKIP needs to be stored in a reliable way, and
using system catalogs is a reasonable way for this purpose. But it's
still unclear to me why all error information that is currently shown
in pg_stat_subscription_workers view, including error-XID and the
error message, relation OID, action, etc., need to be stored in the
catalog. The information other than error-XID doesn't necessarily need
to be reliable compared to error-XID. I think we can have
error-XID/LSN in the pg_subscription catalog and have other error
information in pg_stat_subscription_workers view. After the user
checks the current status of logical replication by checking
error-XID/LSN, they can check pg_stat_subscription_workers for
details.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wednesday, February 2, 2022, Masahiko Sawada <sawada.mshk@gmail.com>
wrote:

and have other error
information in pg_stat_subscription_workers view.

What benefit is there to keeping the existing collector-based
pg_stat_subscripiton_workers view? If we re-write it using shmem IPC then
we might as well put everything there and forego using a catalog. Then it
behaves in a similar manner to pg_stat_activity but for logical replication
workers.

David J.

On Thu, Feb 3, 2022 at 1:48 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Wednesday, February 2, 2022, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

and have other error
information in pg_stat_subscription_workers view.

What benefit is there to keeping the existing collector-based pg_stat_subscripiton_workers view? If we re-write it using shmem IPC then we might as well put everything there and forego using a catalog. Then it behaves in a similar manner to pg_stat_activity but for logical replication workers.

Yes, but if we use shmem IPC, we need to allocate shared memory for
them based on the number of subscriptions, not logical replication
workers (i.e., max_logical_replication_workers). So we cannot estimate
memory in the beginning. Also, IIUC the number of subscriptions that
are concurrently working is limited by max_replication_slots (see
ReplicationStateCtl) but I think we need to remember the state of
disabled subscriptions too.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On 02.02.22 07:54, Amit Kapila wrote:

Where do you propose to store this information?

pg_subscription_worker

The error message and context is very important. Just make sure it is only non-null when the worker state is "syncing failed" (or whatever term we use).

We could name the table something like pg_subscription_worker_error, so
it's explicit that it is collecting error information only.

Sure, but is this the reason you want to store all the error info in
the system catalog? I agree that providing more error info could be
useful and also possibly the previously failed (apply) xacts info as
well but I am not able to see why you want to have that sort of info
in the catalog. I could see storing info like err_lsn/err_xid that can
allow to proceed to apply worker automatically or to slow down the
launch of errored apply worker but not all sort of other error info
(like err_cnt, err_code, err_message, err_time, etc.). I want to know
why you are insisting to make all the error info persistent via the
system catalog?

Let's flip this around and ask, why not? Tables are the place to store
data, by default.

On Thu, Feb 3, 2022 at 3:25 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 02.02.22 07:54, Amit Kapila wrote:

Sure, but is this the reason you want to store all the error info in
the system catalog? I agree that providing more error info could be
useful and also possibly the previously failed (apply) xacts info as
well but I am not able to see why you want to have that sort of info
in the catalog. I could see storing info like err_lsn/err_xid that can
allow to proceed to apply worker automatically or to slow down the
launch of errored apply worker but not all sort of other error info
(like err_cnt, err_code, err_message, err_time, etc.). I want to know
why you are insisting to make all the error info persistent via the
system catalog?

Let's flip this around and ask, why not?

Because we don't necessarily need all this information after the crash
and neither is this information about any system object which we
require for performing operations on objects. OTOH, if we need some
particular error/apply state(s) that should be okay to keep in
persistent form (in system catalog). In walreceiver (for standby), we
don't store the errors/conflicts in any table, they are either
reported in logs or shared via stats. Similarly, the archiver process
do share its failure information either via stats or logs. Similar is
the case for checkpointer which also just logs the error. Now,
similarly in this case also we are sharing the error information via
logs and stats.

--
With Regards,
Amit Kapila.