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

On Fri, Aug 27, 2021 at 3:34 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Fri, Aug 27, 2021 at 10:56 AM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > 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
>
> Done
>
> > 2)
> > + * Otherwise, just open it file for writing, in append mode.
> > */
> >
> > open it file => open the file
>
> Done
>
> >
> > 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 ?
>
> I think this is just structure resetting at the end of the stream.
> Earlier the hash was telling us whether we have ever dirtied that
> structure or not but now we are not maintaining that hash so we just
> reset it at the end of the stream. I don't think its any bad, in fact
> I think this is much cheaper compared to maining the hash.
>
> >
> > 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.
>
> Not really, we were just doing a sanity check of the in memory hash
> entry, now we don't maintain that so we don't need to do that.

Thank you for updating the patch!

The patch looks good to me except for the below comment:

+ /* Delete the subxact file, if it exist. */
+ subxact_filename(path, subid, xid);
+ BufFileDeleteFileSet(stream_fileset, path, true);

s/it exist/it exists/

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/