On Thu, Jun 17, 2021 at 6:22 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > > On Wed, Jun 16, 2021 at 9:08 AM Peter Smith <smithpb2...@gmail.com> wrote: > > > > > > Please find attached the latest patch set v86* > > > > A couple of comments: > > (1) I think one of my suggested changes was missed (or was that > intentional?): > > BEFORE: > + The LSN of the commit prepared. > AFTER: > + The LSN of the commit prepared transaction. >
No, not missed. I already dismissed that one and wrote about it when I posted v85 [1]. > > (2) In light of Tom Lane's recent changes in: > > fe6a20ce54cbbb6fcfe9f6675d563af836ae799a (Don't use Asserts to check > for violations of replication protocol) > > there appear to be some instances of such code in these patches. Yes, I already noted [2] there are likely to be such cases which need to be fixed. > > For example, in the v86-0001 patch: > > +/* > + * Handle PREPARE message. > + */ > +static void > +apply_handle_prepare(StringInfo s) > +{ > + LogicalRepPreparedTxnData prepare_data; > + char gid[GIDSIZE]; > + > + logicalrep_read_prepare(s, &prepare_data); > + > + Assert(prepare_data.prepare_lsn == remote_final_lsn); > > The above Assert() should be changed to something like: > > + if (prepare_data.prepare_lsn != remote_final_lsn) > + ereport(ERROR, > + (errcode(ERRCODE_PROTOCOL_VIOLATION), > + errmsg_internal("incorrect prepare LSN %X/%X in > prepare message (expected %X/%X)", > + LSN_FORMAT_ARGS(prepare_data.prepare_lsn), > + LSN_FORMAT_ARGS(remote_final_lsn)))); > > Without being more familiar with this code, it's difficult for me to > judge exactly how many of such cases are in these patches. Thanks for the above example. I will fix this one later, after receiving some more reviews and reports of other Assert cases just like this one. ------ [1] https://www.postgresql.org/message-id/CAHut%2BPvOVkiVBf4P5chdVSoVs5%3Da%3DF_GtTSHHoXDb4LiOM_8Qw%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAHut%2BPvdio4%3DOE6cz5pr8VcJNcAgt5uGBPdKf-tnGEMa1mANGg%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia