Re: Remove page-read callback from XLogReaderState.
От | Antonin Houska |
---|---|
Тема | Re: Remove page-read callback from XLogReaderState. |
Дата | |
Msg-id | 18581.1556193500@localhost обсуждение исходный текст |
Ответ на | Remove page-read callback from XLogReaderState. (Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>) |
Ответы |
Re: Remove page-read callback from XLogReaderState.
|
Список | pgsql-hackers |
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > Hello. As mentioned before [1], read_page callback in > XLogReaderState is a cause of headaches. Adding another > remote-controlling stuff to xlog readers makes things messier [2]. The patch I posted in thread [2] tries to solve another problem: it tries to merge xlogutils.c:XLogRead(), walsender.c:XLogRead() and pg_waldump.c:XLogDumpXLogRead() into a single function, xlogutils.c:XLogRead(). > [2] > https://www.postgresql.org/message-id/20190412.122711.158276916.horiguchi.kyotaro@lab.ntt.co.jp > I refactored XLOG reading functions so that we don't need the > callback. I was curious about the patch, so I reviewed it: * xlogreader.c ** Comments mention "opcode", "op" and "expression step" - probably leftover from the executor, which seems to have inspired you. ** XLR_DISPATCH() seems to be unused ** Comment: "duplicatedly" -> "repeatedly" ? ** XLogReadRecord(): comment "All internal state need ..." -> "needs" ** XLogNeedData() *** shouldn't only the minimum amount of data needed (SizeOfXLogLongPHD) be requested here? state->loadLen = XLOG_BLCKSZ; XLR_LEAVE(XLND_STATE_SEGHEAD, true); Note that ->loadLen is also set only to the minimum amount of data needed elsewhere. *** you still mention "read_page callback" in a comment. *** state->readLen is checked before one of the calls of XLR_LEAVE(), but I think it should happen before *each* call. Otherwise data can be read from the page even if it's already in the buffer. * xlogreader.h ** XLND_STATE_PAGEFULLHEAD - maybe LONG rather than FULL? And perhaps HEAD -> HDR, so it's clear that it's about (page) header, not e.g. list head. ** XLogReaderState.loadLen - why not reqLen? loadLen sounds to me like "loaded" as opposed to "requested". And assignemnt like this int reqLen = xlogreader->loadLen; will also be less confusing with ->reqLen. Maybe also ->loadPagePtr should be renamed to ->targetPagePtr. * trailing whitespace: xlogreader.h:130, xlogreader.c:1058 * The 2nd argument of SimpleXLogPageRead() is "private", which seems too generic given that the function is no longer used as a callback. Since the XLogPageReadPrivate structure only has two fields, I think it'd be o.k. to pass them to the function directly. * I haven't found CF entry for this patch. -- Antonin Houska Web: https://www.cybertec-postgresql.com
В списке pgsql-hackers по дате отправления: