On Tue, Mar 9, 2021 at 9:55 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Mar 9, 2021 at 3:22 PM Ajin Cherian <itsa...@gmail.com> wrote: > > > > Few comments: > ================== > > 3. In prepare_spoolfile_replay_messages(), it is better to free the > memory allocated for temporary strings buffer and s2.
I guess this was suggested because it is what the apply_handle_stream_commit() function was doing for very similar code. But now the same code cannot work this time for the *_replay_messages() function because those buffers are allocated with the TopTransactionContext and they are already being freed as a side-effect when the last psf message (the LOGICAL_REP_MSG_PREPARE) is replayed/dispatched and ending the transaction. So attempting to free them again causes segmentation violation (I already fixed this exact problem last week when the pfree code was still in the code). > 5. I think prepare_spoolfile_close can be extended to take PsfFile as > input and then it can be also used from > prepare_spoolfile_replay_messages. No, the *_close() is intended only for ending the "current" psf (the global psf_cur) which was being spooled. The function comment says the same. The *_close() is paired with the *_create() which created the psf_cur. Whereas, the reply fd close at commit time is just a locally opened fd unrelated to psf_cur. This close is deliberately self-contained in the *_replay_messages() function, which is not dissimilar to what the other streaming spool file code does - e.g. notice apply_handle_stream_commit function simply closes its own fd using BufFileClose; it doesn’t delegate stream_close_file() to do it. ------ Kind Regards, Peter Smith. Fujitsu Australia
