RE: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o) - Mailing list pgsql-hackers

On Thu, Aug 26, 2021 2:18 PM Dilip Kumar <dilipbalaut@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