Re: logical replication empty transactions - Mailing list pgsql-hackers
From | Ajin Cherian |
---|---|
Subject | Re: logical replication empty transactions |
Date | |
Msg-id | CAFPTHDbuECiq2cZRDUuOeKZ=ucxxyLpsdi5THhY_MU9XWSH65Q@mail.gmail.com Whole thread Raw |
In response to | RE: logical replication empty transactions ("osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>) |
Responses |
RE: logical replication empty transactions
|
List | pgsql-hackers |
On Wed, Jan 26, 2022 at 8:33 PM osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com> wrote: > > On Tuesday, January 11, 2022 6:43 PM Ajin Cherian <itsajin@gmail.com> wrote: > > Minor update to rebase the patch so that it applies clean on HEAD. > Hi, thanks for you rebase. > > Several comments. > > (1) the commit message > > " > transactions, keepalive messages are sent to keep the LSN locations updated > on the standby. > This patch does not skip empty transactions that are "streaming" or "two-phase". > " > > I suggest that one blank line might be needed before the last paragraph. Changed. > > (2) Could you please remove one pair of curly brackets for one sentence below ? > > @@ -1546,10 +1557,13 @@ WalSndWaitForWal(XLogRecPtr loc) > * otherwise idle, this keepalive will trigger a reply. Processing the > * reply will update these MyWalSnd locations. > */ > - if (MyWalSnd->flush < sentPtr && > + if (force_keepalive_syncrep || > + (MyWalSnd->flush < sentPtr && > MyWalSnd->write < sentPtr && > - !waiting_for_ping_response) > + !waiting_for_ping_response)) > + { > WalSndKeepalive(false); > + } > > Changed. > (3) Is this patch's reponsibility to intialize the data in pgoutput_begin_prepare_txn ? > > @@ -433,6 +487,8 @@ static void > pgoutput_begin_prepare_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn) > { > bool send_replication_origin = txn->origin_id != InvalidRepOriginId; > + PGOutputTxnData *txndata = MemoryContextAllocZero(ctx->context, > + sizeof(PGOutputTxnData)); > > OutputPluginPrepareWrite(ctx, !send_replication_origin); > logicalrep_write_begin_prepare(ctx->out, txn); > > > Even if we need this initialization for either non streaming case > or non two_phase case, there can be another issue. > We don't free the allocated memory for this data, right ? > There's only one place to use free in the entire patch, > which is in the pgoutput_commit_txn(). So, > corresponding free of memory looked necessary > in the two phase commit functions. > Actually it is required for begin_prepare to set the data type, so that the checks in the pgoutput_change can make sure that the begin prepare is sent. I've also added a free in commit_prepared code. > (4) SyncRepEnabled's better alignment. > > IIUC, SyncRepEnabled is called not only by the walsender but also by other backends > via CommitTransaction -> RecordTransactionCommit -> SyncRepWaitForLSN. > Then, the place to add the prototype function for SyncRepEnabled seems not appropriate, > strictly speaking or requires a comment like /* called by wal sender or other backends */. > > @@ -90,6 +90,7 @@ extern void SyncRepCleanupAtProcExit(void); > /* called by wal sender */ > extern void SyncRepInitConfig(void); > extern void SyncRepReleaseWaiters(void); > +extern bool SyncRepEnabled(void); > > Even if we intend it is only used by the walsender, the current code place > of SyncRepEnabled in the syncrep.c might not be perfect. > In this file, seemingly we have a section for functions for wal sender processes > and the place where you wrote it is not here. > > at src/backend/replication/syncrep.c, find a comment below. > /* > * =========================================================== > * Synchronous Replication functions for wal sender processes > * =========================================================== > */ Changed. > > (5) minor alignment for expressing a couple of messages. > > @@ -777,6 +846,9 @@ pgoutput_truncate(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, > Oid *relids; > TransactionId xid = InvalidTransactionId; > > + /* If not streaming, should have setup txndata as part of BEGIN/BEGIN PREPARE */ > + Assert(in_streaming || txndata); > > > In the commit message, the way you write is below. > ... > skip BEGIN / COMMIT messages for transactions that are empty. The patch > ... > > In this case, we have spaces back and forth for "BEGIN / COMMIT". > Then, I suggest to unify all of those to show better alignment. fixed. regards, Ajin Cherian
Attachment
pgsql-hackers by date: