MIME-Version: 1.0
References: <E1tsZjz-002Ctd-1G@gemulon.postgresql.org>
 <CA+Tgmoa+GkGNZ86QmqyuOCHWaBkOEJAEPfghEQ1d8_+KudVknw@mail.gmail.com>
In-Reply-To: 
 <CA+Tgmoa+GkGNZ86QmqyuOCHWaBkOEJAEPfghEQ1d8_+KudVknw@mail.gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 18 Mar 2025 12:00:04 +0530
Message-ID: 
 <CAA4eK1JBuaC9R7r22Da+56L-Gi3Xm3TTbutkJwe6i0847fhx8Q@mail.gmail.com>
Subject: Re: pgsql: Avoid invalidating all RelationSyncCache entries on
 publication
To: Robert Haas <robertmhaas@gmail.com>
Cc: Amit Kapila <akapila@postgresql.org>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1JBuaC9R7r22Da%2B56L-Gi3Xm3TTbutkJwe6i0847fhx8Q%40mail.gmail.com>
Precedence: bulk

On Mon, Mar 17, 2025 at 9:35=E2=80=AFPM Robert Haas <robertmhaas@gmail.com>=
 wrote:
>
> On Thu, Mar 13, 2025 at 12:00=E2=80=AFAM Amit Kapila <akapila@postgresql.=
org> wrote:
> > Avoid invalidating all RelationSyncCache entries on publication rename.
> >
> > On Publication rename, we need to only invalidate the RelationSyncCache
> > entries corresponding to relations that are part of the publication bei=
ng
> > renamed.
> >
> > As part of this patch, we introduce a new invalidation message to
> > invalidate the cache maintained by the logical decoding output plugin. =
We
> > can't use existing relcache invalidation for this purpose, as that woul=
d
> > unnecessarily cause relcache invalidations in other backends.
>
> This seems like too much infrastructure for a niche optimization. If
> there are plans to use this new invalidation message type to optimize
> a bunch of other cases, then maybe it's worth it, but adding this just
> to cover the presumably-rare case of ALTER PUBLICATION .. RENAME
> doesn't seem worth it to me.
>

This commit will be helpful for many other existing commands like:
CREATE PUBLICATION ...
ALTER PUBLICATION name SET (publication_parameter)
ALTER PUBLICATION name OWNER ...
DROP PUBLICATION ...

Before this, commit any publication command that modified
pg_publication catalog use to invalidate the entire RelationSyncCache.
The code that led to entire RelationSyncCache invalidation was removed
in this commit:
publication_invalidation_cb(Datum arg, int cacheid, uint32 hashvalue)
 {
    publications_valid =3D false;
-
-   /*
-    * Also invalidate per-relation cache so that next time the filtering i=
nfo
-    * is checked it will be updated with the new publication settings.
-    */
-   rel_sync_cache_publication_cb(arg, cacheid, hashvalue);
 }

We can't remove this without having a solution for ALTER PUBLICATION
.. RENAME command to invalidate the specific RelationSyncCache
entries. As mentioned in the commit message, the other idea was to
register a relcache invalidation for all relations that are part of a
publication which we felt could be harmful to other backends that are
not even involved in decoding. Now, the ALTER PUBLICATION name OWNER
command doesn't need to use this new invalidation at this stage
because it doesn't impact the RelationSyncCache entries but it
benefits from not requiring to invalidate the entire cache. Similarly,
the other commands mentioned above uses relcache invalidation for this
purpose but the difference is that the other commands do need relcache
invalidation for the purpose of correctness whereas ALTER PUBLICATION
.. RENAME command doesn't need it.

In the future, we can use this invalidation in existing publication
commands like altering a publication where we only publish 'INSERT'
(and or 'TRUNCATE') and change other publication properties like
'publish_via_partition_root', 'publish_generated_columns'. Similarly,
I think we can even use new invalidation for SET/ADD/DROP variants of
Alter PUBLICATION where the publication publishes only 'INSERT' and or
'TRUNCATE'. Before this commit, we didn't have a better way so we used
relcache invalidations for these cases as well. Also, any new
publication property or command should prefer to use this new
invalidation instead of using a relcache invalidation unless required
the same for correctness.

The impact of not doing any solution for this problem (aka removing
the above code in publication_invalidation_cb) could be magnified in
future commits where we are trying to solve a long-standing data loss
issue in logical decoding/replication via distributing invalidations
to all in-progress transactions [1]. You can read the commit message
to understand that problem and solution.

I think I should have mentioned the future use and other improvements
of this work in the commit message.

[1] - https://www.postgresql.org/message-id/OSCPR01MB14966DDB92FA7DA8FA8658=
216F5DE2%40OSCPR01MB14966.jpnprd01.prod.outlook.com

--=20
With Regards,
Amit Kapila.