Re: [HACKERS] logical decoding of two-phase transactions

On Tue, Mar 9, 2021 at 9:55 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Mar 9, 2021 at 3:22 PM Ajin Cherian <itsajin(at)gmail(dot)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