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

From: Dilip Kumar <dilipbalaut(at)gmail(dot)com>
To: "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Subject: Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Date: 2021-08-27 06:34:31
Message-ID: CAFiTN-v-zFpmm7Ze1Dai5LJjhhNYXvK8QABs35X89WY0HDG4Ww@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers pgsql-hackers

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.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Attachment Content-Type Size
v6-0001-Refactor-sharedfileset.c-to-separate-out-fileset-.patch text/x-patch 42.1 KB
v6-0002-Using-fileset-more-effectively-in-the-apply-worke.patch text/x-patch 18.6 KB

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Daniel Gustafsson 2021-08-27 14:25:25 pgsql: Avoid invoking PQfnumber in loop constructs
Previous Message houzj.fnst@fujitsu.com 2021-08-27 05:28:48 RE: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2021-08-27 06:46:35 Re: Estimating HugePages Requirements?
Previous Message vignesh C 2021-08-27 06:33:24 Re: Added schema level support for publication.