Hi Shlok,
Thanks very much for the review; the patch is very helpful.
Attached is v3 of the patch
> 1. I think we can improve the debug message in:
> + elog(DEBUG1,
> + "transaction %u committed in database %u %s catalog changes relevant",
> + xid, parsed->dbId,
> + catalog_changes_relevant ? "has" : "does not have");
> Maybe we can change "catalog changes relevant" to relevant
> catalog changes"?
Agreed and applied. The message now reads "has relevant catalog
changes" or "does not have relevant catalog changes"
> 2. In the 002_two_databases.pl, we can club multiple commands:
> +$node->safe_psql('db2', 'CREATE TABLE t2(a int)');
> +$node->safe_psql('db2', 'CREATE ROLE regress_role1');
> +$node->safe_psql('db2', 'CREATE PUBLICATION pub0 FOR ALL TABLES');
> like:
> $node->safe_psql(
> 'db2', qq[
> CREATE TABLE t2(a int);
> CREATE ROLE regress_role1;
> CREATE PUBLICATION pub0 FOR ALL TABLES;
> ]);
> There are multiple places we can do the same.
Agreed and applied. The setup statements are grouped into one
safe_psql() call per database.
> 3. Multiple test use the similar statement:
> +my $session1 = $node->background_psql('db1', on_error_stop => 1);
> +$session1->query_safe('BEGIN');
> +$session1->query_safe('INSERT INTO t1 VALUES (1)');
> Maybe we can create a common function for it?
I would rather keep BEGIN at the call site. In most tests, BEGIN marks
where concurrency control starts. And a common function saves little
code. Following the existing tests and your second advice, I send
query_safe('BEGIN; <statement>') inline.
> 4. The patch requires pgperltidy runs.
Agreed and applied.
Regards,
Boyu Yang