On Tue, Feb 25, 2020 at 10:01:51AM +0900, Kyotaro Horiguchi wrote: > At Sat, 22 Feb 2020 21:12:20 -0800, Noah Misch <n...@leadboat.com> wrote in > > On Fri, Feb 21, 2020 at 04:49:59PM +0900, Kyotaro Horiguchi wrote: > > > At Wed, 19 Feb 2020 17:29:08 +0900 (JST), Kyotaro Horiguchi > > > <horikyota....@gmail.com> wrote in > > > > At Tue, 18 Feb 2020 23:44:52 -0800, Noah Misch <n...@leadboat.com> > > > > wrote in
> > > In swap_relation_files, we can remove rel2-related code when #ifndef > > > USE_ASSERT_CHECKING. > > > > When state is visible to many compilation units, we should avoid making that > > state depend on --enable-cassert. That would be a recipe for a Heisenbug. > > In > > a hot code path, it might be worth the risk. > > I aggree that the new #ifdef can invite a Heisenbug. I thought that > you didn't want that because it doesn't make substantial difference. v35nm added swap_relation_files() code so AssertPendingSyncs_RelationCache() could check rd_droppedSubid relations. v30nm, which did not have rd_droppedSubid, removed swap_relation_files() code that wasn't making a difference. > If we decide to keep the consistency there, I would like to describe > the code is there for consistency, not for the benefit of a specific > assertion. > > (cluster.c:1116) > - * new. The next step for rel2 is deletion, but copy rd_*Subid for the > - * benefit of AssertPendingSyncs_RelationCache(). > + * new. The next step for rel2 is deletion, but copy rd_*Subid for the > + * consistency of the fieles. It is checked later by > + * AssertPendingSyncs_RelationCache(). I think the word "consistency" is too vague for "consistency of the fields" to convey information. May I just remove the last sentence of the comment (everything after "* new.")? > > > config.sgml: > > > + When <varname>wal_level</varname> is <literal>minimal</literal> > > > and a > > > + transaction commits after creating or rewriting a permanent > > > table, > > > + materialized view, or index, this setting determines how to > > > persist > > > > > > "creating or truncation" a permanent table? and maybe "refreshing > > > matview and reindex". I'm not sure that they can be merged that way. > ... > > I like mentioning truncation, but I dislike how this implies that CREATE > > INDEX, CREATE MATERIALIZED VIEW, and ALTER INDEX SET TABLESPACE aren't in > > scope. While I usually avoid the word "relation" in documentation, I can > > justify it here to make the sentence less complex. How about the following? > > > > --- a/doc/src/sgml/config.sgml > > +++ b/doc/src/sgml/config.sgml > > @@ -2484,9 +2484,9 @@ include_dir 'conf.d' > > In <literal>minimal</literal> level, no information is logged for > > - tables or indexes for the remainder of a transaction that creates > > or > > - truncates them. This can make bulk operations much faster (see > > - <xref linkend="populate-pitr"/>). But minimal WAL does not contain > > - enough information to reconstruct the data from a base backup and > > the > > - WAL logs, so <literal>replica</literal> or higher must be used to > > - enable WAL archiving (<xref linkend="guc-archive-mode"/>) and > > - streaming replication. > > + permanent relations for the remainder of a transaction that > > creates, > > + rewrites, or truncates them. This can make bulk operations much > > + faster (see <xref linkend="populate-pitr"/>). But minimal WAL does > > + not contain enough information to reconstruct the data from a base > > + backup and the WAL logs, so <literal>replica</literal> or higher > > must > > + be used to enable WAL archiving (<xref > > linkend="guc-archive-mode"/>) > > + and streaming replication. > > </para> > > @@ -2891,9 +2891,9 @@ include_dir 'conf.d' > > When <varname>wal_level</varname> is <literal>minimal</literal> > > and a > > - transaction commits after creating or rewriting a permanent table, > > - materialized view, or index, this setting determines how to persist > > - the new data. If the data is smaller than this setting, write it > > to > > - the WAL log; otherwise, use an fsync of the data file. Depending > > on > > - the properties of your storage, raising or lowering this value > > might > > - help if such commits are slowing concurrent transactions. The > > default > > - is two megabytes (<literal>2MB</literal>). > > + transaction commits after creating, rewriting, or truncating a > > + permanent relation, this setting determines how to persist the new > > + data. If the data is smaller than this setting, write it to the > > WAL > > + log; otherwise, use an fsync of the data file. Depending on the > > + properties of your storage, raising or lowering this value might > > help > > + if such commits are slowing concurrent transactions. The default > > is > > + two megabytes (<literal>2MB</literal>). > > </para> > > I agree that relation works as the generic name of table-like > objects. Addition to that, doesn't using the word "storage file" make > it more clearly? I'm not confident on the wording itself, but it will > look like the following. > > > @@ -2484,9 +2484,9 @@ include_dir 'conf.d' > In <literal>minimal</literal> level, no information is logged for > permanent relations for the remainder of a transaction that creates, > replaces, or truncates the on-disk file. This can make bulk > operations much The docs rarely use "storage file" or "on-disk file" as terms. I hesitate to put more emphasis on files, because they are part of the implementation, not part of the user interface. The term "rewrites"/"rewriting" has the same problem, though. Yet another alternative would be to talk about operations that change the pg_relation_filenode() return value: In <literal>minimal</literal> level, no information is logged for permanent relations for the remainder of a transaction that creates them or changes what <function>pg_relation_filenode</function> returns for them. What do you think?
