> On Monday, March 21, 2022 6:01 PM Amit Kapila <amit.kapil...@gmail.com> > wrote: > > > > On Sat, Mar 19, 2022 at 9:10 AM Ajin Cherian <itsa...@gmail.com> wrote: > > > > > > On Thu, Mar 17, 2022 at 10:43 PM Amit Kapila > > > <amit.kapil...@gmail.com> > > wrote: > > > > > > > 3. Can we add a simple test for it in one of the existing test > > > > files(say in 001_rep_changes.pl)? > > > > > > added a simple test. > > > > > > > This doesn't verify if the transaction is skipped. I think we should > > extend this test to check for a DEBUG message in the Logs (you need to > > probably set log_min_messages to DEBUG1 for this test). As an example, > > you can check the patch [1]. Also, it seems by mistake you have added > > wait_for_catchup() twice. > > I added a testcase to check the DEBUG message. > > > Few other comments: > > ================= > > 1. Let's keep the parameter name as skipped_empty_xact in > > OutputPluginUpdateProgress so as to not confuse with the other patch's > > [2] keep_alive parameter. I think in this case we must send the > > keep_alive message so as to not make the syncrep wait whereas in the > > other patch we only need to send it periodically based on > > wal_sender_timeout parameter. > > 2. The new function SyncRepEnabled() seems confusing to me as the > > comments in SyncRepWaitForLSN() clearly state why we need to first > > read the parameter 'sync_standbys_defined' without any lock then read > > it again with a lock if the parameter is true. So, I just put that > > check back and also added a similar check in WalSndUpdateProgress. > > 3. > > @@ -1392,11 +1481,21 @@ pgoutput_truncate(LogicalDecodingContext *ctx, > > ReorderBufferTXN *txn, > > continue; > > > > relids[nrelids++] = relid; > > + > > + /* Send BEGIN if we haven't yet */ > > + if (txndata && !txndata->sent_begin_txn) pgoutput_send_begin(ctx, > > + txn); > > maybe_send_schema(ctx, change, relation, relentry); > > } > > > > if (nrelids > 0) > > { > > + txndata = (PGOutputTxnData *) txn->output_plugin_private; > > + > > + /* Send BEGIN if we haven't yet */ > > + if (txndata && !txndata->sent_begin_txn) pgoutput_send_begin(ctx, > > + txn); > > + > > > > Why do we need to try sending the begin in the second check? I think > > it should be sufficient to do it in the above loop. > > > > I have made these and a number of other changes in the attached patch. > > Do let me know what you think of the attached? > > The changes look good to me. > And I did some basic tests for the patch and didn’t find some other problems. > > Attach the new version patch.
Oh, sorry, I posted the wrong patch, here is the correct one. Best regards, Hou zj
v28-0001-Skip-empty-transactions-for-logical-replication.patch
Description: v28-0001-Skip-empty-transactions-for-logical-replication.patch