On 6/6/23 08:50, Jacob Champion wrote:
> Commit 062a84442 necessitated some rework of the new
> pg_get_publication_rels_to_sync() helper. It now takes a list of
> publications so that we can handle conflicts in the pubviaroot settings.
> This is more complicated than before -- unlike partitions, standard
> inheritance trees can selectively publish tables that aren't leaves. But
> I think I've finally settled on some semantics for it which are
> unsurprising.
The semantics I've picked are wrong :( I've accidentally reintroduced a
bug that's similar to the one discussed upthread, where if you have two
leaf tables published through different roots:
tree pub1 pub2
----- ----- -----
A - A
B C B - - -
D D D
then a subscription on both publications will duplicate the data in the
leaf (D in the above example). I need to choose semantics that are
closer to the current behavior of partitions.
> I wonder if pg_set_logical_root() might be better implemented as part of
> ALTER TABLE. Maybe with a relation option? If it all went through ALTER
> TABLE ONLY ... SET, then we wouldn't have to worry about a user
> modifying roots while reading pg_get_publication_rels_to_sync() in the
> same query. The permissions checks should be more consistent with less
> effort, and there's an existing way to set/clear the option that already
> plays well with pg_dump and pg_upgrade.
I've implemented ALTER TABLE in v3; I like it a lot more. The new
reloption is named publish_via_parent. So now we can unset the flag and
dump/restore.
v3 also fixes a nasty uninitialized stack variable, along with a bad
collation assumption I made.
> The downsides I can see are the
> need to handle simultaneous changes to INHERIT and SET (since we'd be
> manipulating pg_inherits in both),
(This didn't turn out to be as bad as I feared.)
> as well as the fact that ALTER TABLE
> ... SET defaults to altering the entire table hierarchy, which may be
> bad UX for this case.
This was just wrong -- ALTER TABLE ... SET does not recurse. I think the
docs are misleading here, and I'm not sure why we allow an explicit '*'
after a table name if we're going to ignore it. But I also don't really
want to poke at that in this patchset, if I can avoid it.
--Jacob