Dear Wang,
Followings are my comments about v23-0003. Currently I do not have any comments
about 0002 and 0004.
09. general
It seems that logicalrep_rel_mark_parallel_apply() is always called when
relations are opened on the subscriber-side,
but is it really needed? There checks are required only for streaming parallel
apply,
so it may be not needed in case of streaming = 'on' or 'off'.
10. commit message
2) There cannot be any non-immutable functions used by the subscriber-side
replicated table. Look for functions in the following places:
* a. Trigger functions
* b. Column default value expressions and domain constraints
* c. Constraint expressions
* d. Foreign keys
"Foreign key" should not be listed here because it is not related with the
mutability. I think it should be listed as 3), not d..
11. create_subscription.sgml
The constraint about foreign key should be described here.
11. relation.c
11.a
+ CacheRegisterSyscacheCallback(PROCOID,
+
logicalrep_relmap_reset_parallel_cb,
+ (Datum) 0);
Isn't it needed another syscache callback for pg_type?
Users can add any constraints via ALTER DOMAIN command, but the added
constraint may be not checked.
I checked AlterDomainAddConstraint(), and it invalidates only the relcache for
pg_type.
11.b
+ /*
+ * If the column is of a DOMAIN type, determine whether
+ * that domain has any CHECK expressions that are not
+ * immutable.
+ */
+ if (get_typtype(att->atttypid) == TYPTYPE_DOMAIN)
+ {
I think the default value of *domain* must be also checked here.
I tested like followings.
===
1. created a domain that has a default value
CREATE DOMAIN tmp INT DEFAULT 1 CHECK (VALUE > 0);
2. created a table
CREATE TABLE foo (id tmp PRIMARY KEY);
3. checked pg_attribute and pg_class
select oid, relname, attname, atthasdef from pg_attribute, pg_class where
pg_attribute.attrelid = pg_class.oid and pg_class.relname = 'foo' and attname =
'id';
oid | relname | attname | atthasdef
-------+---------+---------+-----------
16394 | foo | id | f
(1 row)
Tt meant that functions might be not checked because the if-statement `if
(att->atthasdef)` became false.
===
12. 015_stream.pl, 016_stream_subxact.pl, 022_twophase_cascade.pl,
023_twophase_stream.pl
- my ($node_publisher, $node_subscriber, $appname, $is_parallel) = @_;
+ my ($node_publisher, $node_subscriber, $appname) = @_;
Why the parameter is removed? I think the test that waits the output
from the apply background worker is meaningful.
13. 032_streaming_apply.pl
The filename seems too general because apply background workers are tested in
above tests.
How about "streaming_apply_constraint" or something?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
