On Tue, Mar 7, 2023 at 2:40 AM Aleksander Alekseev
<aleksander@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