On Mon, Jan 12, 2026 at 4:08 PM Chao Li <[email protected]> wrote:
> Thanks for the patch. Here are my comments on v4.
Thanks for the review!
> 1 - 0001
> ```
> + /*
> + * Save the last flushed position as the replication start point. On
> + * reconnect, replication resumes from there to avoid re-sending
> flushed
> + * data.
> + */
> + startpos = output_fsync_lsn;
> ```
>
> Looking at function OutputFsync(), fsync() may fail and there a few branches
> to return early without fsync(), so should we only update startpos after
> fsync()?
Maybe not, but I might be missing something. Could you clarify what
concrete scenario would be problematic with the current code?
> 2 - 0001
> ```
> + if (outfd != -1 && strcmp(outfile, "-") != 0)
> + OutputFsync(feGetCurrentTimestamp());
> ```
>
> Do we need to check return code of OutputFsync()? I checked this file, the
> only caller that doesn’t check return code of OutputFsync() has a comment for
> why:
> ```
> /* no need to jump to error on failure here, we're finishing anyway */
> OutputFsync(t);
> ```
>
> I saw 0004 has changed OutputFsync to return nothing. So, it’s ok to not
> adding a comment here. But I just feel that, if we want to make the commit
> self-contained, it would be better to add a comment here, but that’s not a
> strong opinion.
Yeah, I think that making the patch "self-contained" in that sense isn't
really worth the extra effort.
> 3 - 0001
>
> No a direct issue of this patch. I noticed that, everywhere that calls
> OutputFsync(), it checks outfd != -1 && strcmp(outfile, "-") != 0. However,
> two places miss the check:
The 0001 patch updates pg_recvlogical to call OutputFsync() before
restarting replication. That call is guarded by strcmp(outfile, "-") != 0.
Your comment makes me reconsider this: when "--file -" is used,
OutputFsync() would be skipped, so startpos would not be updated before
restarting replication. That can lead to duplicate output on stdout,
which is clearly problematic. For this reason, I removed that check
in the latest 0001 patch.
In other places where we check strcmp(outfile, "-") != 0, such as:
if (outfd != -1 && output_reopen && strcmp(outfile, "-") != 0)
{
now = feGetCurrentTimestamp();
OutputFsync(now);
close(outfd);
outfd = -1;
}
on second thought, the check seems necessary for close(outfd) and
"outfd = -1", but not for calling OutputFsync(). If that understanding is
correct, it might make sense to adjust where the check is applied.
However, I think that should be handled in a separate patch.
> 4 - 0002
> ```
> + croak "timed out waiting for match: $regexp”;
> ```
>
> Is it more helpful to include filename in the error message?
OK, I've updated the message to include the filename.
> 5 - 0003
> ```
> +my ($stdout, $stderr);
> +my $recv = IPC::Run::start(
> + [@pg_recvlogical_cmd],
> + '>' => \$stdout,
> + '2>' => \$stderr);
> ```
>
> $stdout and $stderr are never used.
Yes, but I'm fine with keeping them as they are.
I've attached the updated version of the patches upthread.
Regards,
--
Fujii Masao
