Re: RFC: logical publication via inheritance root? - Mailing list pgsql-hackers

Hi Jacob,

> I'm going to register this in CF for feedback.

Many thanks for the updated patch.

Despite the fact that the patch is still work in progress all in all
it looks very good to me.

So far I only have a couple of nitpicks, mostly regarding the code coverage [1]:

```
+    tablename = get_rel_name(tableoid);
+    if (tablename == NULL)
+        ereport(ERROR,
+                (errcode(ERRCODE_UNDEFINED_TABLE),
+                 errmsg("OID %u does not refer to a table", tableoid)));
+    rootname = get_rel_name(rootoid);
+    if (rootname == NULL)
+        ereport(ERROR,
+                (errcode(ERRCODE_UNDEFINED_TABLE),
+                 errmsg("OID %u does not refer to a table", rootoid)));
```

```
+        res = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data,
+                          lengthof(descRow), descRow);
+
+        if (res->status != WALRCV_OK_TUPLES)
+            ereport(ERROR,
+                    (errmsg("could not fetch logical descendants for
table \"%s.%s\" from publisher: %s",
+                            nspname, relname, res->err)));
```

```
+        res = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data, 0, NULL);
+        pfree(cmd.data);
+        if (res->status != WALRCV_OK_COPY_OUT)
+            ereport(ERROR,
+                    (errcode(ERRCODE_CONNECTION_FAILURE),
+                     errmsg("could not start initial contents copy
for table \"%s.%s\" from remote %s: %s",
+                            lrel.nspname, lrel.relname, quoted_name,
res->err)));
```

These new ereport() paths are never executed when we run the tests.
I'm not 100% sure if they are "should never happen in practice" cases
or not. If they are, I suggest adding corresponding comments.
Otherwise we have to test these paths.

```
+    else
+    {
+        /* For older servers, we only COPY the table itself. */
+        char   *quoted = quote_qualified_identifier(lrel->nspname,
+                                                    lrel->relname);
+        *to_copy = lappend(*to_copy, quoted);
+    }
```

Also we have to be extra careful with this code path because it is not
test-covered too.

```
+Datum
+pg_get_publication_rels_to_sync(PG_FUNCTION_ARGS)
+{
+#define NUM_SYNC_TABLES_ELEM   1
```

What is this macro for?

```
+{ oid => '8137', descr => 'get list of tables to copy during initial sync',
+  proname => 'pg_get_publication_rels_to_sync', prorows => '10',
proretset => 't',
+  provolatile => 's', prorettype => 'regclass', proargtypes => 'regclass text',
+  proargnames => '{rootid,pubname}',
+  prosrc => 'pg_get_publication_rels_to_sync' },
```

Something seems odd here. Is there a chance that it can return
different results even within one statement, especially considering
the fact that pg_set_logical_root() is VOLATILE? Maybe
pg_get_publication_rels_to_sync() should be VOLATILE too [2].

```
+{ oid => '8136', descr => 'mark a table root for logical replication',
+  proname => 'pg_set_logical_root', provolatile => 'v', proparallel => 'u',
+  prorettype => 'void', proargtypes => 'regclass regclass',
+  prosrc => 'pg_set_logical_root' },
```

Shouldn't we also have pg_unset(reset?)_logical_root?

[1]: https://github.com/afiskon/pgscripts/blob/master/code-coverage.sh
[2]: https://www.postgresql.org/docs/current/xfunc-volatility.html

-- 
Best regards,
Aleksander Alekseev