On Tue, Mar 7, 2023 at 2:40 AM Aleksander Alekseev <aleksan...@timescale.com> wrote: > So far I only have a couple of nitpicks, mostly regarding the code coverage > [1]:
Yeah, I need to work on error cases and their coverage in general. There are more cases that I need to reject as well (marked TODO). > +Datum > +pg_get_publication_rels_to_sync(PG_FUNCTION_ARGS) > +{ > +#define NUM_SYNC_TABLES_ELEM 1 > ``` > > What is this macro for? Whoops, that's cruft from an intermediate implementation. Will fix in the next draft. > +{ 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]. Hm. I'm not sure how this all should behave in the face of concurrent structural changes, or how the existing publication queries handle that same situation (e.g. partition attachment), so that's definitely something for me to look into. At a glance, I'm not sure that returning different results for the same table is more correct. And I feel like a VOLATILE implementation might significantly impact the JOIN/LATERAL performance in the pg_dump query? But I don't really know how that's planned. > +{ 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? My initial thought was that a one-way "upgrade" makes things easier to reason about. But a one-way function is not good UX, so maybe we should provide that. We'd need to verify and test what happens if you undo/"detach" the logical tree during replication. If it's okay to blindly replace any existing inhseqno with, say, 1 (on a table with single inheritance), then we can reverse the process safely. If not, we can't -- at least not with the current implementation -- because we don't save the previous value anywhere. Thanks for the review! --Jacob