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 > > > > - When reusing an index build, instead of storing the dropped relid in > > > > the > > > > IndexStmt and opening the dropped relcache entry in ATExecAddIndex(), > > > > store > > > > the subid fields in the IndexStmt. This is less code, and I felt > > > > RelationIdGetRelationCache() invited misuse. > > > > > > Hmm. I'm not sure that index_create having the new subid parameters is > > > good. And the last if(OidIsValid) clause handles storage persistence > > > so I did that there. But I don't strongly against it. > > Agreed. My choice there was not a clear improvement. > > > The change on alter_table.sql and create_table.sql is expecting to > > cause assertion failure. Don't we need that kind of explanation in > > the comment? > > Test comments generally describe the feature unique to that test, not how the > test might break. Some tests list bug numbers, but that doesn't apply here.
Agreed. > > 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. 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(). > > The patch adds the test for createSubid to pg_visibility.out. It > > doesn't fail without CLOBBER_CACHE_ALWAYS while regression test but > > CLOBBER_CACHE_ALWAYS causes initdb fail and the added check won't be > > reached. I'm not sure it is useful. > > I agree it's not clearly useful, but tests don't need to meet a "clearly > useful" standard. When a fast test is not clearly redundant with another > test, we generally accept it. In the earlier patch version that inspired this > test, RELCACHE_FORCE_RELEASE sufficed to make it fail. > > > 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 > @@ -2891,9 +2891,9 @@ include_dir 'conf.d' When <varname>wal_level</varname> is <literal>minimal</literal> and a transaction commits after creating, replacing, or truncating the on-disk file, this setting determines how to persist the new data. If the data is smaller than this setting, write it to the WAL regards. -- Kyotaro Horiguchi NTT Open Source Software Center