On Mon, Aug 29, 2022 at 11:59 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are some review comments for patch v42-0001: > > ~~ > > 6. > > + warning to notify user to check the publisher tables. The user can ensure > + that publisher tables do not have data which has an origin associated > before > + continuing with any other operations to prevent inconsistent data being > + replicated. > + </para> > > 6a. > > I'm not so sure about this. IMO the warning is not about the > replication – really it is about the COPY which has already happened > anyway. So the user can't really prevent anything from going wrong; > instead, they have to take some action to clean up if anything did go > wrong. > > SUGGESTION > Before continuing with other operations the user should check that > publisher tables did not have data with different origins, otherwise > inconsistent data may have been copied. > > ~
I would like to avoid the use of 'may' here. So, let's change it to something like: "Before continuing with other operations the user should check that publisher tables did not have data with different origins to prevent data inconsistency issues on the subscriber." > > 6b. > > I am also wondering what can the user do now. Assuming there was bad > COPY then the subscriber table has already got unwanted stuff copied > into it. Is there any advice we can give to help users fix this mess? > I don't think the user can do much in that case. She will probably need to re-create the replication. > > 11. > > + ereport(WARNING, > + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > + errmsg("publisher has subscribed table \"%s.%s\" from some other publisher", > + nspname, relname), > + errdetail("Publisher might have subscribed one or more tables from > some other publisher."), > + errhint("Verify that these publisher tables do not have data that > has an origin associated before proceeding to avoid inconsistency.")); > + break; > > 11a. > I'm not sure having that "break" code is correct logic. Won't that > mean the user will only get a warning for the first potential problem > encountered but then other potential problems tables will have no > warnings at all so the user may not be made aware of them? Perhaps you > need to first gather the list of all the suspicious tables before > logging a single warning which shows that list? Or perhaps you need to > log multiple warnings – one warning per suspicious table? > > ~ > > 11b. > The WARNING message seems a bit inside-out because the errmsg is > giving more details than the errdetail. > > SUGGESTIONS (or something similar) > errmsg - "subscription XXX requested origin=NONE but may have copied > data that had a different origin." > errdetail – Publisher YYY has subscribed table \"%s.%s\" from some > other publisher" > Your suggestion is better but we can't say 'copied' because the copy may start afterwards by tablesync worker. Also, using 'may' is not advisable in error messages. How about : "subscription XXX requested origin=NONE but might copy data that had a different origin."? -- With Regards, Amit Kapila.