Hello. I looked through the latest patch.
At Wed, 19 Feb 2020 17:29:08 +0900 (JST), Kyotaro Horiguchi
<[email protected]> wrote in
> At Tue, 18 Feb 2020 23:44:52 -0800, Noah Misch <[email protected]> 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;
