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

On Thu, Aug 26, 2021 2:18 PM Dilip Kumar <dilipbalaut(at)gmail(dot)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