At Tue, 9 Nov 2021 10:47:11 +0900, Michael Paquier <mich...@paquier.xyz> wrote in > Hi all, > > I have noticed $subject while looking at a patch in this area: > https://www.postgresql.org/message-id/yyiqxteqhgb5g...@paquier.xyz > > While we don't expect the four callers of XLogReadRecord() related to > logical decoding to fail in the code paths changed by the patch > attached, I think that it would be good to provide more context to > users rather than just emitting an elog(ERROR) coming directly from > the WAL reader. This way, it becomes a bit easier to guess from where > the failure is coming. > > A patch is attached to improve all those elog()s. I have tried not to > be too fancy about those new error strings, but I am sure that these > could be tweaked. > > Thoughts?
As the whole, I agree that any substantial message is far better than "%s" in regard to identifying the line that emitted a log line. + elog(ERROR, "could not find record to advance replication slot: %s", I prefer "could not read record while advancing replcation slot", because it is not finding a record but it is reading successive records. However, I don't strongly object to the current wording. + elog(ERROR, "could not find record to send logically-decoded data: %s", errm); I prefer something like the above for the same reason. How about "could not read record while sending logically-decoded data:". The same with above, I'm not strongly opposed to the current wording. Otherwise it looks good to me. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
