On Thu, Aug 26, 2021 2:18 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
> The patch looks good to me, I have rebased 0002 atop
> this patch and also done some cosmetic fixes in 0002.
Here are some comments for the 0002 patch.
1)
- * toplevel transaction. Each subscription has a separate set of files.
+ * toplevel transaction. Each subscription has a separate files.
a separate files => a separate file
2)
+ * Otherwise, just open it file for writing, in append mode.
*/
open it file => open the file
3)
if (subxact_data.nsubxacts == 0)
{
- if (ent->subxact_fileset)
- {
- cleanup_subxact_info();
- FileSetDeleteAll(ent->subxact_fileset);
- pfree(ent->subxact_fileset);
- ent->subxact_fileset = NULL;
- }
+ cleanup_subxact_info();
+ BufFileDeleteFileSet(stream_fileset, path, true);
+
Before applying the patch, the code only invoke cleanup_subxact_info() when the
file exists. After applying the patch, it will invoke cleanup_subxact_info()
either the file exists or not. Is it correct ?
4)
/*
- * If this is the first streamed segment, the file must not exist, so
make
- * sure we're the ones creating it. Otherwise just open the file for
- * writing, in append mode.
+ * If this is the first streamed segment, create the changes file.
+ * Otherwise, just open it file for writing, in append mode.
*/
if (first_segment)
- {
...
- if (found)
- ereport(ERROR,
- (errcode(ERRCODE_PROTOCOL_VIOLATION),
- errmsg_internal("incorrect
first-segment flag for streamed replication transaction")));
...
- }
+ stream_fd = BufFileCreateFileSet(stream_fileset, path);
Since the function BufFileCreateFileSet() doesn't check the file's existence,
the change here seems remove the file existence check which the old code did.
Best regards,
Hou zj
