User-Agent: Cyrus-JMAP/3.5.0-alpha0-4524-g5e5d2efdba-fm-20211214.001-g5e5d2efd
Mime-Version: 1.0
Message-Id: <41e5697a-b7eb-45d7-9f7b-d802d3fdbcf6@www.fastmail.com>
In-Reply-To: <YbvR69c7Z7VDjXSg@paquier.xyz>
References: <Yblf/R3h0Dj+Ui2v@paquier.xyz>
 <202112161808.lxanl3r7skol@alvherre.pgsql> <YbvR69c7Z7VDjXSg@paquier.xyz>
Date: Thu, 16 Dec 2021 21:31:15 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: "Michael Paquier" <michael@paquier.xyz>,
 "Alvaro Herrera" <alvherre@alvh.no-ip.org>
Cc: "Ashutosh Bapat" <ashutosh.bapat.oss@gmail.com>,
 "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>,
 "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
 "Andres Freund" <andres@anarazel.de>
Subject: Re: Confused comment about drop replica identity index
Content-Type: multipart/alternative;
 boundary=1bab8af00296442882b5f1f8ca71ea5e
Archived-At: 
 <https://www.postgresql.org/message-id/41e5697a-b7eb-45d7-9f7b-d802d3fdbcf6%40www.fastmail.com>
Precedence: bulk

--1bab8af00296442882b5f1f8ca71ea5e
Content-Type: text/plain

On Thu, Dec 16, 2021, at 8:55 PM, Michael Paquier wrote:
> On Thu, Dec 16, 2021 at 03:08:46PM -0300, Alvaro Herrera wrote:
> > Hmm, so if a table has REPLICA IDENTITY INDEX and there is a publication
> > with an explicit column list, then we need to forbid the DROP INDEX for
> > that index.
> 
> Hmm.  I have not followed this thread very closely.
> 
> > I wonder why don't we just forbid DROP INDEX of an index that's been
> > defined as replica identity.  It seems quite silly an operation to
> > allow.
It would avoid pilot errors.

> The commit logs talk about b23b0f55 here for this code, to ease the
> handling of relcache entries for rd_replidindex.  07cacba is the
> origin of the logic (see RelationGetIndexList).  Andres?
> 
> I don't think that this is really an argument against putting more
> restrictions as anything that deals with an index drop, including the
> internal ones related to constraints, would need to go through
> index_drop(), and new features may want more restrictions in place as
> you say.
> 
> Now, I don't see a strong argument in changing this behavior either
> (aka I have not looked at what this implies for the new publication
> types), and we still need to do something for the comment/docs in
> existing branches, anyway.  So I would still fix this gap as a first
> step, then deal with the rest on HEAD as necessary.
> 
I've never understand the weak dependency between the REPLICA IDENTITY and the
index used by it. I'm afraid we will receive complaints about this unexpected
behavior (my logical replication setup is broken because I dropped an index) as
far as new logical replication features are added.  Row filtering imposes some
restrictions in UPDATEs and DELETEs (an error message is returned and the
replication stops) if a column used in the expression isn't part of the REPLICA
IDENTITY anymore.

It seems we already have some code in RangeVarCallbackForDropRelation() that
deals with a system index error condition. We could save a syscall and provide
a test for indisreplident there.

If this restriction is undesirable, we should at least document this choice and
probably emit a WARNING for DROP INDEX.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

--1bab8af00296442882b5f1f8ca71ea5e
Content-Type: text/html
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><head><title></title><style type=3D"text/css">p.Mso=
Normal,p.MsoNoSpacing{margin:0}</style></head><body><div>On Thu, Dec 16,=
 2021, at 8:55 PM, Michael Paquier wrote:<br></div><blockquote type=3D"c=
ite" id=3D"qt" style=3D""><div>On Thu, Dec 16, 2021 at 03:08:46PM -0300,=
 Alvaro Herrera wrote:<br></div><div>&gt; Hmm, so if a table has REPLICA=
 IDENTITY INDEX and there is a publication<br></div><div>&gt; with an ex=
plicit column list, then we need to forbid the DROP INDEX for<br></div><=
div>&gt; that index.<br></div><div><br></div><div>Hmm.&nbsp; I have not =
followed this thread very closely.<br></div><div><br></div><div>&gt; I w=
onder why don't we just forbid DROP INDEX of an index that's been<br></d=
iv><div>&gt; defined as replica identity.&nbsp; It seems quite silly an =
operation to<br></div><div>&gt; allow.<br></div></blockquote><div>It wou=
ld avoid pilot errors.<br></div><div><br></div><blockquote type=3D"cite"=
 id=3D"qt" style=3D""><div>The commit logs talk about b23b0f55 here for =
this code, to ease the<br></div><div>handling of relcache entries for rd=
_replidindex.&nbsp; 07cacba is the<br></div><div>origin of the logic (se=
e RelationGetIndexList).&nbsp; Andres?<br></div><div><br></div><div>I do=
n't think that this is really an argument against putting more<br></div>=
<div>restrictions as anything that deals with an index drop, including t=
he<br></div><div>internal ones related to constraints, would need to go =
through<br></div><div>index_drop(), and new features may want more restr=
ictions in place as<br></div><div>you say.<br></div><div><br></div><div>=
Now, I don't see a strong argument in changing this behavior either<br><=
/div><div>(aka I have not looked at what this implies for the new public=
ation<br></div><div>types), and we still need to do something for the co=
mment/docs in<br></div><div>existing branches, anyway.&nbsp; So I would =
still fix this gap as a first<br></div><div>step, then deal with the res=
t on HEAD as necessary.<br></div><div><br></div></blockquote><div>I've n=
ever understand the weak dependency between the REPLICA IDENTITY and the=
<br></div><div>index used by it. I'm afraid we will receive complaints a=
bout this unexpected<br></div><div>behavior (my logical replication setu=
p is broken because I dropped an index) as<br></div><div>far as new logi=
cal replication features are added.&nbsp; Row filtering imposes some<br>=
</div><div>restrictions in UPDATEs and DELETEs (an error message is retu=
rned and the<br></div><div>replication stops) if a column used in the ex=
pression isn't part of the REPLICA<br></div><div>IDENTITY anymore.<br></=
div><div><br></div><div>It seems we already have some code in RangeVarCa=
llbackForDropRelation() that<br></div><div>deals with a system index err=
or condition. We could save a syscall and provide<br></div><div>a test f=
or indisreplident there.<br></div><div><br></div><div>If this restrictio=
n is undesirable, we should at least document this choice and<br></div><=
div>probably emit a WARNING for DROP INDEX.<br></div><div><br></div><div=
><br></div><div id=3D"sig112409080"><div class=3D"signature">--<br></div=
><div class=3D"signature">Euler Taveira<br></div><div class=3D"signature=
">EDB&nbsp;&nbsp; <a href=3D"https://www.enterprisedb.com/">https://www.=
enterprisedb.com/</a><br></div></div><div><br></div></body></html>
--1bab8af00296442882b5f1f8ca71ea5e--