Обсуждение: Deduplicate code updating ControleFile's DBState.
Hi, I would like to propose a patch that removes the duplicate code setting database state in the control file. The patch is straightforward but the only concern is that in StartupXLOG(), SharedRecoveryState now gets updated only with spin lock; earlier it also had ControlFileLock in addition to that. AFAICU, I don't see any problem there, since until the startup process exists other backends could not connect and write a WAL record. Regards, Amul Sul. EDB: http://www.enterprisedb.com
Вложения
On 9/13/21, 11:06 PM, "Amul Sul" <sulamul@gmail.com> wrote: > The patch is straightforward but the only concern is that in > StartupXLOG(), SharedRecoveryState now gets updated only with spin > lock; earlier it also had ControlFileLock in addition to that. AFAICU, > I don't see any problem there, since until the startup process exists > other backends could not connect and write a WAL record. It looks like ebdf5bf intentionally made sure that we hold ControlFileLock while updating SharedRecoveryInProgress (now SharedRecoveryState after 4e87c48). The thread for this change [0] has some additional details. As far as the patch goes, I'm not sure why SetControlFileDBState() needs to be exported, and TBH I don't know if this change is really a worthwhile improvement. ISTM the main benefit is that it could help avoid cases where we update the state but not the time. However, there's still nothing preventing that, and I don't get the idea that it was really a big problem to begin with. Nathan [0] https://postgr.es/m/CAB7nPqTS5J3-G_zTow0Kc5oqZn877RDDN1Mfcqm2PscAS7FnAw%40mail.gmail.com
On Wed, Sep 15, 2021 at 12:52 AM Bossart, Nathan <bossartn@amazon.com> wrote: > > On 9/13/21, 11:06 PM, "Amul Sul" <sulamul@gmail.com> wrote: > > The patch is straightforward but the only concern is that in > > StartupXLOG(), SharedRecoveryState now gets updated only with spin > > lock; earlier it also had ControlFileLock in addition to that. AFAICU, > > I don't see any problem there, since until the startup process exists > > other backends could not connect and write a WAL record. > > It looks like ebdf5bf intentionally made sure that we hold > ControlFileLock while updating SharedRecoveryInProgress (now > SharedRecoveryState after 4e87c48). The thread for this change [0] > has some additional details. > Yeah, I saw that and ebdf5bf main intention was to minimize the gap between both of them which was quite big previously. The comments added by the same commit also describe the case that backends can write WAL and the control file is still referring not in DB_IN_PRODUCTION and IIUC, which seems to be acceptable. Then the question is what would be wrong if a process can see an inconsistent shared memory view for a small window? Might be wait-promoting might behave unexpectedly, that I have to test. > As far as the patch goes, I'm not sure why SetControlFileDBState() > needs to be exported, and TBH I don't know if this change is really a > worthwhile improvement. ISTM the main benefit is that it could help > avoid cases where we update the state but not the time. However, > there's still nothing preventing that, and I don't get the idea that > it was really a big problem to begin with. > Oh ok, I was working on a different patch[1] where I want to call this function from checkpointer, but I agree exporting function is not in the scope of this patch. Regards, Amul 1] https://postgr.es/m/CAAJ_b97KZzdJsffwRK7w0XU5HnXkcgKgTR69t8cOZztsyXjkQw@mail.gmail.com
On 9/15/21, 4:47 AM, "Amul Sul" <sulamul@gmail.com> wrote:
> On Wed, Sep 15, 2021 at 12:52 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>> It looks like ebdf5bf intentionally made sure that we hold
>> ControlFileLock while updating SharedRecoveryInProgress (now
>> SharedRecoveryState after 4e87c48). The thread for this change [0]
>> has some additional details.
>>
>
> Yeah, I saw that and ebdf5bf main intention was to minimize the gap
> between both of them which was quite big previously. The comments
> added by the same commit also describe the case that backends can
> write WAL and the control file is still referring not in
> DB_IN_PRODUCTION and IIUC, which seems to be acceptable.
> Then the question is what would be wrong if a process can see an
> inconsistent shared memory view for a small window? Might be
> wait-promoting might behave unexpectedly, that I have to test.
For your proposed change, I would either leave out this particular
call site or add a "WithLock" version of the function.
void
SetControlFileDBState(DBState state)
{
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
SetControlFileDBStateWithLock(state);
LWLockRelease(ControlFileLock);
}
void
SetControlFileDBStateWithLock(DBState state)
{
Assert(LWLockHeldByMeInMode(ControlFileLock, LW_EXCLUSIVE));
ControlFile->state = state;
ControlFile->time = (pg_time_t) time(NULL);
UpdateControlFile();
}
>> As far as the patch goes, I'm not sure why SetControlFileDBState()
>> needs to be exported, and TBH I don't know if this change is really a
>> worthwhile improvement. ISTM the main benefit is that it could help
>> avoid cases where we update the state but not the time. However,
>> there's still nothing preventing that, and I don't get the idea that
>> it was really a big problem to begin with.
>>
>
> Oh ok, I was working on a different patch[1] where I want to call this
> function from checkpointer, but I agree exporting function is not in
> the scope of this patch.
Ah, I was missing this context. Perhaps this should be included in
the patch set for the other thread, especially if it will need to be
exported.
Nathan
On Wed, Sep 15, 2021 at 10:49:39PM +0000, Bossart, Nathan wrote:
> Ah, I was missing this context. Perhaps this should be included in
> the patch set for the other thread, especially if it will need to be
> exported.
This part of the patch is mentioned at the top of the thread:
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- ControlFile->state = DB_IN_PRODUCTION;
- ControlFile->time = (pg_time_t) time(NULL);
-
+ SetControlFileDBState(DB_IN_PRODUCTION);
SpinLockAcquire(&XLogCtl->info_lck);
XLogCtl->SharedRecoveryState = RECOVERY_STATE_DONE;
SpinLockRelease(&XLogCtl->info_lck);
There is an assumption in this code to update SharedRecoveryState
*while* holding ControlFileLock. For example, see the following
comment in xlog.c, ReadRecord():
/*
* We update SharedRecoveryState while holding the lock on
* ControlFileLock so both states are consistent in shared
* memory.
*/
--
Michael
Вложения
On Thu, Sep 16, 2021 at 5:17 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Sep 15, 2021 at 10:49:39PM +0000, Bossart, Nathan wrote: > > Ah, I was missing this context. Perhaps this should be included in > > the patch set for the other thread, especially if it will need to be > > exported. > > This part of the patch is mentioned at the top of the thread: > - LWLockAcquire(ControlFileLock, LW_EXCLUSIVE); > - ControlFile->state = DB_IN_PRODUCTION; > - ControlFile->time = (pg_time_t) time(NULL); > - > + SetControlFileDBState(DB_IN_PRODUCTION); > SpinLockAcquire(&XLogCtl->info_lck); > XLogCtl->SharedRecoveryState = RECOVERY_STATE_DONE; > SpinLockRelease(&XLogCtl->info_lck); > > There is an assumption in this code to update SharedRecoveryState > *while* holding ControlFileLock. For example, see the following > comment in xlog.c, ReadRecord(): > /* > * We update SharedRecoveryState while holding the lock on > * ControlFileLock so both states are consistent in shared > * memory. > */ Ok, understood, let's do that update with ControlFileLock, thanks. Regards, Amul
On Thu, Sep 16, 2021 at 4:19 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 9/15/21, 4:47 AM, "Amul Sul" <sulamul@gmail.com> wrote:
> > On Wed, Sep 15, 2021 at 12:52 AM Bossart, Nathan <bossartn@amazon.com> wrote:
> >> It looks like ebdf5bf intentionally made sure that we hold
> >> ControlFileLock while updating SharedRecoveryInProgress (now
> >> SharedRecoveryState after 4e87c48). The thread for this change [0]
> >> has some additional details.
> >>
> >
> > Yeah, I saw that and ebdf5bf main intention was to minimize the gap
> > between both of them which was quite big previously. The comments
> > added by the same commit also describe the case that backends can
> > write WAL and the control file is still referring not in
> > DB_IN_PRODUCTION and IIUC, which seems to be acceptable.
> > Then the question is what would be wrong if a process can see an
> > inconsistent shared memory view for a small window? Might be
> > wait-promoting might behave unexpectedly, that I have to test.
>
> For your proposed change, I would either leave out this particular
> call site or add a "WithLock" version of the function.
>
> void
> SetControlFileDBState(DBState state)
> {
> LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
> SetControlFileDBStateWithLock(state);
> LWLockRelease(ControlFileLock);
> }
>
> void
> SetControlFileDBStateWithLock(DBState state)
> {
> Assert(LWLockHeldByMeInMode(ControlFileLock, LW_EXCLUSIVE));
>
> ControlFile->state = state;
> ControlFile->time = (pg_time_t) time(NULL);
> UpdateControlFile();
> }
>
+1, since skipping ControlFileLock for the DBState update is not the
right thing, let's have two different functions as per your suggestion
-- did the same in the attached version, thanks.
> >> As far as the patch goes, I'm not sure why SetControlFileDBState()
> >> needs to be exported, and TBH I don't know if this change is really a
> >> worthwhile improvement. ISTM the main benefit is that it could help
> >> avoid cases where we update the state but not the time. However,
> >> there's still nothing preventing that, and I don't get the idea that
> >> it was really a big problem to begin with.
> >>
> >
> > Oh ok, I was working on a different patch[1] where I want to call this
> > function from checkpointer, but I agree exporting function is not in
> > the scope of this patch.
>
> Ah, I was missing this context. Perhaps this should be included in
> the patch set for the other thread, especially if it will need to be
> exported.
>
Ok, reverted those changes in the attached version.
I have one additional concern about the way we update the control
file, at every place where doing the update, we need to set control
file update time explicitly, why can't the time update line be moved
to UpdateControlFile() so that time gets automatically updated?
Regards,
Amul
Вложения
On 9/19/21, 11:07 PM, "Amul Sul" <sulamul@gmail.com> wrote: > +1, since skipping ControlFileLock for the DBState update is not the > right thing, let's have two different functions as per your suggestion > -- did the same in the attached version, thanks. I see that the attached patch reorders the call to UpdateControlFile() to before SharedRecoveryState is updated, which seems to go against the intent of ebdf5bf. I'm not sure if this really creates that much of a problem in practice, but it is a behavior change. Also, I still think it might be better to include this patch in the patch set where the exported function is needed. On its own, this is a very small amount of refactoring that might not be totally necessary. > I have one additional concern about the way we update the control > file, at every place where doing the update, we need to set control > file update time explicitly, why can't the time update line be moved > to UpdateControlFile() so that time gets automatically updated? I see a few places where UpdateControlFile() is called without updating ControlFile->time. I haven't found any obvious reason for that, so perhaps it would be okay to move it to update_controlfile(). Nathan
On Tue, Sep 21, 2021 at 4:44 AM Bossart, Nathan <bossartn@amazon.com> wrote: > > On 9/19/21, 11:07 PM, "Amul Sul" <sulamul@gmail.com> wrote: > > +1, since skipping ControlFileLock for the DBState update is not the > > right thing, let's have two different functions as per your suggestion > > -- did the same in the attached version, thanks. > > I see that the attached patch reorders the call to UpdateControlFile() > to before SharedRecoveryState is updated, which seems to go against > the intent of ebdf5bf. I'm not sure if this really creates that much > of a problem in practice, but it is a behavior change. > I had to have a thought on the same and didn't see any problem and test suits also fine but that doesn't mean the change is perfect, the issue might be hard to reproduce if there are any. Let's see what others think and for now, to be safe I have reverted this change. > Also, I still think it might be better to include this patch in the > patch set where the exported function is needed. On its own, this is > a very small amount of refactoring that might not be totally > necessary. > Well, the other patch set is quite big and complex. In my experience, usually, people avoid downloading big sets due to lack of time and such small refactoring patches usually don't get much detailed attention. Also, even though this patch is small, it is independent and has nothing to do with other patch set whether it gets committed or not. Still, proposing some improvement might not be a big one but nice to have. > > I have one additional concern about the way we update the control > > file, at every place where doing the update, we need to set control > > file update time explicitly, why can't the time update line be moved > > to UpdateControlFile() so that time gets automatically updated? > > I see a few places where UpdateControlFile() is called without > updating ControlFile->time. I haven't found any obvious reason for > that, so perhaps it would be okay to move it to update_controlfile(). > Ok, thanks, did the same in the attached version. Regards, Amul Sul
Вложения
On 9/20/21, 10:07 PM, "Amul Sul" <sulamul@gmail.com> wrote:
> On Tue, Sep 21, 2021 at 4:44 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>> On 9/19/21, 11:07 PM, "Amul Sul" <sulamul@gmail.com> wrote:
>> > I have one additional concern about the way we update the control
>> > file, at every place where doing the update, we need to set control
>> > file update time explicitly, why can't the time update line be moved
>> > to UpdateControlFile() so that time gets automatically updated?
>>
>> I see a few places where UpdateControlFile() is called without
>> updating ControlFile->time. I haven't found any obvious reason for
>> that, so perhaps it would be okay to move it to update_controlfile().
>>
>
> Ok, thanks, did the same in the attached version.
void
UpdateControlFile(void)
{
+ ControlFile->time = (pg_time_t) time(NULL);
update_controlfile(DataDir, ControlFile, true);
}
Shouldn't we update the time in update_controlfile()? Also, can you
split this change into two patches (i.e., one for the timestamp change
and another for the refactoring)? Otherwise, this looks reasonable to
me.
Nathan
On Tue, Sep 21, 2021 at 9:43 PM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 9/20/21, 10:07 PM, "Amul Sul" <sulamul@gmail.com> wrote:
> > On Tue, Sep 21, 2021 at 4:44 AM Bossart, Nathan <bossartn@amazon.com> wrote:
> >> On 9/19/21, 11:07 PM, "Amul Sul" <sulamul@gmail.com> wrote:
> >> > I have one additional concern about the way we update the control
> >> > file, at every place where doing the update, we need to set control
> >> > file update time explicitly, why can't the time update line be moved
> >> > to UpdateControlFile() so that time gets automatically updated?
> >>
> >> I see a few places where UpdateControlFile() is called without
> >> updating ControlFile->time. I haven't found any obvious reason for
> >> that, so perhaps it would be okay to move it to update_controlfile().
> >>
> >
> > Ok, thanks, did the same in the attached version.
>
> void
> UpdateControlFile(void)
> {
> + ControlFile->time = (pg_time_t) time(NULL);
> update_controlfile(DataDir, ControlFile, true);
> }
>
> Shouldn't we update the time in update_controlfile()?
If you see the callers of update_controlfile() except for
RewriteControlFile() no one else updates the timestamp before calling
it, I am not sure if that is intentional or not. That was the one
reason that was added in UpdateControlFile(). And another reason is
that if you look at all the deleting lines followed by
UpdateControlFile() & moving that to UpdateControlFile() wouldn't
change anything drastically.
IMO, anything going to change should update the timestamp as well,
that could be a bug then.
> Also, can you
> split this change into two patches (i.e., one for the timestamp change
> and another for the refactoring)? Otherwise, this looks reasonable to
> me.
Done, thanks for the review.
Regards,
Amul
Вложения
On 9/22/21, 10:03 PM, "Amul Sul" <sulamul@gmail.com> wrote: > On Tue, Sep 21, 2021 at 9:43 PM Bossart, Nathan <bossartn@amazon.com> wrote: >> Shouldn't we update the time in update_controlfile()? > > If you see the callers of update_controlfile() except for > RewriteControlFile() no one else updates the timestamp before calling > it, I am not sure if that is intentional or not. That was the one > reason that was added in UpdateControlFile(). And another reason is > that if you look at all the deleting lines followed by > UpdateControlFile() & moving that to UpdateControlFile() wouldn't > change anything drastically. > > IMO, anything going to change should update the timestamp as well, > that could be a bug then. I'm inclined to agree that anything that calls update_controlfile() should update the timestamp. However, I wonder if the additional calls to time() would have a noticeable impact. Nathan
On Fri, Oct 01, 2021 at 05:47:45PM +0000, Bossart, Nathan wrote:
> I'm inclined to agree that anything that calls update_controlfile()
> should update the timestamp.
pg_control.h tells that:
pg_time_t time; /* time stamp of last pg_control update */
So, yes, that would be more consistent.
> However, I wonder if the additional
> calls to time() would have a noticeable impact.
I would not take that lightly either. Now, I don't think that any of
the code paths where UpdateControlFile() or update_controlfile() is
called are hot enough to worry about that.
UpdateControlFile(void)
{
+ ControlFile->time = (pg_time_t) time(NULL);
update_controlfile(DataDir, ControlFile, true);
}
I have to admit that it is a bit strange to do that in the backend but
not the frontend, so there is a good argument for doing that directly
in update_controlfile(). pg_resetwal does an update of the time, but
pg_rewind does not.
--
Michael
Вложения
On 10/1/21, 10:40 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Fri, Oct 01, 2021 at 05:47:45PM +0000, Bossart, Nathan wrote:
>> I'm inclined to agree that anything that calls update_controlfile()
>> should update the timestamp.
>
> pg_control.h tells that:
> pg_time_t time; /* time stamp of last pg_control update */
> So, yes, that would be more consistent.
>
>> However, I wonder if the additional
>> calls to time() would have a noticeable impact.
>
> I would not take that lightly either. Now, I don't think that any of
> the code paths where UpdateControlFile() or update_controlfile() is
> called are hot enough to worry about that.
>
> UpdateControlFile(void)
> {
> + ControlFile->time = (pg_time_t) time(NULL);
> update_controlfile(DataDir, ControlFile, true);
> }
> I have to admit that it is a bit strange to do that in the backend but
> not the frontend, so there is a good argument for doing that directly
> in update_controlfile(). pg_resetwal does an update of the time, but
> pg_rewind does not.
I don't see any recent updates to this thread from Amul, so I'm
marking this one as waiting-for-author.
Nathan
On Thu, Nov 11, 2021 at 1:30 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 10/1/21, 10:40 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> > On Fri, Oct 01, 2021 at 05:47:45PM +0000, Bossart, Nathan wrote:
> >> I'm inclined to agree that anything that calls update_controlfile()
> >> should update the timestamp.
> >
> > pg_control.h tells that:
> > pg_time_t time; /* time stamp of last pg_control update */
> > So, yes, that would be more consistent.
> >
> >> However, I wonder if the additional
> >> calls to time() would have a noticeable impact.
> >
> > I would not take that lightly either. Now, I don't think that any of
> > the code paths where UpdateControlFile() or update_controlfile() is
> > called are hot enough to worry about that.
> >
> > UpdateControlFile(void)
> > {
> > + ControlFile->time = (pg_time_t) time(NULL);
> > update_controlfile(DataDir, ControlFile, true);
> > }
> > I have to admit that it is a bit strange to do that in the backend but
> > not the frontend, so there is a good argument for doing that directly
> > in update_controlfile(). pg_resetwal does an update of the time, but
> > pg_rewind does not.
>
Thanks for the inputs -- moved timestamp setting inside update_controlfile().
> I don't see any recent updates to this thread from Amul, so I'm
> marking this one as waiting-for-author.
>
Sorry for the delay, please have a look at the attached version --
changing status to Needs review, thanks.
Regards,
Amul
Вложения
On Thu, Nov 25, 2021 at 10:21:40AM +0530, Amul Sul wrote: > Thanks for the inputs -- moved timestamp setting inside update_controlfile(). I have not check the performance implication of that with a micro benchmark or the like, but I can get behind 0001 on consistency grounds between the backend and the frontend. 0002 does not seem worth the trouble, though, as it is changing only two code paths. -- Michael
Вложения
On Thu, Nov 25, 2021 at 04:04:23PM +0900, Michael Paquier wrote:
> I have not check the performance implication of that with a micro
> benchmark or the like, but I can get behind 0001 on consistency
> grounds between the backend and the frontend.
/* Now create pg_control */
InitControlFile(sysidentifier);
- ControlFile->time = checkPoint.time;
ControlFile->checkPoint = checkPoint.redo;
ControlFile->checkPointCopy = checkPoint;
0001 has a mistake here, no? The initial control file creation goes
through WriteControlFile(), and not update_controlfile(), so this
change means that we would miss setting up this timestamp for the
first time.
@@ -714,7 +714,6 @@ GuessControlValues(void)
ControlFile.checkPointCopy.oldestActiveXid = InvalidTransactionId;
ControlFile.state = DB_SHUTDOWNED;
- ControlFile.time = (pg_time_t) time(NULL);
This one had better not be removed, either, as we require pg_resetwal
to guess a set of control file values. Removing the one in
RewriteControlFile() is fine, on the contrary.
--
Michael
Вложения
On Fri, Nov 26, 2021 at 12:16 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Thu, Nov 25, 2021 at 04:04:23PM +0900, Michael Paquier wrote: > > I have not check the performance implication of that with a micro > > benchmark or the like, but I can get behind 0001 on consistency > > grounds between the backend and the frontend. > > /* Now create pg_control */ > InitControlFile(sysidentifier); > - ControlFile->time = checkPoint.time; > ControlFile->checkPoint = checkPoint.redo; > ControlFile->checkPointCopy = checkPoint; > 0001 has a mistake here, no? The initial control file creation goes > through WriteControlFile(), and not update_controlfile(), so this > change means that we would miss setting up this timestamp for the > first time. > > @@ -714,7 +714,6 @@ GuessControlValues(void) > ControlFile.checkPointCopy.oldestActiveXid = InvalidTransactionId; > > ControlFile.state = DB_SHUTDOWNED; > - ControlFile.time = (pg_time_t) time(NULL); > This one had better not be removed, either, as we require pg_resetwal > to guess a set of control file values. Removing the one in > RewriteControlFile() is fine, on the contrary. My bad, sorry for the sloppy change, corrected it in the attached version. Regards, Amul
Вложения
On Fri, Nov 26, 2021 at 2:48 PM Amul Sul <sulamul@gmail.com> wrote: > > ControlFile.state = DB_SHUTDOWNED; > > - ControlFile.time = (pg_time_t) time(NULL); > > This one had better not be removed, either, as we require pg_resetwal > > to guess a set of control file values. Removing the one in > > RewriteControlFile() is fine, on the contrary. > > My bad, sorry for the sloppy change, corrected it in the attached version. Thanks for the patch. By moving the time update to update_controlfile, the patch ensures that we have the correct last updated time. Earlier we were missing (in some places) to update the time before calling UpdateControlFile. Isn't it better if we update the ControlFile->time at the end of the update_controlfile, after file write/sync? Why do we even need UpdateControlFile which just calls another function? It may be there for usability and readability, but can't the pg backend code just call update_controlfile(DataDir, ControlFile, true); directly so that a function call cost can be avoided? Otherwise, why can't we make UpdateControlFile an inline function? I'm not sure if any of the compilers will ever optimize by inlining it without the "inline" keyword. Regards, Bharath Rupireddy.
On Sun, Nov 28, 2021 at 07:53:13AM +0530, Bharath Rupireddy wrote: > Isn't it better if we update the ControlFile->time at the end of the > update_controlfile, after file write/sync? I don't quite understand your point here. We want to update the control file's timestamp when it is written, before calculating its CRC. > Why do we even need UpdateControlFile which just calls another > function? It may be there for usability and readability, but can't the > pg backend code just call update_controlfile(DataDir, ControlFile, > true); directly so that a function call cost can be avoided? > Otherwise, why can't we make UpdateControlFile an inline function? I'm > not sure if any of the compilers will ever optimize by inlining it > without the "inline" keyword. I would leave it as-is as UpdateControlFile() is a public API old enough to vote (a70e74b0). Anyway, that's a useful wrapper for the backend. -- Michael
Вложения
On Sun, Nov 28, 2021 at 10:03 AM Michael Paquier <michael@paquier.xyz> wrote: > We want to update the > control file's timestamp when it is written, before calculating its > CRC. Okay. > > Why do we even need UpdateControlFile which just calls another > > function? It may be there for usability and readability, but can't the > > pg backend code just call update_controlfile(DataDir, ControlFile, > > true); directly so that a function call cost can be avoided? > > Otherwise, why can't we make UpdateControlFile an inline function? I'm > > not sure if any of the compilers will ever optimize by inlining it > > without the "inline" keyword. > > I would leave it as-is as UpdateControlFile() is a public API old > enough to vote (a70e74b0). Anyway, that's a useful wrapper for the > backend. In that case, why can't we inline UpdateControlFile to avoid the function call cost? Do you see any issues with it? BTW, the v6 patch proposed by Amul at [1], looks good to me. [1] - https://www.postgresql.org/message-id/CAAJ_b94_s-VQs3Vtn_X-ReYr1DzaEakwPi80D1UYSmV3-f%2B_pw%40mail.gmail.com Regards, Bharath Rupireddy.
On Mon, Nov 29, 2021 at 09:28:23AM +0530, Bharath Rupireddy wrote: > In that case, why can't we inline UpdateControlFile to avoid the > function call cost? Do you see any issues with it? This routine is IMO not something worth bothering about. > BTW, the v6 patch proposed by Amul at [1], looks good to me. Yes, I have no problems with this part, so done. -- Michael
Вложения
On Mon, Nov 29, 2021 at 10:12 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Nov 29, 2021 at 09:28:23AM +0530, Bharath Rupireddy wrote: > > In that case, why can't we inline UpdateControlFile to avoid the > > function call cost? Do you see any issues with it? > > This routine is IMO not something worth bothering about. > > > BTW, the v6 patch proposed by Amul at [1], looks good to me. > > Yes, I have no problems with this part, so done. Thank you, Michael. Regards, Amul