On 3/6/17 05:27, Petr Jelinek wrote:
> updated and rebased version of the patch attached.
Some comments on this patch:
Can we have a better explanation of different snapshot options in
CREATE_REPLICATION_SLOT. We use all these variants in different
places, but it's not fully documented why. Think about interested
users reading this.
errmsg("subscription table %u in subscription %u does not exist",
Use names instead of IDs if possible.
+ libpqrcv_table_list,
+ libpqrcv_table_info,
+ libpqrcv_table_copy,
I don't think these functions belong into the WAL receiver API, since
they are specific to this particular logical replication
implementation. I would just make an API function libpqrc_exec_sql()
that runs a query, and then put the table_*() functions as wrappers
around that into tablesync.c.
Not sure what the worker init stuff in ApplyLauncherShmemInit() is
doing. Could be commented more.
There are a lot of places that do one of
MyLogicalRepWorker->relid == InvalidOid
OidIsValid(MyLogicalRepWorker->relid)
To check whether the current worker is a sync worker. Wrap that into
a function.
Not a fan of adding CommandCounterIncrement() calls at the end of
functions. In some cases, they are not necessary at all. In cases
where they are, the CCI call should be at a higher level between two
function calls with a comment for why the call below needs to see the
changes from the call above.
The index name pg_subscription_rel_map_index/SubscriptionRelMapIndexId
doesn't seem to match existing style, since there is no "map" column.
How about pg_subscription_rel_rel_sub_index? I see we have a
similarly named index for publications.
max_sync_workers_per_subscription could be PGC_SIGHUP, I think. And
the minimum could be 0, to stop any syncing?
pg_subscription_rel.h: I'm not sure under which circumstances we need
to use BKI_ROWTYPE_OID.
Does pg_subscription_rel need an OID column? The index
SubscriptionRelOidIndexId is not used anywhere.
You removed this command from the tests:
ALTER SUBSCRIPTION testsub SET PUBLICATION testpub, testpub1;
I suppose because it causes a connection. We should have an option to
prevent that (NOCONNECT/NOREFRESH?).
Why was the test 'check replication origin was dropped on subscriber'
removed?
Attached also a small patch with some stylistic changes.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
