|From:||Michael Paquier <michael(at)paquier(dot)xyz>|
|To:||Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>|
|Cc:||Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com>, Arseny Sher <a(dot)sher(at)postgrespro(dot)ru>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>, konstantin knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>|
|Subject:||Re: Possible bug in logical replication.|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
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
> 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
> - /* 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.
|Next Message||Michael Paquier||2018-07-19 01:54:23||Re: Add SKIP LOCKED to VACUUM and ANALYZE|
|Previous Message||Thomas Munro||2018-07-19 01:00:46||Re: [PATCH] pg_hba.conf : new auth option : clientcert=verify-full|