Date: Mon, 24 Jan 2022 22:31:31 -0800
From: Andres Freund <andres@anarazel.de>
To: pgsql-hackers@postgresql.org
Cc: Masahiko Sawada <sawada.mshk@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>
Subject: Design of pg_stat_subscription_workers vs pgstats
Message-ID: <20220125063131.4cmvsxbz2tdg6g65@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Archived-At: <https://www.postgresql.org/message-id/20220125063131.4cmvsxbz2tdg6g65%40alap3.anarazel.de>
Precedence: bulk

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: https://postgr.es/m/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).

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

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


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?

Greetings,

Andres Freund

[1] https://www.postgresql.org/message-id/CAD21AoDeScrsHhLyEPYqN3sydg6PxAPVBboK%3D30xJfUVihNZDA%40mail.gmail.com
[2] https://postgr.es/m/OSBPR01MB48887CA8F40C8D984A6DC00CED199%40OSBPR01MB4888.jpnprd01.prod.outlook.com