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. > 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. > 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. > --- a/doc/src/sgml/config.sgml > +++ b/doc/src/sgml/config.sgml > @@ -2889,13 +2889,13 @@ include_dir 'conf.d' > <listitem> > <para> > 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 or truncating a permanent table, > + refreshing a materialized view, or reindexing, 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 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>