[Patch] remove duplicated smgrclose
Hello, hackers, I think there may be some duplicated codes. Function smgrDoPendingDeletes() calls both smgrdounlinkall() and smgrclose(). But both functions would close SMgrRelation object, it's dupliacted behavior? So I make this patch. Could someone take a look at it? Thanks for your help, Steven >From Highgo.com 0001-remove-duplicated-smgrclose.patch Description: Binary data
Re: [Patch] remove duplicated smgrclose
Hi, Junwang, Thank you for the review and excellent summary in commit message! This is my first contribution to community, and not so familiar with the overall process. After reading the process again, it looks like that I'm not qualified to submit the patch to commitfest as I never had reviewed others' work. :( If so, could you please help to submit it to commitfest? Best Regards, Steven Junwang Zhao 于2024年8月1日周四 20:32写道： > Hi Steven, > > On Wed, Jul 31, 2024 at 11:16 AM Steven Niu wrote: > > > > Hello, hackers, > > > > I think there may be some duplicated codes. > > Function smgrDoPendingDeletes() calls both smgrdounlinkall() and > smgrclose(). > > But both functions would close SMgrRelation object, it's dupliacted > behavior? > > > > So I make this patch. Could someone take a look at it? > > > > Thanks for your help, > > Steven > > > > From Highgo.com > > > > > You change LGTM, but the patch seems not to be applied to HEAD, > I generate the attached v2 using `git format` with some commit message. > > -- > Regards > Junwang Zhao >
Re: [Patch] remove duplicated smgrclose
Thanks, I have set my name in the Authors column of CF. Steven Junwang Zhao 于2024年8月2日周五 13:22写道： > Hi Steven, > > On Fri, Aug 2, 2024 at 12:12 PM Steven Niu wrote: > > > > Hi, Junwang, > > > > Thank you for the review and excellent summary in commit message! > > > > This is my first contribution to community, and not so familiar with the > overall process. > > After reading the process again, it looks like that I'm not qualified to > submit the patch to commitfest as I never had reviewed others' work. :( > > If so, could you please help to submit it to commitfest? > > > > https://commitfest.postgresql.org/49/5149/ > > I can not find your profile on commitfest so I left the author as empty, > have you ever registered? If you have a account, you can put your > name in the Authors list. > > > Best Regards, > > Steven > > > > Junwang Zhao 于2024年8月1日周四 20:32写道： > >> > >> Hi Steven, > >> > >> On Wed, Jul 31, 2024 at 11:16 AM Steven Niu wrote: > >> > > >> > Hello, hackers, > >> > > >> > I think there may be some duplicated codes. > >> > Function smgrDoPendingDeletes() calls both smgrdounlinkall() and > smgrclose(). > >> > But both functions would close SMgrRelation object, it's dupliacted > behavior? > >> > > >> > So I make this patch. Could someone take a look at it? > >> > > >> > Thanks for your help, > >> > Steven > >> > > >> > From Highgo.com > >> > > >> > > >> You change LGTM, but the patch seems not to be applied to HEAD, > >> I generate the attached v2 using `git format` with some commit message. > >> > >> -- > >> Regards > >> Junwang Zhao > > > > -- > Regards > Junwang Zhao >
Re: [Patch] remove duplicated smgrclose
Kirill, Good catch! I will split the patch into two to cover both cases. Thanks, Steven Junwang Zhao 于2024年8月9日周五 18:19写道： > On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke > wrote: > > > > On Thu, 1 Aug 2024 at 17:32, Junwang Zhao wrote: > > > > > > Hi Steven, > > > > > > On Wed, Jul 31, 2024 at 11:16 AM Steven Niu > wrote: > > > > > > > > Hello, hackers, > > > > > > > > I think there may be some duplicated codes. > > > > Function smgrDoPendingDeletes() calls both smgrdounlinkall() and > smgrclose(). > > > > But both functions would close SMgrRelation object, it's dupliacted > behavior? > > > > > > > > So I make this patch. Could someone take a look at it? > > > > > > > > Thanks for your help, > > > > Steven > > > > > > > > From Highgo.com > > > > > > > > > > > You change LGTM, but the patch seems not to be applied to HEAD, > > > I generate the attached v2 using `git format` with some commit message. > > > > > > -- > > > Regards > > > Junwang Zhao > > > > Hi all! > > This change looks good to me. However, i have an objection to these > > lines from v2: > > > > > /* Close the forks at smgr level */ > > > - for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) > > > - smgrsw[which].smgr_close(rels[i], forknum); > > > + smgrclose(rels[i]); > > > > Why do we do this? This seems to be an unrelated change given thread > > $subj. This is just a pure refactoring job, which deserves a separate > > patch. There is similar coding in > > smgrdestroy function: > > > > ``` > > for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) > > smgrsw[reln->smgr_which].smgr_close(reln, forknum); > > ``` > > > > So, I feel like these two places should be either changed together or > > not be altered at all. And is it definitely a separate change. > > Yeah, I tend to agree with you, maybe we should split the patch > into two. > > Steven, could you do this? > > > > > -- > > Best regards, > > Kirill Reshke > > > > -- > Regards > Junwang Zhao >
Use function smgrclose() to replace the loop
Hi, Kirill, Junwang, I made this patch to address the refactor issue in our previous email discussion. https://www.postgresql.org/message-id/flat/CABBtG=cdtcbdcbk7mcsy6bjr3s5xutog0vsffuw8olduyyc...@mail.gmail.com That is, the for loop in function smgrdestroy() and smgrdounlinkall can be replaced with smgrclose(). for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) smgrsw[reln->smgr_which].smgr_close(reln, forknum); --> smgrclose(rels[i]); Please let me know if you have any questions. Best Regards, Steven from Highgo.com 0001-Use-function-smgrclose-to-replace-the-loop.patch Description: Binary data
Re: [Patch] remove duplicated smgrclose
Junwang, Kirill, The split work has been done. I created a new patch for removing redundant smgrclose() function as attached. Please help review it. Thanks, Steven Steven Niu 于2024年8月12日周一 18:11写道： > Kirill, > > Good catch! > I will split the patch into two to cover both cases. > > Thanks, > Steven > > > Junwang Zhao 于2024年8月9日周五 18:19写道： > >> On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke >> wrote: >> > >> > On Thu, 1 Aug 2024 at 17:32, Junwang Zhao wrote: >> > > >> > > Hi Steven, >> > > >> > > On Wed, Jul 31, 2024 at 11:16 AM Steven Niu >> wrote: >> > > > >> > > > Hello, hackers, >> > > > >> > > > I think there may be some duplicated codes. >> > > > Function smgrDoPendingDeletes() calls both smgrdounlinkall() and >> smgrclose(). >> > > > But both functions would close SMgrRelation object, it's dupliacted >> behavior? >> > > > >> > > > So I make this patch. Could someone take a look at it? >> > > > >> > > > Thanks for your help, >> > > > Steven >> > > > >> > > > From Highgo.com >> > > > >> > > > >> > > You change LGTM, but the patch seems not to be applied to HEAD, >> > > I generate the attached v2 using `git format` with some commit >> message. >> > > >> > > -- >> > > Regards >> > > Junwang Zhao >> > >> > Hi all! >> > This change looks good to me. However, i have an objection to these >> > lines from v2: >> > >> > > /* Close the forks at smgr level */ >> > > - for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) >> > > - smgrsw[which].smgr_close(rels[i], forknum); >> > > + smgrclose(rels[i]); >> > >> > Why do we do this? This seems to be an unrelated change given thread >> > $subj. This is just a pure refactoring job, which deserves a separate >> > patch. There is similar coding in >> > smgrdestroy function: >> > >> > ``` >> > for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) >> > smgrsw[reln->smgr_which].smgr_close(reln, forknum); >> > ``` >> > >> > So, I feel like these two places should be either changed together or >> > not be altered at all. And is it definitely a separate change. >> >> Yeah, I tend to agree with you, maybe we should split the patch >> into two. >> >> Steven, could you do this? >> >> > >> > -- >> > Best regards, >> > Kirill Reshke >> >> >> >> -- >> Regards >> Junwang Zhao >> > v3-0001-remove-duplicated-smgrclose.patch Description: Binary data
Re: [Patch] remove duplicated smgrclose
Junwang Zhao 于2024年8月15日周四 18:03写道： > On Wed, Aug 14, 2024 at 2:35 PM Steven Niu wrote: > > > > Junwang, Kirill, > > > > The split work has been done. I created a new patch for removing > redundant smgrclose() function as attached. > > Please help review it. > > Patch looks good, actually you can make the refactoring code as v3-0002-xxx > by using: > > git format-patch -2 -v 3 > > Another kind reminder: Do not top-post when you reply ;) > > > > > Thanks, > > Steven > > > > Steven Niu 于2024年8月12日周一 18:11写道： > >> > >> Kirill, > >> > >> Good catch! > >> I will split the patch into two to cover both cases. > >> > >> Thanks, > >> Steven > >> > >> > >> Junwang Zhao 于2024年8月9日周五 18:19写道： > >>> > >>> On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke > wrote: > >>> > > >>> > On Thu, 1 Aug 2024 at 17:32, Junwang Zhao wrote: > >>> > > > >>> > > Hi Steven, > >>> > > > >>> > > On Wed, Jul 31, 2024 at 11:16 AM Steven Niu > wrote: > >>> > > > > >>> > > > Hello, hackers, > >>> > > > > >>> > > > I think there may be some duplicated codes. > >>> > > > Function smgrDoPendingDeletes() calls both smgrdounlinkall() and > smgrclose(). > >>> > > > But both functions would close SMgrRelation object, it's > dupliacted behavior? > >>> > > > > >>> > > > So I make this patch. Could someone take a look at it? > >>> > > > > >>> > > > Thanks for your help, > >>> > > > Steven > >>> > > > > >>> > > > From Highgo.com > >>> > > > > >>> > > > > >>> > > You change LGTM, but the patch seems not to be applied to HEAD, > >>> > > I generate the attached v2 using `git format` with some commit > message. > >>> > > > >>> > > -- > >>> > > Regards > >>> > > Junwang Zhao > >>> > > >>> > Hi all! > >>> > This change looks good to me. However, i have an objection to these > >>> > lines from v2: > >>> > > >>> > > /* Close the forks at smgr level */ > >>> > > - for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) > >>> > > - smgrsw[which].smgr_close(rels[i], forknum); > >>> > > + smgrclose(rels[i]); > >>> > > >>> > Why do we do this? This seems to be an unrelated change given thread > >>> > $subj. This is just a pure refactoring job, which deserves a separate > >>> > patch. There is similar coding in > >>> > smgrdestroy function: > >>> > > >>> > ``` > >>> > for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) > >>> > smgrsw[reln->smgr_which].smgr_close(reln, forknum); > >>> > ``` > >>> > > >>> > So, I feel like these two places should be either changed together or > >>> > not be altered at all. And is it definitely a separate change. > >>> > >>> Yeah, I tend to agree with you, maybe we should split the patch > >>> into two. > >>> > >>> Steven, could you do this? > >>> > >>> > > >>> > -- > >>> > Best regards, > >>> > Kirill Reshke > >>> > >>> > >>> > >>> -- > >>> Regards > >>> Junwang Zhao > > > > -- > Regards > Junwang Zhao > OK， thanks for your kind help. Steven
Re: Use function smgrclose() to replace the loop
Junwang Zhao 于2024年10月15日周二 18:56写道： > On Mon, Oct 14, 2024 at 6:30 PM Ilia Evdokimov > wrote: > > > > > > On 14.08.2024 09:32, Steven Niu wrote: > > > Hi, Kirill, Junwang, > > > > > > I made this patch to address the refactor issue in our previous email > > > discussion. > > > > https://www.postgresql.org/message-id/flat/CABBtG=cdtcbdcbk7mcsy6bjr3s5xutog0vsffuw8olduyyc...@mail.gmail.com > > > > > > > > > That is, the for loop in function smgrdestroy() and smgrdounlinkall > > > can be replaced with smgrclose(). > > > > > > for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) > > > smgrsw[reln->smgr_which].smgr_close(reln, forknum); > > > --> > > > smgrclose(rels[i]); > > > > > > Please let me know if you have any questions. > > > > > > Best Regards, > > > Steven from Highgo.com > > > > Hello, > > > > Are you sure we can refactor loop by 'smgrclose()'? I see it is not > > quite the same. > > smgrclose does more by setting smgr_cached_nblocks[] and smgr_targblock > to InvalidBlockNumber, I see no harm with the replacement, not 100% sure > though. > > > > > Regards, > > Ilia Evdokimov, > > Tantor Labs LLC. > > > > > -- > Regards > Junwang Zhao > When I look into smgrrelease(), which also resets smgr_cached_nblocks and smgr_targblock, I would say this is of good style. So a bare loop of calling smgr_close() without other cleanup actions is kind of weird to me. Unless there is some reason for the current code, I'd like to replace it. Regards, Steven
Re: Patching for increasing the number of columns
Mayeul Kauffmann 于2024年12月13日周五 15:11写道： > > On 20/08/14 18:17, Tom Lane wrote: > > Hm. I think the without_oid test is not showing that anything is broken; > > > The other tests aren't showing any functional issue either AFAICS. > Thanks a lot Tom! That's very helpful. > I have written more details and some basic SQL tests in the wiki of the > application (LimeSurvey) which requires this: > > > http://manual.limesurvey.org/Instructions_for_increasing_the_maximum_number_of_columns_in_PostgreSQL_on_Linux > > I will give update here or on that wiki (where most relevant) should I > find issues while testing. > > Cheers, > > mayeulk > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hack...@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > > > Hi, Mayeul , I tried your solution on PG17.2 and it does works when the columns is less than 2048. However, when there are 2048 columns in one table, you cannot read out the content of table. postgres=# CREATE TABLE large_table ( col1 char, col2 char, col3 char, col4 char, col5 char, .. col2045 char, col2046 char, col2047 char, col2048 char ); CREATE TABLE postgres=# insert into large_table values(3); INSERT 0 1 postgres=# select col1 from large_table; col1 -- (1 row) Yes, there does be a tuple in this table and the result shows you one row is retrieved. But you got nothing. Definitely some limitation still exists somewhere. Steven
Re: [Patch] remove duplicated smgrclose
Masahiko Sawada 于2025年3月8日周六 12:04写道：
> Hi,
>
> On Sun, Oct 27, 2024 at 12:05 PM Kirill Reshke
> wrote:
> >
> > On Wed, 14 Aug 2024 at 11:35, Steven Niu wrote:
> > >
> > > Junwang, Kirill,
> > >
> > > The split work has been done. I created a new patch for removing
> redundant smgrclose() function as attached.
> > > Please help review it.
> > >
> > > Thanks,
> > > Steven
> > >
> > > Steven Niu 于2024年8月12日周一 18:11写道：
> > >>
> > >> Kirill,
> > >>
> > >> Good catch!
> > >> I will split the patch into two to cover both cases.
> > >>
> > >> Thanks,
> > >> Steven
> > >>
> > >>
> > >> Junwang Zhao 于2024年8月9日周五 18:19写道：
> > >>>
> > >>> On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke
> wrote:
> > >>> >
> > >>> > On Thu, 1 Aug 2024 at 17:32, Junwang Zhao
> wrote:
> > >>> > >
> > >>> > > Hi Steven,
> > >>> > >
> > >>> > > On Wed, Jul 31, 2024 at 11:16 AM Steven Niu
> wrote:
> > >>> > > >
> > >>> > > > Hello, hackers,
> > >>> > > >
> > >>> > > > I think there may be some duplicated codes.
> > >>> > > > Function smgrDoPendingDeletes() calls both smgrdounlinkall()
> and smgrclose().
> > >>> > > > But both functions would close SMgrRelation object, it's
> dupliacted behavior?
> > >>> > > >
> > >>> > > > So I make this patch. Could someone take a look at it?
> > >>> > > >
> > >>> > > > Thanks for your help,
> > >>> > > > Steven
> > >>> > > >
> > >>> > > > From Highgo.com
> > >>> > > >
> > >>> > > >
> > >>> > > You change LGTM, but the patch seems not to be applied to HEAD,
> > >>> > > I generate the attached v2 using `git format` with some commit
> message.
> > >>> > >
> > >>> > > --
> > >>> > > Regards
> > >>> > > Junwang Zhao
> > >>> >
> > >>> > Hi all!
> > >>> > This change looks good to me. However, i have an objection to these
> > >>> > lines from v2:
> > >>> >
> > >>> > > /* Close the forks at smgr level */
> > >>> > > - for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
> > >>> > > - smgrsw[which].smgr_close(rels[i], forknum);
> > >>> > > + smgrclose(rels[i]);
> > >>> >
> > >>> > Why do we do this? This seems to be an unrelated change given
> thread
> > >>> > $subj. This is just a pure refactoring job, which deserves a
> separate
> > >>> > patch. There is similar coding in
> > >>> > smgrdestroy function:
> > >>> >
> > >>> > ```
> > >>> > for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
> > >>> > smgrsw[reln->smgr_which].smgr_close(reln, forknum);
> > >>> > ```
> > >>> >
> > >>> > So, I feel like these two places should be either changed together
> or
> > >>> > not be altered at all. And is it definitely a separate change.
> > >>>
> > >>> Yeah, I tend to agree with you, maybe we should split the patch
> > >>> into two.
> > >>>
> > >>> Steven, could you do this?
> > >>>
> > >>> >
> > >>> > --
> > >>> > Best regards,
> > >>> > Kirill Reshke
> > >>>
> > >>>
> > >>>
> > >>> --
> > >>> Regards
> > >>> Junwang Zhao
> >
> > Hi!
> > Looks like discussion on the subject is completed, and no open items
> > left, so I will try to mark commitfest [1] entry as Ready For
> > Committer.
> >
>
> I've looked at the patch and have some comments:
>
> The patch removes smgrclose() calls following smgrdounlinkall(), for
> example:
>
> --- a/src/backend/catalog/storage.c
> +++ b/src/backend/catalog/storage.c
> @@ -686,9 +686,6 @@ smgrDoPendingDeletes(bool isCommit)
> {
> smgrdounlinkall(sre
Re: [Patch] remove duplicated smgrclose
在 2025/3/12 6:31, Masahiko Sawada 写道: On Mon, Mar 10, 2025 at 3:08 AM Steven Niu wrote: Hi, Masahiko Thanks for your comments! I understand your concern as you stated. However, my initial patch was split into two parts as Kirill suggested. This thread is about the first part. Another part is here: https://commitfest.postgresql.org/patch/5149/ Or you can take a look at the v2-0001-remove-duplicated-smgrclose.patch in this thread for the complete change. I think either we review the v2-patch, or review the both 5149 and 5196 CFs, for my complete change. There should be no missing operations. Please let me know if you have more comments. What is the main point of removing these duplication? While I can see that in smgrDoPendingDeletes(), we do 'smgrsw[reln->smgr_which].smgr_close(reln, forknum);' twice for each relation: in smgrdounlinkall() and in smgrrelease(), I'm not sure what this change benefits to us. Particularly, we quick return from the second mdclose() call as all segment files are already closed. Have you experienced a case like where the system got stuck or got very slower due to these duplicated calls? Also, we might need to pay attention that with the patch smgrdounlinkall() came to depend on smgrclose(). For example, the more works we add in smgrclose() in the future, the more works smgrdounlinkall() will do, which doesn't happen with the current code as smgrdounlinkall() depends on mdclose(). Given that the patched codes doesn't do exactly the same things as before (e.g, smgrdounlinkall() would end up resetting reln->smgr_cached_nblocks[forknum] too), I think we need some reasons for legitimating this change. Regards, Hi, Masahiko Thank you for your detailed review and valuable insights. I understand your concern about the immediate benefits of removing the duplicate smgr_close() call, especially since the second call effectively becomes a no-op due to the prior file closures. However, the primary intent of my change is not driven by performance improvements or addressing a specific issue, but rather by enhancing the code's structure and clarity. Having two "Close the forks at smgr level" operations might lead to confusion for readers of the code. Additionally, the smgrclose() function not only closes the smgr but also resets smgr_cached_nblocks and smgr_targblock, making it a comprehensive operation. In the current implementation, the smgr is closed inside smgrdounlinkall(), while smgr_cached_nblocks and smgr_targblock are reset outside of it. This creates a fragmentation in the code logic, which could be streamlined. Would it make sense to remove the smgr closing operation within smgrdounlinkall() and leave the rest of the code unchanged? This approach would eliminate the duplication while ensuring no operations are missed. I'd appreciate your thoughts on this suggestion. Thanks, Steven
Re: Forbid to DROP temp tables of other sessions
Hi, I have some comments: 1. namespace.c, if relation->schemaname is pg_temp but myTempNamespace isn't set, the error information might be misleading. Consider checking OidIsValid(myTempNamespace) first. 2."you have not any temporary relations" --> "you have no any temporary relations" 3. Regarding to the code "strncmp(nspname, "pg_temp", 7)", is it ok when the nspname contains something like "pg_temp_1234"? I think we should use strcmp instead of strncmp for exact matching. Thanks, Steven 在 2025/3/17 17:03, Daniil Davydov 写道: Hi, I see that the presence of isolation tests in the patch is controversial. First things first, let's concentrate on fixing the bug. I attach a new version of patch (for `master` branch) to this letter. It contains better comments and a few small improvements. P.S. Sorry for bad formatting in previous letter (idk how to fix it in gmail client) -- Best regards, Daniil Davydov
Re: Forbid to DROP temp tables of other sessions
在 2025/3/17 18:13, Daniil Davydov 写道: Hi, On Mon, Mar 17, 2025 at 4:48 PM Steven Niu wrote: 1. namespace.c, if relation->schemaname is pg_temp but myTempNamespace isn't set, the error information might be misleading. Consider checking OidIsValid(myTempNamespace) first. Could you please clarify exactly which place in the code we are talking about? I think we handle this case in the LookupExplicitNamespace call (with all appropriate error information). I mean RangeVarGetRelidExtended(), you deleted the following code: if (!OidIsValid(myTempNamespace)) relId = InvalidOid; /* this probably can't happen? */ 2."you have not any temporary relations" --> "you have no any temporary relations" I am not an English speaker, but it seems that "have not" would be more correct. Someone has to judge us :) 3. Regarding to the code "strncmp(nspname, "pg_temp", 7)", is it ok when the nspname contains something like "pg_temp_1234"? I think we should use strcmp instead of strncmp for exact matching. Great catch! I'll fix it. Please, see v3 patch. -- Best regards, Daniil Davydov
Re: Forbid to DROP temp tables of other sessions
在 2025/3/17 18:56, Daniil Davydov 写道:
Hi,
On Mon, Mar 17, 2025 at 5:33 PM Steven Niu wrote:
I mean RangeVarGetRelidExtended(), you deleted the following code:
if (!OidIsValid(myTempNamespace))
relId = InvalidOid; /* this probably can't happen? */
Hm, I got it. Let's start with the fact that the comment "this
probably can't happen?" is incorrect. Even if we don't have a
temporary namespace in our session, we still can specify "pg_temp_N"
in the psql query.
Next, if relation->schemaname is pg_temp, then we firstly call
LookupExplicitNamespace, where you can find if
(!OidIsValid(myTempNamespace)) check.
--
Best regards,
Daniil Davydov
If the (relation->relpersistence == RELPERSISTENCE_TEMP) can ensure the
myTempNamespace is always valid, then my comment can be ignored.
Otherwise I think the modified RangeVarGetRelidExtended() should keep
check of myTempNamespace, like this:
if (relation->relpersistence == RELPERSISTENCE_TEMP)
{
Oid namespaceId;
if (!OidIsValid(myTempNamespace))
relId = InvalidOid; /* this probably can't happen? */
else
{
if (relation->schemaname)
{
namespaceId = LookupExplicitNamespace(relation->schemaname,
missing_ok);
if (namespaceId != myTempNamespace)
{
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("could not access temporary relations of other
sessions")));
}
}
else
{
namespaceId = myTempNamespace;
Assert(OidIsValid(namespaceId));
}
if (missing_ok && !OidIsValid(namespaceId))
relId = InvalidOid;
else
relId = get_relname_relid(relation->relname, namespaceId);
}
...
...
Thanks,
Steven
Re: Not-terribly-safe checks for CRC intrinsic support
+# is missing, we must link not just compile, and store the results in global The "compile" should be "compiler"? Regards, Steven 在 2025/3/15 7:04, Tom Lane 写道: I noticed that our configuration-time checks for the presence of CRC intrinsics generally look like unsigned int crc = 0; crc = __crc32cb(crc, 0); crc = __crc32ch(crc, 0); crc = __crc32cw(crc, 0); crc = __crc32cd(crc, 0); /* return computed value, to prevent the above being optimized away */ return crc == 0; The trouble with this is that "crc" is a local variable, so the compiler would be perfectly within its rights to optimize the whole thing down to "return some_constant". While that outcome sufficiently proves that the compiler has heard of these intrinsics, it fails to prove that the platform has any necessary library infrastructure, assembler support for the opcodes, etc etc. Whoever originally wrote this evidently had concern for that hazard, or they'd not have bothered with forcing a dependency on the final value; but that seems insufficient. We have other nearby tests that try to avoid this problem by making the functions-under-test operate on global variables, so I think we should do likewise here. In connection with bug #18839[1], I checked to see if this might already be happening. At least with gcc 12.2 on armhf Debian, it doesn't seem to: the compiler still generates the crc opcodes. But the same compiler is perfectly willing to optimize a call to sin(3) down to a constant under similar conditions. So I think this is just a matter of they didn't get round to it, not that there's a principled reason to think they won't ever get round to it. There might be other cases where these probes are already missing something, and we've not noticed because there's-compiler-support-but-no- library-support is surely a very rare case in the field. In short, I think we ought to apply and perhaps back-patch something like the attached. BTW, it looks to me like PGAC_AVX512_POPCNT_INTRINSICS is at similar hazard, but I'm not entirely sure how to fix that one. Thoughts? regards, tom lane [1] https://www.postgresql.org/message-id/18839-7615d0f8267dc015%40postgresql.org
Add missing PQclear for StreamLogicalLog function
Hi, hackers,
During browsing the code, I found one missing PQclear in function
StreamLogicalLog(). It's a very small problem as it only happens in
error condition. However since another similar patch was accepted,
maybe we should also fix this one.
https://www.postgresql.org/message-id/flat/3DA7CECD-5A05-416D-8527-ABD794AEFE8B%40yesql.se#c5d662ba7bdb07e56ddbd9aaa90dea5d
Regards,
StevenFrom cc87aec988c483e9118d9d31b1dad1f1ca1fb4e7 Mon Sep 17 00:00:00 2001
From: Steven Niu
Date: Wed, 19 Mar 2025 13:02:18 +0800
Subject: Add missing PQclear for StreamLogicalLog function
---
src/bin/pg_basebackup/pg_recvlogical.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c
b/src/bin/pg_basebackup/pg_recvlogical.c
index b9ea23e1426..b68f8b82b92 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -633,6 +633,7 @@ StreamLogicalLog(void)
{
pg_log_error("unexpected termination of replication stream: %s",
PQresultErrorMessage(res));
+ PQclear(res);
goto error;
}
PQclear(res);
--
2.43.0
Add missing PQclear for StreamLogicalLog function
Hi, hackers,
During browsing the code, I found one missing PQclear in function
StreamLogicalLog(). It's a very small problem as it only happens in
error condition. However since another similar patch was accepted,
maybe we should also fix this one.
https://www.postgresql.org/message-id/flat/3DA7CECD-5A05-416D-8527-ABD794AEFE8B%40yesql.se#c5d662ba7bdb07e56ddbd9aaa90dea5d
Regards,
StevenFrom cc87aec988c483e9118d9d31b1dad1f1ca1fb4e7 Mon Sep 17 00:00:00 2001
From: Steven Niu
Date: Wed, 19 Mar 2025 13:02:18 +0800
Subject: Add missing PQclear for StreamLogicalLog function
---
src/bin/pg_basebackup/pg_recvlogical.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c
b/src/bin/pg_basebackup/pg_recvlogical.c
index b9ea23e1426..b68f8b82b92 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -633,6 +633,7 @@ StreamLogicalLog(void)
{
pg_log_error("unexpected termination of replication stream: %s",
PQresultErrorMessage(res));
+ PQclear(res);
goto error;
}
PQclear(res);
--
2.43.0
Re: [PATCH] avoid double scanning in function byteain
在 2025/3/26 16:37, Kirill Reshke 写道: On Wed, 26 Mar 2025 at 12:17, Steven Niu wrote: Hi, Hi! This double scanning can be inefficient, especially for large inputs. So I optimized the function to eliminate the need for two scans, while preserving correctness and efficiency. While the argument that processing input once not twice is fast is generally true, may we have some simple bench here just to have an idea how valuable this patch is? Patch: + /* Handle traditional escaped style in a single pass */ + input_len = strlen(inputText); + result = palloc(input_len + VARHDRSZ); /* Allocate max possible size */ rp = VARDATA(result); + tp = inputText; + while (*tp != '\0') Isn't this `strlen` O(n) + `while` O(n)? Where is the speed up? [0] https://github.com/bminor/glibc/blob/master/string/strlen.c#L43-L45 Hi, Kirill, Your deep insight suprised me! Yes, you are correct that strlen() actually performed a loop operation. So maybe the performance difference is not so obvious. However, there are some other reasons that drive me to make this change. 1. The author of original code left comment: "BUGS: The input is scanned twice." . You can find this line of code in my patch. This indicates a left work to be done. 2. If I were the author of this function, I would not be satisfied with myself that I used two loops to do something which actually can be done with one loop. I prefer to choose a way that would not add more burden to readers. 3. The while (*tp != '\0') loop has some unnecessary codes and I made some change. Thanks, Steven
Re: Doc: Fixup misplaced filelist.sgml entities and add some commentary
Hi, David, In the file docguide.sgml, there is a typo. "Within the book are parts, mostly defined within the same file, expect for the", the "expect" here should be "except". Thanks, Steven 在 2025/3/20 5:13, David G. Johnston 写道: Hi. Having been in filelist.sgml a bit recently I've noticed that the original alphabetical ordering of the entities therein hasn't been adhered to. Partly, I suspect, because there is no guidance about these files and how they are organized. The attached puts things back into alphabetical order (by section) and adds some commentary to this and related files, and the manual. I made the choice to move the special %allfiles; reference to the top since placement doesn't matter and burying the one unique thing in the middle of the file didn't seem helpful. Now both its immediate presence and the comment point out the existence and purpose of ref/allfiles.sgml. David J.
[PATCH] avoid double scanning in function byteain
Hi,
The byteain function converts a string input into a bytea type.
The original implementation processes two input formats:
a hex format (starting with \x) and a traditional escaped format.
For the escaped format, the function scans the input string twice
— once to calculate the exact size of the output and allocate memory,
and again to fill the allocated memory with the parsed data.
This double scanning can be inefficient, especially for large inputs.
So I optimized the function to eliminate the need for two scans,
while preserving correctness and efficiency.
Please help review it and share your valuable comments.
Thanks,
Steven Niu
https://www.highgo.com/From db0352fb7fa463bd7a02f73f29760d1400cef402 Mon Sep 17 00:00:00 2001
From: Steven Niu
Date: Wed, 26 Mar 2025 14:43:43 +0800
Subject: [PATCH] Optimize function byteain() to avoid double scanning
Optimized the function to eliminate the need for two scans,
while preserving correctness and efficiency.
Author: Steven Niu
---
src/backend/utils/adt/varlena.c | 66 +++--
1 file changed, 22 insertions(+), 44 deletions(-)
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 95631eb2099..de422cafbd5 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -291,7 +291,6 @@ text_to_cstring_buffer(const text *src, char *dst, size_t
dst_len)
* ereport(ERROR, ...) if bad form.
*
* BUGS:
- * The input is scanned twice.
* The error checking of input is minimal.
*/
Datum
@@ -302,6 +301,7 @@ byteain(PG_FUNCTION_ARGS)
char *tp;
char *rp;
int bc;
+ size_t input_len;
bytea *result;
/* Recognize hex input */
@@ -318,45 +318,28 @@ byteain(PG_FUNCTION_ARGS)
PG_RETURN_BYTEA_P(result);
}
- /* Else, it's the traditional escaped style */
- for (bc = 0, tp = inputText; *tp != '\0'; bc++)
- {
- if (tp[0] != '\\')
- tp++;
- else if ((tp[0] == '\\') &&
-(tp[1] >= '0' && tp[1] <= '3') &&
-(tp[2] >= '0' && tp[2] <= '7') &&
-(tp[3] >= '0' && tp[3] <= '7'))
- tp += 4;
- else if ((tp[0] == '\\') &&
-(tp[1] == '\\'))
- tp += 2;
- else
- {
- /*
-* one backslash, not followed by another or ### valid
octal
-*/
- ereturn(escontext, (Datum) 0,
-
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
-errmsg("invalid input syntax for type
%s", "bytea")));
- }
- }
-
- bc += VARHDRSZ;
-
- result = (bytea *) palloc(bc);
- SET_VARSIZE(result, bc);
-
- tp = inputText;
+ /* Handle traditional escaped style in a single pass */
+ input_len = strlen(inputText);
+ result = palloc(input_len + VARHDRSZ); /* Allocate max possible size */
rp = VARDATA(result);
+ tp = inputText;
+
while (*tp != '\0')
{
if (tp[0] != '\\')
+ {
*rp++ = *tp++;
- else if ((tp[0] == '\\') &&
-(tp[1] >= '0' && tp[1] <= '3') &&
-(tp[2] >= '0' && tp[2] <= '7') &&
-(tp[3] >= '0' && tp[3] <= '7'))
+ continue;
+ }
+
+ if (tp[1] == '\\')
+ {
+ *rp++ = '\\';
+ tp += 2;
+ }
+ else if ((tp[1] >= '0' && tp[1] <= '3') &&
+(tp[2] >= '0' && tp[2] <= '7') &&
+(tp[3] >= '0' && tp[3] <= '7'))
{
bc = VAL(tp[1]);
bc <<= 3;
@@ -366,23 +349,18 @@ byteain(PG_FUNCTION_ARGS)
tp += 4;
}
- else if ((tp[0] == '\\') &&
-(tp[1] == '\\'))
- {
- *rp++ = '
