Hi, On Thu, Aug 6, 2026 at 9:59 AM Masahiko Sawada <[email protected]> wrote: > > > 2/ > > + /* > > + * The message doesn't belong to any remote transaction, so there is > > + * no remote commit LSN nor timestamp to record. Clear the state left > > + * over by the previously applied transaction so that this commit > > + * doesn't inherit it. > > + */ > > + replorigin_xact_clear(false); > > > > Why is this a problem if we let the non-transactional message inherit it? > > IIUC non-transactional messages would have the same commit timestamp > as the previously applied transaction, which is wrong to me.
Having replorigin_xact_clear there looks fine to me. The next transaction commit would anyway set the origin LSN and timestamp. > > Wrapping the hook with begin and end replication step is nice. This > > lets the hook see the correct command ID, snapshot, and memory > > context. There are callers that do the begin first and read message > > next (insert), but it seems okay this way because read message doesn't > > do any catalog or table accesses, so it should be fine. > > > > begin_replication_step() switches the memory context to > ApplyMessageContext. Given logicalrep_read_message() palloc's for > messages, it should be called after begin_replication_step(). Fixed it. Right. I verified other places and wherever the read does a palloc, it is wrapped within begin and end replication step. > I've attached the updated patch. Thanks. The v4 patch looks good to me. pgindent and tests are happy. I have no further comments. I marked the CF entry RfC (https://commitfest.postgresql.org/patch/7092/). FWIW, the CF bot complains with "needs rebase": https://cfbot.cputube.org/patch_7092.log. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
