Thread: unnecessary creation of FSM files during bootstrap mode
As pointed out by John Naylor [1], it seems during bootstrap mode, we are always creating FSM files which are not required. In commit's b9d01fe288 and 3908473c80, we have added some code where we allowed the creation of files during mdopen even if they didn't exist during the bootstrap mode. The comments in the code say: "During bootstrap, there are cases where a system relation will be accessed (by internal backend processes) before the bootstrap script nominally creates it." I am sure this will be the case when that code is added but is it required today? While going through commit 3908473c80, I came across below comment: - * During bootstrap processing, we skip that check, because pg_time, - * pg_variable, and pg_log get created before their .bki file entries - * are processed. - */ + fd = FileNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, 0600); The system tables mentioned in above commit are not present today, so do we really need that code and even if it is required shall we do it only for 'main' or 'init' forks? Tom, as you are a committer of the commits b9d01fe288 and 3908473c80, do you remember anything in this regard? [1] - https://www.postgresql.org/message-id/CAJVSVGVtf%2B-2sQGVyEZJQh15dpVicpFA6BBiPLugaD4oBEaiHg%40mail.gmail.com -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Thought I'd ping... (sorry for the top post) On Sat, Dec 15, 2018 at 12:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > As pointed out by John Naylor [1], it seems during bootstrap mode, we > are always creating FSM files which are not required. In commit's > b9d01fe288 and 3908473c80, we have added some code where we allowed > the creation of files during mdopen even if they didn't exist during > the bootstrap mode. The comments in the code say: "During bootstrap, > there are cases where a system relation will be accessed (by internal > backend processes) before the bootstrap script nominally creates it." > I am sure this will be the case when that code is added but is it > required today? While going through commit 3908473c80, I came across > below comment: > > - * During bootstrap processing, we skip that check, because pg_time, > - * pg_variable, and pg_log get created before their .bki file entries > - * are processed. > - */ > + fd = FileNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY, 0600); > > The system tables mentioned in above commit are not present today, so > do we really need that code and even if it is required shall we do it > only for 'main' or 'init' forks? > > Tom, as you are a committer of the commits b9d01fe288 and 3908473c80, > do you remember anything in this regard? > > > [1] - https://www.postgresql.org/message-id/CAJVSVGVtf%2B-2sQGVyEZJQh15dpVicpFA6BBiPLugaD4oBEaiHg%40mail.gmail.com > > -- > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com >
John Naylor <jcnaylor@gmail.com> writes:
> Thought I'd ping...
Sorry, I'd not been paying attention to this thread.
> On Sat, Dec 15, 2018 at 12:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> As pointed out by John Naylor [1], it seems during bootstrap mode, we
>> are always creating FSM files which are not required. In commit's
>> b9d01fe288 and 3908473c80, we have added some code where we allowed
>> the creation of files during mdopen even if they didn't exist during
>> the bootstrap mode. The comments in the code say: "During bootstrap,
>> there are cases where a system relation will be accessed (by internal
>> backend processes) before the bootstrap script nominally creates it."
>> I am sure this will be the case when that code is added but is it
>> required today?
I'm surprised to hear that it isn't, but if we get through initdb
then it must not be. The special case was certainly still necessary
as of 3908473c80, for the BKI_BOOTSTRAP catalogs. It didn't bother me
at the time that those were special --- how are you going to create
pg_class, in particular, without cheating like mad? But I guess we must
have cleaned up something in higher levels of bootstrapping to the point
where those do get mdcreate'd in advance of being opened.
It's also possible that you just aren't exercising the cases where
trouble occurs. In particular, noting this bit in InsertOneValue():
/*
* We use ereport not elog here so that parameters aren't evaluated unless
* the message is going to be printed, which generally it isn't
*/
ereport(DEBUG4,
(errmsg_internal("inserted -> %s",
OidOutputFunctionCall(typoutput, values[i]))));
I'd counsel running initdb under DEBUG4 or higher before deciding
you're out of the woods.
(If that does turn out to be a problem, and it's the only problem,
we could discuss whether we really need that debug message at all.)
regards, tom lane
On Fri, Jan 11, 2019 at 5:00 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It's also possible that you just aren't exercising the cases where
> trouble occurs. In particular, noting this bit in InsertOneValue():
>
> /*
> * We use ereport not elog here so that parameters aren't evaluated unless
> * the message is going to be printed, which generally it isn't
> */
> ereport(DEBUG4,
> (errmsg_internal("inserted -> %s",
> OidOutputFunctionCall(typoutput, values[i]))));
>
> I'd counsel running initdb under DEBUG4 or higher before deciding
> you're out of the woods.
>
I have tried initdb with --debug option (If I am not wrong, it runs
initdb under DEBUG5 mode) and didn't hit any problem after applying
the patch. Are you expecting that we might try to open pg_proc at
that place which can lead to the problem?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Amit Kapila <amit.kapila16@gmail.com> writes:
> On Fri, Jan 11, 2019 at 5:00 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It's also possible that you just aren't exercising the cases where
>> trouble occurs. In particular, noting this bit in InsertOneValue():
>> OidOutputFunctionCall(typoutput, values[i]))));
> I have tried initdb with --debug option (If I am not wrong, it runs
> initdb under DEBUG5 mode) and didn't hit any problem after applying
> the patch. Are you expecting that we might try to open pg_proc at
> that place which can lead to the problem?
Yes, I was concerned about regprocout in particular. It might be
okay as long as we don't try to add any regproc columns to the
BKI_BOOTSTRAP catalogs.
regards, tom lane
On Fri, Jan 11, 2019 at 8:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Kapila <amit.kapila16@gmail.com> writes: > > On Fri, Jan 11, 2019 at 5:00 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> It's also possible that you just aren't exercising the cases where > >> trouble occurs. In particular, noting this bit in InsertOneValue(): > >> OidOutputFunctionCall(typoutput, values[i])))); > > > I have tried initdb with --debug option (If I am not wrong, it runs > > initdb under DEBUG5 mode) and didn't hit any problem after applying > > the patch. Are you expecting that we might try to open pg_proc at > > that place which can lead to the problem? > > Yes, I was concerned about regprocout in particular. It might be > okay as long as we don't try to add any regproc columns to the > BKI_BOOTSTRAP catalogs. > Okay, I think if we get such a requirement in the future, we might want to restore the current behavior by selectively doing it for non-FSM and or non-VM forks. I will commit the patch removing these changes before committing the avoid fsm creation stuff [1] by John which is still under review. [1] - https://www.postgresql.org/message-id/CACPNZCuEijBJ9gq9jqjwvQeDB6%3Dig2egRzHhZDfTtRwhmPLVtw%40mail.gmail.com -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com