RE: Perform streaming logical transactions by background workers and parallel apply - Mailing list pgsql-hackers

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