Hello. I looked through the latest patch. 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. > > Please give a bit more time to look it.
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? In swap_relation_files, we can remove rel2-related code when #ifndef USE_ASSERT_CHECKING. 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. 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. Other than the item related to pg_visibility.sql are in the attached. The others look good to me. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 3068e1e94a..38a2edf860 100644 --- 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> </listitem> </varlistentry> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 391a8a9ea3..682619c9db 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1118,16 +1118,18 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, */ { Relation rel1, - rel2; + rel2 PG_USED_FOR_ASSERTS_ONLY; rel1 = relation_open(r1, NoLock); +#ifdef USE_ASSERT_CHECKING rel2 = relation_open(r2, NoLock); rel2->rd_createSubid = rel1->rd_createSubid; rel2->rd_newRelfilenodeSubid = rel1->rd_newRelfilenodeSubid; rel2->rd_firstRelfilenodeSubid = rel1->rd_firstRelfilenodeSubid; + relation_close(rel2, NoLock); +#endif RelationAssumeNewRelfilenode(rel1); relation_close(rel1, NoLock); - relation_close(rel2, NoLock); } /* diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 7c2181ac2f..3c500944cd 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1985,7 +1985,8 @@ select * from another; drop table another; -- Create an index that skips WAL, then perform a SET DATA TYPE that skips --- rewriting the index. +-- rewriting the index. Inadvertent changes on rd_createSubid causes +-- asseertion failure. begin; create table skip_wal_skip_rewrite_index (c varchar(10) primary key); alter table skip_wal_skip_rewrite_index alter c type varchar(20); diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out index 6acf31725f..dae7595957 100644 --- a/src/test/regress/expected/create_table.out +++ b/src/test/regress/expected/create_table.out @@ -332,6 +332,7 @@ ERROR: set-returning functions are not allowed in DEFAULT expressions LINE 1: CREATE TABLE default_expr_agg (a int DEFAULT (generate_serie... ^ -- Verify that subtransaction rollback restores rd_createSubid. +-- This and the following are expected not causing assertion failure. BEGIN; CREATE TABLE remember_create_subid (c int); SAVEPOINT q; DROP TABLE remember_create_subid; ROLLBACK TO q; diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 1b1315f316..ce87ed9ab0 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1361,7 +1361,8 @@ select * from another; drop table another; -- Create an index that skips WAL, then perform a SET DATA TYPE that skips --- rewriting the index. +-- rewriting the index. Inadvertent changes on rd_createSubid causes +-- asseertion failure. begin; create table skip_wal_skip_rewrite_index (c varchar(10) primary key); alter table skip_wal_skip_rewrite_index alter c type varchar(20); diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql index a670438c48..3051b5d4e6 100644 --- a/src/test/regress/sql/create_table.sql +++ b/src/test/regress/sql/create_table.sql @@ -319,6 +319,7 @@ CREATE TABLE default_expr_agg (a int DEFAULT (select 1)); CREATE TABLE default_expr_agg (a int DEFAULT (generate_series(1,3))); -- Verify that subtransaction rollback restores rd_createSubid. +-- This and the following are expected not causing assertion failure. BEGIN; CREATE TABLE remember_create_subid (c int); SAVEPOINT q; DROP TABLE remember_create_subid; ROLLBACK TO q;