Subject: Re: Design of pg_stat_subscription_workers vs pgstats

From: Andres Freund <[email-masked]>
Date: Thu, 27 Jan 2022 13:15:18 -0800

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.
>
> https://www.postgresql.org/message-id/CAKFQuwZbFuPSV1WLiNFuODst1sUZon2Qwbj8d9tT%3D38hMhJfvw%40mail.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.
>
> https://www.postgresql.org/message-id/CAKFQuwYJ7dsW%2BStsw5%2BZVoY3nwQ9j6pPt-7oYjGddH-h7uVb%2Bg%40mail.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

---

From: "David G. Johnston" <[email-masked]>
Date: Thu, 27 Jan 2022 15:35:57 -0700

> 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.

>     CommitTransactionCommand();

Thank you.

> 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.