On Tue, Jun 30, 2020 at 10:13 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Tue, Jun 30, 2020 at 9:20 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > Can't we name the last parameter as 'commit_lsn' as that is how > > documentation in the patch spells it and it sounds more appropriate? > > You are right commit_lsn seems more appropriate here. > > > Also, is there a reason for assigning report_location and > > write_location differently than what we do in commit_cb_wrapper? > > Basically, assign those as txn->final_lsn and txn->end_lsn > > respectively. > > Yes, I think it should be handled in same way as commit_cb_wrapper. > Because before calling ReorderBufferStreamCommit in > ReorderBufferCommit, we are properly updating the final_lsn as well as > the end_lsn. >
Okay, I have made these changes in the attached patch and there are
few more changes in
0003-Extend-the-output-plugin-API-with-stream-methods.
1. In pg_decode_stream_message, for transactional messages, we were
displaying message contents which is different from other streaming
APIs. I have changed it so that streaming API doesn't display message
contents for transactional messages.
2.
+ /* in streaming mode, stream_change_cb is required */
+ if (ctx->callbacks.stream_change_cb == NULL)
+ ereport(ERROR,
+ (errmsg("Output plugin supports streaming, but has not registered "
+ "stream_change_cb callback.")));
The error messages seem a bit weird. (a) doesn't include error code,
(b) not in PG style. I have changed all the error messages to fix
these two issues and change the message as well
3. Rearranged the functions stream_* so that the optional functions
are at the end and also arranged other functions in a way that looks
more logical to me.
4. Updated comments, commit message, and edited docs in the patch.
I have made a few changes in
0004-Gracefully-handle-concurrent-aborts-of-transacti as well.
1. The variable bsysscan was not being reset in case of error. I have
introduced a new function to reset both bsysscan and CheckXidAlive
during transaction abort. Also, snapmgr.c doesn't seem right place
for these variables, so I moved them to xact.c. I think this will
make the initialization of CheckXidAlive during catch in
ReorderBufferProcessTXN redundant.
2. Updated comments and commit message.
Let me know what you think about the above changes.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
v31-0001-Immediately-WAL-log-subtransaction-and-top-level.patch
Description: Binary data
v31-0004-Gracefully-handle-concurrent-aborts-of-transacti.patch
Description: Binary data
v31-0002-WAL-Log-invalidations-at-command-end-with-wal_le.patch
Description: Binary data
v31-0003-Extend-the-logical-decoding-output-plugin-API-wi.patch
Description: Binary data
