From: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, 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> |
Subject: | Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o) |
Date: | 2021-09-01 08:23:05 |
Message-ID: | CAFiTN-vYaUw1U2EdHt3_iX1NAwaA-gjQF15mNaT=2av2u7Em8g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-committers pgsql-hackers |
On Tue, Aug 31, 2021 at 3:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Aug 27, 2021 at 12:04 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
>
> Few comments on v6-0002*
> =========================
> 1.
> -BufFileDeleteFileSet(FileSet *fileset, const char *name)
> +BufFileDeleteFileSet(FileSet *fileset, const char *name, bool missing_ok)
> {
> char segment_name[MAXPGPATH];
> int segment = 0;
> @@ -358,7 +369,7 @@ BufFileDeleteFileSet(FileSet *fileset, const char *name)
> for (;;)
> {
> FileSetSegmentName(segment_name, name, segment);
> - if (!FileSetDelete(fileset, segment_name, true))
> + if (!FileSetDelete(fileset, segment_name, !missing_ok))
>
> I don't think the usage of missing_ok is correct here. If you see
> FileSetDelete->PathNameDeleteTemporaryFile, it already tolerates that
> the file doesn't exist but gives an error only when it is unable to
> link. So, with this missing_ok users (say like worker.c) won't even
> get errors when they are not able to remove files whereas I think the
> need for the patch is to not get an error when the file doesn't exist.
> I think you don't need to change anything in the way we invoke
> FileSetDelete.
Right, fixed.
> 2.
> -static HTAB *xidhash = NULL;
> +static FileSet *stream_fileset = NULL;
>
> Can we keep this in LogicalRepWorker and initialize it accordingly?
Done
> 3.
> + /* Open the subxact file, if it does not exist, create it. */
> + fd = BufFileOpenFileSet(stream_fileset, path, O_RDWR, true);
> + if (fd == NULL)
> + fd = BufFileCreateFileSet(stream_fileset, path);
>
> I think retaining the existing comment: "Create the subxact file if it
> not already created, otherwise open the existing file." seems better
> here.
Done
> 4.
> /*
> - * If there is no subtransaction then nothing to do, but if already have
> - * subxact file then delete that.
> + * If there are no subtransactions, there is nothing to be done, but if
> + * subxacts already exist, delete it.
> */
>
> How about changing the above comment to something like: "Delete the
> subxacts file, if exists"?
Done
> 5. Can we slightly change the commit message as:
> Optimize fileset usage in apply worker.
>
> Use one fileset for the entire worker lifetime instead of using
> separate filesets for each streaming transaction. Now, the
> changes/subxacts files for every streaming transaction will be created
> under the same fileset and the files will be deleted after the
> transaction is completed.
>
> This patch extends the BufFileOpenFileSet and BufFileDeleteFileSet
> APIs to allow users to specify whether to give an error on missing
> files.
Done
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
v7-0001-Optimize-fileset-usage-in-apply-worker.patch | text/x-patch | 19.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2021-09-01 08:49:33 | pgsql: Fix incorrect format placeholders |
Previous Message | Fujii Masao | 2021-09-01 08:06:45 | pgsql: pgbench: Fix bug in measurement of disconnection delays. |
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2021-09-01 08:24:00 | Re: Failure of subscription tests with topminnow |
Previous Message | Fujii Masao | 2021-09-01 08:07:43 | Re: Fix around conn_duration in pgbench |