Feedback-ID: i0c21471d:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-527-gee7b8d90aa-fm-20230629.001-gee7b8d90
Mime-Version: 1.0
Message-Id: <f851860d-566a-4334-8a41-5a6b47041a4b@app.fastmail.com>
In-Reply-To: <20230705105639.lsz7ogusvkukbvs5@alvherre.pgsql>
References: <20230705105639.lsz7ogusvkukbvs5@alvherre.pgsql>
Date: Wed, 05 Jul 2023 10:37:05 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: "Alvaro Herrera" <alvherre@alvh.no-ip.org>,
 "Amit Kapila" <amit.kapila16@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org,
 "Masahiko Sawada" <sawada.mshk@gmail.com>
Subject: Re: logicalrep_message_type throws an error
Content-Type: multipart/alternative;
 boundary=965de1bba6a046aa827aa219fce32c2c
Archived-At: <https://www.postgresql.org/message-id/f851860d-566a-4334-8a41-5a6b47041a4b%40app.fastmail.com>
Precedence: bulk

--965de1bba6a046aa827aa219fce32c2c
Content-Type: text/plain

On Wed, Jul 5, 2023, at 7:56 AM, Alvaro Herrera wrote:
> On 2023-Jul-05, Amit Kapila wrote:
> 
> > I think after returning "???" from logicalrep_message_type(), the
> > above is possible when we get the error: "invalid logical replication
> > message type "X"" from apply_dispatch(), right? If so, then what about
> > the case when we forgot to handle some message in
> > logicalrep_message_type() but handled it in apply_dispatch()? Isn't it
> > better to return the 'action' from the function
> > logicalrep_message_type() for unknown type? That way the information
> > could be a bit better and we can easily catch the code bug as well.
> 
> Are you suggesting that logicalrep_message_type should include the
> numerical value of 'action' in the ??? message? Something like this:
> 
> ERROR:  invalid logical replication message type "X"
> CONTEXT:  processing remote data for replication origin "pg_16638" during message type "??? (123)" in transaction 796, finished at 0/16266F8

Isn't this numerical value already exposed in the error message (X = 88)?
In this example, it is:

ERROR:  invalid logical replication message type "X"
CONTEXT:  processing remote data for replication origin "pg_16638" during message type "??? (88)" in transaction 796, finished at 0/1626698

IMO it could be confusing if we provide two representations of the same data (X
and 88). Since we already provide "X" I don't think we need "88". Another
option, is to remove "X" from apply_dispatch() and add "??? (88)" to 
logicalrep_message_type().

Opinions?


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

--965de1bba6a046aa827aa219fce32c2c
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 Wed, Jul 5, =
2023, at 7:56 AM, Alvaro Herrera wrote:<br></div><blockquote type=3D"cit=
e" id=3D"qt" style=3D""><div>On 2023-Jul-05, Amit Kapila wrote:<br></div=
><div><br></div><div>&gt; I think after returning "???" from logicalrep_=
message_type(), the<br></div><div>&gt; above is possible when we get the=
 error: "invalid logical replication<br></div><div>&gt; message type "X"=
" from apply_dispatch(), right? If so, then what about<br></div><div>&gt=
; the case when we forgot to handle some message in<br></div><div>&gt; l=
ogicalrep_message_type() but handled it in apply_dispatch()? Isn't it<br=
></div><div>&gt; better to return the 'action' from the function<br></di=
v><div>&gt; logicalrep_message_type() for unknown type? That way the inf=
ormation<br></div><div>&gt; could be a bit better and we can easily catc=
h the code bug as well.<br></div><div><br></div><div>Are you suggesting =
that logicalrep_message_type should include the<br></div><div>numerical =
value of 'action' in the ??? message? Something like this:<br></div><div=
><br></div><div>ERROR:&nbsp; invalid logical replication message type "X=
"<br></div><div>CONTEXT:&nbsp; processing remote data for replication or=
igin "pg_16638" during message type "??? (123)" in transaction 796, fini=
shed at 0/16266F8<br></div></blockquote><div><br></div><div>Isn't this n=
umerical value already exposed in the error message (X =3D 88)?<br></div=
><div>In this example, it is:<br></div><div><br></div><div>ERROR:&nbsp; =
invalid logical replication message type "X"<br></div><div>CONTEXT:&nbsp=
; processing remote data for replication origin "pg_16638" during messag=
e type "??? (88)" in transaction 796, finished at 0/1626698</div><div><b=
r></div><div>IMO it could be confusing if we provide two representations=
 of the same data (X<br></div><div>and 88). Since we already provide "X"=
 I don't think we need "88". Another<br></div><div>option, is to remove =
"X" from apply_dispatch() and add "??? (88)" to <br></div><div>logicalre=
p_message_type().</div><div><br></div><div>Opinions?</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"sign=
ature">EDB&nbsp;&nbsp; <a href=3D"https://www.enterprisedb.com/">https:/=
/www.enterprisedb.com/</a><br></div></div><div><br></div></body></html>
--965de1bba6a046aa827aa219fce32c2c--