On Wed, Jul 18, 2018 at 02:30:53PM -0400, Alvaro Herrera wrote: > In the immortal words of Julian Bream: "yeah, I didn't like any of > that".
One wikipedia lookup later, I still don't know where this quote comes from, but at least I understand who the man is. I may be missing something, but I cannot apply your patch on HEAD so I have not tested it. Anyway, I read through it and the thing does not look logically wrong. > I also moved some assignments from the declaration section to the code > section, so that I could attach proper comments to each, to improve > clarity of *why* we do those things. To be pedantic here, you could move the declarations of startlsn, old_resowner and ctx directly inside the PG_TRY block. > I then noticed that we get a XLogRecord from XLogReadRecord, but then > fail to do anything with it, so I changed the code to use a bool > instead, which I think is clearer. Matter of taste perhaps, I was fine with just manipulating the record pointer. > I think the proposed comment before the LogicalDecodingProcessRecord > call failed to convey the important ideas, so I rewrote that one also. > > There is no struct member called confirmed_flush_lsn anywhere. This is referring to the system catalog field in pg_replication_slots. > BTW I think I'm starting to have a vague idea of logical decoding > now. Nice. > PG_TRY(); > { > - /* restart at slot's confirmed_flush */ > + /* > + * Create our decoding context in fast_forward mode, passing > start_lsn > + * as Invalid, so that we start processing from confirmed_flush. > + */ I'd rather mention InvalidXLogRecPtr directly here. Invalid alone makes no real sense. > + gotrecord = XLogReadRecord(ctx->reader, startlsn, &errm) != NULL; I would put parenthesis for clarity. -- Michael
signature.asc
Description: PGP signature