From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: Ibrar Ahmed <ibrar.ahmad@gmail.com>,
 Amit Khandekar <amitdkhan.pg@gmail.com>, pgsql-hackers@lists.postgresql.org
Cc: Andres Freund <andres@anarazel.de>, fabriziomello@gmail.com,
 Alvaro Herrera <alvherre@2ndquadrant.com>,
 tushar <tushar.ahuja@enterprisedb.com>,
 "[pgdg] Robert Haas" <robertmhaas@gmail.com>,
 Rahila Syed <rahila.syed@2ndquadrant.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>, "Drouvot,
 Bertrand" <bdrouvot@amazon.com>
Subject: Re: Minimal logical decoding on standbys
Date: Mon, 02 Aug 2021 13:57:22 +0200
Message-ID: <1655255.DiLJNMXF8A@aivenronan>
Organization: aiven
In-Reply-To: <fa90206a-6bf1-2121-cf0c-15b0734a2731@amazon.com>
References: 
 <CAFcNs+q9FvSW7gMqM__aeWOoDNqfcQCoB8LiO-X33fvqGO8mkQ@mail.gmail.com>
 <CALtqXTfXsyq-G7x3Ti3bm4KgpepqZA_-ZukzJ5W5vFLjRrhGSg@mail.gmail.com>
 <fa90206a-6bf1-2121-cf0c-15b0734a2731@amazon.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Archived-At: 
 <https://www.postgresql.org/message-id/1655255.DiLJNMXF8A%40aivenronan>
Precedence: bulk

Le mardi 27 juillet 2021, 09:23:48 CEST Drouvot, Bertrand a =E9crit :
> Thanks for the warning, rebase done and new v21 version attached.
>=20
> Bertrand

Hello,

I've taken a look at this patch, and it looks like you adressed every prior=
=20
remark, including the race condition Andres was worried about.

As for the basics: make check-world and make installcheck-world pass.=20

I think the beahviour when dropping a database on the primary should be=20
documented, and proper procedures for handling it correctly should be=20
suggested.

Something along the lines of:

"If a database is dropped on the primary server, the logical replication sl=
ot=20
on the standby will be dropped as well. This means that you should ensure t=
hat=20
the client usually connected to this slot has had the opportunity to stream=
=20
the latest changes before the database is dropped."

As for the patches themselves, I only have two small comments to make.

In patch 0002, in InvalidateConflictingLogicalReplicationSlots, I don't see=
 the=20
need to check for an InvalidOid since we already check the SlotIsLogical:

+		/* We are only dealing with *logical* slot conflicts. */
+		if (!SlotIsLogical(s))
+			continue;
+
+		/* not our database and we don't want all the database,=20
skip */
+		if ((s->data.database !=3D InvalidOid && s->data.database=20
!=3D dboid) && TransactionIdIsValid(xid))
+			continue;

In patch 0004, small typo in the test file:
+##################################################
+# Test standby promotion and logical decoding bheavior
+# after the standby gets promoted.
+##################################################

Thank you for working on this !

Regards,

=2D-=20
Ronan Dunklau